linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] Fix UART DMA freezes for iMX6
@ 2019-09-11 14:49 Philipp Puschmann
  2019-09-11 14:49 ` [PATCH 1/4] dmaengine: imx-sdma: fix buffer ownership Philipp Puschmann
                   ` (7 more replies)
  0 siblings, 8 replies; 31+ messages in thread
From: Philipp Puschmann @ 2019-09-11 14:49 UTC (permalink / raw)
  To: linux-kernel
  Cc: vkoul, dan.j.williams, shawnguo, s.hauer, kernel, festevam,
	linux-imx, gregkh, jslaby, dmaengine, linux-arm-kernel,
	linux-serial, Philipp Puschmann

For some years and since many kernel versions there are reports that
RX UART DMA channel stops working at one point. So far the usual workaround was
to disable RX DMA. This patches try to fix the underlying problem.

When a running sdma script does not find any usable destination buffer to put
its data into it just leads to stopping the channel being scheduled again. As
solution we we manually retrigger the sdma script for this channel and by this
dissolve the freeze.

While this seems to work fine so far a further patch in this series increases
the number of RX DMA periods for UART to reduce use cases running into such
a situation.

This patch series was tested with the current kernel and backported to
kernel 4.15 with a special use case using a WL1837MOD via UART and provoking
the hanging of UART RX DMA within seconds after starting a test application.
It resulted in well known
  "Bluetooth: hci0: command 0x0408 tx timeout"
errors and complete stop of UART data reception. Our Bluetooth traffic consists
of many independent small packets, mostly only a few bytes, causing high usage
of periods.


Philipp Puschmann (4):
  dmaengine: imx-sdma: fix buffer ownership
  dmaengine: imx-sdma: fix dma freezes
  serial: imx: adapt rx buffer and dma periods
  dmaengine: imx-sdma: drop redundant variable

 drivers/dma/imx-sdma.c   | 32 ++++++++++++++++++++++----------
 drivers/tty/serial/imx.c |  5 ++---
 2 files changed, 24 insertions(+), 13 deletions(-)

-- 
2.23.0


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 1/4] dmaengine: imx-sdma: fix buffer ownership
  2019-09-11 14:49 [PATCH 0/4] Fix UART DMA freezes for iMX6 Philipp Puschmann
@ 2019-09-11 14:49 ` Philipp Puschmann
  2019-09-16 14:17   ` Lucas Stach
  2019-09-11 14:49 ` [PATCH 2/4] dmaengine: imx-sdma: fix dma freezes Philipp Puschmann
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 31+ messages in thread
From: Philipp Puschmann @ 2019-09-11 14:49 UTC (permalink / raw)
  To: linux-kernel
  Cc: vkoul, dan.j.williams, shawnguo, s.hauer, kernel, festevam,
	linux-imx, gregkh, jslaby, dmaengine, linux-arm-kernel,
	linux-serial, Philipp Puschmann

BD_DONE flag marks ownership of the buffer. When 1 SDMA owns the buffer,
when 0 ARM owns it. When processing the buffers in
sdma_update_channel_loop the ownership of the currently processed buffer
was set to SDMA again before running the callback function of the the
buffer and while the sdma script may be running in parallel. So there was
the possibility to get the buffer overwritten by SDMA before it has been
processed by kernel leading to kind of random errors in the upper layers,
e.g. bluetooth.

It may be further a good idea to make the status struct member volatile or
access it using writel or similar to rule out that the compiler sets the
BD_DONE flag before the callback routine has finished.

Signed-off-by: Philipp Puschmann <philipp.puschmann@emlix.com>
---
 drivers/dma/imx-sdma.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index a01f4b5d793c..1abb14ff394d 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -802,7 +802,6 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
 		*/
 
 		desc->chn_real_count = bd->mode.count;
-		bd->mode.status |= BD_DONE;
 		bd->mode.count = desc->period_len;
 		desc->buf_ptail = desc->buf_tail;
 		desc->buf_tail = (desc->buf_tail + 1) % desc->num_bd;
@@ -817,6 +816,8 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
 		dmaengine_desc_get_callback_invoke(&desc->vd.tx, NULL);
 		spin_lock(&sdmac->vc.lock);
 
+		bd->mode.status |= BD_DONE;
+
 		if (error)
 			sdmac->status = old_status;
 	}
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 2/4] dmaengine: imx-sdma: fix dma freezes
  2019-09-11 14:49 [PATCH 0/4] Fix UART DMA freezes for iMX6 Philipp Puschmann
  2019-09-11 14:49 ` [PATCH 1/4] dmaengine: imx-sdma: fix buffer ownership Philipp Puschmann
@ 2019-09-11 14:49 ` Philipp Puschmann
  2019-09-16 14:22   ` Lucas Stach
  2019-09-11 14:49 ` [PATCH 3/4] serial: imx: adapt rx buffer and dma periods Philipp Puschmann
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 31+ messages in thread
From: Philipp Puschmann @ 2019-09-11 14:49 UTC (permalink / raw)
  To: linux-kernel
  Cc: vkoul, dan.j.williams, shawnguo, s.hauer, kernel, festevam,
	linux-imx, gregkh, jslaby, dmaengine, linux-arm-kernel,
	linux-serial, Philipp Puschmann

For some years and since many kernel versions there are reports that the
RX UART SDMA channel stops working at some point. The workaround was to
disable DMA for RX. This commit tries to fix the problem itself.

Due to its license i wasn't able to debug the sdma script itself but it
somehow leads to blocking the scheduling of the channel script when a
running sdma script does not find any usable destination buffer to put its
data into.

If we detect such a potential case we manually retrigger the sdma script
for this channel and by this reenable the scipt being run by scheduler.

As sdmac->desc is constant we can move desc out of the loop.

Signed-off-by: Philipp Puschmann <philipp.puschmann@emlix.com>
---
 drivers/dma/imx-sdma.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 1abb14ff394d..6a5a84504858 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -775,21 +775,23 @@ static void sdma_start_desc(struct sdma_channel *sdmac)
 static void sdma_update_channel_loop(struct sdma_channel *sdmac)
 {
 	struct sdma_buffer_descriptor *bd;
-	int error = 0;
-	enum dma_status	old_status = sdmac->status;
+	struct sdma_desc *desc = sdmac->desc;
+	int error = 0, cnt = 0;
+	enum dma_status old_status = sdmac->status;
 
 	/*
 	 * loop mode. Iterate over descriptors, re-setup them and
 	 * call callback function.
 	 */
-	while (sdmac->desc) {
-		struct sdma_desc *desc = sdmac->desc;
+	while (desc) {
 
 		bd = &desc->bd[desc->buf_tail];
 
 		if (bd->mode.status & BD_DONE)
 			break;
 
+		cnt++;
+
 		if (bd->mode.status & BD_RROR) {
 			bd->mode.status &= ~BD_RROR;
 			sdmac->status = DMA_ERROR;
@@ -821,6 +823,18 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
 		if (error)
 			sdmac->status = old_status;
 	}
+
+	/* In some situations it happens that the sdma stops serving
+	 * dma interrupt requests. It happens when running dma script
+	 * does not find any usable destination buffer to put its data into.
+	 *
+	 * While there is no specific error condition we can check for, a
+	 * necessary condition is that all available buffers for the current
+	 * channel have been written to by the sdma script. In such a case we
+	 * will trigger the sdma script manually.
+	 */
+	if (cnt >= desc->num_bd)
+		sdma_enable_channel(sdmac->sdma, sdmac->channel);
 }
 
 static void mxc_sdma_handle_channel_normal(struct sdma_channel *data)
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 3/4] serial: imx: adapt rx buffer and dma periods
  2019-09-11 14:49 [PATCH 0/4] Fix UART DMA freezes for iMX6 Philipp Puschmann
  2019-09-11 14:49 ` [PATCH 1/4] dmaengine: imx-sdma: fix buffer ownership Philipp Puschmann
  2019-09-11 14:49 ` [PATCH 2/4] dmaengine: imx-sdma: fix dma freezes Philipp Puschmann
@ 2019-09-11 14:49 ` Philipp Puschmann
  2019-09-16 14:24   ` Lucas Stach
  2019-09-11 14:49 ` [PATCH 4/4] dmaengine: imx-sdma: drop redundant variable Philipp Puschmann
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 31+ messages in thread
From: Philipp Puschmann @ 2019-09-11 14:49 UTC (permalink / raw)
  To: linux-kernel
  Cc: vkoul, dan.j.williams, shawnguo, s.hauer, kernel, festevam,
	linux-imx, gregkh, jslaby, dmaengine, linux-arm-kernel,
	linux-serial, Philipp Puschmann

Using only 4 DMA periods for UART RX is very few if we have a high
frequency of small transfers - like in our case using Bluetooth with many
small packets via UART - causing many dma transfers but in each only
filling a fraction of a single buffer. Such a case may lead to the
situation that DMA RX transfer is triggered but no buffer is available.
While we have addressed the dma handling already we still want to avoid
UART RX FIFO overrun. So we decrease the size of the buffers and increase
their number and the total buffer size.

Signed-off-by: Philipp Puschmann <philipp.puschmann@emlix.com>
---
 drivers/tty/serial/imx.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 57d6e6ba556e..cdc51569237c 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1028,8 +1028,6 @@ static void imx_uart_timeout(struct timer_list *t)
 	}
 }
 
-#define RX_BUF_SIZE	(PAGE_SIZE)
-
 /*
  * There are two kinds of RX DMA interrupts(such as in the MX6Q):
  *   [1] the RX DMA buffer is full.
@@ -1112,7 +1110,8 @@ static void imx_uart_dma_rx_callback(void *data)
 }
 
 /* RX DMA buffer periods */
-#define RX_DMA_PERIODS 4
+#define RX_DMA_PERIODS	16
+#define RX_BUF_SIZE	(PAGE_SIZE / 4)
 
 static int imx_uart_start_rx_dma(struct imx_port *sport)
 {
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 4/4] dmaengine: imx-sdma: drop redundant variable
  2019-09-11 14:49 [PATCH 0/4] Fix UART DMA freezes for iMX6 Philipp Puschmann
                   ` (2 preceding siblings ...)
  2019-09-11 14:49 ` [PATCH 3/4] serial: imx: adapt rx buffer and dma periods Philipp Puschmann
