omap-dma + 8250_omap: fix the dmaengine_pause()

omap-dma + 8250_omap: fix the dmaengine_pause()

[Sebastian Andrzej Siewior]

#1 is something that can go stable and disables RX-DMA should it
   notice that it does not work.
#2 adds the anotation as suggest by Russell.
#3 adds the missing feature to omap-dma so dmaengine_pause() works. Once
   this is merged, the warning from #1 disappears.

[PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported

[Sebastian Andrzej Siewior]

The 8250-omap driver requires the DMA-engine driver to support the pause
command in order to properly turn off programmed RX transfer before the
driver stars manually reading from the FIFO.
The lacking support of the requirement has been discovered recently. In
order to stay safe here we disable support for RX-DMA as soon as we
notice that it does not work. This should happen very early.
If the user does not want to see this backtrace he can either disable
DMA support (completely or RX-only) or backport the required patches for
edma / omap-dma once they hit mainline.

Cc: <stable@vger.kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/tty/serial/8250/8250_omap.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index 0340ee6ba970..07a11e0935e4 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -112,6 +112,7 @@ struct omap8250_priv {
 	struct work_struct qos_work;
 	struct uart_8250_dma omap8250_dma;
 	spinlock_t rx_dma_lock;
+	bool rx_dma_broken;
 };
 
 static u32 uart_read(struct uart_8250_port *up, u32 reg)
@@ -761,6 +762,7 @@ static void omap_8250_rx_dma_flush(struct uart_8250_port *p)
 	struct omap8250_priv	*priv = p->port.private_data;
 	struct uart_8250_dma	*dma = p->dma;
 	unsigned long		flags;
+	int ret;
 
 	spin_lock_irqsave(&priv->rx_dma_lock, flags);
 
@@ -769,7 +771,9 @@ static void omap_8250_rx_dma_flush(struct uart_8250_port *p)
 		return;
 	}
 
-	dmaengine_pause(dma->rxchan);
+	ret = dmaengine_pause(dma->rxchan);
+	if (WARN_ON_ONCE(ret))
+		priv->rx_dma_broken = true;
 
 	spin_unlock_irqrestore(&priv->rx_dma_lock, flags);
 
@@ -813,6 +817,9 @@ static int omap_8250_rx_dma(struct uart_8250_port *p, unsigned int iir)
 		break;
 	}
 
+	if (priv->rx_dma_broken)
+		return -EINVAL;
+
 	spin_lock_irqsave(&priv->rx_dma_lock, flags);
 
 	if (dma->rx_running)
-- 
2.5.0

[PATCH 2/3] dma: add __must_check annotation for dmaengine_pause()

[Sebastian Andrzej Siewior]

In 8250-omap I learned it the hard way that ignoring the return code
of dmaengine_pause() might be bad because the underlying DMA driver
might not support the function at all and so not doing what one is
expecting.
This patch adds the __must_check annotation as suggested by Russell King.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 include/linux/dmaengine.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 8ad9a4e839f6..4eac4716bded 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -825,7 +825,7 @@ static inline int dmaengine_terminate_all(struct dma_chan *chan)
 	return -ENOSYS;
 }
 
-static inline int dmaengine_pause(struct dma_chan *chan)
+static inline int __must_check dmaengine_pause(struct dma_chan *chan)
 {
 	if (chan->device->device_pause)
 		return chan->device->device_pause(chan);
-- 
2.5.0

[PATCH v3 3/3] dma: omap-dma: add support for pause of non-cyclic transfers

[Sebastian Andrzej Siewior]

This DMA driver is used by 8250-omap on DRA7-evm. There is one
requirement that is to pause a transfer. This is currently used on the RX
side. It is possible that the UART HW aborted the RX (UART's RX-timeout)
but the DMA controller starts the transfer shortly after.
Before we can manually purge the FIFO we need to pause the transfer,
check how many bytes it already received and terminate the transfer
without it making any progress.

>From testing on the TX side it seems that it is possible that we invoke
pause once the transfer has completed which is indicated by the missing
CCR_ENABLE bit but before the interrupt has been noticed. In that case the
interrupt will come even after disabling it.

The AM572x manual says that we have to wait for the CCR_RD_ACTIVE &
CCR_WR_ACTIVE bits to be gone before programming it again here is the
drain loop. Also it looks like without the drain the TX-transfer makes
sometimes progress.

One note: The pause + resume combo is broken because after resume the
the complete transfer will be programmed again. That means the already
transferred bytes (until the pause event) will be sent again. This is
currently not important for my UART user because it does only pause +
terminate.

v2…v3:
  - rephrase the comment based on Russell's information / feedback.

v1…v2:
  - move the drain loop into omap_dma_drain_chan() instead of having it
    twice.
  - allow pause only for DMA_DEV_TO_MEM transfers if non-cyclic. Add a
    comment why DMA_MEM_TO_DEV not allowed.
  - clear pause on terminate_all. Otherwise pause() + terminate_all()
    will keep the pause bit set and we can't pause the following
    transfer.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/dma/omap-dma.c | 122 +++++++++++++++++++++++++++++++++++--------------
 1 file changed, 88 insertions(+), 34 deletions(-)

diff --git a/drivers/dma/omap-dma.c b/drivers/dma/omap-dma.c
index 249445c8a4c6..cb217fab472e 100644
--- a/drivers/dma/omap-dma.c
+++ b/drivers/dma/omap-dma.c
@@ -299,7 +299,30 @@ static void omap_dma_start(struct omap_chan *c, struct omap_desc *d)
 	omap_dma_chan_write(c, CCR, d->ccr | CCR_ENABLE);
 }
 
-static void omap_dma_stop(struct omap_chan *c)
+static void omap_dma_drain_chan(struct omap_chan *c)
+{
+	int i;
+	uint32_t val;
+
+	/* Wait for sDMA FIFO to drain */
+	for (i = 0; ; i++) {
+		val = omap_dma_chan_read(c, CCR);
+		if (!(val & (CCR_RD_ACTIVE | CCR_WR_ACTIVE)))
+			break;
+
+		if (i > 100)
+			break;
+
+		udelay(5);
+	}
+
+	if (val & (CCR_RD_ACTIVE | CCR_WR_ACTIVE))
+		dev_err(c->vc.chan.device->dev,
+			"DMA drain did not complete on lch %d\n",
+			c->dma_ch);
+}
+
+static int omap_dma_stop(struct omap_chan *c)
 {
 	struct omap_dmadev *od = to_omap_dma_dev(c->vc.chan.device);
 	uint32_t val;
@@ -312,7 +335,6 @@ static void omap_dma_stop(struct omap_chan *c)
 	val = omap_dma_chan_read(c, CCR);
 	if (od->plat->errata & DMA_ERRATA_i541 && val & CCR_TRIGGER_SRC) {
 		uint32_t sysconfig;
-		unsigned i;
 
 		sysconfig = omap_dma_glbl_read(od, OCP_SYSCONFIG);
 		val = sysconfig & ~DMA_SYSCONFIG_MIDLEMODE_MASK;
@@ -323,27 +345,18 @@ static void omap_dma_stop(struct omap_chan *c)
 		val &= ~CCR_ENABLE;
 		omap_dma_chan_write(c, CCR, val);
 
-		/* Wait for sDMA FIFO to drain */
-		for (i = 0; ; i++) {
-			val = omap_dma_chan_read(c, CCR);
-			if (!(val & (CCR_RD_ACTIVE | CCR_WR_ACTIVE)))
-				break;
-
-			if (i > 100)
-				break;
-
-			udelay(5);
-		}
-
-		if (val & (CCR_RD_ACTIVE | CCR_WR_ACTIVE))
-			dev_err(c->vc.chan.device->dev,
-				"DMA drain did not complete on lch %d\n",
-			        c->dma_ch);
+		omap_dma_drain_chan(c);
 
 		omap_dma_glbl_write(od, OCP_SYSCONFIG, sysconfig);
 	} else {
+
+		if (!(val & CCR_ENABLE))
+			return -EINVAL;
+
 		val &= ~CCR_ENABLE;
 		omap_dma_chan_write(c, CCR, val);
+
+		omap_dma_drain_chan(c);
 	}
 
 	mb();
@@ -358,6 +371,7 @@ static void omap_dma_stop(struct omap_chan *c)
 
 		omap_dma_chan_write(c, CLNK_CTRL, val);
 	}
+	return 0;
 }
 
 static void omap_dma_start_sg(struct omap_chan *c, struct omap_desc *d,
@@ -728,6 +742,8 @@ static enum dma_status omap_dma_tx_status(struct dma_chan *chan,
 	} else {
 		txstate->residue = 0;
 	}
+	if (ret == DMA_IN_PROGRESS && c->paused)
+		ret = DMA_PAUSED;
 	spin_unlock_irqrestore(&c->vc.lock, flags);
 
 	return ret;
@@ -1038,10 +1054,8 @@ static int omap_dma_terminate_all(struct dma_chan *chan)
 			omap_dma_stop(c);
 	}
 
