* omap-dma + 8250_omap: fix the dmaengine_pause() @ 2015-08-07 20:00 Sebastian Andrzej Siewior 2015-08-07 20:00 ` [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported Sebastian Andrzej Siewior ` (2 more replies) 0 siblings, 3 replies; 22+ messages in thread From: Sebastian Andrzej Siewior @ 2015-08-07 20:00 UTC (permalink / raw) To: Vinod Koul, Russell King, peter Cc: Dan Williams, dmaengine, linux-kernel, nsekhar, linux-omap, linux-serial, john.ogness, Peter Ujfalusi #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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported 2015-08-07 20:00 omap-dma + 8250_omap: fix the dmaengine_pause() Sebastian Andrzej Siewior @ 2015-08-07 20:00 ` Sebastian Andrzej Siewior 2015-08-08 0:28 ` Peter Hurley 2015-08-10 11:54 ` Peter Ujfalusi 2015-08-07 20:00 ` [PATCH 2/3] dma: add __must_check annotation for dmaengine_pause() Sebastian Andrzej Siewior 2015-08-07 20:00 ` [PATCH v3 3/3] dma: omap-dma: add support for pause of non-cyclic transfers Sebastian Andrzej Siewior 2 siblings, 2 replies; 22+ messages in thread From: Sebastian Andrzej Siewior @ 2015-08-07 20:00 UTC (permalink / raw) To: Vinod Koul, Russell King, peter Cc: Dan Williams, dmaengine, linux-kernel, nsekhar, linux-omap, linux-serial, john.ogness, Peter Ujfalusi 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported 2015-08-07 20:00 ` [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported Sebastian Andrzej Siewior @ 2015-08-08 0:28 ` Peter Hurley 2015-08-08 9:03 ` Russell King - ARM Linux 2015-08-08 9:32 ` Sebastian Andrzej Siewior 2015-08-10 11:54 ` Peter Ujfalusi 1 sibling, 2 replies; 22+ messages in thread From: Peter Hurley @ 2015-08-08 0:28 UTC (permalink / raw) To: Sebastian Andrzej Siewior, Vinod Koul, Russell King Cc: Dan Williams, dmaengine, linux-kernel, nsekhar, linux-omap, linux-serial, john.ogness, Peter Ujfalusi 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) > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported 2015-08-08 0:28 ` Peter Hurley @ 2015-08-08 9:03 ` Russell King - ARM Linux 2015-08-11 9:57 ` Vinod Koul 2015-08-08 9:32 ` Sebastian Andrzej Siewior 1 sibling, 1 reply; 22+ messages in thread From: Russell King - ARM Linux @ 2015-08-08 9:03 UTC (permalink / raw) To: Peter Hurley Cc: Sebastian Andrzej Siewior, Vinod Koul, Dan Williams, dmaengine, linux-kernel, nsekhar, linux-omap, linux-serial, john.ogness, Peter Ujfalusi 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported 2015-08-08 9:03 ` Russell King - ARM Linux @ 2015-08-11 9:57 ` Vinod Koul 0 siblings, 0 replies; 22+ messages in thread From: Vinod Koul @ 2015-08-11 9:57 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Peter Hurley, Sebastian Andrzej Siewior, Dan Williams, dmaengine, linux-kernel, nsekhar, linux-omap, linux-serial, john.ogness, Peter Ujfalusi 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported 2015-08-08 0:28 ` Peter Hurley 2015-08-08 9:03 ` Russell King - ARM Linux @ 2015-08-08 9:32 ` Sebastian Andrzej Siewior 2015-08-08 9:57 ` Russell King - ARM Linux 2015-08-08 15:40 ` Greg KH 1 sibling, 2 replies; 22+ messages in thread From: Sebastian Andrzej Siewior @ 2015-08-08 9:32 UTC (permalink / raw) To: Peter Hurley, Vinod Koul, Russell King Cc: Dan Williams, dmaengine, linux-kernel, nsekhar, linux-omap, linux-serial, john.ogness, Peter Ujfalusi 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported 2015-08-08 9:32 ` Sebastian Andrzej Siewior @ 2015-08-08 9:57 ` Russell King - ARM Linux 2015-08-08 15:40 ` Greg KH 1 sibling, 0 replies; 22+ messages in thread From: Russell King - ARM Linux @ 2015-08-08 9:57 UTC (permalink / raw) To: Sebastian Andrzej Siewior Cc: Peter Hurley, Vinod Koul, Dan Williams, dmaengine, linux-kernel, nsekhar, linux-omap, linux-serial, john.ogness, Peter Ujfalusi 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported 2015-08-08 9:32 ` Sebastian Andrzej Siewior 2015-08-08 9:57 ` Russell King - ARM Linux @ 2015-08-08 15:40 ` Greg KH 1 sibling, 0 replies; 22+ messages in thread From: Greg KH @ 2015-08-08 15:40 UTC (permalink / raw) To: Sebastian Andrzej Siewior Cc: Peter Hurley, Vinod Koul, Russell King, Dan Williams, dmaengine, linux-kernel, nsekhar, linux-omap, linux-serial, john.ogness, Peter Ujfalusi 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported 2015-08-07 20:00 ` [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported Sebastian Andrzej Siewior 2015-08-08 0:28 ` Peter Hurley @ 2015-08-10 11:54 ` Peter Ujfalusi 2015-08-10 12:19 ` Sebastian Andrzej Siewior 2015-08-10 13:00 ` Peter Hurley 1 sibling, 2 replies; 22+ messages in thread From: Peter Ujfalusi @ 2015-08-10 11:54 UTC (permalink / raw) To: Sebastian Andrzej Siewior, Vinod Koul, Russell King, peter Cc: Dan Williams, dmaengine, linux-kernel, nsekhar, linux-omap, linux-serial, john.ogness 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported 2015-08-10 11:54 ` Peter Ujfalusi @ 2015-08-10 12:19 ` Sebastian Andrzej Siewior 2015-08-10 13:00 ` Peter Hurley 1 sibling, 0 replies; 22+ messages in thread From: Sebastian Andrzej Siewior @ 2015-08-10 12:19 UTC (permalink / raw) To: Peter Ujfalusi, Vinod Koul, Russell King, peter Cc: Dan Williams, dmaengine, linux-kernel, nsekhar, linux-omap, linux-serial, john.ogness 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported 2015-08-10 11:54 ` Peter Ujfalusi 2015-08-10 12:19 ` Sebastian Andrzej Siewior @ 2015-08-10 13:00 ` Peter Hurley 2015-08-10 17:15 ` Russell King - ARM Linux 1 sibling, 1 reply; 22+ messages in thread From: Peter Hurley @ 2015-08-10 13:00 UTC (permalink / raw) To: Peter Ujfalusi, Sebastian Andrzej Siewior, Vinod Koul, Russell King Cc: Dan Williams, dmaengine, linux-kernel, nsekhar, linux-omap, linux-serial, john.ogness 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported 2015-08-10 13:00 ` Peter Hurley @ 2015-08-10 17:15 ` Russell King - ARM Linux 0 siblings, 0 replies; 22+ messages in thread From: Russell King - ARM Linux @ 2015-08-10 17:15 UTC (permalink / raw) To: Peter Hurley Cc: Peter Ujfalusi, Sebastian Andrzej Siewior, Vinod Koul, Dan Williams, dmaengine, linux-kernel, nsekhar, linux-omap, linux-serial, john.ogness 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/3] dma: add __must_check annotation for dmaengine_pause() 2015-08-07 20:00 omap-dma + 8250_omap: fix the dmaengine_pause() Sebastian Andrzej Siewior 2015-08-07 20:00 ` [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported Sebastian Andrzej Siewior @ 2015-08-07 20:00 ` Sebastian Andrzej Siewior 2015-08-08 0:40 ` Peter Hurley 2015-08-11 9:58 ` Vinod Koul 2015-08-07 20:00 ` [PATCH v3 3/3] dma: omap-dma: add support for pause of non-cyclic transfers Sebastian Andrzej Siewior 2 siblings, 2 replies; 22+ messages in thread From: Sebastian Andrzej Siewior @ 2015-08-07 20:00 UTC (permalink / raw) To: Vinod Koul, Russell King, peter Cc: Dan Williams, dmaengine, linux-kernel, nsekhar, linux-omap, linux-serial, john.ogness, Peter Ujfalusi 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] dma: add __must_check annotation for dmaengine_pause() 2015-08-07 20:00 ` [PATCH 2/3] dma: add __must_check annotation for dmaengine_pause() Sebastian Andrzej Siewior @ 2015-08-08 0:40 ` Peter Hurley 2015-08-11 9:58 ` Vinod Koul 1 sibling, 0 replies; 22+ messages in thread From: Peter Hurley @ 2015-08-08 0:40 UTC (permalink / raw) To: Sebastian Andrzej Siewior, Vinod Koul, Russell King Cc: Dan Williams, dmaengine, linux-kernel, nsekhar, linux-omap, linux-serial, john.ogness, Peter Ujfalusi 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] dma: add __must_check annotation for dmaengine_pause() 2015-08-07 20:00 ` [PATCH 2/3] dma: add __must_check annotation for dmaengine_pause() Sebastian Andrzej Siewior 2015-08-08 0:40 ` Peter Hurley @ 2015-08-11 9:58 ` Vinod Koul 2015-08-11 10:06 ` Russell King - ARM Linux 1 sibling, 1 reply; 22+ messages in thread From: Vinod Koul @ 2015-08-11 9:58 UTC (permalink / raw) To: Sebastian Andrzej Siewior Cc: Russell King, peter, Dan Williams, dmaengine, linux-kernel, nsekhar, linux-omap, linux-serial, john.ogness, Peter Ujfalusi 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] dma: add __must_check annotation for dmaengine_pause() 2015-08-11 9:58 ` Vinod Koul @ 2015-08-11 10:06 ` Russell King - ARM Linux 2015-08-11 12:34 ` Sebastian Andrzej Siewior 0 siblings, 1 reply; 22+ messages in thread From: Russell King - ARM Linux @ 2015-08-11 10:06 UTC (permalink / raw) To: Vinod Koul Cc: Sebastian Andrzej Siewior, peter, Dan Williams, dmaengine, linux-kernel, nsekhar, linux-omap, linux-serial, john.ogness, Peter Ujfalusi 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] dma: add __must_check annotation for dmaengine_pause() 2015-08-11 10:06 ` Russell King - ARM Linux @ 2015-08-11 12:34 ` Sebastian Andrzej Siewior 2015-08-21 8:32 ` Vinod Koul 0 siblings, 1 reply; 22+ messages in thread From: Sebastian Andrzej Siewior @ 2015-08-11 12:34 UTC (permalink / raw) To: Russell King - ARM Linux, Vinod Koul Cc: peter, Dan Williams, dmaengine, linux-kernel, nsekhar, linux-omap, linux-serial, john.ogness, Peter Ujfalusi 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/3] dma: add __must_check annotation for dmaengine_pause() 2015-08-11 12:34 ` Sebastian Andrzej Siewior @ 2015-08-21 8:32 ` Vinod Koul 0 siblings, 0 replies; 22+ messages in thread From: Vinod Koul @ 2015-08-21 8:32 UTC (permalink / raw) To: Sebastian Andrzej Siewior Cc: Russell King - ARM Linux, peter, Dan Williams, dmaengine, linux-kernel, nsekhar, linux-omap, linux-serial, john.ogness, Peter Ujfalusi 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v3 3/3] dma: omap-dma: add support for pause of non-cyclic transfers 2015-08-07 20:00 omap-dma + 8250_omap: fix the dmaengine_pause() Sebastian Andrzej Siewior 2015-08-07 20:00 ` [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported Sebastian Andrzej Siewior 2015-08-07 20:00 ` [PATCH 2/3] dma: add __must_check annotation for dmaengine_pause() Sebastian Andrzej Siewior @ 2015-08-07 20:00 ` Sebastian Andrzej Siewior 2015-08-11 12:02 ` Peter Ujfalusi 2 siblings, 1 reply; 22+ messages in thread From: Sebastian Andrzej Siewior @ 2015-08-07 20:00 UTC (permalink / raw) To: Vinod Koul, Russell King, peter Cc: Dan Williams, dmaengine, linux-kernel, nsekhar, linux-omap, linux-serial, john.ogness, Peter Ujfalusi 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v3 3/3] dma: omap-dma: add support for pause of non-cyclic transfers 2015-08-07 20:00 ` [PATCH v3 3/3] dma: omap-dma: add support for pause of non-cyclic transfers Sebastian Andrzej Siewior @ 2015-08-11 12:02 ` Peter Ujfalusi 2015-08-11 12:30 ` Russell King - ARM Linux 0 siblings, 1 reply; 22+ messages in thread From: Peter Ujfalusi @ 2015-08-11 12:02 UTC (permalink / raw) To: Sebastian Andrzej Siewior, Vinod Koul, Russell King, peter Cc: Dan Williams, dmaengine, linux-kernel, nsekhar, linux-omap, linux-serial, john.ogness 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 3/3] dma: omap-dma: add support for pause of non-cyclic transfers 2015-08-11 12:02 ` Peter Ujfalusi @ 2015-08-11 12:30 ` Russell King - ARM Linux 2015-08-11 12:43 ` Peter Ujfalusi 0 siblings, 1 reply; 22+ messages in thread From: Russell King - ARM Linux @ 2015-08-11 12:30 UTC (permalink / raw) To: Peter Ujfalusi Cc: Sebastian Andrzej Siewior, Vinod Koul, peter, Dan Williams, dmaengine, linux-kernel, nsekhar, linux-omap, linux-serial, john.ogness 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 3/3] dma: omap-dma: add support for pause of non-cyclic transfers 2015-08-11 12:30 ` Russell King - ARM Linux @ 2015-08-11 12:43 ` Peter Ujfalusi 0 siblings, 0 replies; 22+ messages in thread From: Peter Ujfalusi @ 2015-08-11 12:43 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Sebastian Andrzej Siewior, Vinod Koul, peter, Dan Williams, dmaengine, linux-kernel, nsekhar, linux-omap, linux-serial, john.ogness 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2015-08-21 8:30 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-08-07 20:00 omap-dma + 8250_omap: fix the dmaengine_pause() Sebastian Andrzej Siewior 2015-08-07 20:00 ` [PATCH 1/3] tty: serial: 8250_omap: do not use RX DMA if pause is not supported Sebastian Andrzej Siewior 2015-08-08 0:28 ` Peter Hurley 2015-08-08 9:03 ` Russell King - ARM Linux 2015-08-11 9:57 ` Vinod Koul 2015-08-08 9:32 ` Sebastian Andrzej Siewior 2015-08-08 9:57 ` Russell King - ARM Linux 2015-08-08 15:40 ` Greg KH 2015-08-10 11:54 ` Peter Ujfalusi 2015-08-10 12:19 ` Sebastian Andrzej Siewior 2015-08-10 13:00 ` Peter Hurley 2015-08-10 17:15 ` Russell King - ARM Linux 2015-08-07 20:00 ` [PATCH 2/3] dma: add __must_check annotation for dmaengine_pause() Sebastian Andrzej Siewior 2015-08-08 0:40 ` Peter Hurley 2015-08-11 9:58 ` Vinod Koul 2015-08-11 10:06 ` Russell King - ARM Linux 2015-08-11 12:34 ` Sebastian Andrzej Siewior 2015-08-21 8:32 ` Vinod Koul 2015-08-07 20:00 ` [PATCH v3 3/3] dma: omap-dma: add support for pause of non-cyclic transfers Sebastian Andrzej Siewior 2015-08-11 12:02 ` Peter Ujfalusi 2015-08-11 12:30 ` Russell King - ARM Linux 2015-08-11 12:43 ` Peter Ujfalusi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).