lkml.org 
[lkml]   [2011]   [Nov]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v2 1/5] dmaengine: add ep93xx DMA support
Hello.

> Nice to see someone is trying to get IDE support for the ep93xx into mainline!
> Unfortunately none of my ep93xx hardware supports IDE... :-(
>

This driver is a result of work of several people, which I've seen on linux-ide
mailing list. I only added this dmaengine support. I really would like to
see it in mainline, but I think it's still not ready for inclusion.

>> default:
>> @@ -668,24 +669,28 @@ static void ep93xx_dma_unmap_buffers(str
>> static void ep93xx_dma_tasklet(unsigned long data)
>> {
>> struct ep93xx_dma_chan *edmac = (struct ep93xx_dma_chan *)data;
>> - struct ep93xx_dma_desc *desc, *d;
>> - dma_async_tx_callback callback;
>> - void *callback_param;
>> + struct ep93xx_dma_desc *desc = NULL, *d;
>> + dma_async_tx_callback callback = NULL;
>> + void *callback_param = NULL;
>> LIST_HEAD(list);
>>
>> spin_lock_irq(&edmac->lock);
>> - desc = ep93xx_dma_get_active(edmac);
>> - if (desc->complete) {
>> - edmac->last_completed = desc->txd.cookie;
>> - list_splice_init(&edmac->active, &list);
>> + if (!list_empty(&edmac->active)) {
>> + desc = ep93xx_dma_get_active(edmac);
>> + if (desc->complete) {
>> + edmac->last_completed = desc->txd.cookie;
>> + list_splice_init(&edmac->active, &list);
>> + }
>
> It looks like this might actually catch your BUG_ON issue above.

Yes, I only inserted BUG_ON in ep93xx_dma_get_active() to be sure, that
nowhere else in code happens similar problem.
But now I'm not so sure, that I encountered bug in ep93xx_dma.c, or
maybe I'm misusing dmaengine api (calling dmaengine_termiante_all from
invalid context?). I don't have enough knowledge to judge this.

>
>> }
>> spin_unlock_irq(&edmac->lock);
>>
>> /* Pick up the next descriptor from the queue */
>> ep93xx_dma_advance_work(edmac);
>>
>> - callback = desc->txd.callback;
>> - callback_param = desc->txd.callback_param;
>> + if (desc) {
>> + callback = desc->txd.callback;
>> + callback_param = desc->txd.callback_param;
>> + }
>
> These could be moved up to where 'desc' is getting set. You have already
> verified that the list is not empty and have a valid 'desc' pointer. Set
> the callback pointers there to remove this extra if (desc) test.
>

Following is a patch with suggestions applied (patch addressing problem with
incorrect programming of control register is in another mail sent in reply to
Mika Westerberg).

Regards,
Rafal Prylowski.

Index: linux-2.6/drivers/dma/ep93xx_dma.c
===================================================================
--- linux-2.6.orig/drivers/dma/ep93xx_dma.c
+++ linux-2.6/drivers/dma/ep93xx_dma.c
@@ -669,24 +669,25 @@ static void ep93xx_dma_tasklet(unsigned
{
struct ep93xx_dma_chan *edmac = (struct ep93xx_dma_chan *)data;
struct ep93xx_dma_desc *desc, *d;
- dma_async_tx_callback callback;
- void *callback_param;
+ dma_async_tx_callback callback = NULL;
+ void *callback_param = NULL;
LIST_HEAD(list);

spin_lock_irq(&edmac->lock);
- desc = ep93xx_dma_get_active(edmac);
- if (desc->complete) {
- edmac->last_completed = desc->txd.cookie;
- list_splice_init(&edmac->active, &list);
+ if (!list_empty(&edmac->active)) {
+ desc = ep93xx_dma_get_active(edmac);
+ if (desc->complete) {
+ edmac->last_completed = desc->txd.cookie;
+ list_splice_init(&edmac->active, &list);
+ }
+ callback = desc->txd.callback;
+ callback_param = desc->txd.callback_param;
}
spin_unlock_irq(&edmac->lock);

/* Pick up the next descriptor from the queue */
ep93xx_dma_advance_work(edmac);

- callback = desc->txd.callback;
- callback_param = desc->txd.callback_param;
-
/* Now we can release all the chained descriptors */
list_for_each_entry_safe(desc, d, &list, node) {
/*


\
 
 \ /
  Last update: 2011-11-16 10:03    [W:0.176 / U:50.248 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site