-	if (c->cyclic) {
-		c->cyclic = false;
-		c->paused = false;
-	}
+	c->cyclic = false;
+	c->paused = false;
 
 	vchan_get_all_descriptors(&c->vc, &head);
 	spin_unlock_irqrestore(&c->vc.lock, flags);
@@ -1053,28 +1067,66 @@ static int omap_dma_terminate_all(struct dma_chan *chan)
 static int omap_dma_pause(struct dma_chan *chan)
 {
 	struct omap_chan *c = to_omap_dma_chan(chan);
+	struct omap_dmadev *od = to_omap_dma_dev(chan->device);
+	unsigned long flags;
+	int ret = -EINVAL;
+	bool can_pause;
 
-	/* Pause/Resume only allowed with cyclic mode */
-	if (!c->cyclic)
-		return -EINVAL;
+	spin_lock_irqsave(&od->irq_lock, flags);
 
-	if (!c->paused) {
-		omap_dma_stop(c);
-		c->paused = true;
+	if (!c->desc)
+		goto out;
+
+	if (c->cyclic)
+		can_pause = true;
+
+	/*
+	 * We do not allow DMA_MEM_TO_DEV transfers to be paused.
+	 * From the AM572x TRM, 16.1.4.18 Disabling a Channel During Transfer:
+	 * "When a channel is disabled during a transfer, the channel undergoes
+	 * an abort, unless it is hardware-source-synchronized …".
+	 * A source-synchronised channel is one where the fetching of data is
+	 * under control of the device. In other words, a device-to-memory
+	 * transfer. So, a destination-synchronised channel (which would be a
+	 * memory-to-device transfer) undergoes an abort if the the CCR_ENABLE
+	 * bit is cleared.
+	 * From 16.1.4.20.4.6.2 Abort: "If an abort trigger occurs, the channel
+	 * aborts immediately after completion of current read/write
+	 * transactions and then the FIFO is cleaned up." The term "cleaned up"
+	 * is not defined. TI recommends to check that RD_ACTIVE and WR_ACTIVE
+	 * are both clear _before_ disabling the channel, otherwise data loss
+	 * will occur.
+	 * The problem is that if the channel is active, then device activity
+	 * can result in DMA activity starting between reading those as both
+	 * clear and the write to DMA_CCR to clear the enable bit hitting the
+	 * hardware. If the DMA hardware can't drain the data in its FIFO to the
+	 * destination, then data loss "might" occur (say if we write to an UART
+	 * and the UART is not accepting any further data).
+	 */
+	else if (c->desc->dir == DMA_DEV_TO_MEM)
+		can_pause = true;
+
+	if (can_pause && !c->paused) {
+		ret = omap_dma_stop(c);
+		if (!ret)
+			c->paused = true;
 	}
+out:
+	spin_unlock_irqrestore(&od->irq_lock, flags);
 
-	return 0;
+	return ret;
 }
 
 static int omap_dma_resume(struct dma_chan *chan)
 {
 	struct omap_chan *c = to_omap_dma_chan(chan);
+	struct omap_dmadev *od = to_omap_dma_dev(chan->device);
+	unsigned long flags;
+	int ret = -EINVAL;
 
-	/* Pause/Resume only allowed with cyclic mode */
-	if (!c->cyclic)
-		return -EINVAL;
+	spin_lock_irqsave(&od->irq_lock, flags);
 
-	if (c->paused) {
+	if (c->paused && c->desc) {
 		mb();
 
 		/* Restore channel link register */
@@ -1082,9 +1134,11 @@ static int omap_dma_resume(struct dma_chan *chan)
 
 		omap_dma_start(c, c->desc);
 		c->paused = false;
+		ret = 0;
 	}
+	spin_unlock_irqrestore(&od->irq_lock, flags);
 
-	return 0;
+	return ret;
 }
 
 static int omap_dma_chan_init(struct omap_dmadev *od)
-- 
2.5.0

Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported

[Peter Hurley]

On 08/07/2015 04:00 PM, Sebastian Andrzej Siewior wrote:
> The 8250-omap driver requires the DMA-engine driver to support the pause
> command in order to properly turn off programmed RX transfer before the
> driver stars manually reading from the FIFO.
> The lacking support of the requirement has been discovered recently. In
> order to stay safe here we disable support for RX-DMA as soon as we
> notice that it does not work. This should happen very early.
> If the user does not want to see this backtrace he can either disable
> DMA support (completely or RX-only) or backport the required patches for
> edma / omap-dma once they hit mainline.
> 
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/tty/serial/8250/8250_omap.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> index 0340ee6ba970..07a11e0935e4 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -112,6 +112,7 @@ struct omap8250_priv {
>  	struct work_struct qos_work;
>  	struct uart_8250_dma omap8250_dma;
>  	spinlock_t rx_dma_lock;
> +	bool rx_dma_broken;
>  };
>  
>  static u32 uart_read(struct uart_8250_port *up, u32 reg)
> @@ -761,6 +762,7 @@ static void omap_8250_rx_dma_flush(struct uart_8250_port *p)
>  	struct omap8250_priv	*priv = p->port.private_data;
>  	struct uart_8250_dma	*dma = p->dma;
>  	unsigned long		flags;
> +	int ret;
>  
>  	spin_lock_irqsave(&priv->rx_dma_lock, flags);
>  
> @@ -769,7 +771,9 @@ static void omap_8250_rx_dma_flush(struct uart_8250_port *p)
>  		return;
>  	}
>  
> -	dmaengine_pause(dma->rxchan);
> +	ret = dmaengine_pause(dma->rxchan);
> +	if (WARN_ON_ONCE(ret))
> +		priv->rx_dma_broken = true;

No offense, Sebastian, but it boggles my mind that anyone could defend this
as solid api design. We're in the middle of an interrupt handler and the
slave dma driver is /just/ telling us now that it doesn't implement this
functionality?!!?

The dmaengine api has _so much_ setup and none of it contemplates telling the
consumer that critical functionality is missing?

Even dma_get_slave_caps() returns _true_ for cmd_pause support; ok, that
interface is pointless.

Rather than losing /critical data/ here, the interrupt handler should just
busy-wait until dmaengine_tx_status() returns DMA_COMPLETE for the rx_cookie.

Regards,
Peter Hurley

>  	spin_unlock_irqrestore(&priv->rx_dma_lock, flags);
>  
> @@ -813,6 +817,9 @@ static int omap_8250_rx_dma(struct uart_8250_port *p, unsigned int iir)
>  		break;
>  	}
>  
> +	if (priv->rx_dma_broken)
> +		return -EINVAL;
> +
>  	spin_lock_irqsave(&priv->rx_dma_lock, flags);
>  
>  	if (dma->rx_running)
> 

