[PATCH v2 1/2] dma/ep93xx_dma: fix initialization of M2M control register

[PATCH v2 1/2] dma/ep93xx_dma: fix initialization of M2M control register

[Mika Westerberg]

From: Rafal Prylowski <prylowski@metasoft.pl>

Setting the flags in case of IDE didn't have any effect since the control
register is overwritten few lines below. So move these to be after the control
register is initialized.

Signed-off-by: Rafal Prylowski <prylowski@metasoft.pl>
Signed-off-by: Mika Westerberg <mika.westerberg@iki.fi>
Acked-by: H Hartley Sweeten <hsweeten@visionengravers.com>
---
 drivers/dma/ep93xx_dma.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/ep93xx_dma.c b/drivers/dma/ep93xx_dma.c
index b47e2b8..6181811 100644
--- a/drivers/dma/ep93xx_dma.c
+++ b/drivers/dma/ep93xx_dma.c
@@ -459,10 +459,6 @@ static int m2m_hw_setup(struct ep93xx_dma_chan *edmac)
 		 * 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 */
 			control = (3 << M2M_CONTROL_PWSC_SHIFT);
@@ -473,6 +469,10 @@ static int m2m_hw_setup(struct ep93xx_dma_chan *edmac)
 			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;
 
 	default:
-- 
1.7.7

[PATCH v2 2/2] dma/ep93xx_dma: prevent ep93xx_dma_tasklet() to reference an empty list

[Mika Westerberg]