@ 2019-09-11 14:49 ` Philipp Puschmann
  2019-09-16 14:30   ` Lucas Stach
  2019-09-12 15:31 ` [PATCH 0/4] Fix UART DMA freezes for iMX6 Fabio Estevam
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 31+ messages in thread
From: Philipp Puschmann @ 2019-09-11 14:49 UTC (permalink / raw)
  To: linux-kernel
  Cc: vkoul, dan.j.williams, shawnguo, s.hauer, kernel, festevam,
	linux-imx, gregkh, jslaby, dmaengine, linux-arm-kernel,
	linux-serial, Philipp Puschmann

In sdma_prep_dma_cyclic buf is redundant. Drop it.

Signed-off-by: Philipp Puschmann <philipp.puschmann@emlix.com>
---
 drivers/dma/imx-sdma.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 6a5a84504858..5b6beeee9f0e 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -1544,7 +1544,7 @@ static struct dma_async_tx_descriptor *sdma_prep_dma_cyclic(
 	struct sdma_engine *sdma = sdmac->sdma;
 	int num_periods = buf_len / period_len;
 	int channel = sdmac->channel;
-	int i = 0, buf = 0;
+	int i;
 	struct sdma_desc *desc;
 
 	dev_dbg(sdma->dev, "%s channel: %d\n", __func__, channel);
@@ -1565,7 +1565,7 @@ static struct dma_async_tx_descriptor *sdma_prep_dma_cyclic(
 		goto err_bd_out;
 	}
 
-	while (buf < buf_len) {
+	for (i = 0; i < num_periods; i++) {
 		struct sdma_buffer_descriptor *bd = &desc->bd[i];
 		int param;
 
@@ -1592,9 +1592,6 @@ static struct dma_async_tx_descriptor *sdma_prep_dma_cyclic(
 		bd->mode.status = param;
 
 		dma_addr += period_len;
-		buf += period_len;
-
-		i++;
 	}
 
 	return vchan_tx_prep(&sdmac->vc, &desc->vd, flags);
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH 0/4] Fix UART DMA freezes for iMX6
  2019-09-11 14:49 [PATCH 0/4] Fix UART DMA freezes for iMX6 Philipp Puschmann
                   ` (3 preceding siblings ...)
  2019-09-11 14:49 ` [PATCH 4/4] dmaengine: imx-sdma: drop redundant variable Philipp Puschmann
@ 2019-09-12 15:31 ` Fabio Estevam
  2019-09-12 18:23 ` Fabio Estevam
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 31+ messages in thread
From: Fabio Estevam @ 2019-09-12 15:31 UTC (permalink / raw)
  To: Philipp Puschmann, Robin Gong, Fugang Duan
  Cc: linux-kernel, Vinod, Dan Williams, Shawn Guo, Sascha Hauer,
	Sascha Hauer, NXP Linux Team, Greg Kroah-Hartman, Jiri Slaby,
	dmaengine,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	linux-serial

[Adding Robin and Andy]

On Wed, Sep 11, 2019 at 11:50 AM Philipp Puschmann
<philipp.puschmann@emlix.com> wrote:
>
> For some years and since many kernel versions there are reports that
> RX UART DMA channel stops working at one point. So far the usual workaround was
> to disable RX DMA. This patches try to fix the underlying problem.
>
> When a running sdma script does not find any usable destination buffer to put
> its data into it just leads to stopping the channel being scheduled again. As
> solution we we manually retrigger the sdma script for this channel and by this
> dissolve the freeze.
>
> While this seems to work fine so far a further patch in this series increases
> the number of RX DMA periods for UART to reduce use cases running into such
> a situation.
>
> This patch series was tested with the current kernel and backported to
> kernel 4.15 with a special use case using a WL1837MOD via UART and provoking
> the hanging of UART RX DMA within seconds after starting a test application.
> It resulted in well known
>   "Bluetooth: hci0: command 0x0408 tx timeout"
> errors and complete stop of UART data reception. Our Bluetooth traffic consists
> of many independent small packets, mostly only a few bytes, causing high usage
> of periods.
>
>
> Philipp Puschmann (4):
>   dmaengine: imx-sdma: fix buffer ownership
>   dmaengine: imx-sdma: fix dma freezes
>   serial: imx: adapt rx buffer and dma periods
>   dmaengine: imx-sdma: drop redundant variable
>
>  drivers/dma/imx-sdma.c   | 32 ++++++++++++++++++++++----------
>  drivers/tty/serial/imx.c |  5 ++---
>  2 files changed, 24 insertions(+), 13 deletions(-)
>
> --
> 2.23.0
>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 0/4] Fix UART DMA freezes for iMX6
  2019-09-11 14:49 [PATCH 0/4] Fix UART DMA freezes for iMX6 Philipp Puschmann
                   ` (4 preceding siblings ...)
  2019-09-12 15:31 ` [PATCH 0/4] Fix UART DMA freezes for iMX6 Fabio Estevam
@ 2019-09-12 18:23 ` Fabio Estevam
  2019-09-16 13:55   ` Philipp Puschmann
  2019-09-16  8:02 ` Robin Gong
  2019-09-19 10:23 ` [PATCH v2 0/3] Fix UART DMA freezes for i.MX SOCs Philipp Puschmann
  7 siblings, 1 reply; 31+ messages in thread
From: Fabio Estevam @ 2019-09-12 18:23 UTC (permalink / raw)
  To: Philipp Puschmann, Robin Gong, Fugang Duan
  Cc: linux-kernel, Vinod, Dan Williams, Shawn Guo, Sascha Hauer,
	Sascha Hauer, NXP Linux Team, Greg Kroah-Hartman, Jiri Slaby,
	dmaengine,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	linux-serial

Hi Philipp,

Thanks for submitting these fixes.

On Wed, Sep 11, 2019 at 11:50 AM Philipp Puschmann
<philipp.puschmann@emlix.com> wrote:
>
> For some years and since many kernel versions there are reports that
> RX UART DMA channel stops working at one point. So far the usual workaround was
> to disable RX DMA. This patches try to fix the underlying problem.
>
> When a running sdma script does not find any usable destination buffer to put
> its data into it just leads to stopping the channel being scheduled again. As
> solution we we manually retrigger the sdma script for this channel and by this
> dissolve the freeze.
>
> While this seems to work fine so far a further patch in this series increases
> the number of RX DMA periods for UART to reduce use cases running into such
> a situation.
>
> This patch series was tested with the current kernel and backported to
> kernel 4.15 with a special use case using a WL1837MOD via UART and provoking
> the hanging of UART RX DMA within seconds after starting a test application.
> It resulted in well known
>   "Bluetooth: hci0: command 0x0408 tx timeout"
> errors and complete stop of UART data reception. Our Bluetooth traffic consists
> of many independent small packets, mostly only a few bytes, causing high usage
> of periods.
>
>
> Philipp Puschmann (4):
>   dmaengine: imx-sdma: fix buffer ownership
>   dmaengine: imx-sdma: fix dma freezes
>   serial: imx: adapt rx buffer and dma periods
>   dmaengine: imx-sdma: drop redundant variable

I have some suggestions:

1. Please split this in two series: one for dmaengine and other one for serial

2. Please add Fixes tag when appropriate, so that the fixes can be
backported to stable kernels.

3. Please Cc Robin and Andy

Thanks

^ permalink raw reply	[flat|nested] 31+ messages in thread

* RE: [PATCH 0/4] Fix UART DMA freezes for iMX6
  2019-09-11 14:49 [PATCH 0/4] Fix UART DMA freezes for iMX6 Philipp Puschmann
                   ` (5 preceding siblings ...)
  2019-09-12 18:23 ` Fabio Estevam
@ 2019-09-16  8:02 ` Robin Gong
  2019-09-16 13:41   ` Philipp Puschmann
  2019-09-19 10:23 ` [PATCH v2 0/3] Fix UART DMA freezes for i.MX SOCs Philipp Puschmann
  7 siblings, 1 reply; 31+ messages in thread
From: Robin Gong @ 2019-09-16  8:02 UTC (permalink / raw)
  To: Philipp Puschmann, linux-kernel
  Cc: vkoul, dan.j.williams, shawnguo, s.hauer, kernel, festevam,
	dl-linux-imx, gregkh, jslaby, dmaengine, linux-arm-kernel,
	linux-serial

On 2019/9/11 Philipp Puschmann <philipp.puschmann@emlix.com> wrote:
> For some years and since many kernel versions there are reports that RX
> UART DMA channel stops working at one point. So far the usual workaround
> was to disable RX DMA. This patches try to fix the underlying problem.
> 
> When a running sdma script does not find any usable destination buffer to put
> its data into it just leads to stopping the channel being scheduled again. As
> solution we we manually retrigger the sdma script for this channel and by this
> dissolve the freeze.
> 
> While this seems to work fine so far a further patch in this series increases the
> number of RX DMA periods for UART to reduce use cases running into such a
> situation.
> 
> This patch series was tested with the current kernel and backported to kernel
> 4.15 with a special use case using a WL1837MOD via UART and provoking the
Hi Philipp, Could your Bluetooth issue be reproduce on latest linux-next? Or did
your kernel which can be reproduced include the below patch?

commit d1a792f3b4072bfac4150bb62aa34917b77fdb6d
Author: Russell King - ARM Linux <linux@arm.linux.org.uk>
Date:   Wed Jun 25 13:00:33 2014 +0100

    Update imx-sdma cyclic handling to report residue
> hanging of UART RX DMA within seconds after starting a test application.
> It resulted in well known
>   "Bluetooth: hci0: command 0x0408 tx timeout"
> errors and complete stop of UART data reception. Our Bluetooth traffic
> consists of many independent small packets, mostly only a few bytes, causing
> high usage of periods.
> 
> 
> Philipp Puschmann (4):
>   dmaengine: imx-sdma: fix buffer ownership
>   dmaengine: imx-sdma: fix dma freezes
>   serial: imx: adapt rx buffer and dma periods
>   dmaengine: imx-sdma: drop redundant variable
> 
>  drivers/dma/imx-sdma.c   | 32 ++++++++++++++++++++++----------
>  drivers/tty/serial/imx.c |  5 ++---
>  2 files changed, 24 insertions(+), 13 deletions(-)
> 
> --
> 2.23.0


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 0/4] Fix UART DMA freezes for iMX6
  2019-09-16  8:02 ` Robin Gong
@ 2019-09-16 13:41   ` Philipp Puschmann
  0 siblings, 0 replies; 31+ messages in thread
From: Philipp Puschmann @ 2019-09-16 13:41 UTC (permalink / raw)
  To: Robin Gong, linux-kernel
  Cc: vkoul, dan.j.williams, shawnguo, s.hauer, kernel, festevam,
	dl-linux-imx, gregkh, jslaby, dmaengine, linux-arm-kernel,
	linux-serial