Re: [PATCH 2/3] dma: add __must_check annotation for dmaengine_pause()

[Peter Hurley]

On 08/07/2015 04:00 PM, Sebastian Andrzej Siewior wrote:
> In 8250-omap I learned it the hard way that ignoring the return code
> of dmaengine_pause() might be bad because the underlying DMA driver
> might not support the function at all and so not doing what one is
> expecting.
> This patch adds the __must_check annotation as suggested by Russell King.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  include/linux/dmaengine.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index 8ad9a4e839f6..4eac4716bded 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -825,7 +825,7 @@ static inline int dmaengine_terminate_all(struct dma_chan *chan)
>  	return -ENOSYS;
>  }
>  
> -static inline int dmaengine_pause(struct dma_chan *chan)
> +static inline int __must_check dmaengine_pause(struct dma_chan *chan)
>  {
>  	if (chan->device->device_pause)
>  		return chan->device->device_pause(chan);
> 

Not that this is your responsibility, Sebastian, but considering there are
fewer than 20 users of dmaengine_pause() in the entire tree, we should add
WARN_ON_ONCE() around those uses with this patch to avoid a bunch needless
one-off "fixes".

Regards,
Peter Hurley

Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported

[Russell King - ARM Linux]

On Fri, Aug 07, 2015 at 08:28:57PM -0400, Peter Hurley wrote:
> Even dma_get_slave_caps() returns _true_ for cmd_pause support; ok, that
> interface is pointless.

How about reporting that as a bug then, because if you look back in the
git history, as you are fully capable of, you will find that the slave
capability stuff went in _after_ omap-dma, and *many* DMA engine drivers
have not been updated.  Here, let me do _your_ work for you:

commit cb8cea513c80db1dfe2dce468d2d0772005bb9a1
Author: Maxime Ripard <maxime.ripard@free-electrons.com>
Date:   Mon Nov 17 14:42:04 2014 +0100

    dmaengine: Create a generic dma_slave_caps callback

commit 2dcdf570936168d488acf90be9b04a3d32dafce7
Author: Peter Ujfalusi <peter.ujfalusi@ti.com>
Date:   Fri Sep 14 15:05:45 2012 +0300

    dmaengine: omap: Add support for pause/resume in cyclic dma mode

Oh look, omap-dma pre-dates the creation of dma_slave_caps by over two
years!

However, it's *not* as trivial as you think: the dma_slave_caps() call
has no knowledge whether a channel will be used in cyclic mode or not,
or which direction it will be used.  So, it really _can't_ report
whether pause mode is supported or not.  So actually, this is something
that can't be fixed trivially, and was a detail missed when the slave
caps was reviewed (I probably missed the review or missed the point.)

So, how about you stop playing the blame game and trying to shovel your
shit onto DMA engine.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported

[Sebastian Andrzej Siewior]

On 08/08/2015 02:28 AM, Peter Hurley wrote:
>> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
>> index 0340ee6ba970..07a11e0935e4 100644
>> --- a/drivers/tty/serial/8250/8250_omap.c
>> +++ b/drivers/tty/serial/8250/8250_omap.c
>> @@ -769,7 +771,9 @@ static void omap_8250_rx_dma_flush(struct uart_8250_port *p)
>>  		return;
>>  	}
>>  
>> -	dmaengine_pause(dma->rxchan);
>> +	ret = dmaengine_pause(dma->rxchan);
>> +	if (WARN_ON_ONCE(ret))
>> +		priv->rx_dma_broken = true;
> 
> No offense, Sebastian, but it boggles my mind that anyone could defend this
> as solid api design. We're in the middle of an interrupt handler and the
> slave dma driver is /just/ telling us now that it doesn't implement this
> functionality?!!?

