Home Home > GIT Browse > stable
summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorJiri Slaby <jslaby@suse.cz>2019-02-12 22:18:26 +0100
committerJiri Slaby <jslaby@suse.cz>2019-02-12 22:21:20 +0100
commit51b2a9ea3029e688d7d63799998dd1fac60c4991 (patch)
treed9c1c97aac4fdbe96036544e0023b30fe7d97019
parentebdc530edd06737819e4912fc1854d0d20413766 (diff)
dmaengine: bcm2835: Fix interrupt race on RT (bnc#1012628).
-rw-r--r--patches.kernel.org/4.20.8-316-dmaengine-bcm2835-Fix-interrupt-race-on-RT.patch160
-rw-r--r--series.conf1
2 files changed, 161 insertions, 0 deletions
diff --git a/patches.kernel.org/4.20.8-316-dmaengine-bcm2835-Fix-interrupt-race-on-RT.patch b/patches.kernel.org/4.20.8-316-dmaengine-bcm2835-Fix-interrupt-race-on-RT.patch
new file mode 100644
index 0000000000..130ed182d5
--- /dev/null
+++ b/patches.kernel.org/4.20.8-316-dmaengine-bcm2835-Fix-interrupt-race-on-RT.patch
@@ -0,0 +1,160 @@
+From: Lukas Wunner <lukas@wunner.de>
+Date: Wed, 23 Jan 2019 09:26:00 +0100
+Subject: [PATCH] dmaengine: bcm2835: Fix interrupt race on RT
+References: bnc#1012628
+Patch-mainline: 4.20.8
+Git-commit: f7da7782aba92593f7b82f03d2409a1c5f4db91b
+
+commit f7da7782aba92593f7b82f03d2409a1c5f4db91b upstream.
+
+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>
+Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
+Acked-by: Florian Kauer <florian.kauer@koalo.de>
+Signed-off-by: Vinod Koul <vkoul@kernel.org>
+Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
+Signed-off-by: Jiri Slaby <jslaby@suse.cz>
+---
+ 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 cad55ab80d41..f5a2506917b4 100644
+--- a/drivers/dma/bcm2835-dma.c
++++ b/drivers/dma/bcm2835-dma.c
+@@ -421,7 +421,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 */
+@@ -485,8 +490,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;
+
+@@ -494,11 +506,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);
+ }
+@@ -798,11 +806,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;
+@@ -810,8 +814,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
+
diff --git a/series.conf b/series.conf
index f07e290432..3d704074e1 100644
--- a/series.conf
+++ b/series.conf
@@ -1052,6 +1052,7 @@
patches.kernel.org/4.20.8-313-fuse-handle-zero-sized-retrieve-correctly.patch
patches.kernel.org/4.20.8-314-cuse-fix-ioctl.patch
patches.kernel.org/4.20.8-315-HID-debug-fix-the-ring-buffer-implementation.patch
+ patches.kernel.org/4.20.8-316-dmaengine-bcm2835-Fix-interrupt-race-on-RT.patch
########################################################
# Build fixes that apply to the vanilla kernel too.