Am 16.09.19 um 10:02 schrieb Robin Gong:
> On 2019/9/11 Philipp Puschmann <philipp.puschmann@emlix.com> wrote:
>> For some years and since many kernel versions there are reports that RX
>> UART DMA channel stops working at one point. So far the usual workaround
>> was to disable RX DMA. This patches try to fix the underlying problem.
>>
>> When a running sdma script does not find any usable destination buffer to put
>> its data into it just leads to stopping the channel being scheduled again. As
>> solution we we manually retrigger the sdma script for this channel and by this
>> dissolve the freeze.
>>
>> While this seems to work fine so far a further patch in this series increases the
>> number of RX DMA periods for UART to reduce use cases running into such a
>> situation.
>>
>> This patch series was tested with the current kernel and backported to kernel
>> 4.15 with a special use case using a WL1837MOD via UART and provoking the
> Hi Philipp, Could your Bluetooth issue be reproduce on latest linux-next? Or did
> your kernel which can be reproduced include the below patch?
> 
> commit d1a792f3b4072bfac4150bb62aa34917b77fdb6d
> Author: Russell King - ARM Linux <linux@arm.linux.org.uk>
> Date:   Wed Jun 25 13:00:33 2014 +0100
> 

Hi Robin, yes i have reproduced the Bluetooth issue with my test case with kernel 4.15
and the newest 5.3.0-rc8-next-20190915. My test-case includes several custom-boards
communicating via Bluetooth with each other. I see the error within seconds to few minutes.

>     Update imx-sdma cyclic handling to report residue
>> hanging of UART RX DMA within seconds after starting a test application.
>> It resulted in well known
>>   "Bluetooth: hci0: command 0x0408 tx timeout"
>> errors and complete stop of UART data reception. Our Bluetooth traffic
>> consists of many independent small packets, mostly only a few bytes, causing
>> high usage of periods.
>>
>>
>> Philipp Puschmann (4):
>>   dmaengine: imx-sdma: fix buffer ownership
>>   dmaengine: imx-sdma: fix dma freezes
>>   serial: imx: adapt rx buffer and dma periods
>>   dmaengine: imx-sdma: drop redundant variable
>>
>>  drivers/dma/imx-sdma.c   | 32 ++++++++++++++++++++++----------
>>  drivers/tty/serial/imx.c |  5 ++---
>>  2 files changed, 24 insertions(+), 13 deletions(-)
>>
>> --
>> 2.23.0
> 

-- 

Philipp Puschmann, emlix GmbH, http://www.emlix.com
Fon +49 551 30664-0, Fax +49 551 30664-11
Gothaer Platz 3, 37083 Göttingen, Germany
Sitz der Gesellschaft: Göttingen, Amtsgericht Goettingen HR B 3160
Geschaeftsführung: Heike Jordan, Dr. Uwe Kracke
Ust-IdNr.: DE 205 198 055

emlix - smart embedded open source

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 0/4] Fix UART DMA freezes for iMX6
  2019-09-12 18:23 ` Fabio Estevam
@ 2019-09-16 13:55   ` Philipp Puschmann
  2019-09-16 14:00     ` Fabio Estevam
  2019-09-16 14:10     ` [EXT] " Andy Duan
  0 siblings, 2 replies; 31+ messages in thread
From: Philipp Puschmann @ 2019-09-16 13:55 UTC (permalink / raw)
  To: Fabio Estevam, Robin Gong, Fugang Duan
  Cc: linux-kernel, Vinod, Dan Williams, Shawn Guo, Sascha Hauer,
	NXP Linux Team, Greg Kroah-Hartman, Jiri Slaby, dmaengine,
	linux-arm-kernel, linux-serial

Hi Fabio,

Am 12.09.19 um 20:23 schrieb Fabio Estevam:
> Hi Philipp,
> 
> Thanks for submitting these fixes.
> 
> On Wed, Sep 11, 2019 at 11:50 AM Philipp Puschmann
> <philipp.puschmann@emlix.com> wrote:
>>
>> For some years and since many kernel versions there are reports that
>> RX UART DMA channel stops working at one point. So far the usual workaround was
>> to disable RX DMA. This patches try to fix the underlying problem.
>>
>> When a running sdma script does not find any usable destination buffer to put
>> its data into it just leads to stopping the channel being scheduled again. As
>> solution we we manually retrigger the sdma script for this channel and by this
>> dissolve the freeze.
>>
>> While this seems to work fine so far a further patch in this series increases
>> the number of RX DMA periods for UART to reduce use cases running into such
>> a situation.
>>
>> This patch series was tested with the current kernel and backported to
>> kernel 4.15 with a special use case using a WL1837MOD via UART and provoking
>> the hanging of UART RX DMA within seconds after starting a test application.
>> It resulted in well known
>>   "Bluetooth: hci0: command 0x0408 tx timeout"
>> errors and complete stop of UART data reception. Our Bluetooth traffic consists
>> of many independent small packets, mostly only a few bytes, causing high usage
>> of periods.
>>
>>
>> Philipp Puschmann (4):
>>   dmaengine: imx-sdma: fix buffer ownership
>>   dmaengine: imx-sdma: fix dma freezes
>>   serial: imx: adapt rx buffer and dma periods
>>   dmaengine: imx-sdma: drop redundant variable
> 
> I have some suggestions:
> 
> 1. Please split this in two series: one for dmaengine and other one for serial
> 
> 2. Please add Fixes tag when appropriate, so that the fixes can be
> backported to stable kernels.
> 
> 3. Please Cc Robin and Andy
> 
> Thanks
> 

Thanks for the hints. I will apply them if the contentual feedback is positive.

p.s. Did you forget to add Andy? I don't see a Andy in the to- and cc-list.


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 0/4] Fix UART DMA freezes for iMX6
  2019-09-16 13:55   ` Philipp Puschmann
@ 2019-09-16 14:00     ` Fabio Estevam
  2019-09-16 14:10     ` [EXT] " Andy Duan
  1 sibling, 0 replies; 31+ messages in thread
From: Fabio Estevam @ 2019-09-16 14:00 UTC (permalink / raw)
  To: Philipp Puschmann
  Cc: Robin Gong, Fugang Duan, linux-kernel, Vinod, Dan Williams,
	Shawn Guo, Sascha Hauer, NXP Linux Team, Greg Kroah-Hartman,
	Jiri Slaby, dmaengine,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	linux-serial

Hi Philipp,

On Mon, Sep 16, 2019 at 10:55 AM Philipp Puschmann
<philipp.puschmann@emlix.com> wrote:

> Thanks for the hints. I will apply them if the contentual feedback is positive.
>
> p.s. Did you forget to add Andy? I don't see a Andy in the to- and cc-list.

Andy's e-mail is fugang.duan@nxp.com, which I added on Cc.

I think your patches look good and are in good shape to be resubmitted.

Thanks for fixing these hard to debug issues!

^ permalink raw reply	[flat|nested] 31+ messages in thread

* RE: [EXT] Re: [PATCH 0/4] Fix UART DMA freezes for iMX6
  2019-09-16 13:55   ` Philipp Puschmann
  2019-09-16 14:00     ` Fabio Estevam
@ 2019-09-16 14:10     ` Andy Duan
  1 sibling, 0 replies; 31+ messages in thread
From: Andy Duan @ 2019-09-16 14:10 UTC (permalink / raw)
  To: Philipp Puschmann, Fabio Estevam, Robin Gong
  Cc: linux-kernel, Vinod, Dan Williams, Shawn Guo, Sascha Hauer,
	dl-linux-imx, Greg Kroah-Hartman, Jiri Slaby, dmaengine,
	linux-arm-kernel, linux-serial

From: Philipp Puschmann <philipp.puschmann@emlix.com> Sent: Monday, September 16, 2019 9:55 PM
> Hi Fabio,
> 
> Am 12.09.19 um 20:23 schrieb Fabio Estevam:
> > Hi Philipp,
> >
> > Thanks for submitting these fixes.
> >
> > On Wed, Sep 11, 2019 at 11:50 AM Philipp Puschmann
> > <philipp.puschmann@emlix.com> wrote:
> >>
> >> For some years and since many kernel versions there are reports that
> >> RX UART DMA channel stops working at one point. So far the usual
> >> workaround was to disable RX DMA. This patches try to fix the underlying
> problem.
> >>
> >> When a running sdma script does not find any usable destination
> >> buffer to put its data into it just leads to stopping the channel
> >> being scheduled again. As solution we we manually retrigger the sdma
> >> script for this channel and by this dissolve the freeze.
> >>
> >> While this seems to work fine so far a further patch in this series
> >> increases the number of RX DMA periods for UART to reduce use cases
> >> running into such a situation.
> >>
> >> This patch series was tested with the current kernel and backported
> >> to kernel 4.15 with a special use case using a WL1837MOD via UART and
> >> provoking the hanging of UART RX DMA within seconds after starting a test
> application.
> >> It resulted in well known
> >>   "Bluetooth: hci0: command 0x0408 tx timeout"
> >> errors and complete stop of UART data reception. Our Bluetooth
> >> traffic consists of many independent small packets, mostly only a few
> >> bytes, causing high usage of periods.
> >>
> >>
> >> Philipp Puschmann (4):
> >>   dmaengine: imx-sdma: fix buffer ownership
> >>   dmaengine: imx-sdma: fix dma freezes
> >>   serial: imx: adapt rx buffer and dma periods
> >>   dmaengine: imx-sdma: drop redundant variable
> >
> > I have some suggestions:
> >
> > 1. Please split this in two series: one for dmaengine and other one
> > for serial
> >
> > 2. Please add Fixes tag when appropriate, so that the fixes can be
> > backported to stable kernels.
> >
> > 3. Please Cc Robin and Andy
> >
> > Thanks
> >
> 
> Thanks for the hints. I will apply them if the contentual feedback is positive.
> 
> p.s. Did you forget to add Andy? I don't see a Andy in the to- and cc-list.

For dma and uart, please to- me and yibin.gong@nxp.com, thanks.