This is the best thing I could come up with. But to be fair: This
happens once _very_ early in the process and is 100% reproducible. The
WARN_ON should trigger user attention.

> The dmaengine api has _so much_ setup and none of it contemplates telling the
> consumer that critical functionality is missing?

Lets say it is a missing feature which was not noticed / required until
now. I have the missing functionality in patch #3. I don't see the
point in adding a lookup API for this feature which has to be
backported stable just to figure out that RX-DMA might not work.
Again: the WARN_ON triggers on the first short RX read _or_ on shutdown
of the port which is not after hours using the port.

> Even dma_get_slave_caps() returns _true_ for cmd_pause support; ok, that
> interface is pointless.
> 
> Rather than losing /critical data/ here, the interrupt handler should just
> busy-wait until dmaengine_tx_status() returns DMA_COMPLETE for the rx_cookie.

This might not happen at all. At 115200 I *never* encouraged this. Once
the FIFO is filled with less than RX-trigger size than the UART sends
the time-out interrupt and the DMA *never* completes. Even if the FIFO
reaches trigger size later. So you have to abort the DMA transfer and
read data manually from the FIFO. That is why the omap enqueues the
RX-transfer upfront (while the FIFO is still empty).

It happens *sometimes* that the UART sends a timeout interrupt and the
DMA transfer just is started and it has been only observed at 3mbaud
(but there were no tests 115200 > x > 3mbaud that I know of).

Therefor I think this is a fair fix without hiding anything.

> Regards,
> Peter Hurley

Sebastian

Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported

[Russell King - ARM Linux]

On Sat, Aug 08, 2015 at 11:32:05AM +0200, Sebastian Andrzej Siewior wrote:
> This might not happen at all. At 115200 I *never* encouraged this. Once
> the FIFO is filled with less than RX-trigger size than the UART sends
> the time-out interrupt and the DMA *never* completes.

Careful with statements like that, they have a habbit of backfiring :)

It just needs the right timing - you need a character sequence which
has a pause long enough to trigger the timeout, but short enough to
get characters in between the UART deciding to report a timeout, and
the DMA being terminated.

You'll then be in the situation where the UART is receiving characters
and using the DMA, but you've been interrupted to handle the RX timeout
event.  Whether the UART resets the IIR in that situation to indicate
something other than a RX timeout, I'm not sure - if it doesn't, then
there's a race there (which from your behaviour at faster rates seems
likely.)

The more loaded the system, the longer an IRQ may take to be serviced
(especially if there are interrupt handlers which aren't fast) and the
bigger the window is for that to happen.  I've seen some long service
times with USB with HID in older kernels (milliseconds), but that seems
to have been fixed now - longest IRQ handling I'm now seeing is around
500us.  Given 115200 baud, the character time is 87us, that's probably
more than enough if you get the timing just right.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported

[Greg KH]

On Sat, Aug 08, 2015 at 11:32:05AM +0200, Sebastian Andrzej Siewior wrote:
> On 08/08/2015 02:28 AM, Peter Hurley wrote:
> >> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> >> index 0340ee6ba970..07a11e0935e4 100644
> >> --- a/drivers/tty/serial/8250/8250_omap.c
> >> +++ b/drivers/tty/serial/8250/8250_omap.c
> >> @@ -769,7 +771,9 @@ static void omap_8250_rx_dma_flush(struct uart_8250_port *p)
> >>  		return;
> >>  	}
> >>  
> >> -	dmaengine_pause(dma->rxchan);
> >> +	ret = dmaengine_pause(dma->rxchan);
> >> +	if (WARN_ON_ONCE(ret))
> >> +		priv->rx_dma_broken = true;
> > 
> > No offense, Sebastian, but it boggles my mind that anyone could defend this
> > as solid api design. We're in the middle of an interrupt handler and the
> > slave dma driver is /just/ telling us now that it doesn't implement this
> > functionality?!!?
> 
> This is the best thing I could come up with. But to be fair: This
> happens once _very_ early in the process and is 100% reproducible. The
> WARN_ON should trigger user attention.

Never expect a _user_ to do anything with a WARN_ON except ignore it.

WARN_ON is for developers when something really bad went wrong that you
want to be notified so that you can fix the code to prevent that from
ever happening again.

So this isn't ok, sorry, a user would not know what to do with this at
all except email me asking why the driver is broken because a kernel
"oops" just happened, when in reality, their hardware is broken in a way
that you already know about when you wrote the patch.

You know better than this.

greg k-h

Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported

[Peter Ujfalusi]

On 08/07/2015 11:00 PM, Sebastian Andrzej Siewior wrote:
> The 8250-omap driver requires the DMA-engine driver to support the pause
> command in order to properly turn off programmed RX transfer before the
> driver stars manually reading from the FIFO.
> The lacking support of the requirement has been discovered recently. In
> order to stay safe here we disable support for RX-DMA as soon as we
> notice that it does not work. This should happen very early.
> If the user does not want to see this backtrace he can either disable
> DMA support (completely or RX-only) or backport the required patches for
> edma / omap-dma once they hit mainline.
> 
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/tty/serial/8250/8250_omap.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> index 0340ee6ba970..07a11e0935e4 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -112,6 +112,7 @@ struct omap8250_priv {
>  	struct work_struct qos_work;
>  	struct uart_8250_dma omap8250_dma;
>  	spinlock_t rx_dma_lock;
> +	bool rx_dma_broken;
>  };
>  
>  static u32 uart_read(struct uart_8250_port *up, u32 reg)
> @@ -761,6 +762,7 @@ static void omap_8250_rx_dma_flush(struct uart_8250_port *p)
>  	struct omap8250_priv	*priv = p->port.private_data;
>  	struct uart_8250_dma	*dma = p->dma;
>  	unsigned long		flags;
> +	int ret;
>  
>  	spin_lock_irqsave(&priv->rx_dma_lock, flags);
>  
> @@ -769,7 +771,9 @@ static void omap_8250_rx_dma_flush(struct uart_8250_port *p)
>  		return;
>  	}
>  
> -	dmaengine_pause(dma->rxchan);
> +	ret = dmaengine_pause(dma->rxchan);
> +	if (WARN_ON_ONCE(ret))
> +		priv->rx_dma_broken = true;

