[PATCH 0/4] Fix UART DMA freezes for iMX6

[PATCH 0/4] Fix UART DMA freezes for iMX6

[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

[PATCH 1/4] dmaengine: imx-sdma: fix buffer ownership

[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

[PATCH 2/4] dmaengine: imx-sdma: fix dma freezes

[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

[PATCH 3/4] serial: imx: adapt rx buffer and dma periods

[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

[PATCH 4/4] dmaengine: imx-sdma: drop redundant variable

[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

Re: [PATCH 0/4] Fix UART DMA freezes for iMX6

[Fabio Estevam]

[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
>

Re: [PATCH 0/4] Fix UART DMA freezes for iMX6

[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

RE: [PATCH 0/4] Fix UART DMA freezes for iMX6

[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

    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

Re: [PATCH 0/4] Fix UART DMA freezes for iMX6

[Philipp Puschmann]

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

Re: [PATCH 0/4] Fix UART DMA freezes for iMX6

[Philipp Puschmann]

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.

Re: [PATCH 0/4] Fix UART DMA freezes for iMX6

[Fabio Estevam]

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!

RE: [EXT] Re: [PATCH 0/4] Fix UART DMA freezes for iMX6

[Andy Duan]

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

Re: [PATCH 1/4] dmaengine: imx-sdma: fix buffer ownership

[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

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

Re: [PATCH 2/4] dmaengine: imx-sdma: fix dma freezes

[Lucas Stach]

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)

Re: [PATCH 3/4] serial: imx: adapt rx buffer and dma periods

[Lucas Stach]

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

Re: [PATCH 4/4] dmaengine: imx-sdma: drop redundant variable

[Lucas Stach]

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

Re: [PATCH 1/4] dmaengine: imx-sdma: fix buffer ownership

[Philipp Puschmann]

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;
>>  	}
> 

Re: [PATCH v2 1/3] dmaengine: imx-sdma: fix buffer ownership

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

Regards,
Lucas

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

Re: [PATCH v2 1/3] dmaengine: imx-sdma: fix buffer ownership

[Philipp Puschmann]

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;
>>  	}
>