Andy

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 1/4] dmaengine: imx-sdma: fix buffer ownership
  2019-09-11 14:49 ` [PATCH 1/4] dmaengine: imx-sdma: fix buffer ownership Philipp Puschmann
@ 2019-09-16 14:17   ` Lucas Stach
  2019-09-19  9:20     ` Philipp Puschmann
  0 siblings, 1 reply; 31+ messages in thread
From: Lucas Stach @ 2019-09-16 14:17 UTC (permalink / raw)
  To: Philipp Puschmann, linux-kernel
  Cc: linux-serial, shawnguo, s.hauer, jslaby, vkoul, linux-imx,
	kernel, gregkh, dmaengine, dan.j.williams, festevam,
	linux-arm-kernel

On Mi, 2019-09-11 at 16:49 +0200, Philipp Puschmann wrote:
> BD_DONE flag marks ownership of the buffer. When 1 SDMA owns the buffer,
> when 0 ARM owns it. When processing the buffers in
> sdma_update_channel_loop the ownership of the currently processed buffer
> was set to SDMA again before running the callback function of the the
> buffer and while the sdma script may be running in parallel. So there was
> the possibility to get the buffer overwritten by SDMA before it has been
> processed by kernel leading to kind of random errors in the upper layers,
> e.g. bluetooth.
> 
> It may be further a good idea to make the status struct member volatile or
> access it using writel or similar to rule out that the compiler sets the
> BD_DONE flag before the callback routine has finished.
> 
> Signed-off-by: Philipp Puschmann <philipp.puschmann@emlix.com>
> ---
>  drivers/dma/imx-sdma.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index a01f4b5d793c..1abb14ff394d 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -802,7 +802,6 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
>  		*/
>  
>  		desc->chn_real_count = bd->mode.count;
> -		bd->mode.status |= BD_DONE;
>  		bd->mode.count = desc->period_len;
>  		desc->buf_ptail = desc->buf_tail;
>  		desc->buf_tail = (desc->buf_tail + 1) % desc->num_bd;
> @@ -817,6 +816,8 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
>  		dmaengine_desc_get_callback_invoke(&desc->vd.tx, NULL);
>  		spin_lock(&sdmac->vc.lock);

To address your comment from the second paragraph of the commit message
there should be a dma_wmb() here before changing the status flag.

Regards,
Lucas

> +		bd->mode.status |= BD_DONE;
> +
>  		if (error)
>  			sdmac->status = old_status;
>  	}


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 2/4] dmaengine: imx-sdma: fix dma freezes
  2019-09-11 14:49 ` [PATCH 2/4] dmaengine: imx-sdma: fix dma freezes Philipp Puschmann
@ 2019-09-16 14:22   ` Lucas Stach
  0 siblings, 0 replies; 31+ messages in thread
From: Lucas Stach @ 2019-09-16 14:22 UTC (permalink / raw)
  To: Philipp Puschmann, linux-kernel
  Cc: linux-serial, shawnguo, s.hauer, jslaby, vkoul, linux-imx,
	kernel, gregkh, dmaengine, dan.j.williams, festevam,
	linux-arm-kernel

On Mi, 2019-09-11 at 16:49 +0200, Philipp Puschmann wrote:
> For some years and since many kernel versions there are reports that the
> RX UART SDMA channel stops working at some point. The workaround was to
> disable DMA for RX. This commit tries to fix the problem itself.
> 
> Due to its license i wasn't able to debug the sdma script itself but it
> somehow leads to blocking the scheduling of the channel script when a
> running sdma script does not find any usable destination buffer to put its
> data into.
> 
> If we detect such a potential case we manually retrigger the sdma script
> for this channel and by this reenable the scipt being run by scheduler.
> 
> As sdmac->desc is constant we can move desc out of the loop.
> 
> Signed-off-by: Philipp Puschmann <philipp.puschmann@emlix.com>
> ---
>  drivers/dma/imx-sdma.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index 1abb14ff394d..6a5a84504858 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -775,21 +775,23 @@ static void sdma_start_desc(struct sdma_channel *sdmac)
>  static void sdma_update_channel_loop(struct sdma_channel *sdmac)
>  {
>  	struct sdma_buffer_descriptor *bd;
> -	int error = 0;
> -	enum dma_status	old_status = sdmac->status;
> +	struct sdma_desc *desc = sdmac->desc;
> +	int error = 0, cnt = 0;
> +	enum dma_status old_status = sdmac->status;
>  
>  	/*
>  	 * loop mode. Iterate over descriptors, re-setup them and
>  	 * call callback function.
>  	 */
> -	while (sdmac->desc) {
> -		struct sdma_desc *desc = sdmac->desc;
> +	while (desc) {
>  
>  		bd = &desc->bd[desc->buf_tail];
>  
>  		if (bd->mode.status & BD_DONE)
>  			break;
>  
> +		cnt++;
> +
>  		if (bd->mode.status & BD_RROR) {
>  			bd->mode.status &= ~BD_RROR;
>  			sdmac->status = DMA_ERROR;
> @@ -821,6 +823,18 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
>  		if (error)
>  			sdmac->status = old_status;
>  	}
> +
> +	/* In some situations it happens that the sdma stops serving
> +	 * dma interrupt requests. It happens when running dma script
> +	 * does not find any usable destination buffer to put its data into.
> +	 *

This part of the comment is slightly confusing, as what happens is that
the SDMA channel is stopped when there are no free descriptors in the
ring anymore. After the channel is stopped it needs to be kicked back
to life after there are descriptors available again.

But apart from this nitpick the change looks good to me:
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

Regards,
Lucas

> +	 * While there is no specific error condition we can check for, a
> +	 * necessary condition is that all available buffers for the current
> +	 * channel have been written to by the sdma script. In such a case we
> +	 * will trigger the sdma script manually.
> +	 */
> +	if (cnt >= desc->num_bd)
> +		sdma_enable_channel(sdmac->sdma, sdmac->channel);
>  }
>  
>  static void mxc_sdma_handle_channel_normal(struct sdma_channel *data)


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 3/4] serial: imx: adapt rx buffer and dma periods
  2019-09-11 14:49 ` [PATCH 3/4] serial: imx: adapt rx buffer and dma periods Philipp Puschmann
@ 2019-09-16 14:24   ` Lucas Stach
  0 siblings, 0 replies; 31+ messages in thread
From: Lucas Stach @ 2019-09-16 14:24 UTC (permalink / raw)
  To: Philipp Puschmann, linux-kernel
  Cc: linux-serial, shawnguo, s.hauer, jslaby, vkoul, linux-imx,
	kernel, gregkh, dmaengine, dan.j.williams, festevam,
	linux-arm-kernel

On Mi, 2019-09-11 at 16:49 +0200, Philipp Puschmann wrote:
> Using only 4 DMA periods for UART RX is very few if we have a high
> frequency of small transfers - like in our case using Bluetooth with many
> small packets via UART - causing many dma transfers but in each only
> filling a fraction of a single buffer. Such a case may lead to the
> situation that DMA RX transfer is triggered but no buffer is available.
> While we have addressed the dma handling already we still want to avoid
> UART RX FIFO overrun. So we decrease the size of the buffers and increase
> their number and the total buffer size.
> 
> Signed-off-by: Philipp Puschmann <philipp.puschmann@emlix.com>

Reasoning seems sound:

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
>  drivers/tty/serial/imx.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 57d6e6ba556e..cdc51569237c 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -1028,8 +1028,6 @@ static void imx_uart_timeout(struct timer_list *t)
>  	}
>  }
>  
> -#define RX_BUF_SIZE	(PAGE_SIZE)
> -
>  /*
>   * There are two kinds of RX DMA interrupts(such as in the MX6Q):
>   *   [1] the RX DMA buffer is full.
> @@ -1112,7 +1110,8 @@ static void imx_uart_dma_rx_callback(void *data)
>  }
>  
>  /* RX DMA buffer periods */
> -#define RX_DMA_PERIODS 4
> +#define RX_DMA_PERIODS	16
> +#define RX_BUF_SIZE	(PAGE_SIZE / 4)
>  
>  static int imx_uart_start_rx_dma(struct imx_port *sport)
>  {


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 4/4] dmaengine: imx-sdma: drop redundant variable
  2019-09-11 14:49 ` [PATCH 4/4] dmaengine: imx-sdma: drop redundant variable Philipp Puschmann
@ 2019-09-16 14:30   ` Lucas Stach
  0 siblings, 0 replies; 31+ messages in thread
From: Lucas Stach @ 2019-09-16 14:30 UTC (permalink / raw)
  To: Philipp Puschmann, linux-kernel
  Cc: linux-serial, shawnguo, s.hauer, jslaby, vkoul, linux-imx,
	kernel, gregkh, dmaengine, dan.j.williams, festevam,
	linux-arm-kernel

On Mi, 2019-09-11 at 16:49 +0200, Philipp Puschmann wrote:
> In sdma_prep_dma_cyclic buf is redundant. Drop it.
> 
> Signed-off-by: Philipp Puschmann <philipp.puschmann@emlix.com>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
>  drivers/dma/imx-sdma.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index 6a5a84504858..5b6beeee9f0e 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -1544,7 +1544,7 @@ static struct dma_async_tx_descriptor
> *sdma_prep_dma_cyclic(
>  	struct sdma_engine *sdma = sdmac->sdma;
>  	int num_periods = buf_len / period_len;
>  	int channel = sdmac->channel;
> -	int i = 0, buf = 0;
> +	int i;
>  	struct sdma_desc *desc;
>  
>  	dev_dbg(sdma->dev, "%s channel: %d\n", __func__, channel);
> @@ -1565,7 +1565,7 @@ static struct dma_async_tx_descriptor
> *sdma_prep_dma_cyclic(
>  		goto err_bd_out;
>  	}
>  
> -	while (buf < buf_len) {
> +	for (i = 0; i < num_periods; i++) {
>  		struct sdma_buffer_descriptor *bd = &desc->bd[i];
>  		int param;
>  
> @@ -1592,9 +1592,6 @@ static struct dma_async_tx_descriptor
> *sdma_prep_dma_cyclic(
>  		bd->mode.status = param;
>  
>  		dma_addr += period_len;
> -		buf += period_len;
> -
> -		i++;
>  	}
>  
>  	return vchan_tx_prep(&sdmac->vc, &desc->vd, flags);


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 1/4] dmaengine: imx-sdma: fix buffer ownership
  2019-09-16 14:17   ` Lucas Stach