I don't think this is good thing for the stable _and_ for the mainline at the
same time:
in stable the rx DMA should not be allowed since the stable kernels does not
allow pause/resume with omap-dma, so there the rx DMA should be just disabled
for UART. This change will cause regression since it introduce a WARN_ON_ONCE,
which will be printed if the user tries to use non working feature.

In mainline you will eventually going to have pause/resume support so this
patch will make no sense there.

>  
>  	spin_unlock_irqrestore(&priv->rx_dma_lock, flags);
>  
> @@ -813,6 +817,9 @@ static int omap_8250_rx_dma(struct uart_8250_port *p, unsigned int iir)
>  		break;
>  	}
>  
> +	if (priv->rx_dma_broken)
> +		return -EINVAL;
> +
>  	spin_lock_irqsave(&priv->rx_dma_lock, flags);
>  
>  	if (dma->rx_running)
> 


-- 
Péter

Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported

[Sebastian Andrzej Siewior]

On 08/10/2015 01:54 PM, Peter Ujfalusi wrote:
>> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
>> index 0340ee6ba970..07a11e0935e4 100644
>> --- a/drivers/tty/serial/8250/8250_omap.c
>> @@ -769,7 +771,9 @@ static void omap_8250_rx_dma_flush(struct uart_8250_port *p)
>>  		return;
>>  	}
>>  
>> -	dmaengine_pause(dma->rxchan);
>> +	ret = dmaengine_pause(dma->rxchan);
>> +	if (WARN_ON_ONCE(ret))
>> +		priv->rx_dma_broken = true;
> 
> I don't think this is good thing for the stable _and_ for the mainline at the
> same time:
> in stable the rx DMA should not be allowed since the stable kernels does not
> allow pause/resume with omap-dma, so there the rx DMA should be just disabled
> for UART. This change will cause regression since it introduce a WARN_ON_ONCE,
> which will be printed if the user tries to use non working feature.

Okay. We do have pause support in mainline for edma since v4.2-rc1.
This driver can use edma or sdma depending on the configuration. But it
is not yet released. So you suggest remove RX-DMA support completely
from the 8250-omap, mark it stable, and revert that patch once we have
it fixed in sdma?

> In mainline you will eventually going to have pause/resume support so this
> patch will make no sense there.

The way this works is that it has to be fixed upstream before it can be
backported stable. Also Russell made clear (for a good reason) that the
RX problem has to be fixed upstream before he thinks about acking the
SDMA patch.
So if you prefer to instead remove RX-DMA until it is fixed, I am all
yours.


Sebastian

Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported

[Peter Hurley]

On 08/10/2015 07:54 AM, Peter Ujfalusi wrote:
> On 08/07/2015 11:00 PM, Sebastian Andrzej Siewior wrote:

> I don't think this is good thing for the stable _and_ for the mainline at the
> same time:
> in stable the rx DMA should not be allowed since the stable kernels does not
> allow pause/resume with omap-dma, so there the rx DMA should be just disabled
> for UART. This change will cause regression since it introduce a WARN_ON_ONCE,
> which will be printed if the user tries to use non working feature.
> 
> In mainline you will eventually going to have pause/resume support so this
> patch will make no sense there.

No, the whole point of this mess is that omap-dma does not provide pause/resume
support (without data loss). omap-dma will only be suitable for pause/terminate dma.

And adding pause support doesn't address the underlying problem that dmaengine
is not providing a means of determining if suitable support is available for
use by serial drivers, so this same problem is just waiting to happen again.

I think the way forward is,

For -stable, disable dma in the 8250_omap driver.
Then for mainline,
* extend the dma_get_slave_caps() api to differentiate the types of pause support
* query dma_get_slave_caps() when setting up the slave channel in 8250_dma & 8250_omap
  and only enable dma if suitable pause support is available
* add required dmaengine error checking in 8250_dma & 8250_omap _for unexpected errors_
  (so _not_ WARNs)
* do whatever with omap-dma. Not even sure it's worth trying to support dma with that;
  it still won't fully support tx dma which is forcing all kinds of goofy workarounds


Russell seemed to think that the current dma operation was necessary information to
differentiate types of pause support, but I don't think that's required.
As Sebastian's omap-dma driver patch shows, partial pause support has more
to do with how it's being used.

Regards,
Peter Hurley

Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported

[Russell King - ARM Linux]

On Mon, Aug 10, 2015 at 09:00:29AM -0400, Peter Hurley wrote:
> Russell seemed to think that the current dma operation was necessary information to
> differentiate types of pause support, but I don't think that's required.
> As Sebastian's omap-dma driver patch shows, partial pause support has more
> to do with how it's being used.

Do you think you can rewrite the first sentence above in gramatically
correct English please, I'm failing to understand what you're saying.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported

[Vinod Koul]

On Sat, Aug 08, 2015 at 10:03:43AM +0100, Russell King - ARM Linux wrote:
> On Fri, Aug 07, 2015 at 08:28:57PM -0400, Peter Hurley wrote:
> > Even dma_get_slave_caps() returns _true_ for cmd_pause support; ok, that
> > interface is pointless.
> 
> How about reporting that as a bug then, because if you look back in the
> git history, as you are fully capable of, you will find that the slave
> capability stuff went in _after_ omap-dma, and *many* DMA engine drivers
> have not been updated.  Here, let me do _your_ work for you:
> 
> commit cb8cea513c80db1dfe2dce468d2d0772005bb9a1
> Author: Maxime Ripard <maxime.ripard@free-electrons.com>
> Date:   Mon Nov 17 14:42:04 2014 +0100
> 
>     dmaengine: Create a generic dma_slave_caps callback
> 
> commit 2dcdf570936168d488acf90be9b04a3d32dafce7
> Author: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Date:   Fri Sep 14 15:05:45 2012 +0300
> 
>     dmaengine: omap: Add support for pause/resume in cyclic dma mode
> 
> Oh look, omap-dma pre-dates the creation of dma_slave_caps by over two
> years!
> 
> However, it's *not* as trivial as you think: the dma_slave_caps() call
> has no knowledge whether a channel will be used in cyclic mode or not,
> or which direction it will be used.  So, it really _can't_ report
> whether pause mode is supported or not.  So actually, this is something
> that can't be fixed trivially, and was a detail missed when the slave
> caps was reviewed (I probably missed the review or missed the point.)

The API queries the capabilities for a channel. So a channel has been
allocated. BUT it would not imply the direction as that is descriptor based,
so should we query the capabilities for a descriptor and use those in
clients ?

Also the current dma_slave_caps() has been moved to framework and reports
based on driver callbacks.
Now we have a hardware which supports pause for one direction only so we
should model it bit differently

