LinuxLists.cc - [PATCH 0/4] Fix UART DMA freezes for iMX6

[permalink] [raw]

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

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 <[email protected]>
---
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

2019-09-11 14:58:06

[permalink] [raw]

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

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 <[email protected]>
---
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

2019-09-11 15:00:43

[permalink] [raw]

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

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 <[email protected]>
---
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

2019-09-11 15:01:01

[permalink] [raw]

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

In sdma_prep_dma_cyclic buf is redundant. Drop it.

Signed-off-by: Philipp Puschmann <[email protected]>
---
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

2019-09-12 15:36:13

[permalink] [raw]

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

[Adding Robin and Andy]

On Wed, Sep 11, 2019 at 11:50 AM Philipp Puschmann
<[email protected]> 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
>

2019-09-12 22:10:50

[permalink] [raw]

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

Hi Philipp,

Thanks for submitting these fixes.

On Wed, Sep 11, 2019 at 11:50 AM Philipp Puschmann
<[email protected]> 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

2019-09-16 10:23:23

by Robin Gong

[permalink] [raw]

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

On 2019/9/11 Philipp Puschmann <[email protected]> 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 <[email protected]>
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

2019-09-16 13:58:00

[permalink] [raw]

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

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
> <[email protected]> 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.

2019-09-16 14:13:32

by Andy Duan

[permalink] [raw]

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

From: Philipp Puschmann <[email protected]> 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
> > <[email protected]> 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 [email protected], thanks.

Andy

2019-09-16 14:29:09

[permalink] [raw]

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

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 <[email protected]>

Reasoning seems sound:

Reviewed-by: Lucas Stach <[email protected]>

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

2019-09-16 14:36:10

[permalink] [raw]

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

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 <[email protected]>

Reviewed-by: Lucas Stach <[email protected]>

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

2019-09-16 19:13:35

[permalink] [raw]

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

Am 16.09.19 um 10:02 schrieb Robin Gong:
> On 2019/9/11 Philipp Puschmann <[email protected]> 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 <[email protected]>
> 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

2019-09-16 19:25:47

[permalink] [raw]

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

Hi Philipp,

On Mon, Sep 16, 2019 at 10:55 AM Philipp Puschmann
<[email protected]> 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 [email protected], 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!

2019-09-16 19:31:18

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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;
> }

2019-09-16 23:40:03

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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 <[email protected]>

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)

2019-09-19 09:23:20

[permalink] [raw]

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

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 <[email protected]>
>> ---
>> 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;
>> }
>

2019-09-19 12:18:16

[permalink] [raw]

Subject: [PATCH v2 0/3] Fix UART DMA freezes for i.MX SOCs

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

2019-09-19 12:19:36

[permalink] [raw]

Subject: [PATCH v2 2/3] dmaengine: imx-sdma: fix dma freezes

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 <[email protected]>
Reviewed-by: Lucas Stach <[email protected]>
---

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

2019-09-19 14:26:07

[permalink] [raw]

Subject: Re: [PATCH v2 0/3] Fix UART DMA freezes for i.MX SOCs

Hi Philipp,

On Thu, Sep 19, 2019 at 7:23 AM Philipp Puschmann
<[email protected]> 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

2019-09-19 14:26:19

[permalink] [raw]

Subject: [PATCH v3 0/3] Fix UART DMA freezes for i.MX SOCs

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

2019-09-19 14:27:53

[permalink] [raw]

Subject: [PATCH v3 1/3] dmaengine: imx-sdma: fix buffer ownership

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 <[email protected]>

---

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

2019-09-19 14:44:42

[permalink] [raw]

Subject: [PATCH v2 3/3] dmaengine: imx-sdma: drop redundant variable

In sdma_prep_dma_cyclic buf is redundant. Drop it.

Signed-off-by: Philipp Puschmann <[email protected]>
Reviewed-by: Lucas Stach <[email protected]>
---

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

2019-09-19 14:58:25

[permalink] [raw]

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

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 <[email protected]>

---

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

2019-09-19 15:13:40

[permalink] [raw]

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

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
> <[email protected]> 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

2019-09-19 15:34:05