@ 2019-09-19  9:20     ` Philipp Puschmann
  0 siblings, 0 replies; 31+ messages in thread
From: Philipp Puschmann @ 2019-09-19  9:20 UTC (permalink / raw)
  To: Lucas Stach, linux-kernel
  Cc: linux-serial, shawnguo, s.hauer, jslaby, vkoul, linux-imx,
	kernel, gregkh, dmaengine, dan.j.williams, festevam,
	linux-arm-kernel

Am 16.09.19 um 16:17 schrieb Lucas Stach:
> On Mi, 2019-09-11 at 16:49 +0200, Philipp Puschmann wrote:
>> BD_DONE flag marks ownership of the buffer. When 1 SDMA owns the buffer,
>> when 0 ARM owns it. When processing the buffers in
>> sdma_update_channel_loop the ownership of the currently processed buffer
>> was set to SDMA again before running the callback function of the the
>> buffer and while the sdma script may be running in parallel. So there was
>> the possibility to get the buffer overwritten by SDMA before it has been
>> processed by kernel leading to kind of random errors in the upper layers,
>> e.g. bluetooth.
>>
>> It may be further a good idea to make the status struct member volatile or
>> access it using writel or similar to rule out that the compiler sets the
>> BD_DONE flag before the callback routine has finished.
>>
>> Signed-off-by: Philipp Puschmann <philipp.puschmann@emlix.com>
>> ---
>>  drivers/dma/imx-sdma.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
>> index a01f4b5d793c..1abb14ff394d 100644
>> --- a/drivers/dma/imx-sdma.c
>> +++ b/drivers/dma/imx-sdma.c
>> @@ -802,7 +802,6 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
>>  		*/
>>  
>>  		desc->chn_real_count = bd->mode.count;
>> -		bd->mode.status |= BD_DONE;
>>  		bd->mode.count = desc->period_len;
>>  		desc->buf_ptail = desc->buf_tail;
>>  		desc->buf_tail = (desc->buf_tail + 1) % desc->num_bd;
>> @@ -817,6 +816,8 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
>>  		dmaengine_desc_get_callback_invoke(&desc->vd.tx, NULL);
>>  		spin_lock(&sdmac->vc.lock);
> 
> To address your comment from the second paragraph of the commit message
> there should be a dma_wmb() here before changing the status flag.
> 
> Regards,
> Lucas

Hi Lucas,

thanks for your feedback. I will apply the hints to v2 of the patches.

Regards,
Philipp
> 
>> +		bd->mode.status |= BD_DONE;
>> +
>>  		if (error)
>>  			sdmac->status = old_status;
>>  	}
> 

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v2 0/3] Fix UART DMA freezes for i.MX SOCs
  2019-09-11 14:49 [PATCH 0/4] Fix UART DMA freezes for iMX6 Philipp Puschmann
                   ` (6 preceding siblings ...)
  2019-09-16  8:02 ` Robin Gong
@ 2019-09-19 10:23 ` Philipp Puschmann
  2019-09-19 10:23   ` [PATCH v2 1/3] dmaengine: imx-sdma: fix buffer ownership Philipp Puschmann
                     ` (4 more replies)
  7 siblings, 5 replies; 31+ messages in thread
From: Philipp Puschmann @ 2019-09-19 10:23 UTC (permalink / raw)
  To: linux-kernel
  Cc: yibin.gong, fugang.duan, l.stach, dan.j.williams, vkoul,
	shawnguo, s.hauer, kernel, festevam, linux-imx, dmaengine,
	linux-arm-kernel, Philipp Puschmann

For some years and since many kernel versions there are reports that
RX UART DMA channel stops working at one point. So far the usual
workaround was to disable RX DMA. This patches fix the underlying
problem.

When a running sdma script does not find any usable destination buffer
to put its data into it just leads to stopping the channel being
scheduled again. As solution we manually retrigger the sdma script for
this channel and by this dissolve the freeze.

While this seems to work fine so far, it may come to buffer overruns
when the channel - even temporary - is stopped. This case has to be
addressed by device drivers by increasing the number of DMA periods.

This patch series was tested with the current kernel and backported to
kernel 4.15 with a special use case using a WL1837MOD via UART and
provoking the hanging of UART RX DMA within seconds after starting a
test application. It resulted in well known
  "Bluetooth: hci0: command 0x0408 tx timeout"
errors and complete stop of UART data reception. Our Bluetooth traffic
consists of many independent small packets, mostly only a few bytes,
causing high usage of periods.

Changelog v2:
 - adapt title (this patches are not only for i.MX6)
 - improve some comments and patch descriptions
 - add a dma_wb() around BD_DONE flag
 - add Reviewed-by tags
 - split off  "serial: imx: adapt rx buffer and dma periods"

Philipp Puschmann (3):
  dmaengine: imx-sdma: fix buffer ownership
  dmaengine: imx-sdma: fix dma freezes
  dmaengine: imx-sdma: drop redundant variable

 drivers/dma/imx-sdma.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

-- 
2.23.0


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v2 1/3] dmaengine: imx-sdma: fix buffer ownership
  2019-09-19 10:23 ` [PATCH v2 0/3] Fix UART DMA freezes for i.MX SOCs Philipp Puschmann
@ 2019-09-19 10:23   ` Philipp Puschmann
  2019-09-19 10:27     ` Lucas Stach
  2019-09-21 15:15     ` kbuild test robot
  2019-09-19 10:23   ` [PATCH v2 2/3] dmaengine: imx-sdma: fix dma freezes Philipp Puschmann
                     ` (3 subsequent siblings)
  4 siblings, 2 replies; 31+ messages in thread
From: Philipp Puschmann @ 2019-09-19 10:23 UTC (permalink / raw)
  To: linux-kernel
  Cc: yibin.gong, fugang.duan, l.stach, dan.j.williams, vkoul,
	shawnguo, s.hauer, kernel, festevam, linux-imx, dmaengine,
	linux-arm-kernel, Philipp Puschmann

BD_DONE flag marks ownership of the buffer. When 1 SDMA owns the
buffer, when 0 ARM owns it. When processing the buffers in
sdma_update_channel_loop the ownership of the currently processed
buffer was set to SDMA again before running the callback function of
the buffer and while the sdma script may be running in parallel. So
there was the possibility to get the buffer overwritten by SDMA before
it has been processed by kernel leading to kind of random errors in the
upper layers, e.g. bluetooth.

Signed-off-by: Philipp Puschmann <philipp.puschmann@emlix.com>

---

Changelog v2:
 - add dma_wb()

 drivers/dma/imx-sdma.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 9ba74ab7e912..e029a2443cfc 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -802,7 +802,6 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
 		*/
 
 		desc->chn_real_count = bd->mode.count;
-		bd->mode.status |= BD_DONE;
 		bd->mode.count = desc->period_len;
 		desc->buf_ptail = desc->buf_tail;
 		desc->buf_tail = (desc->buf_tail + 1) % desc->num_bd;
@@ -817,6 +816,9 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
 		dmaengine_desc_get_callback_invoke(&desc->vd.tx, NULL);
 		spin_lock(&sdmac->vc.lock);
 
+		dma_wb();
+		bd->mode.status |= BD_DONE;
+
 		if (error)
 			sdmac->status = old_status;
 	}
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v2 2/3] dmaengine: imx-sdma: fix dma freezes
  2019-09-19 10:23 ` [PATCH v2 0/3] Fix UART DMA freezes for i.MX SOCs Philipp Puschmann
  2019-09-19 10:23   ` [PATCH v2 1/3] dmaengine: imx-sdma: fix buffer ownership Philipp Puschmann
@ 2019-09-19 10:23   ` Philipp Puschmann
  2019-09-19 10:23   ` [PATCH v2 3/3] dmaengine: imx-sdma: drop redundant variable Philipp Puschmann
                     ` (2 subsequent siblings)
  4 siblings, 0 replies; 31+ messages in thread
From: Philipp Puschmann @ 2019-09-19 10:23 UTC (permalink / raw)
  To: linux-kernel
  Cc: yibin.gong, fugang.duan, l.stach, dan.j.williams, vkoul,
	shawnguo, s.hauer, kernel, festevam, linux-imx, dmaengine,
	linux-arm-kernel, Philipp Puschmann

For some years and since many kernel versions there are reports that the
RX UART SDMA channel stops working at some point. The workaround was to
disable DMA for RX. This commit tries to fix the problem itself.

Due to its license i wasn't able to debug the sdma script itself but it
somehow leads to blocking the scheduling of the channel script when a
running sdma script does not find any free descriptor in the ring to put
its data into.

If we detect such a potential case we manually restart the channel.

As sdmac->desc is constant we can move desc out of the loop.

Signed-off-by: Philipp Puschmann <philipp.puschmann@emlix.com>
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
---

Changelog v2:
 - clarify comment and commit description

 drivers/dma/imx-sdma.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index e029a2443cfc..a32b5962630e 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -775,21 +775,23 @@ static void sdma_start_desc(struct sdma_channel *sdmac)
 static void sdma_update_channel_loop(struct sdma_channel *sdmac)
 {
 	struct sdma_buffer_descriptor *bd;
-	int error = 0;
-	enum dma_status	old_status = sdmac->status;
+	struct sdma_desc *desc = sdmac->desc;
+	int error = 0, cnt = 0;
+	enum dma_status old_status = sdmac->status;
 
 	/*
 	 * loop mode. Iterate over descriptors, re-setup them and
 	 * call callback function.
 	 */
-	while (sdmac->desc) {
-		struct sdma_desc *desc = sdmac->desc;
+	while (desc) {
 
 		bd = &desc->bd[desc->buf_tail];
 
 		if (bd->mode.status & BD_DONE)
 			break;
 
+		cnt++;
+
 		if (bd->mode.status & BD_RROR) {
 			bd->mode.status &= ~BD_RROR;
 			sdmac->status = DMA_ERROR;
@@ -822,6 +824,17 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
 		if (error)
 			sdmac->status = old_status;
 	}
+
+	/* In some situations it may happen that the sdma does not found any
+	 * usable descriptor in the ring to put data into. The channel is
+	 * stopped then. While there is no specific error condition we can
+	 * check for, a necessary condition is that all available buffers for
+	 * the current channel have been written to by the sdma script. In
+	 * this case and after we have made the buffers available again,
+	 * we restart the channel.
+	 */
+	if (cnt >= desc->num_bd)
+		sdma_enable_channel(sdmac->sdma, sdmac->channel);
 }
 
 static void mxc_sdma_handle_channel_normal(struct sdma_channel *data)
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v2 3/3] dmaengine: imx-sdma: drop redundant variable
  2019-09-19 10:23 ` [PATCH v2 0/3] Fix UART DMA freezes for i.MX SOCs Philipp Puschmann
  2019-09-19 10:23   ` [PATCH v2 1/3] dmaengine: imx-sdma: fix buffer ownership Philipp Puschmann
  2019-09-19 10:23   ` [PATCH v2 2/3] dmaengine: imx-sdma: fix dma freezes Philipp Puschmann