If dma_terminate_all() is called before the ep93xx_dma_tasklet() gets to run,
it tries to access an empty ->active list which results following OOPS:

  Internal error: Oops - undefined instruction: 0 [#1]
  CPU: 0    Not tainted  (3.2.0-rc1EP-1+ #1008)
  PC is at 0xc184c868
  LR is at ep93xx_dma_tasklet+0xec/0x164
  pc : [<c184c868>]    lr : [<c012b528>]    psr: 00000013
  sp : c02b7e70  ip : ffffffff  fp : c02b7ea4
  r10: 00000100  r9 : 80000013  r8 : c02b7e50
  r7 : c02b7e70  r6 : c02b7ea4  r5 : 000000a4  r4 : c02b7e70
  r3 : c02b751d  r2 : 8ae34598  r1 : c184c6e0  r0 : c02b7ea4
  Flags: nzcv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
  Control: c000717f  Table: c0004000  DAC: 00000017
  Process swapper (pid: 0, stack limit = 0xc02b6270)
  Stack: (0xc02b7e70 to 0xc02b8000)
  7e60:                                     c02b7ea4 c02b7e70 c0008b64 c02bd5c4
  7e80: c02d60e0 00000000 00000000 c02bd44c c02d60e0 00000100 c02b7ec4 c02b7ea8
  7ea0: c001c49c c012b44c 00000018 00000001 c02d60e0 c02b6000 c02b7f04 c02b7ec8
  7ec0: c001cbc0 c001c3e4 c02b7eec c02b7ed8 00000006 0000000a c02bf674 c02c458c
  7ee0: 00000011 00000000 c02b7f7c c0004000 41129200 c02b0c80 c02b7f14 c02b7f08
  7f00: c001cdd0 c001cb38 c02b7f34 c02b7f18 c000983c c001cd98 c0009a60 60000013
  7f20: fefb0001 c02b7f7c c02b7f44 c02b7f38 c0008190 c0009810 c02b7f9c c02b7f48
  7f40: c0008b64 c0008190 c02c2bf8 00000002 c02b7f90 60000013 c02b6000 c02d1504
  7f60: c02baa88 c02baa80 c0004000 41129200 c02b0c80 c02b7f9c c02b7fa0 c02b7f90
  7f80: c0009a54 c0009a60 60000013 ffffffff c02b7fbc c02b7fa0 c000a03c c0009a40
  7fa0: c02b80b0 c02b19dc c02b19d8 c02baa80 c02b7fcc c02b7fc0 c02384e4 c0009fd4
  7fc0: c02b7ff4 c02b7fd0 c029d924 c0238494 c029d49c 00000000 00000000 c02b19dc
  7fe0: c0007175 c02b803c 00000000 c02b7ff8 c000803c c029d700 00000000 00000000
  Backtrace:
  [<c012b43c>] (ep93xx_dma_tasklet+0x0/0x164) from [<c001c49c>] (tasklet_action+0xc8/0xdc)
  [<c001c3d4>] (tasklet_action+0x0/0xdc) from [<c001cbc0>] (__do_softirq+0x98/0x154)
   r7:c02b6000 r6:c02d60e0 r5:00000001 r4:00000018
  [<c001cb28>] (__do_softirq+0x0/0x154) from [<c001cdd0>] (irq_exit+0x48/0x50)
  [<c001cd88>] (irq_exit+0x0/0x50) from [<c000983c>] (handle_IRQ+0x3c/0x8c)
  [<c0009800>] (handle_IRQ+0x0/0x8c) from [<c0008190>] (asm_do_IRQ+0x10/0x14)
   r7:c02b7f7c r6:fefb0001 r5:60000013 r4:c0009a60
  [<c0008180>] (asm_do_IRQ+0x0/0x14) from [<c0008b64>] (__irq_svc+0x24/0xc0)
  Exception stack(0xc02b7f48 to 0xc02b7f90)
  7f40:                   c02c2bf8 00000002 c02b7f90 60000013 c02b6000 c02d1504
  7f60: c02baa88 c02baa80 c0004000 41129200 c02b0c80 c02b7f9c c02b7fa0 c02b7f90
  7f80: c0009a54 c0009a60 60000013 ffffffff
  [<c0009a30>] (default_idle+0x0/0x34) from [<c000a03c>] (cpu_idle+0x78/0xb0)
  [<c0009fc4>] (cpu_idle+0x0/0xb0) from [<c02384e4>] (rest_init+0x60/0x78)
   r7:c02baa80 r6:c02b19d8 r5:c02b19dc r4:c02b80b0
  [<c0238484>] (rest_init+0x0/0x78) from [<c029d924>] (start_kernel+0x234/0x278)
  [<c029d6f0>] (start_kernel+0x0/0x278) from [<c000803c>] (0xc000803c)
   r5:c02b803c r4:c0007175
  Code: 42555300 54535953 643d4d45 65766972 (53007372)

To make the code a bit more robust against things like these, we modify
ep93xx_dma_get_active() to return NULL in case of empty ->active list and make
sure that callers handle this correctly.

Reported-by: Rafal Prylowski <prylowski@metasoft.pl>
Signed-off-by: Mika Westerberg <mika.westerberg@iki.fi>
---
 drivers/dma/ep93xx_dma.c |   60 ++++++++++++++++++++++++++++++++++++---------
 1 files changed, 48 insertions(+), 12 deletions(-)

diff --git a/drivers/dma/ep93xx_dma.c b/drivers/dma/ep93xx_dma.c
index 6181811..745fa8b 100644
--- a/drivers/dma/ep93xx_dma.c
+++ b/drivers/dma/ep93xx_dma.c
@@ -246,6 +246,9 @@ static void ep93xx_dma_set_active(struct ep93xx_dma_chan *edmac,
 static struct ep93xx_dma_desc *
 ep93xx_dma_get_active(struct ep93xx_dma_chan *edmac)
 {
+	if (list_empty(&edmac->active))
+		return NULL;
+
 	return list_first_entry(&edmac->active, struct ep93xx_dma_desc, node);
 }
 
@@ -263,16 +266,22 @@ ep93xx_dma_get_active(struct ep93xx_dma_chan *edmac)
  */
 static bool ep93xx_dma_advance_active(struct ep93xx_dma_chan *edmac)
 {
+	struct ep93xx_dma_desc *desc;
+
 	list_rotate_left(&edmac->active);
 
 	if (test_bit(EP93XX_DMA_IS_CYCLIC, &edmac->flags))
 		return true;
 
+	desc = ep93xx_dma_get_active(edmac);
+	if (!desc)
+		return false;
+
 	/*
 	 * If txd.cookie is set it means that we are back in the first
 	 * descriptor in the chain and hence done with it.
 	 */
-	return !ep93xx_dma_get_active(edmac)->txd.cookie;
+	return !desc->txd.cookie;
 }
 
 /*
@@ -327,9 +336,15 @@ static void m2p_hw_shutdown(struct ep93xx_dma_chan *edmac)
 
 static void m2p_fill_desc(struct ep93xx_dma_chan *edmac)
 {
-	struct ep93xx_dma_desc *desc = ep93xx_dma_get_active(edmac);
+	struct ep93xx_dma_desc *desc;
 	u32 bus_addr;
 
+	desc = ep93xx_dma_get_active(edmac);
+	if (!desc) {
+		dev_warn(chan2dev(edmac), "M2P: empty descriptor list\n");
+		return;
+	}
+
 	if (ep93xx_dma_chan_direction(&edmac->chan) == DMA_TO_DEVICE)
 		bus_addr = desc->src_addr;
 	else
@@ -491,7 +506,13 @@ static void m2m_hw_shutdown(struct ep93xx_dma_chan *edmac)
 
 static void m2m_fill_desc(struct ep93xx_dma_chan *edmac)
 {
-	struct ep93xx_dma_desc *desc = ep93xx_dma_get_active(edmac);
+	struct ep93xx_dma_desc *desc;
+
+	desc = ep93xx_dma_get_active(edmac);
+	if (!desc) {
+		dev_warn(chan2dev(edmac), "M2M: empty descriptor list\n");
+		return;
+	}
 
 	if (edmac->buffer == 0) {
 		writel(desc->src_addr, edmac->regs + M2M_SAR_BASE0);
@@ -669,24 +690,30 @@ 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;
+	dma_async_tx_callback callback = NULL;
+	void *callback_param = NULL;
 	LIST_HEAD(list);
 
 	spin_lock_irq(&edmac->lock);
+	/*
+	 * If dma_terminate_all() was called before we get to run, the active
+	 * list has become empty. If that happens we aren't supposed to do
+	 * anything more than call ep93xx_dma_advance_work().
+	 */
 	desc = ep93xx_dma_get_active(edmac);
-	if (desc->complete) {
-		edmac->last_completed = desc->txd.cookie;
-		list_splice_init(&edmac->active, &list);
+	if (desc) {
+		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) {
 		/*
@@ -706,13 +733,22 @@ static void ep93xx_dma_tasklet(unsigned long data)
 static irqreturn_t ep93xx_dma_interrupt(int irq, void *dev_id)
 {
 	struct ep93xx_dma_chan *edmac = dev_id;
+	struct ep93xx_dma_desc *desc;
 	irqreturn_t ret = IRQ_HANDLED;
 
 	spin_lock(&edmac->lock);
 
+	desc = ep93xx_dma_get_active(edmac);
+	if (!desc) {
+		dev_warn(chan2dev(edmac),
+			 "got interrupt while active list is empty\n");
+		spin_unlock(&edmac->lock);
+		return IRQ_NONE;
+	}
+
 	switch (edmac->edma->hw_interrupt(edmac)) {
 	case INTERRUPT_DONE:
-		ep93xx_dma_get_active(edmac)->complete = true;
+		desc->complete = true;
 		tasklet_schedule(&edmac->tasklet);
 		break;
 
-- 
1.7.7

RE: [PATCH v2 2/2] dma/ep93xx_dma: prevent ep93xx_dma_tasklet() to reference an empty list

[H Hartley Sweeten]

On Saturday, November 26, 2011 3:54 AM, Mika Westerberg wrote:
> 
> If dma_terminate_all() is called before the ep93xx_dma_tasklet() gets to run,
> it tries to access an empty ->active list which results following OOPS:

[OOPS snipped...]

> To make the code a bit more robust against things like these, we modify
> ep93xx_dma_get_active() to return NULL in case of empty ->active list and make
> sure that callers handle this correctly.
> 
> Reported-by: Rafal Prylowski <prylowski@metasoft.pl>
> Signed-off-by: Mika Westerberg <mika.westerberg@iki.fi>
> ---
>  drivers/dma/ep93xx_dma.c |   60 ++++++++++++++++++++++++++++++++++++---------
>  1 files changed, 48 insertions(+), 12 deletions(-)

Looks good.  Thanks.

Acked-by: H Hartley Sweeten <hsweeten@visionengravers.com>

Re: [PATCH v2 1/2] dma/ep93xx_dma: fix initialization of M2M control register

[Vinod Koul]

On Sat, 2011-11-26 at 12:54 +0200, Mika Westerberg wrote:
> From: Rafal Prylowski <prylowski@metasoft.pl>
> 
> Setting the flags in case of IDE didn't have any effect since the control
> register is overwritten few lines below. So move these to be after the control
> register is initialized.
> 
> Signed-off-by: Rafal Prylowski <prylowski@metasoft.pl>
> Signed-off-by: Mika Westerberg <mika.westerberg@iki.fi>
> Acked-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Applied both, Thanks

I got a trivial merge conflict on both patches as I think these patches
were of made on top of dma_slave_direction changes we did sometime back.

So please do check if merges were right...


-- 
~Vinod

Re: [PATCH v2 1/2] dma/ep93xx_dma: fix initialization of M2M control register

[Mika Westerberg]

On Mon, Dec 05, 2011 at 08:18:29AM +0530, Vinod Koul wrote:
> On Sat, 2011-11-26 at 12:54 +0200, Mika Westerberg wrote:
> > From: Rafal Prylowski <prylowski@metasoft.pl>
> > 
> > Setting the flags in case of IDE didn't have any effect since the control
> > register is overwritten few lines below. So move these to be after the control
> > register is initialized.
> > 
> > Signed-off-by: Rafal Prylowski <prylowski@metasoft.pl>
> > Signed-off-by: Mika Westerberg <mika.westerberg@iki.fi>
> > Acked-by: H Hartley Sweeten <hsweeten@visionengravers.com>
> Applied both, Thanks
> 
> I got a trivial merge conflict on both patches as I think these patches
> were of made on top of dma_slave_direction changes we did sometime back.
> 
> So please do check if merges were right...

Looks right to me. Thanks.