Thoughts... ??

-- 
~Vinod

Re: [PATCH 2/3] dma: add __must_check annotation for dmaengine_pause()

[Vinod Koul]

On Fri, Aug 07, 2015 at 10:00:18PM +0200, Sebastian Andrzej Siewior wrote:
> In 8250-omap I learned it the hard way that ignoring the return code
> of dmaengine_pause() might be bad because the underlying DMA driver
> might not support the function at all and so not doing what one is
> expecting.
> This patch adds the __must_check annotation as suggested by Russell King.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  include/linux/dmaengine.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index 8ad9a4e839f6..4eac4716bded 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -825,7 +825,7 @@ static inline int dmaengine_terminate_all(struct dma_chan *chan)
>  	return -ENOSYS;
>  }
>  
> -static inline int dmaengine_pause(struct dma_chan *chan)
> +static inline int __must_check dmaengine_pause(struct dma_chan *chan)
>  {
>  	if (chan->device->device_pause)
>  		return chan->device->device_pause(chan);

Give that there are bunch of users of this call which may or maynot be using
this, I think putting this check is too stringent

-- 
~Vinod

Re: [PATCH 2/3] dma: add __must_check annotation for dmaengine_pause()

[Russell King - ARM Linux]

On Tue, Aug 11, 2015 at 03:28:52PM +0530, Vinod Koul wrote:
> On Fri, Aug 07, 2015 at 10:00:18PM +0200, Sebastian Andrzej Siewior wrote:
> > In 8250-omap I learned it the hard way that ignoring the return code
> > of dmaengine_pause() might be bad because the underlying DMA driver
> > might not support the function at all and so not doing what one is
> > expecting.
> > This patch adds the __must_check annotation as suggested by Russell King.
> > 
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > ---
> >  include/linux/dmaengine.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> > index 8ad9a4e839f6..4eac4716bded 100644
> > --- a/include/linux/dmaengine.h
> > +++ b/include/linux/dmaengine.h
> > @@ -825,7 +825,7 @@ static inline int dmaengine_terminate_all(struct dma_chan *chan)
> >  	return -ENOSYS;
> >  }
> >  
> > -static inline int dmaengine_pause(struct dma_chan *chan)
> > +static inline int __must_check dmaengine_pause(struct dma_chan *chan)
> >  {
> >  	if (chan->device->device_pause)
> >  		return chan->device->device_pause(chan);
> 
> Give that there are bunch of users of this call which may or maynot be using
> this, I think putting this check is too stringent

I think what people need to learn is that an API in the kernel which
returns an int _can_ fail - it returns an int so it _can_ return an
error code.  If it _can_ return an error code, there _will_ be
implementations which _do_.

If you don't check the return code, either your code doesn't care whether
the function was successful or not, or you're playing with fire.  This is
a prime example of playing with fire.

Let's leave the crappy userspace laziness with regard to error checking
to userspace, and keep it out of the kernel.

Yes, the DMA engine capabilities may not be sufficient to describe every
detail of DMA engines, but that's absolutely no reason to skimp on error
checking.  Had there been some kind of error checking at the site, this
problem would have been spotted before the 8250-omap driver was merged.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v3 3/3] dma: omap-dma: add support for pause of non-cyclic transfers

[Peter Ujfalusi]