@ 2019-09-19 10:23   ` Philipp Puschmann
  2019-09-19 10:33   ` [PATCH v2 0/3] Fix UART DMA freezes for i.MX SOCs Fabio Estevam
  2019-09-19 10:45   ` [PATCH v3 " Philipp Puschmann
  4 siblings, 0 replies; 31+ messages in thread
From: Philipp Puschmann @ 2019-09-19 10:23 UTC (permalink / raw)
  To: linux-kernel
  Cc: yibin.gong, fugang.duan, l.stach, dan.j.williams, vkoul,
	shawnguo, s.hauer, kernel, festevam, linux-imx, dmaengine,
	linux-arm-kernel, Philipp Puschmann

In sdma_prep_dma_cyclic buf is redundant. Drop it.

Signed-off-by: Philipp Puschmann <philipp.puschmann@emlix.com>
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
---

Changelog v2:
 - add Reviewed-by tag

 drivers/dma/imx-sdma.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index a32b5962630e..17961451941a 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -1544,7 +1544,7 @@ static struct dma_async_tx_descriptor *sdma_prep_dma_cyclic(
 	struct sdma_engine *sdma = sdmac->sdma;
 	int num_periods = buf_len / period_len;
 	int channel = sdmac->channel;
-	int i = 0, buf = 0;
+	int i;
 	struct sdma_desc *desc;
 
 	dev_dbg(sdma->dev, "%s channel: %d\n", __func__, channel);
@@ -1565,7 +1565,7 @@ static struct dma_async_tx_descriptor *sdma_prep_dma_cyclic(
 		goto err_bd_out;
 	}
 
-	while (buf < buf_len) {
+	for (i = 0; i < num_periods; i++) {
 		struct sdma_buffer_descriptor *bd = &desc->bd[i];
 		int param;
 
@@ -1592,9 +1592,6 @@ static struct dma_async_tx_descriptor *sdma_prep_dma_cyclic(
 		bd->mode.status = param;
 
 		dma_addr += period_len;
-		buf += period_len;
-
-		i++;
 	}
 
 	return vchan_tx_prep(&sdmac->vc, &desc->vd, flags);
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 1/3] dmaengine: imx-sdma: fix buffer ownership
  2019-09-19 10:23   ` [PATCH v2 1/3] dmaengine: imx-sdma: fix buffer ownership Philipp Puschmann
@ 2019-09-19 10:27     ` Lucas Stach
  2019-09-19 10:34       ` Philipp Puschmann
  2019-09-21 15:15     ` kbuild test robot
  1 sibling, 1 reply; 31+ messages in thread
From: Lucas Stach @ 2019-09-19 10:27 UTC (permalink / raw)
  To: Philipp Puschmann, linux-kernel
  Cc: yibin.gong, fugang.duan, dan.j.williams, vkoul, shawnguo,
	s.hauer, kernel, festevam, linux-imx, dmaengine,
	linux-arm-kernel

Hi Philipp,

On Do, 2019-09-19 at 12:23 +0200, Philipp Puschmann wrote:
> BD_DONE flag marks ownership of the buffer. When 1 SDMA owns the
> buffer, when 0 ARM owns it. When processing the buffers in
> sdma_update_channel_loop the ownership of the currently processed
> buffer was set to SDMA again before running the callback function of
> the buffer and while the sdma script may be running in parallel. So
> there was the possibility to get the buffer overwritten by SDMA
> before
> it has been processed by kernel leading to kind of random errors in
> the
> upper layers, e.g. bluetooth.
> 
> Signed-off-by: Philipp Puschmann <philipp.puschmann@emlix.com>
> 
> ---
> 
> Changelog v2:
>  - add dma_wb()
> 
>  drivers/dma/imx-sdma.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index 9ba74ab7e912..e029a2443cfc 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -802,7 +802,6 @@ static void sdma_update_channel_loop(struct
> sdma_channel *sdmac)
>  		*/
>  
>  		desc->chn_real_count = bd->mode.count;
> -		bd->mode.status |= BD_DONE;
>  		bd->mode.count = desc->period_len;
>  		desc->buf_ptail = desc->buf_tail;
>  		desc->buf_tail = (desc->buf_tail + 1) % desc->num_bd;
> @@ -817,6 +816,9 @@ static void sdma_update_channel_loop(struct
> sdma_channel *sdmac)
>  		dmaengine_desc_get_callback_invoke(&desc->vd.tx, NULL);
>  		spin_lock(&sdmac->vc.lock);
>  
> +		dma_wb();

Has this change been tested? The function you want here is called
dma_wmb().

Regards,
Lucas

> +		bd->mode.status |= BD_DONE;
> +
>  		if (error)
>  			sdmac->status = old_status;
>  	}


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 0/3] Fix UART DMA freezes for i.MX SOCs
  2019-09-19 10:23 ` [PATCH v2 0/3] Fix UART DMA freezes for i.MX SOCs Philipp Puschmann
                     ` (2 preceding siblings ...)
  2019-09-19 10:23   ` [PATCH v2 3/3] dmaengine: imx-sdma: drop redundant variable Philipp Puschmann
@ 2019-09-19 10:33   ` Fabio Estevam
  2019-09-19 10:45   ` [PATCH v3 " Philipp Puschmann
  4 siblings, 0 replies; 31+ messages in thread
From: Fabio Estevam @ 2019-09-19 10:33 UTC (permalink / raw)
  To: Philipp Puschmann
  Cc: linux-kernel, Robin Gong, Fugang Duan, Lucas Stach, Dan Williams,
	Vinod, Shawn Guo, Sascha Hauer, Sascha Hauer, NXP Linux Team,
	dmaengine,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE

Hi Philipp,

On Thu, Sep 19, 2019 at 7:23 AM Philipp Puschmann
<philipp.puschmann@emlix.com> wrote:

> Philipp Puschmann (3):
>   dmaengine: imx-sdma: fix buffer ownership
>   dmaengine: imx-sdma: fix dma freezes

These two fixes deserve a Fixes tag so that they could be backported
to the stable tree.

Thanks

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 1/3] dmaengine: imx-sdma: fix buffer ownership
  2019-09-19 10:27     ` Lucas Stach
@ 2019-09-19 10:34       ` Philipp Puschmann
  0 siblings, 0 replies; 31+ messages in thread
From: Philipp Puschmann @ 2019-09-19 10:34 UTC (permalink / raw)
  To: Lucas Stach, linux-kernel
  Cc: yibin.gong, fugang.duan, dan.j.williams, vkoul, shawnguo,
	s.hauer, kernel, festevam, linux-imx, dmaengine,
	linux-arm-kernel

Hi Lucas,


Am 19.09.19 um 12:27 schrieb Lucas Stach:
> Hi Philipp,
> 
> On Do, 2019-09-19 at 12:23 +0200, Philipp Puschmann wrote:
>> BD_DONE flag marks ownership of the buffer. When 1 SDMA owns the
>> buffer, when 0 ARM owns it. When processing the buffers in
>> sdma_update_channel_loop the ownership of the currently processed
>> buffer was set to SDMA again before running the callback function of
>> the buffer and while the sdma script may be running in parallel. So
>> there was the possibility to get the buffer overwritten by SDMA
>> before
>> it has been processed by kernel leading to kind of random errors in
>> the
>> upper layers, e.g. bluetooth.
>>
>> Signed-off-by: Philipp Puschmann <philipp.puschmann@emlix.com>
>>
>> ---
>>
>> Changelog v2:
>>  - add dma_wb()
>>
>>  drivers/dma/imx-sdma.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
>> index 9ba74ab7e912..e029a2443cfc 100644
>> --- a/drivers/dma/imx-sdma.c
>> +++ b/drivers/dma/imx-sdma.c
>> @@ -802,7 +802,6 @@ static void sdma_update_channel_loop(struct
>> sdma_channel *sdmac)
>>  		*/
>>  
>>  		desc->chn_real_count = bd->mode.count;
>> -		bd->mode.status |= BD_DONE;
>>  		bd->mode.count = desc->period_len;
>>  		desc->buf_ptail = desc->buf_tail;
>>  		desc->buf_tail = (desc->buf_tail + 1) % desc->num_bd;
>> @@ -817,6 +816,9 @@ static void sdma_update_channel_loop(struct
>> sdma_channel *sdmac)
>>  		dmaengine_desc_get_callback_invoke(&desc->vd.tx, NULL);
>>  		spin_lock(&sdmac->vc.lock);
>>  
>> +		dma_wb();
> 
> Has this change been tested? The function you want here is called
> dma_wmb().
embarrassingly you are right. c&p error and even have not tried to build it :/
V3 comes soon..

Regards,
Philipp

> 
> Regards,
> Lucas
> 
>> +		bd->mode.status |= BD_DONE;
>> +
>>  		if (error)
>>  			sdmac->status = old_status;
>>  	}
> 


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v3 0/3] Fix UART DMA freezes for i.MX SOCs
  2019-09-19 10:23 ` [PATCH v2 0/3] Fix UART DMA freezes for i.MX SOCs Philipp Puschmann
                     ` (3 preceding siblings ...)
  2019-09-19 10:33   ` [PATCH v2 0/3] Fix UART DMA freezes for i.MX SOCs Fabio Estevam
@ 2019-09-19 10:45   ` Philipp Puschmann
  2019-09-19 10:45     ` [PATCH v3 1/3] dmaengine: imx-sdma: fix buffer ownership Philipp Puschmann
                       ` (2 more replies)
  4 siblings, 3 replies; 31+ messages in thread
From: Philipp Puschmann @ 2019-09-19 10:45 UTC (permalink / raw)
  To: linux-kernel
  Cc: yibin.gong, fugang.duan, l.stach, dan.j.williams, vkoul,
	shawnguo, s.hauer, kernel, festevam, linux-imx, dmaengine,
	linux-arm-kernel, Philipp Puschmann

For some years and since many kernel versions there are reports that
RX UART DMA channel stops working at one point. So far the usual
workaround was to disable RX DMA. This patches fix the underlying
problem.

When a running sdma script does not find any usable destination buffer
to put its data into it just leads to stopping the channel being
scheduled again. As solution we manually retrigger the sdma script for
this channel and by this dissolve the freeze.

While this seems to work fine so far, it may come to buffer overruns
when the channel - even temporary - is stopped. This case has to be
addressed by device drivers by increasing the number of DMA periods.

This patch series was tested with the current kernel and backported to
kernel 4.15 with a special use case using a WL1837MOD via UART and
provoking the hanging of UART RX DMA within seconds after starting a
test application. It resulted in well known
  "Bluetooth: hci0: command 0x0408 tx timeout"
errors and complete stop of UART data reception. Our Bluetooth traffic
consists of many independent small packets, mostly only a few bytes,
causing high usage of periods.

Changelog v3:
 - fixes typo in dma_wmb
 - add fixes tags
 
Changelog v2:
 - adapt title (this patches are not only for i.MX6)
 - improve some comments and patch descriptions
 - add a dma_wb() around BD_DONE flag
 - add Reviewed-by tags
 - split off  "serial: imx: adapt rx buffer and dma periods"

