Messages in this thread Patch in this message | | | Date | Wed, 16 Nov 2011 10:00:46 +0100 | From | Rafal Prylowski <> | Subject | Re: [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) { /*
| |