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:21 +0100
commit6cebf8eea2534dfdf36ef6613c7623fca4cfd0d3 (patch)
tree7a9df5d898048a62f3c79e9d1df583579ddcd35b
parent51b2a9ea3029e688d7d63799998dd1fac60c4991 (diff)
dmaengine: bcm2835: Fix abort of transactions (bnc#1012628).
-rw-r--r--patches.kernel.org/4.20.8-317-dmaengine-bcm2835-Fix-abort-of-transactions.patch167
-rw-r--r--series.conf1
2 files changed, 168 insertions, 0 deletions
diff --git a/patches.kernel.org/4.20.8-317-dmaengine-bcm2835-Fix-abort-of-transactions.patch b/patches.kernel.org/4.20.8-317-dmaengine-bcm2835-Fix-abort-of-transactions.patch
new file mode 100644
index 0000000000..098724d77e
--- /dev/null
+++ b/patches.kernel.org/4.20.8-317-dmaengine-bcm2835-Fix-abort-of-transactions.patch
@@ -0,0 +1,167 @@
+From: Lukas Wunner <lukas@wunner.de>
+Date: Wed, 23 Jan 2019 09:26:00 +0100
+Subject: [PATCH] dmaengine: bcm2835: Fix abort of transactions
+References: bnc#1012628
+Patch-mainline: 4.20.8
+Git-commit: 9e528c799d17a4ac37d788c81440b50377dd592d
+
+commit 9e528c799d17a4ac37d788c81440b50377dd592d upstream.
+
+There are multiple issues with bcm2835_dma_abort() (which is called on
+termination of a transaction):
+
+* The algorithm to abort the transaction first pauses the channel by
+ clearing the ACTIVE flag in the CS register, then waits for the PAUSED
+ flag to clear. Page 49 of the spec documents the latter as follows:
+
+ "Indicates if the DMA is currently paused and not transferring data.
+ This will occur if the active bit has been cleared [...]"
+ https://www.raspberrypi.org/app/uploads/2012/02/BCM2835-ARM-Peripherals.pdf
+
+ So the function is entering an infinite loop because it is waiting for
+ PAUSED to clear which is always set due to the function having cleared
+ the ACTIVE flag. The only thing that's saving it from itself is the
+ upper bound of 10000 loop iterations.
+
+ The code comment says that the intention is to "wait for any current
+ AXI transfer to complete", so the author probably wanted to check the
+ WAITING_FOR_OUTSTANDING_WRITES flag instead. Amend the function
+ accordingly.
+
+* The CS register is only read at the beginning of the function. It
+ needs to be read again after pausing the channel and before checking
+ for outstanding writes, otherwise writes which were issued between
+ the register read at the beginning of the function and pausing the
+ channel may not be waited for.
+
+* The function seeks to abort the transfer by writing 0 to the NEXTCONBK
+ register and setting the ABORT and ACTIVE flags. Thereby, the 0 in
+ NEXTCONBK is sought to be loaded into the CONBLK_AD register. However
+ experimentation has shown this approach to not work: The CONBLK_AD
+ register remains the same as before and the CS register contains
+ 0x00000030 (PAUSED | DREQ_STOPS_DMA). In other words, the control
+ block is not aborted but merely paused and it will be resumed once the
+ next DMA transaction is started. That is absolutely not the desired
+ behavior.
+
+ A simpler approach is to set the channel's RESET flag instead. This
+ reliably zeroes the NEXTCONBK as well as the CS register. It requires
+ less code and only a single MMIO write. This is also what popular
+ user space DMA drivers do, e.g.:
+ https://github.com/metachris/RPIO/blob/master/source/c_pwm/pwm.c
+
+ Note that the spec is contradictory whether the NEXTCONBK register
+ is writeable at all. On the one hand, page 41 claims:
+
+ "The value loaded into the NEXTCONBK register can be overwritten so
+ that the linked list of Control Block data structures can be
+ dynamically altered. However it is only safe to do this when the DMA
+ is paused."
+
+ On the other hand, page 40 specifies:
+
+ "Only three registers in each channel's register set are directly
+ writeable (CS, CONBLK_AD and DEBUG). The other registers (TI,
+ SOURCE_AD, DEST_AD, TXFR_LEN, STRIDE & NEXTCONBK), are automatically
+ loaded from a Control Block data structure held in external memory."
+
+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 | 41 +++++++++------------------------------
+ 1 file changed, 9 insertions(+), 32 deletions(-)
+
+diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
+index f5a2506917b4..15795175160a 100644
+--- a/drivers/dma/bcm2835-dma.c
++++ b/drivers/dma/bcm2835-dma.c
+@@ -415,13 +415,11 @@ static void bcm2835_dma_fill_cb_chain_with_sg(
+ }
+ }
+
+-static int bcm2835_dma_abort(void __iomem *chan_base)
++static int bcm2835_dma_abort(struct bcm2835_chan *c)
+ {
+- unsigned long cs;
++ void __iomem *chan_base = c->chan_base;
+ long int timeout = 10000;
+
+- cs = readl(chan_base + BCM2835_DMA_CS);
+-
+ /*
+ * A zero control block address means the channel is idle.
+ * (The ACTIVE flag in the CS register is not a reliable indicator.)
+@@ -433,25 +431,16 @@ static int bcm2835_dma_abort(void __iomem *chan_base)
+ writel(0, chan_base + BCM2835_DMA_CS);
+
+ /* Wait for any current AXI transfer to complete */
+- while ((cs & BCM2835_DMA_ISPAUSED) && --timeout) {
++ while ((readl(chan_base + BCM2835_DMA_CS) &
++ BCM2835_DMA_WAITING_FOR_WRITES) && --timeout)
+ cpu_relax();
+- cs = readl(chan_base + BCM2835_DMA_CS);
+- }
+
+- /* We'll un-pause when we set of our next DMA */
++ /* Peripheral might be stuck and fail to signal AXI write responses */
+ if (!timeout)
+- return -ETIMEDOUT;
+-
+- if (!(cs & BCM2835_DMA_ACTIVE))
+- return 0;
+-
+- /* Terminate the control block chain */
+- writel(0, chan_base + BCM2835_DMA_NEXTCB);
+-
+- /* Abort the whole DMA */
+- writel(BCM2835_DMA_ABORT | BCM2835_DMA_ACTIVE,
+- chan_base + BCM2835_DMA_CS);
++ dev_err(c->vc.chan.device->dev,
++ "failed to complete outstanding writes\n");
+
++ writel(BCM2835_DMA_RESET, chan_base + BCM2835_DMA_CS);
+ return 0;
+ }
+
+@@ -796,7 +785,6 @@ static int bcm2835_dma_terminate_all(struct dma_chan *chan)
+ struct bcm2835_chan *c = to_bcm2835_dma_chan(chan);
+ struct bcm2835_dmadev *d = to_bcm2835_dma_dev(c->vc.chan.device);
+ unsigned long flags;
+- int timeout = 10000;
+ LIST_HEAD(head);
+
+ spin_lock_irqsave(&c->vc.lock, flags);
+@@ -810,18 +798,7 @@ static int bcm2835_dma_terminate_all(struct dma_chan *chan)
+ if (c->desc) {
+ vchan_terminate_vdesc(&c->desc->vd);
+ c->desc = NULL;
+- bcm2835_dma_abort(c->chan_base);
+-
+- /* Wait for stopping */
+- while (--timeout) {
+- if (!readl(c->chan_base + BCM2835_DMA_ADDR))
+- break;
+-
+- cpu_relax();
+- }
+-
+- if (!timeout)
+- dev_err(d->ddev.dev, "DMA transfer could not be terminated\n");
++ bcm2835_dma_abort(c);
+ }
+
+ vchan_get_all_descriptors(&c->vc, &head);
+--
+2.20.1
+
diff --git a/series.conf b/series.conf
index 3d704074e1..cbc34d73a0 100644
--- a/series.conf
+++ b/series.conf
@@ -1053,6 +1053,7 @@
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
+ patches.kernel.org/4.20.8-317-dmaengine-bcm2835-Fix-abort-of-transactions.patch
########################################################
# Build fixes that apply to the vanilla kernel too.