Philipp Puschmann (3):
  dmaengine: imx-sdma: fix buffer ownership
  dmaengine: imx-sdma: fix dma freezes
  dmaengine: imx-sdma: drop redundant variable

 drivers/dma/imx-sdma.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

-- 
2.23.0


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v3 1/3] dmaengine: imx-sdma: fix buffer ownership
  2019-09-19 10:45   ` [PATCH v3 " Philipp Puschmann
@ 2019-09-19 10:45     ` Philipp Puschmann
  2019-09-19 11:37       ` Fabio Estevam
  2019-09-19 10:45     ` [PATCH v3 2/3] dmaengine: imx-sdma: fix dma freezes Philipp Puschmann
  2019-09-19 10:45     ` [PATCH v3 3/3] dmaengine: imx-sdma: drop redundant variable Philipp Puschmann
  2 siblings, 1 reply; 31+ messages in thread
From: Philipp Puschmann @ 2019-09-19 10:45 UTC (permalink / raw)
  To: linux-kernel
  Cc: yibin.gong, fugang.duan, l.stach, dan.j.williams, vkoul,
	shawnguo, s.hauer, kernel, festevam, linux-imx, dmaengine,
	linux-arm-kernel, Philipp Puschmann

BD_DONE flag marks ownership of the buffer. When 1 SDMA owns the
buffer, when 0 ARM owns it. When processing the buffers in
sdma_update_channel_loop the ownership of the currently processed
buffer was set to SDMA again before running the callback function of
the buffer and while the sdma script may be running in parallel. So
there was the possibility to get the buffer overwritten by SDMA before
it has been processed by kernel leading to kind of random errors in the
upper layers, e.g. bluetooth.

Fixes: broken since start
Signed-off-by: Philipp Puschmann <philipp.puschmann@emlix.com>

---

Changelog v3:
 - use correct dma_wmb() instead of dma_wb()
 - add fixes tag

Changelog v2:
 - add dma_wb()

 drivers/dma/imx-sdma.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 9ba74ab7e912..e029a2443cfc 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -802,7 +802,6 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
 		*/
 
 		desc->chn_real_count = bd->mode.count;
-		bd->mode.status |= BD_DONE;
 		bd->mode.count = desc->period_len;
 		desc->buf_ptail = desc->buf_tail;
 		desc->buf_tail = (desc->buf_tail + 1) % desc->num_bd;
@@ -817,6 +816,9 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
 		dmaengine_desc_get_callback_invoke(&desc->vd.tx, NULL);
 		spin_lock(&sdmac->vc.lock);
 
+		dma_wmb();
+		bd->mode.status |= BD_DONE;
+
 		if (error)
 			sdmac->status = old_status;
 	}
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 2/3] dmaengine: imx-sdma: fix dma freezes
  2019-09-19 10:45   ` [PATCH v3 " Philipp Puschmann
  2019-09-19 10:45     ` [PATCH v3 1/3] dmaengine: imx-sdma: fix buffer ownership Philipp Puschmann
@ 2019-09-19 10:45     ` Philipp Puschmann
  2019-09-19 10:45     ` [PATCH v3 3/3] dmaengine: imx-sdma: drop redundant variable Philipp Puschmann
  2 siblings, 0 replies; 31+ messages in thread
From: Philipp Puschmann @ 2019-09-19 10:45 UTC (permalink / raw)
  To: linux-kernel
  Cc: yibin.gong, fugang.duan, l.stach, dan.j.williams, vkoul,
	shawnguo, s.hauer, kernel, festevam, linux-imx, dmaengine,
	linux-arm-kernel, Philipp Puschmann

For some years and since many kernel versions there are reports that the
RX UART SDMA channel stops working at some point. The workaround was to
disable DMA for RX. This commit tries to fix the problem itself.

Due to its license i wasn't able to debug the sdma script itself but it
somehow leads to blocking the scheduling of the channel script when a
running sdma script does not find any free descriptor in the ring to put
its data into.

If we detect such a potential case we manually restart the channel.

As sdmac->desc is constant we can move desc out of the loop.

Fixes: broken UART RX DMA. broken since start
Signed-off-by: Philipp Puschmann <philipp.puschmann@emlix.com>
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
---

Changelog v3:
 - use correct dma_wmb() instead of dma_wb()
 - add fixes tag
 
Changelog v2:
 - clarify comment and commit description

 drivers/dma/imx-sdma.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index e029a2443cfc..a32b5962630e 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -775,21 +775,23 @@ static void sdma_start_desc(struct sdma_channel *sdmac)
 static void sdma_update_channel_loop(struct sdma_channel *sdmac)
 {
 	struct sdma_buffer_descriptor *bd;
-	int error = 0;
-	enum dma_status	old_status = sdmac->status;
+	struct sdma_desc *desc = sdmac->desc;
+	int error = 0, cnt = 0;
+	enum dma_status old_status = sdmac->status;
 
 	/*
 	 * loop mode. Iterate over descriptors, re-setup them and
 	 * call callback function.
 	 */
-	while (sdmac->desc) {
-		struct sdma_desc *desc = sdmac->desc;
+	while (desc) {
 
 		bd = &desc->bd[desc->buf_tail];
 
 		if (bd->mode.status & BD_DONE)
 			break;
 
+		cnt++;
+
 		if (bd->mode.status & BD_RROR) {
 			bd->mode.status &= ~BD_RROR;
 			sdmac->status = DMA_ERROR;
@@ -822,6 +824,17 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
 		if (error)
 			sdmac->status = old_status;
 	}
+
+	/* In some situations it may happen that the sdma does not found any
+	 * usable descriptor in the ring to put data into. The channel is
+	 * stopped then. While there is no specific error condition we can
+	 * check for, a necessary condition is that all available buffers for
+	 * the current channel have been written to by the sdma script. In
+	 * this case and after we have made the buffers available again,
+	 * we restart the channel.
+	 */
+	if (cnt >= desc->num_bd)
+		sdma_enable_channel(sdmac->sdma, sdmac->channel);
 }
 
 static void mxc_sdma_handle_channel_normal(struct sdma_channel *data)
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v3 3/3] dmaengine: imx-sdma: drop redundant variable
  2019-09-19 10:45   ` [PATCH v3 " Philipp Puschmann
  2019-09-19 10:45     ` [PATCH v3 1/3] dmaengine: imx-sdma: fix buffer ownership Philipp Puschmann
  2019-09-19 10:45     ` [PATCH v3 2/3] dmaengine: imx-sdma: fix dma freezes Philipp Puschmann
@ 2019-09-19 10:45     ` Philipp Puschmann
  2 siblings, 0 replies; 31+ messages in thread
From: Philipp Puschmann @ 2019-09-19 10:45 UTC (permalink / raw)
  To: linux-kernel
  Cc: yibin.gong, fugang.duan, l.stach, dan.j.williams, vkoul,
	shawnguo, s.hauer, kernel, festevam, linux-imx, dmaengine,
	linux-arm-kernel, Philipp Puschmann

In sdma_prep_dma_cyclic buf is redundant. Drop it.

Signed-off-by: Philipp Puschmann <philipp.puschmann@emlix.com>
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
---

Changelog v3:
 - no changes

Changelog v2:
 - add Reviewed-by tag

 drivers/dma/imx-sdma.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index a32b5962630e..17961451941a 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -1544,7 +1544,7 @@ static struct dma_async_tx_descriptor *sdma_prep_dma_cyclic(
 	struct sdma_engine *sdma = sdmac->sdma;
 	int num_periods = buf_len / period_len;
 	int channel = sdmac->channel;
-	int i = 0, buf = 0;
+	int i;
 	struct sdma_desc *desc;
 
 	dev_dbg(sdma->dev, "%s channel: %d\n", __func__, channel);
@@ -1565,7 +1565,7 @@ static struct dma_async_tx_descriptor *sdma_prep_dma_cyclic(
 		goto err_bd_out;
 	}
 
-	while (buf < buf_len) {
+	for (i = 0; i < num_periods; i++) {
 		struct sdma_buffer_descriptor *bd = &desc->bd[i];
 		int param;
 
@@ -1592,9 +1592,6 @@ static struct dma_async_tx_descriptor *sdma_prep_dma_cyclic(
 		bd->mode.status = param;
 
 		dma_addr += period_len;
-		buf += period_len;
-
-		i++;
 	}
 
 	return vchan_tx_prep(&sdmac->vc, &desc->vd, flags);
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 1/3] dmaengine: imx-sdma: fix buffer ownership
  2019-09-19 10:45     ` [PATCH v3 1/3] dmaengine: imx-sdma: fix buffer ownership Philipp Puschmann
@ 2019-09-19 11:37       ` Fabio Estevam
  2019-09-19 11:42         ` Philipp Puschmann
  0 siblings, 1 reply; 31+ messages in thread
From: Fabio Estevam @ 2019-09-19 11:37 UTC (permalink / raw)
  To: Philipp Puschmann
  Cc: linux-kernel, Robin Gong, Fugang Duan, Lucas Stach, Dan Williams,
	Vinod, Shawn Guo, Sascha Hauer, Sascha Hauer, NXP Linux Team,
	dmaengine,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE

Hi Philipp,

On Thu, Sep 19, 2019 at 7:45 AM Philipp Puschmann
<philipp.puschmann@emlix.com> wrote:
>
> BD_DONE flag marks ownership of the buffer. When 1 SDMA owns the
> buffer, when 0 ARM owns it. When processing the buffers in
> sdma_update_channel_loop the ownership of the currently processed
> buffer was set to SDMA again before running the callback function of
> the buffer and while the sdma script may be running in parallel. So
> there was the possibility to get the buffer overwritten by SDMA before
> it has been processed by kernel leading to kind of random errors in the
> upper layers, e.g. bluetooth.
>
> Fixes: broken since start

The Fixes tag requires a commit ID like this:

Fixes: 1ec1e82f2510 ("dmaengine: Add Freescale i.MX SDMA support")

Same applies to the other patch.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v3 1/3] dmaengine: imx-sdma: fix buffer ownership
  2019-09-19 11:37       ` Fabio Estevam
@ 2019-09-19 11:42         ` Philipp Puschmann
  0 siblings, 0 replies; 31+ messages in thread
From: Philipp Puschmann @ 2019-09-19 11:42 UTC (permalink / raw)
  To: Fabio Estevam
  Cc: linux-kernel, Robin Gong, Fugang Duan, Lucas Stach, Dan Williams,
	Vinod, Shawn Guo, Sascha Hauer, Sascha Hauer, NXP Linux Team,
	dmaengine,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE

Hi Fabio,


Am 19.09.19 um 13:37 schrieb Fabio Estevam:
> Hi Philipp,
> 
> On Thu, Sep 19, 2019 at 7:45 AM Philipp Puschmann
> <philipp.puschmann@emlix.com> wrote:
>>
>> BD_DONE flag marks ownership of the buffer. When 1 SDMA owns the
>> buffer, when 0 ARM owns it. When processing the buffers in
>> sdma_update_channel_loop the ownership of the currently processed
>> buffer was set to SDMA again before running the callback function of
>> the buffer and while the sdma script may be running in parallel. So
>> there was the possibility to get the buffer overwritten by SDMA before
>> it has been processed by kernel leading to kind of random errors in the
>> upper layers, e.g. bluetooth.
>>
>> Fixes: broken since start
> 
> The Fixes tag requires a commit ID like this:
> 
> Fixes: 1ec1e82f2510 ("dmaengine: Add Freescale i.MX SDMA support")
> 
> Same applies to the other patch.
> 
oh okay, thanks. I will fix this with v4.

Regards,
Philipp

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 1/3] dmaengine: imx-sdma: fix buffer ownership
  2019-09-19 10:23   ` [PATCH v2 1/3] dmaengine: imx-sdma: fix buffer ownership Philipp Puschmann
  2019-09-19 10:27     ` Lucas Stach
