[PATCH 1/2] DMA: PL08x: fix infinite wait when terminating transfers

[PATCH 1/2] DMA: PL08x: fix infinite wait when terminating transfers

[Russell King - ARM Linux]

If we try to pause a channel when terminating a transfer, we could end
up spinning for it to become inactive indefinitely, and can result in
an uninterruptible wait requiring a reset to recover from.

Terminating a transfer is supposed to take effect immediately, but may
result in data loss.

To make this clear, rename the function to pl08x_terminate_phy_chan().
Also, make sure it is always consistently called - with the spinlock
held and IRQs disabled, and ensure that the TC and ERR interrupt status
is always cleared.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/dma/amba-pl08x.c |   32 ++++++++++++++++++--------------
 1 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 297f48b..8321a39 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -267,19 +267,24 @@ static void pl08x_resume_phy_chan(struct pl08x_phy_chan *ch)
 }
 
 
-/* Stops the channel */
-static void pl08x_stop_phy_chan(struct pl08x_phy_chan *ch)
+/*
+ * pl08x_terminate_phy_chan() stops the channel, clears the FIFO and
+ * clears any pending interrupt status.  This should not be used for
+ * an on-going transfer, but as a method of shutting down a channel
+ * (eg, when it's no longer used) or terminating a transfer.
+ */
+static void pl08x_terminate_phy_chan(struct pl08x_driver_data *pl08x,
+	struct pl08x_phy_chan *ch)
 {
-	u32 val;
+	u32 val = readl(ch->base + PL080_CH_CONFIG);
 
-	pl08x_pause_phy_chan(ch);
+	val &= ~(PL080_CONFIG_ENABLE | PL080_CONFIG_ERR_IRQ_MASK |
+	         PL080_CONFIG_TC_IRQ_MASK);
 
-	/* Disable channel */
-	val = readl(ch->base + PL080_CH_CONFIG);
-	val &= ~PL080_CONFIG_ENABLE;
-	val &= ~PL080_CONFIG_ERR_IRQ_MASK;
-	val &= ~PL080_CONFIG_TC_IRQ_MASK;
 	writel(val, ch->base + PL080_CH_CONFIG);
+
+	writel(1 << ch->id, pl08x->base + PL080_ERR_CLEAR);
+	writel(1 << ch->id, pl08x->base + PL080_TC_CLEAR);
 }
 
 static inline u32 get_bytes_in_cctl(u32 cctl)
