* [PATCH v2 1/6] dmaengine: bcm2835: Fix interrupt race on RT
2019-01-23 8:26 [PATCH v2 0/6] Raspberry Pi DMA fixes + cleanups Lukas Wunner
@ 2019-01-23 8:26 ` Lukas Wunner
2019-01-23 16:10 ` [PATCH v2 0/6] Raspberry Pi DMA fixes + cleanups Stefan Wahren
2019-02-04 7:12 ` Vinod Koul
2 siblings, 0 replies; 5+ messages in thread
From: Lukas Wunner @ 2019-01-23 8:26 UTC (permalink / raw)
To: Vinod Koul, Eric Anholt, Stefan Wahren
Cc: Frank Pavlic, Martin Sperl, Florian Meier, Clive Messer,
Matthias Reichl, dmaengine, linux-rpi-kernel, Tiejun Chen,
linux-rt-users
If IRQ handlers are threaded (either because CONFIG_PREEMPT_RT_BASE is
enabled or "threadirqs" was passed on the command line) and if system
load is sufficiently high that wakeup latency of IRQ threads degrades,
SPI DMA transactions on the BCM2835 occasionally break like this:
ks8851 spi0.0: SPI transfer timed out
bcm2835-dma 3f007000.dma: DMA transfer could not be terminated
ks8851 spi0.0 eth2: ks8851_rdfifo: spi_sync() failed
The root cause is an assumption made by the DMA driver which is
documented in a code comment in bcm2835_dma_terminate_all():
/*
* Stop DMA activity: we assume the callback will not be called
* after bcm_dma_abort() returns (even if it does, it will see
* c->desc is NULL and exit.)
*/
That assumption falls apart if the IRQ handler bcm2835_dma_callback() is
threaded: A client may terminate a descriptor and issue a new one
before the IRQ handler had a chance to run. In fact the IRQ handler may
miss an *arbitrary* number of descriptors. The result is the following
race condition:
1. A descriptor finishes, its interrupt is deferred to the IRQ thread.
2. A client calls dma_terminate_async() which sets channel->desc = NULL.
3. The client issues a new descriptor. Because channel->desc is NULL,
bcm2835_dma_issue_pending() immediately starts the descriptor.
4. Finally the IRQ thread runs and writes BCM2835_DMA_INT to the CS
register to acknowledge the interrupt. This clears the ACTIVE flag,
so the newly issued descriptor is paused in the middle of the
transaction. Because channel->desc is not NULL, the IRQ thread
finalizes the descriptor and tries to start the next one.
I see two possible solutions: The first is to call synchronize_irq()
in bcm2835_dma_issue_pending() to wait until the IRQ thread has
finished before issuing a new descriptor. The downside of this approach
is unnecessary latency if clients desire rapidly terminating and
re-issuing descriptors and don't have any use for an IRQ callback.
(The SPI TX DMA channel is a case in point.)
A better alternative is to make the IRQ thread recognize that it has
missed descriptors and avoid finalizing the newly issued descriptor.
So first of all, set the ACTIVE flag when acknowledging the interrupt.
This keeps a newly issued descriptor running.
If the descriptor was finished, the channel remains idle despite the
ACTIVE flag being set. However the ACTIVE flag can then no longer be
used to check whether the channel is idle, so instead check whether
the register containing the current control block address is zero
and finalize the current descriptor only if so.
That way, there is no impact on latency and throughput if the client
doesn't care for the interrupt: Only minimal additional overhead is
introduced for non-cyclic descriptors as one further MMIO read is
necessary per interrupt to check for idleness of the channel. Cyclic
descriptors are sped up slightly by removing one MMIO write per
interrupt.
Fixes: 96286b576690 ("dmaengine: Add support for BCM2835")
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: stable@vger.kernel.org # v3.14+
Cc: Frank Pavlic <f.pavlic@kunbus.de>
Cc: Martin Sperl <kernel@martin.sperl.org>
Cc: Florian Meier <florian.meier@koalo.de>
Cc: Clive Messer <clive.m.messer@gmail.com>
Cc: Matthias Reichl <hias@horus.com>
---
drivers/dma/bcm2835-dma.c | 33 ++++++++++++++++++---------------
1 file changed, 18 insertions(+), 15 deletions(-)
diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
index 7beec403c2c9..7c1b9ef2f71f 100644
--- a/drivers/dma/bcm2835-dma.c
+++ b/drivers/dma/bcm2835-dma.c
@@ -411,7 +411,12 @@ static int bcm2835_dma_abort(void __iomem *chan_base)
long int timeout = 10000;
cs = readl(chan_base + BCM2835_DMA_CS);
- if (!(cs & BCM2835_DMA_ACTIVE))
+
+ /*
+ * A zero control block address means the channel is idle.
+ * (The ACTIVE flag in the CS register is not a reliable indicator.)
+ */
+ if (!readl(chan_base + BCM2835_DMA_ADDR))
return 0;
/* Write 0 to the active bit - Pause the DMA */
@@ -475,8 +480,15 @@ static irqreturn_t bcm2835_dma_callback(int irq, void *data)
spin_lock_irqsave(&c->vc.lock, flags);
- /* Acknowledge interrupt */
- writel(BCM2835_DMA_INT, c->chan_base + BCM2835_DMA_CS);
+ /*
+ * Clear the INT flag to receive further interrupts. Keep the channel
+ * active in case the descriptor is cyclic or in case the client has
+ * already terminated the descriptor and issued a new one. (May happen
+ * if this IRQ handler is threaded.) If the channel is finished, it
+ * will remain idle despite the ACTIVE flag being set.
+ */
+ writel(BCM2835_DMA_INT | BCM2835_DMA_ACTIVE,
+ c->chan_base + BCM2835_DMA_CS);
d = c->desc;
@@ -484,11 +496,7 @@ static irqreturn_t bcm2835_dma_callback(int irq, void *data)
if (d->cyclic) {
/* call the cyclic callback */
vchan_cyclic_callback(&d->vd);
-
- /* Keep the DMA engine running */
- writel(BCM2835_DMA_ACTIVE,
- c->chan_base + BCM2835_DMA_CS);
- } else {
+ } else if (!readl(c->chan_base + BCM2835_DMA_ADDR)) {
vchan_cookie_complete(&c->desc->vd);
bcm2835_dma_start_desc(c);
}
@@ -788,11 +796,7 @@ static int bcm2835_dma_terminate_all(struct dma_chan *chan)
list_del_init(&c->node);
spin_unlock(&d->lock);
- /*
- * Stop DMA activity: we assume the callback will not be called
- * after bcm_dma_abort() returns (even if it does, it will see
- * c->desc is NULL and exit.)
- */
+ /* stop DMA activity */
if (c->desc) {
vchan_terminate_vdesc(&c->desc->vd);
c->desc = NULL;
@@ -800,8 +804,7 @@ static int bcm2835_dma_terminate_all(struct dma_chan *chan)
/* Wait for stopping */
while (--timeout) {
- if (!(readl(c->chan_base + BCM2835_DMA_CS) &
- BCM2835_DMA_ACTIVE))
+ if (!readl(c->chan_base + BCM2835_DMA_ADDR))
break;
cpu_relax();
--
2.20.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v2 0/6] Raspberry Pi DMA fixes + cleanups
2019-01-23 8:26 [PATCH v2 0/6] Raspberry Pi DMA fixes + cleanups Lukas Wunner
2019-01-23 8:26 ` [PATCH v2 1/6] dmaengine: bcm2835: Fix interrupt race on RT Lukas Wunner
@ 2019-01-23 16:10 ` Stefan Wahren
2019-01-23 16:55 ` Florian Kauer
2019-02-04 7:12 ` Vinod Koul
2 siblings, 1 reply; 5+ messages in thread
From: Stefan Wahren @ 2019-01-23 16:10 UTC (permalink / raw)
To: Lukas Wunner, Vinod Koul, Eric Anholt
Cc: Frank Pavlic, Martin Sperl, Florian Meier, Clive Messer,
Matthias Reichl, dmaengine, linux-rpi-kernel, Tiejun Chen,
linux-rt-users
> Lukas Wunner <lukas@wunner.de> hat am 23. Januar 2019 um 09:26 geschrieben:
>
>
> Two fixes and four cleanups for the Raspberry Pi DMA driver, v2.
>
> Changes since v1:
>
> * Patch [1/6]: Rework the patch to fix a race condition which could
> still occur with v1, and no longer check for idleness of a channel
> using the ACTIVE flag in the CS register (which is unreliable),
> but check for a zero control block address instead.
>
> * Patch [2/6]: If outstanding writes fail to complete on abort of a
> transaction, print an error message and document a possible cause
> in a code comment. (Stefan Wahren)
>
> * Patch [5/6] and [6/6]: Split patch to remove dead code in two and
> rephrase commit subject to be less generic. (Stefan Wahren, Vinod Koul)
>
> Thanks,
>
> Lukas
>
>
> Lukas Wunner (6):
> dmaengine: bcm2835: Fix interrupt race on RT
> dmaengine: bcm2835: Fix abort of transactions
> dmaengine: bcm2835: Return void from abort of transactions
> dmaengine: bcm2835: Enforce control block alignment
> dmaengine: bcm2835: Drop gratuitous list deletion
> dmaengine: bcm2835: Drop outdated comment on supported transactions
>
The whole series is:
Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
Thanks
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 0/6] Raspberry Pi DMA fixes + cleanups
2019-01-23 8:26 [PATCH v2 0/6] Raspberry Pi DMA fixes + cleanups Lukas Wunner
2019-01-23 8:26 ` [PATCH v2 1/6] dmaengine: bcm2835: Fix interrupt race on RT Lukas Wunner
2019-01-23 16:10 ` [PATCH v2 0/6] Raspberry Pi DMA fixes + cleanups Stefan Wahren
@ 2019-02-04 7:12 ` Vinod Koul
2 siblings, 0 replies; 5+ messages in thread
From: Vinod Koul @ 2019-02-04 7:12 UTC (permalink / raw)
To: Lukas Wunner
Cc: Eric Anholt, Stefan Wahren, Frank Pavlic, Martin Sperl,
Florian Meier, Clive Messer, Matthias Reichl, dmaengine,
linux-rpi-kernel, Tiejun Chen, linux-rt-users
On 23-01-19, 09:26, Lukas Wunner wrote:
> Two fixes and four cleanups for the Raspberry Pi DMA driver, v2.
Applied all, thanks
>
> Changes since v1:
>
> * Patch [1/6]: Rework the patch to fix a race condition which could
> still occur with v1, and no longer check for idleness of a channel
> using the ACTIVE flag in the CS register (which is unreliable),
> but check for a zero control block address instead.
>
> * Patch [2/6]: If outstanding writes fail to complete on abort of a
> transaction, print an error message and document a possible cause
> in a code comment. (Stefan Wahren)
>
> * Patch [5/6] and [6/6]: Split patch to remove dead code in two and
> rephrase commit subject to be less generic. (Stefan Wahren, Vinod Koul)
>
> Thanks,
>
> Lukas
>
>
> Lukas Wunner (6):
> dmaengine: bcm2835: Fix interrupt race on RT
> dmaengine: bcm2835: Fix abort of transactions
> dmaengine: bcm2835: Return void from abort of transactions
> dmaengine: bcm2835: Enforce control block alignment
> dmaengine: bcm2835: Drop gratuitous list deletion
> dmaengine: bcm2835: Drop outdated comment on supported transactions
>
> drivers/dma/bcm2835-dma.c | 92 +++++++++++++--------------------------
> 1 file changed, 31 insertions(+), 61 deletions(-)
>
> --
> 2.20.1
--
~Vinod
^ permalink raw reply [flat|nested] 5+ messages in thread