On 08/07/2015 11:00 PM, Sebastian Andrzej Siewior wrote:
> -static void omap_dma_stop(struct omap_chan *c)
> +static void omap_dma_drain_chan(struct omap_chan *c)
> +{
> +	int i;
> +	uint32_t val;
> +
> +	/* Wait for sDMA FIFO to drain */
> +	for (i = 0; ; i++) {
> +		val = omap_dma_chan_read(c, CCR);
> +		if (!(val & (CCR_RD_ACTIVE | CCR_WR_ACTIVE)))
> +			break;
> +
> +		if (i > 100)
> +			break;
> +
> +		udelay(5);
> +	}
> +
> +	if (val & (CCR_RD_ACTIVE | CCR_WR_ACTIVE))
> +		dev_err(c->vc.chan.device->dev,
> +			"DMA drain did not complete on lch %d\n",
> +			c->dma_ch);
> +}
> +
> +static int omap_dma_stop(struct omap_chan *c)
>  {
>  	struct omap_dmadev *od = to_omap_dma_dev(c->vc.chan.device);
>  	uint32_t val;
> @@ -312,7 +335,6 @@ static void omap_dma_stop(struct omap_chan *c)
>  	val = omap_dma_chan_read(c, CCR);
>  	if (od->plat->errata & DMA_ERRATA_i541 && val & CCR_TRIGGER_SRC) {
>  		uint32_t sysconfig;
> -		unsigned i;
>  
>  		sysconfig = omap_dma_glbl_read(od, OCP_SYSCONFIG);
>  		val = sysconfig & ~DMA_SYSCONFIG_MIDLEMODE_MASK;
> @@ -323,27 +345,18 @@ static void omap_dma_stop(struct omap_chan *c)
>  		val &= ~CCR_ENABLE;
>  		omap_dma_chan_write(c, CCR, val);
>  
> -		/* Wait for sDMA FIFO to drain */
> -		for (i = 0; ; i++) {
> -			val = omap_dma_chan_read(c, CCR);
> -			if (!(val & (CCR_RD_ACTIVE | CCR_WR_ACTIVE)))
> -				break;
> -
> -			if (i > 100)
> -				break;
> -
> -			udelay(5);
> -		}
> -
> -		if (val & (CCR_RD_ACTIVE | CCR_WR_ACTIVE))
> -			dev_err(c->vc.chan.device->dev,
> -				"DMA drain did not complete on lch %d\n",
> -			        c->dma_ch);
> +		omap_dma_drain_chan(c);
>  
>  		omap_dma_glbl_write(od, OCP_SYSCONFIG, sysconfig);
>  	} else {
> +
> +		if (!(val & CCR_ENABLE))
> +			return -EINVAL;
> +
>  		val &= ~CCR_ENABLE;
>  		omap_dma_chan_write(c, CCR, val);
> +
> +		omap_dma_drain_chan(c);

Note that the FIFO drain only applies to source synchronized transfers... When
the BUFFERING is _not_ disabled - in most of the cases this is true.

> +	/*
> +	 * We do not allow DMA_MEM_TO_DEV transfers to be paused.
> +	 * From the AM572x TRM, 16.1.4.18 Disabling a Channel During Transfer:
> +	 * "When a channel is disabled during a transfer, the channel undergoes
> +	 * an abort, unless it is hardware-source-synchronized …".
> +	 * A source-synchronised channel is one where the fetching of data is
> +	 * under control of the device. In other words, a device-to-memory
> +	 * transfer. So, a destination-synchronised channel (which would be a
> +	 * memory-to-device transfer) undergoes an abort if the the CCR_ENABLE
> +	 * bit is cleared.
> +	 * From 16.1.4.20.4.6.2 Abort: "If an abort trigger occurs, the channel
> +	 * aborts immediately after completion of current read/write
> +	 * transactions and then the FIFO is cleaned up." The term "cleaned up"
> +	 * is not defined. TI recommends to check that RD_ACTIVE and WR_ACTIVE
> +	 * are both clear _before_ disabling the channel, otherwise data loss
> +	 * will occur.
> +	 * The problem is that if the channel is active, then device activity
> +	 * can result in DMA activity starting between reading those as both
> +	 * clear and the write to DMA_CCR to clear the enable bit hitting the
> +	 * hardware. If the DMA hardware can't drain the data in its FIFO to the
> +	 * destination, then data loss "might" occur (say if we write to an UART
> +	 * and the UART is not accepting any further data).

I don't know if you have checked it, but probably the TX DMA could be also
used when the PRZEFETCH is disabled for the channel? Just a guess

> +	 */
> +	else if (c->desc->dir == DMA_DEV_TO_MEM)
> +		can_pause = true;
> +
> +	if (can_pause && !c->paused) {
> +		ret = omap_dma_stop(c);
> +		if (!ret)
> +			c->paused = true;
>  	}
> +out:
> +	spin_unlock_irqrestore(&od->irq_lock, flags);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int omap_dma_resume(struct dma_chan *chan)
>  {
>  	struct omap_chan *c = to_omap_dma_chan(chan);
> +	struct omap_dmadev *od = to_omap_dma_dev(chan->device);
> +	unsigned long flags;
> +	int ret = -EINVAL;
>  
> -	/* Pause/Resume only allowed with cyclic mode */
> -	if (!c->cyclic)
> -		return -EINVAL;
> +	spin_lock_irqsave(&od->irq_lock, flags);
>  
> -	if (c->paused) {
> +	if (c->paused && c->desc) {
>  		mb();
>  
>  		/* Restore channel link register */
> @@ -1082,9 +1134,11 @@ static int omap_dma_resume(struct dma_chan *chan)
>  
>  		omap_dma_start(c, c->desc);
>  		c->paused = false;
> +		ret = 0;
>  	}
> +	spin_unlock_irqrestore(&od->irq_lock, flags);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int omap_dma_chan_init(struct omap_dmadev *od)
> 


-- 
Péter

Re: [PATCH v3 3/3] dma: omap-dma: add support for pause of non-cyclic transfers

[Russell King - ARM Linux]

On Tue, Aug 11, 2015 at 03:02:44PM +0300, Peter Ujfalusi wrote:
> On 08/07/2015 11:00 PM, Sebastian Andrzej Siewior wrote:
> > +	/*
> > +	 * We do not allow DMA_MEM_TO_DEV transfers to be paused.
> > +	 * From the AM572x TRM, 16.1.4.18 Disabling a Channel During Transfer:
> > +	 * "When a channel is disabled during a transfer, the channel undergoes
> > +	 * an abort, unless it is hardware-source-synchronized …".
> > +	 * A source-synchronised channel is one where the fetching of data is
> > +	 * under control of the device. In other words, a device-to-memory
> > +	 * transfer. So, a destination-synchronised channel (which would be a
> > +	 * memory-to-device transfer) undergoes an abort if the the CCR_ENABLE
> > +	 * bit is cleared.
> > +	 * From 16.1.4.20.4.6.2 Abort: "If an abort trigger occurs, the channel
> > +	 * aborts immediately after completion of current read/write
> > +	 * transactions and then the FIFO is cleaned up." The term "cleaned up"
> > +	 * is not defined. TI recommends to check that RD_ACTIVE and WR_ACTIVE
> > +	 * are both clear _before_ disabling the channel, otherwise data loss
> > +	 * will occur.
> > +	 * The problem is that if the channel is active, then device activity
> > +	 * can result in DMA activity starting between reading those as both
> > +	 * clear and the write to DMA_CCR to clear the enable bit hitting the
> > +	 * hardware. If the DMA hardware can't drain the data in its FIFO to the
> > +	 * destination, then data loss "might" occur (say if we write to an UART
> > +	 * and the UART is not accepting any further data).
> 
> I don't know if you have checked it, but probably the TX DMA could be also
> used when the PRZEFETCH is disabled for the channel? Just a guess

The docs aren't very clear on that... and iirc Santosh's reply didn't
suggest that the prefetch bit had any influence on this behaviour.  Given
the wording in the documentation which seems to be quite explicit about
the conditions, and it omits talking about the prefetch bit, I can only
assume that the prefetch bit has no influence over this behaviour.

For example, what happens if the DMA to the device has started - the
device has raised its DMA request line.  The DMA controller has then gone
to memory and has fetched some data and incremented the source address.
Meanwhile, we've cleared the ENABLE bit.  What happens then?  Does the
DMA controller drain the read data to the device, or does it "clean up"
the FIFO by discarding the data?

Given that the conditions under which the FIFO is drained to the
destination are very specific, and which explicitly excludes destination-
synchronised transfers, the only conclusion that's possible without
knowing the implementation intimately is that the FIFO is "cleaned up"
which suggests that it's discarded rather than drained to the destination.

As this DMA controller is in all of the OMAP devices and similar, I
don't think we can rely on the behaviour of any one implementation
either - we don't know what the differences are between the
implementations in different generations of devices without TI providing
more detailed documentation in this area across their various devices.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 2/3] dma: add __must_check annotation for dmaengine_pause()

[Sebastian Andrzej Siewior]

On 08/11/2015 12:06 PM, Russell King - ARM Linux wrote:
> I think what people need to learn is that an API in the kernel which
> returns an int _can_ fail - it returns an int so it _can_ return an
> error code.  If it _can_ return an error code, there _will_ be
> implementations which _do_.
> 
> If you don't check the return code, either your code doesn't care whether
> the function was successful or not, or you're playing with fire.  This is
> a prime example of playing with fire.
> 
> Let's leave the crappy userspace laziness with regard to error checking
> to userspace, and keep it out of the kernel.
> 
> Yes, the DMA engine capabilities may not be sufficient to describe every
> detail of DMA engines, but that's absolutely no reason to skimp on error
> checking.  Had there been some kind of error checking at the site, this
> problem would have been spotted before the 8250-omap driver was merged.

Let me disable RX-DMA in 8250-omap code and push that stable. Then we
won't need a special annotation for pause support because it remains
off and is currently about one user. I browsed each driver in
drivers/dma each one which does support pause supports it and all of
them implement it unconditionally (ipu_idmac grabs a mutex first but
this is another story).
Adding error checking to 8250-omap like I have it in #1 and disabling
RX-DMA in case pause fails looks be reasonable since there is nothing
else that can be done I guess.
Once we have the missing piece in omap-dma the RX-DMA can be enabled in
8250-omap.
Does this sound like a plan we can agree on?

Sebastian

Re: [PATCH v3 3/3] dma: omap-dma: add support for pause of non-cyclic transfers

[Peter Ujfalusi]

On 08/11/2015 03:30 PM, Russell King - ARM Linux wrote:
> On Tue, Aug 11, 2015 at 03:02:44PM +0300, Peter Ujfalusi wrote:
>> On 08/07/2015 11:00 PM, Sebastian Andrzej Siewior wrote:
>>> +	/*
>>> +	 * We do not allow DMA_MEM_TO_DEV transfers to be paused.
>>> +	 * From the AM572x TRM, 16.1.4.18 Disabling a Channel During Transfer:
>>> +	 * "When a channel is disabled during a transfer, the channel undergoes
>>> +	 * an abort, unless it is hardware-source-synchronized …".
>>> +	 * A source-synchronised channel is one where the fetching of data is
>>> +	 * under control of the device. In other words, a device-to-memory
>>> +	 * transfer. So, a destination-synchronised channel (which would be a
>>> +	 * memory-to-device transfer) undergoes an abort if the the CCR_ENABLE
>>> +	 * bit is cleared.
>>> +	 * From 16.1.4.20.4.6.2 Abort: "If an abort trigger occurs, the channel
>>> +	 * aborts immediately after completion of current read/write
>>> +	 * transactions and then the FIFO is cleaned up." The term "cleaned up"
>>> +	 * is not defined. TI recommends to check that RD_ACTIVE and WR_ACTIVE
>>> +	 * are both clear _before_ disabling the channel, otherwise data loss
>>> +	 * will occur.
>>> +	 * The problem is that if the channel is active, then device activity
>>> +	 * can result in DMA activity starting between reading those as both
>>> +	 * clear and the write to DMA_CCR to clear the enable bit hitting the
>>> +	 * hardware. If the DMA hardware can't drain the data in its FIFO to the
>>> +	 * destination, then data loss "might" occur (say if we write to an UART
>>> +	 * and the UART is not accepting any further data).
>>
>> I don't know if you have checked it, but probably the TX DMA could be also
>> used when the PRZEFETCH is disabled for the channel? Just a guess
> 
> The docs aren't very clear on that... and iirc Santosh's reply didn't
> suggest that the prefetch bit had any influence on this behaviour.  Given
> the wording in the documentation which seems to be quite explicit about
> the conditions, and it omits talking about the prefetch bit, I can only
> assume that the prefetch bit has no influence over this behaviour.
> 
> For example, what happens if the DMA to the device has started - the
> device has raised its DMA request line.  The DMA controller has then gone
> to memory and has fetched some data and incremented the source address.
> Meanwhile, we've cleared the ENABLE bit.  What happens then?  Does the
> DMA controller drain the read data to the device, or does it "clean up"
> the FIFO by discarding the data?

Hrm, yes. If we do not have prefetch and the destination is using FIFO - so
DMA pushes multiple elemets per DMA request this might be the case. But I
think - nothing backs this up - if the transfer is element syncronized than we
would not loose data if the prefetch is not enabled. If we have prefetch then
the DMA is prefetching data to it's FIFO and an abort will just send the
content of the FIFO to /dev/null (or something).

> Given that the conditions under which the FIFO is drained to the
> destination are very specific, and which explicitly excludes destination-
> synchronised transfers, the only conclusion that's possible without
> knowing the implementation intimately is that the FIFO is "cleaned up"
> which suggests that it's discarded rather than drained to the destination.

Yes, the wording is not explicit and this is also my take on the issue - the
content of the FIFO is just dropped after it finished the ongoing element.

> As this DMA controller is in all of the OMAP devices and similar, I
> don't think we can rely on the behaviour of any one implementation
> either - we don't know what the differences are between the
> implementations in different generations of devices without TI providing
> more detailed documentation in this area across their various devices.

Yep. Let me try to get more information if I can.

-- 
Péter

Re: [PATCH 2/3] dma: add __must_check annotation for dmaengine_pause()

[Vinod Koul]

On Tue, Aug 11, 2015 at 02:34:29PM +0200, Sebastian Andrzej Siewior wrote:
> On 08/11/2015 12:06 PM, Russell King - ARM Linux wrote:
> > I think what people need to learn is that an API in the kernel which
> > returns an int _can_ fail - it returns an int so it _can_ return an
> > error code.  If it _can_ return an error code, there _will_ be
> > implementations which _do_.
> > 
> > If you don't check the return code, either your code doesn't care whether
> > the function was successful or not, or you're playing with fire.  This is
> > a prime example of playing with fire.
> > 
> > Let's leave the crappy userspace laziness with regard to error checking
> > to userspace, and keep it out of the kernel.
> > 
> > Yes, the DMA engine capabilities may not be sufficient to describe every
> > detail of DMA engines, but that's absolutely no reason to skimp on error
> > checking.  Had there been some kind of error checking at the site, this
> > problem would have been spotted before the 8250-omap driver was merged.
> 
> Let me disable RX-DMA in 8250-omap code and push that stable. Then we
> won't need a special annotation for pause support because it remains
> off and is currently about one user. I browsed each driver in
> drivers/dma each one which does support pause supports it and all of
> them implement it unconditionally (ipu_idmac grabs a mutex first but
> this is another story).
> Adding error checking to 8250-omap like I have it in #1 and disabling
> RX-DMA in case pause fails looks be reasonable since there is nothing
> else that can be done I guess.
> Once we have the missing piece in omap-dma the RX-DMA can be enabled in
> 8250-omap.
> Does this sound like a plan we can agree on?

Yes sounds good to me..

-- 
~Vinod