lkml.org 
[lkml]   [2011]   [Nov]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRE: [PATCH v2 1/5] dmaengine: add ep93xx DMA support
On Tuesday, November 15, 2011 8:02 AM, Rafal Prylowski wrote:
>
> Hello.
>
> During adaptation of my experimental ep93xx ide driver to the new dmaengine api,
> I discovered two issues with ep93xx dma implementation:

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

> 1) Control register is incorrectly programmed for IDE (m2m_hw_setup),
> probably copy-paste bug ;)
> 2) Kernel oops when trying to stop running transfers by calling
> dmaengine_terminate_all(...) - caused by dereferencing empty list
> in ep93xx_dma_get_active
>
> Following is a patch, which solves these problems for me.
>
> 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
> @@ -246,6 +246,7 @@ static void ep93xx_dma_set_active(struct
> static struct ep93xx_dma_desc *
> ep93xx_dma_get_active(struct ep93xx_dma_chan *edmac)
> {
> + BUG_ON(list_empty(&edmac->active));

BUG_ON is evil...

I'm not sure this is the right way to handle this.

If your doing dma the list should never be empty. If it is I think it should
have been caught way before this. Mika, do you have any comments?

> return list_first_entry(&edmac->active, struct ep93xx_dma_desc, node);
> }
>
> @@ -459,9 +460,6 @@ static int m2m_hw_setup(struct ep93xx_dm
> * This IDE part is totally untested. Values below are taken
> * from the EP93xx Users's Guide and might not be correct.
> */
> - control |= M2M_CONTROL_NO_HDSK;
> - control |= M2M_CONTROL_RSS_IDE;
> - control |= M2M_CONTROL_PW_16;
>
> if (data->direction == DMA_TO_DEVICE) {
> /* Worst case from the UG */
> @@ -473,6 +471,9 @@ static int m2m_hw_setup(struct ep93xx_dm
> control |= M2M_CONTROL_SAH;
> control |= M2M_CONTROL_TM_RX;
> }
> + control |= M2M_CONTROL_NO_HDSK;
> + control |= M2M_CONTROL_RSS_IDE;
> + control |= M2M_CONTROL_PW_16;
> break;

This looks right. The common OR'ed in bits should be added after setting the
direction bits.

> 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.

> }
> 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.
>
> /* Now we can release all the chained descriptors */
> list_for_each_entry_safe(desc, d, &list, node) {

Thanks,
Hartley
\
 
 \ /
  Last update: 2011-11-15 19:03    [from the cache]
©2003-2011 Jasper Spaans