@@ -404,13 +409,12 @@ static inline void pl08x_put_phy_channel(struct pl08x_driver_data *pl08x,
 {
 	unsigned long flags;
 
+	spin_lock_irqsave(&ch->lock, flags);
+
 	/* Stop the channel and clear its interrupts */
-	pl08x_stop_phy_chan(ch);
-	writel((1 << ch->id), pl08x->base + PL080_ERR_CLEAR);
-	writel((1 << ch->id), pl08x->base + PL080_TC_CLEAR);
+	pl08x_terminate_phy_chan(pl08x, ch);
 
 	/* Mark it as free */
-	spin_lock_irqsave(&ch->lock, flags);
 	ch->serving = NULL;
 	spin_unlock_irqrestore(&ch->lock, flags);
 }
@@ -1449,7 +1453,7 @@ static int pl08x_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
 		plchan->state = PL08X_CHAN_IDLE;
 
 		if (plchan->phychan) {
-			pl08x_stop_phy_chan(plchan->phychan);
+			pl08x_terminate_phy_chan(pl08x, plchan->phychan);
 
 			/*
 			 * Mark physical channel as free and free any slave
-- 
1.6.2.5

[PATCH 2/2] DMA: PL08x: fix channel pausing to timeout rather than lockup

[Russell King - ARM Linux]

If a transfer is initiated from memory to a peripheral, then data is
fetched and the channel is marked busy.  This busy status persists until
the HALT bit is set and the queued data has been transfered to the
peripheral.  Waiting indefinitely after setting the HALT bit results in
system lockups.  Timeout this operation, and print an error when this
happens.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
This happens when a M->P transfer is started, but the peripheral doesn't
take the data.  The DMA controller will mark itself active and fetch data
from memory, in anticipation of the peripheral wanting the data.  This
advances the source address.

An alternative approach here is to just set the HALT bit and return
immediately, without waiting for the primecell to show that it is no
longer active.  In any case, resuming the channel will allow the
transfer to continue without data loss.

 drivers/dma/amba-pl08x.c |   21 +++++++++++++++------
 1 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index 8321a39..07bca49 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -79,6 +79,7 @@
 #include <linux/module.h>
 #include <linux/interrupt.h>
 #include <linux/slab.h>
+#include <linux/delay.h>
 #include <linux/dmapool.h>
 #include <linux/dmaengine.h>
 #include <linux/amba/bus.h>
@@ -235,16 +236,19 @@ static void pl08x_start_txd(struct pl08x_dma_chan *plchan,
 }
 
 /*
- * Overall DMAC remains enabled always.
+ * Pause the channel by setting the HALT bit.
  *
- * Disabling individual channels could lose data.
+ * For M->P transfers, pause the DMAC first and then stop the peripheral -
+ * the FIFO can only drain if the peripheral is still requesting data.
+ * (note: this can still timeout if the DMAC FIFO never drains of data.)
  *
- * Disable the peripheral DMA after disabling the DMAC in order to allow
- * the DMAC FIFO to drain, and hence allow the channel to show inactive
+ * For P->M transfers, disable the peripheral first to stop it filling
+ * the DMAC FIFO, and then pause the DMAC.
  */
 static void pl08x_pause_phy_chan(struct pl08x_phy_chan *ch)
 {
 	u32 val;
+	int timeout;
 
 	/* Set the HALT bit and wait for the FIFO to drain */
 	val = readl(ch->base + PL080_CH_CONFIG);
@@ -252,8 +256,13 @@ static void pl08x_pause_phy_chan(struct pl08x_phy_chan *ch)
 	writel(val, ch->base + PL080_CH_CONFIG);
 
 	/* Wait for channel inactive */
-	while (pl08x_phy_channel_busy(ch))
-		cpu_relax();
+	for (timeout = 1000; timeout; timeout--) {
+		if (!pl08x_phy_channel_busy(ch))
+			break;
+		udelay(1);
+	}
+	if (pl08x_phy_channel_busy(ch))
+		pr_err("pl08x: channel%u timeout waiting for pause\n", ch->id);
 }
 
 static void pl08x_resume_phy_chan(struct pl08x_phy_chan *ch)
-- 
1.6.2.5

[PATCH 1/2] DMA: PL08x: fix infinite wait when terminating transfers

[Linus Walleij]

2011/1/27 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> If we try to pause a channel when terminating a transfer, we could end
> up spinning for it to become inactive indefinitely, and can result in
> an uninterruptible wait requiring a reset to recover from.
>
> Terminating a transfer is supposed to take effect immediately, but may
> result in data loss.
>
> To make this clear, rename the function to pl08x_terminate_phy_chan().
> Also, make sure it is always consistently called - with the spinlock
> held and IRQs disabled, and ensure that the TC and ERR interrupt status
> is always cleared.
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

Acked-by: Linus Walleij <linus.walleij@stericsson.com>

Yours,
Linus Walleij

[PATCH 2/2] DMA: PL08x: fix channel pausing to timeout rather than lockup

[Linus Walleij]

2011/1/27 Russell King - ARM Linux <linux@arm.linux.org.uk>:

> If a transfer is initiated from memory to a peripheral, then data is
> fetched and the channel is marked busy. ?This busy status persists until
> the HALT bit is set and the queued data has been transfered to the
> peripheral. ?Waiting indefinitely after setting the HALT bit results in
> system lockups. ?Timeout this operation, and print an error when this
> happens.
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

Acked-by: Linus Walleij <linus.walleij@stericsson.com>

Yours,
Linus Walleij

[PATCH 2/2] DMA: PL08x: fix channel pausing to timeout rather than lockup

[Dan Williams]

On Thu, Jan 27, 2011 at 4:37 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> If a transfer is initiated from memory to a peripheral, then data is
> fetched and the channel is marked busy. ?This busy status persists until
> the HALT bit is set and the queued data has been transfered to the
> peripheral. ?Waiting indefinitely after setting the HALT bit results in
> system lockups. ?Timeout this operation, and print an error when this
> happens.
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
> This happens when a M->P transfer is started, but the peripheral doesn't
> take the data. ?The DMA controller will mark itself active and fetch data
> from memory, in anticipation of the peripheral wanting the data. ?This
> advances the source address.
>
> An alternative approach here is to just set the HALT bit and return
> immediately, without waiting for the primecell to show that it is no
> longer active. ?In any case, resuming the channel will allow the
> transfer to continue without data loss.
>

Ok, and the other infinite busy wait in pl08x_start_txd() is
guaranteed to be after a resume operation, or does it also need a
timeout?

--
Dan

[PATCH 2/2] DMA: PL08x: fix channel pausing to timeout rather than lockup

[Russell King - ARM Linux]

On Sun, Jan 30, 2011 at 08:48:27AM -0800, Dan Williams wrote:
> On Thu, Jan 27, 2011 at 4:37 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > If a transfer is initiated from memory to a peripheral, then data is
> > fetched and the channel is marked busy. ?This busy status persists until
> > the HALT bit is set and the queued data has been transfered to the
> > peripheral. ?Waiting indefinitely after setting the HALT bit results in
> > system lockups. ?Timeout this operation, and print an error when this
> > happens.
> >
> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > ---
> > This happens when a M->P transfer is started, but the peripheral doesn't
> > take the data. ?The DMA controller will mark itself active and fetch data
> > from memory, in anticipation of the peripheral wanting the data. ?This
> > advances the source address.
> >
> > An alternative approach here is to just set the HALT bit and return
> > immediately, without waiting for the primecell to show that it is no
> > longer active. ?In any case, resuming the channel will allow the
> > transfer to continue without data loss.
> >
> 
> Ok, and the other infinite busy wait in pl08x_start_txd() is
> guaranteed to be after a resume operation, or does it also need a
> timeout?

The channel should not be active at that point.  If it is, that means
we're trying to setup a new transfer on a physical channel on top of an
already active transfer, which obviously could result in the previous
transfer being destroyed.

Maybe a BUG_ON(pl08x_phy_channel_busy(phychan)) would be more appropriate
there - but I think that's something to address in future patches.

[PATCH 2/2] DMA: PL08x: fix channel pausing to timeout rather than lockup

[Dan Williams]

On Sun, Jan 30, 2011 at 8:53 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sun, Jan 30, 2011 at 08:48:27AM -0800, Dan Williams wrote:
>> On Thu, Jan 27, 2011 at 4:37 AM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > If a transfer is initiated from memory to a peripheral, then data is
>> > fetched and the channel is marked busy. ?This busy status persists until
>> > the HALT bit is set and the queued data has been transfered to the
>> > peripheral. ?Waiting indefinitely after setting the HALT bit results in
>> > system lockups. ?Timeout this operation, and print an error when this
>> > happens.
>> >
>> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
>> > ---
>> > This happens when a M->P transfer is started, but the peripheral doesn't
>> > take the data. ?The DMA controller will mark itself active and fetch data
>> > from memory, in anticipation of the peripheral wanting the data. ?This
>> > advances the source address.
>> >
>> > An alternative approach here is to just set the HALT bit and return
>> > immediately, without waiting for the primecell to show that it is no
>> > longer active. ?In any case, resuming the channel will allow the
>> > transfer to continue without data loss.
>> >
>>
>> Ok, and the other infinite busy wait in pl08x_start_txd() is
>> guaranteed to be after a resume operation, or does it also need a
>> timeout?
>
> The channel should not be active at that point. ?If it is, that means
> we're trying to setup a new transfer on a physical channel on top of an
> already active transfer, which obviously could result in the previous
> transfer being destroyed.
>
> Maybe a BUG_ON(pl08x_phy_channel_busy(phychan)) would be more appropriate
> there - but I think that's something to address in future patches.

Applied 1 and 2.

Thanks,
Dan