@ 2019-09-21 15:15     ` kbuild test robot
  1 sibling, 0 replies; 31+ messages in thread
From: kbuild test robot @ 2019-09-21 15:15 UTC (permalink / raw)
  To: Philipp Puschmann
  Cc: kbuild-all, linux-kernel, yibin.gong, fugang.duan, l.stach,
	dan.j.williams, vkoul, shawnguo, s.hauer, kernel, festevam,
	linux-imx, dmaengine, linux-arm-kernel, Philipp Puschmann

[-- Attachment #1: Type: text/plain, Size: 6069 bytes --]

Hi Philipp,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[cannot apply to v5.3 next-20190918]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Philipp-Puschmann/dmaengine-imx-sdma-fix-buffer-ownership/20190919-182516
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=arm 
:::::: branch date: 4 hours ago
:::::: commit date: 4 hours ago

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/dma/imx-sdma.c: In function 'sdma_update_channel_loop':
>> drivers/dma/imx-sdma.c:819:3: error: implicit declaration of function 'dma_wb'; did you mean 'dma_wmb'? [-Werror=implicit-function-declaration]
      dma_wb();
      ^~~~~~
      dma_wmb
   cc1: some warnings being treated as errors

# https://github.com/0day-ci/linux/commit/750aab0984840c5966a75794c457c1e79e88d34e
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 750aab0984840c5966a75794c457c1e79e88d34e
vim +819 drivers/dma/imx-sdma.c

57b772b86871e0 Robin Gong               2018-06-20  774  
d1a792f3b4072b Russell King - ARM Linux 2014-06-25  775  static void sdma_update_channel_loop(struct sdma_channel *sdmac)
d1a792f3b4072b Russell King - ARM Linux 2014-06-25  776  {
1ec1e82f2510e2 Sascha Hauer             2010-09-30  777  	struct sdma_buffer_descriptor *bd;
5881826ded79cf Nandor Han               2016-08-08  778  	int error = 0;
5881826ded79cf Nandor Han               2016-08-08  779  	enum dma_status	old_status = sdmac->status;
1ec1e82f2510e2 Sascha Hauer             2010-09-30  780  
1ec1e82f2510e2 Sascha Hauer             2010-09-30  781  	/*
1ec1e82f2510e2 Sascha Hauer             2010-09-30  782  	 * loop mode. Iterate over descriptors, re-setup them and
1ec1e82f2510e2 Sascha Hauer             2010-09-30  783  	 * call callback function.
1ec1e82f2510e2 Sascha Hauer             2010-09-30  784  	 */
57b772b86871e0 Robin Gong               2018-06-20  785  	while (sdmac->desc) {
76c33d27073e29 Sascha Hauer             2018-06-20  786  		struct sdma_desc *desc = sdmac->desc;
76c33d27073e29 Sascha Hauer             2018-06-20  787  
76c33d27073e29 Sascha Hauer             2018-06-20  788  		bd = &desc->bd[desc->buf_tail];
1ec1e82f2510e2 Sascha Hauer             2010-09-30  789  
1ec1e82f2510e2 Sascha Hauer             2010-09-30  790  		if (bd->mode.status & BD_DONE)
1ec1e82f2510e2 Sascha Hauer             2010-09-30  791  			break;
1ec1e82f2510e2 Sascha Hauer             2010-09-30  792  
5881826ded79cf Nandor Han               2016-08-08  793  		if (bd->mode.status & BD_RROR) {
5881826ded79cf Nandor Han               2016-08-08  794  			bd->mode.status &= ~BD_RROR;
1ec1e82f2510e2 Sascha Hauer             2010-09-30  795  			sdmac->status = DMA_ERROR;
5881826ded79cf Nandor Han               2016-08-08  796  			error = -EIO;
5881826ded79cf Nandor Han               2016-08-08  797  		}
1ec1e82f2510e2 Sascha Hauer             2010-09-30  798  
5881826ded79cf Nandor Han               2016-08-08  799  	       /*
5881826ded79cf Nandor Han               2016-08-08  800  		* We use bd->mode.count to calculate the residue, since contains
5881826ded79cf Nandor Han               2016-08-08  801  		* the number of bytes present in the current buffer descriptor.
5881826ded79cf Nandor Han               2016-08-08  802  		*/
5881826ded79cf Nandor Han               2016-08-08  803  
76c33d27073e29 Sascha Hauer             2018-06-20  804  		desc->chn_real_count = bd->mode.count;
76c33d27073e29 Sascha Hauer             2018-06-20  805  		bd->mode.count = desc->period_len;
76c33d27073e29 Sascha Hauer             2018-06-20  806  		desc->buf_ptail = desc->buf_tail;
76c33d27073e29 Sascha Hauer             2018-06-20  807  		desc->buf_tail = (desc->buf_tail + 1) % desc->num_bd;
15f30f51311152 Nandor Han               2016-08-08  808  
15f30f51311152 Nandor Han               2016-08-08  809  		/*
15f30f51311152 Nandor Han               2016-08-08  810  		 * The callback is called from the interrupt context in order
15f30f51311152 Nandor Han               2016-08-08  811  		 * to reduce latency and to avoid the risk of altering the
15f30f51311152 Nandor Han               2016-08-08  812  		 * SDMA transaction status by the time the client tasklet is
15f30f51311152 Nandor Han               2016-08-08  813  		 * executed.
15f30f51311152 Nandor Han               2016-08-08  814  		 */
57b772b86871e0 Robin Gong               2018-06-20  815  		spin_unlock(&sdmac->vc.lock);
57b772b86871e0 Robin Gong               2018-06-20  816  		dmaengine_desc_get_callback_invoke(&desc->vd.tx, NULL);
57b772b86871e0 Robin Gong               2018-06-20  817  		spin_lock(&sdmac->vc.lock);
15f30f51311152 Nandor Han               2016-08-08  818  
750aab0984840c Philipp Puschmann        2019-09-19 @819  		dma_wb();
750aab0984840c Philipp Puschmann        2019-09-19  820  		bd->mode.status |= BD_DONE;
750aab0984840c Philipp Puschmann        2019-09-19  821  
5881826ded79cf Nandor Han               2016-08-08  822  		if (error)
5881826ded79cf Nandor Han               2016-08-08  823  			sdmac->status = old_status;
1ec1e82f2510e2 Sascha Hauer             2010-09-30  824  	}
1ec1e82f2510e2 Sascha Hauer             2010-09-30  825  }
1ec1e82f2510e2 Sascha Hauer             2010-09-30  826  

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 71749 bytes --]

^ permalink raw reply	[flat|nested] 31+ messages in thread

end of thread, other threads:[~2019-09-21 15:10 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-11 14:49 [PATCH 0/4] Fix UART DMA freezes for iMX6 Philipp Puschmann
2019-09-11 14:49 ` [PATCH 1/4] dmaengine: imx-sdma: fix buffer ownership Philipp Puschmann
2019-09-16 14:17   ` Lucas Stach
2019-09-19  9:20     ` Philipp Puschmann
2019-09-11 14:49 ` [PATCH 2/4] dmaengine: imx-sdma: fix dma freezes Philipp Puschmann
2019-09-16 14:22   ` Lucas Stach
2019-09-11 14:49 ` [PATCH 3/4] serial: imx: adapt rx buffer and dma periods Philipp Puschmann
2019-09-16 14:24   ` Lucas Stach
2019-09-11 14:49 ` [PATCH 4/4] dmaengine: imx-sdma: drop redundant variable Philipp Puschmann
2019-09-16 14:30   ` Lucas Stach
2019-09-12 15:31 ` [PATCH 0/4] Fix UART DMA freezes for iMX6 Fabio Estevam
2019-09-12 18:23 ` Fabio Estevam
2019-09-16 13:55   ` Philipp Puschmann
2019-09-16 14:00     ` Fabio Estevam
2019-09-16 14:10     ` [EXT] " Andy Duan
2019-09-16  8:02 ` Robin Gong
2019-09-16 13:41   ` Philipp Puschmann
2019-09-19 10:23 ` [PATCH v2 0/3] Fix UART DMA freezes for i.MX SOCs Philipp Puschmann
2019-09-19 10:23   ` [PATCH v2 1/3] dmaengine: imx-sdma: fix buffer ownership Philipp Puschmann
2019-09-19 10:27     ` Lucas Stach
2019-09-19 10:34       ` Philipp Puschmann
2019-09-21 15:15     ` kbuild test robot
2019-09-19 10:23   ` [PATCH v2 2/3] dmaengine: imx-sdma: fix dma freezes Philipp Puschmann
2019-09-19 10:23   ` [PATCH v2 3/3] dmaengine: imx-sdma: drop redundant variable Philipp Puschmann
2019-09-19 10:33   ` [PATCH v2 0/3] Fix UART DMA freezes for i.MX SOCs Fabio Estevam
2019-09-19 10:45   ` [PATCH v3 " Philipp Puschmann
2019-09-19 10:45     ` [PATCH v3 1/3] dmaengine: imx-sdma: fix buffer ownership Philipp Puschmann
2019-09-19 11:37       ` Fabio Estevam
2019-09-19 11:42         ` Philipp Puschmann
2019-09-19 10:45     ` [PATCH v3 2/3] dmaengine: imx-sdma: fix dma freezes Philipp Puschmann
2019-09-19 10:45     ` [PATCH v3 3/3] dmaengine: imx-sdma: drop redundant variable Philipp Puschmann

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).