linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] dmaengine: imx-sdma: fix context cache
@ 2020-01-29 13:40 Martin Fuzzey
  2020-01-29 20:19 ` Fabio Estevam
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Martin Fuzzey @ 2020-01-29 13:40 UTC (permalink / raw)
  To: dmaengine
  Cc: stable, Shawn Guo, Sascha Hauer, Fabio Estevam, NXP Linux Team,
	Robin Gong, linux-arm-kernel, linux-kernel

There is a DMA problem with the serial ports on i.MX6.

When the following sequence is performed:

1) Open a port
2) Write some data
3) Close the port
4) Open a *different* port
5) Write some data
6) Close the port

The second write sends nothing and the second close hangs.
If the first close() is omitted it works.

Adding logs to the the UART driver shows that the DMA is being setup but
the callback is never invoked for the second write.

This used to work in 4.19.

Git bisect leads to:
	ad0d92d: "dmaengine: imx-sdma: refine to load context only once"

This commit adds a "context_loaded" flag used to avoid unnecessary context
setups.
However the flag is only reset in sdma_channel_terminate_work(),
which is only invoked in a worker triggered by sdma_terminate_all() IF
there is an active descriptor.

So, if no active descriptor remains when the channel is terminated, the
flag is not reset and, when the channel is later reused the old context
is used.

Fix the problem by always resetting the flag in sdma_free_chan_resources().

Fixes: ad0d92d: "dmaengine: imx-sdma: refine to load context only once"
Cc: stable@vger.kernel.org
Signed-off-by: Martin Fuzzey <martin.fuzzey@flowbird.group>

---

The following python script may be used to reproduce the problem:

import re, serial, sys

ports=(0, 4) # Can be any ports not used (no need to connect anything) NOT console...

def get_tx_counts():
        pattern = re.compile("(\d+):.*tx:(\d+).*")
        tx_counts = {}
        with open("/proc/tty/driver/IMX-uart", "r") as f:
                for line in f:
                        match = pattern.match(line)
                        if match:
                                tx_counts[int(match.group(1))] = int(match.group(2))
        return tx_counts

before = get_tx_counts()

a = serial.Serial("/dev/ttymxc%d" % ports[0])
a.write("polop")
a.close()
b = serial.Serial("/dev/ttymxc%d" % ports[1])
b.write("test")

after = get_tx_counts()

if (after[ports[0]] - before[ports[0]]  > 0) and (after[ports[1]] - before[ports[1]] > 0):
        print "PASS"
        sys.exit(0)
else:
        print "FAIL"
        print "Before: %s" % before
        print "After: %s" % after
        sys.exit(1)
---
 drivers/dma/imx-sdma.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index 066b21a..332ca50 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -1338,6 +1338,7 @@ static void sdma_free_chan_resources(struct dma_chan *chan)
 
 	sdmac->event_id0 = 0;
 	sdmac->event_id1 = 0;
+	sdmac->context_loaded = false;
 
 	sdma_set_channel_priority(sdmac, 0);
 
-- 
1.9.1


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

* Re: [PATCH] dmaengine: imx-sdma: fix context cache
  2020-01-29 13:40 [PATCH] dmaengine: imx-sdma: fix context cache Martin Fuzzey
@ 2020-01-29 20:19 ` Fabio Estevam
  2020-02-24 16:57   ` Vinod Koul
  2020-02-28 12:44 ` Richard Leitner
  2020-03-02  7:36 ` Robin Gong
  2 siblings, 1 reply; 5+ messages in thread
From: Fabio Estevam @ 2020-01-29 20:19 UTC (permalink / raw)
  To: Martin Fuzzey
  Cc: dmaengine, stable, Shawn Guo, Sascha Hauer, NXP Linux Team,
	Robin Gong,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	linux-kernel

Hi Martin,

Thanks for the fix.

On Wed, Jan 29, 2020 at 10:41 AM Martin Fuzzey
<martin.fuzzey@flowbird.group> wrote:
>
> There is a DMA problem with the serial ports on i.MX6.
>
> When the following sequence is performed:
>
> 1) Open a port
> 2) Write some data
> 3) Close the port
> 4) Open a *different* port
> 5) Write some data
> 6) Close the port
>
> The second write sends nothing and the second close hangs.
> If the first close() is omitted it works.
>
> Adding logs to the the UART driver shows that the DMA is being setup but
> the callback is never invoked for the second write.
>
> This used to work in 4.19.
>
> Git bisect leads to:
>         ad0d92d: "dmaengine: imx-sdma: refine to load context only once"
>
> This commit adds a "context_loaded" flag used to avoid unnecessary context
> setups.
> However the flag is only reset in sdma_channel_terminate_work(),
> which is only invoked in a worker triggered by sdma_terminate_all() IF
> there is an active descriptor.
>
> So, if no active descriptor remains when the channel is terminated, the
> flag is not reset and, when the channel is later reused the old context
> is used.
>
> Fix the problem by always resetting the flag in sdma_free_chan_resources().
>
> Fixes: ad0d92d: "dmaengine: imx-sdma: refine to load context only once"

Nit: in the Fixes tag we use 12 digits for the commit ID and the
Subject is enclosed by parenthesis.

The preferred format would be:

Fixes: ad0d92d7ba6a ("dmaengine: imx-sdma: refine to load context only once")

Reviewed-by: Fabio Estevam <festevam@gmail.com>

Thanks

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

* Re: [PATCH] dmaengine: imx-sdma: fix context cache
  2020-01-29 20:19 ` Fabio Estevam
@ 2020-02-24 16:57   ` Vinod Koul
  0 siblings, 0 replies; 5+ messages in thread
From: Vinod Koul @ 2020-02-24 16:57 UTC (permalink / raw)
  To: Fabio Estevam
  Cc: Martin Fuzzey, dmaengine, stable, Shawn Guo, Sascha Hauer,
	NXP Linux Team, Robin Gong,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	linux-kernel

On 29-01-20, 17:19, Fabio Estevam wrote:
> Hi Martin,
> 
> Thanks for the fix.
> 
> On Wed, Jan 29, 2020 at 10:41 AM Martin Fuzzey
> <martin.fuzzey@flowbird.group> wrote:
> >
> > There is a DMA problem with the serial ports on i.MX6.
> >
> > When the following sequence is performed:
> >
> > 1) Open a port
> > 2) Write some data
> > 3) Close the port
> > 4) Open a *different* port
> > 5) Write some data
> > 6) Close the port
> >
> > The second write sends nothing and the second close hangs.
> > If the first close() is omitted it works.
> >
> > Adding logs to the the UART driver shows that the DMA is being setup but
> > the callback is never invoked for the second write.
> >
> > This used to work in 4.19.
> >
> > Git bisect leads to:
> >         ad0d92d: "dmaengine: imx-sdma: refine to load context only once"
> >
> > This commit adds a "context_loaded" flag used to avoid unnecessary context
> > setups.
> > However the flag is only reset in sdma_channel_terminate_work(),
> > which is only invoked in a worker triggered by sdma_terminate_all() IF
> > there is an active descriptor.
> >
> > So, if no active descriptor remains when the channel is terminated, the
> > flag is not reset and, when the channel is later reused the old context
> > is used.
> >
> > Fix the problem by always resetting the flag in sdma_free_chan_resources().
> >
> > Fixes: ad0d92d: "dmaengine: imx-sdma: refine to load context only once"
> 
> Nit: in the Fixes tag we use 12 digits for the commit ID and the
> Subject is enclosed by parenthesis.
> 
> The preferred format would be:
> 
> Fixes: ad0d92d7ba6a ("dmaengine: imx-sdma: refine to load context only once")
> 
> Reviewed-by: Fabio Estevam <festevam@gmail.com>

Applied, with updated Fixes line. Thanks

-- 
~Vinod

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

* Re: [PATCH] dmaengine: imx-sdma: fix context cache
  2020-01-29 13:40 [PATCH] dmaengine: imx-sdma: fix context cache Martin Fuzzey
  2020-01-29 20:19 ` Fabio Estevam
@ 2020-02-28 12:44 ` Richard Leitner
  2020-03-02  7:36 ` Robin Gong
  2 siblings, 0 replies; 5+ messages in thread
From: Richard Leitner @ 2020-02-28 12:44 UTC (permalink / raw)
  To: Martin Fuzzey
  Cc: dmaengine, stable, Shawn Guo, Sascha Hauer, Fabio Estevam,
	NXP Linux Team, Robin Gong, linux-arm-kernel, linux-kernel

Hi,

On Wed, Jan 29, 2020 at 02:40:06PM +0100, Martin Fuzzey wrote:
> There is a DMA problem with the serial ports on i.MX6.
> 
> When the following sequence is performed:
> 
> 1) Open a port
> 2) Write some data
> 3) Close the port
> 4) Open a *different* port
> 5) Write some data
> 6) Close the port
> 
> The second write sends nothing and the second close hangs.
> If the first close() is omitted it works.
> 
> Adding logs to the the UART driver shows that the DMA is being setup but
> the callback is never invoked for the second write.
> 
> This used to work in 4.19.
> 
> Git bisect leads to:
> 	ad0d92d: "dmaengine: imx-sdma: refine to load context only once"
> 
> This commit adds a "context_loaded" flag used to avoid unnecessary context
> setups.
> However the flag is only reset in sdma_channel_terminate_work(),
> which is only invoked in a worker triggered by sdma_terminate_all() IF
> there is an active descriptor.
> 
> So, if no active descriptor remains when the channel is terminated, the
> flag is not reset and, when the channel is later reused the old context
> is used.
> 
> Fix the problem by always resetting the flag in sdma_free_chan_resources().
> 
> Fixes: ad0d92d: "dmaengine: imx-sdma: refine to load context only once"
> Cc: stable@vger.kernel.org
> Signed-off-by: Martin Fuzzey <martin.fuzzey@flowbird.group>
> 

Thanks for the patch!
We were chasing this issue for days and just found your patch as we were
preparing our (quite similar) solution for submission ;-)

I've successfully tested your patch on a custom i.MX6Solo board.
Therefore feel free to add

Tested-by: Richard Leitner <richard.leitner@skidata.com>

regards;rl

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

* RE: [PATCH] dmaengine: imx-sdma: fix context cache
  2020-01-29 13:40 [PATCH] dmaengine: imx-sdma: fix context cache Martin Fuzzey
  2020-01-29 20:19 ` Fabio Estevam
  2020-02-28 12:44 ` Richard Leitner
@ 2020-03-02  7:36 ` Robin Gong
  2 siblings, 0 replies; 5+ messages in thread
From: Robin Gong @ 2020-03-02  7:36 UTC (permalink / raw)
  To: Martin Fuzzey, dmaengine
  Cc: stable, Shawn Guo, Sascha Hauer, Fabio Estevam, dl-linux-imx,
	linux-arm-kernel, linux-kernel

On 2020/01/29 Martin Fuzzey <martin.fuzzey@flowbird.group> wrote:
> 
> There is a DMA problem with the serial ports on i.MX6.
> 
> When the following sequence is performed:
> 
> 1) Open a port
> 2) Write some data
> 3) Close the port
> 4) Open a *different* port
> 5) Write some data
> 6) Close the port
> 
> The second write sends nothing and the second close hangs.
> If the first close() is omitted it works.
> 
> Adding logs to the the UART driver shows that the DMA is being setup but the
> callback is never invoked for the second write.
> 
> This used to work in 4.19.
> 
> Git bisect leads to:
> 	ad0d92d: "dmaengine: imx-sdma: refine to load context only once"
> 
> This commit adds a "context_loaded" flag used to avoid unnecessary context
> setups.
> However the flag is only reset in sdma_channel_terminate_work(), which is
> only invoked in a worker triggered by sdma_terminate_all() IF there is an active
> descriptor.
> 
> So, if no active descriptor remains when the channel is terminated, the flag is
> not reset and, when the channel is later reused the old context is used.
> 
> Fix the problem by always resetting the flag in sdma_free_chan_resources().
> 
> Fixes: ad0d92d: "dmaengine: imx-sdma: refine to load context only once"
> Cc: stable@vger.kernel.org
> Signed-off-by: Martin Fuzzey <martin.fuzzey@flowbird.group>
> 
> ---
> 
> The following python script may be used to reproduce the problem:
> 
> import re, serial, sys
> 
> ports=(0, 4) # Can be any ports not used (no need to connect anything) NOT
> console...
> 
> def get_tx_counts():
>         pattern = re.compile("(\d+):.*tx:(\d+).*")
>         tx_counts = {}
>         with open("/proc/tty/driver/IMX-uart", "r") as f:
>                 for line in f:
>                         match = pattern.match(line)
>                         if match:
>                                 tx_counts[int(match.group(1))] =
> int(match.group(2))
>         return tx_counts
> 
> before = get_tx_counts()
> 
> a = serial.Serial("/dev/ttymxc%d" % ports[0])
> a.write("polop")
> a.close()
> b = serial.Serial("/dev/ttymxc%d" % ports[1])
> b.write("test")
> 
> after = get_tx_counts()
> 
> if (after[ports[0]] - before[ports[0]]  > 0) and (after[ports[1]] - before[ports[1]] >
> 0):
>         print "PASS"
>         sys.exit(0)
> else:
>         print "FAIL"
>         print "Before: %s" % before
>         print "After: %s" % after
>         sys.exit(1)
> ---
>  drivers/dma/imx-sdma.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index
> 066b21a..332ca50 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -1338,6 +1338,7 @@ static void sdma_free_chan_resources(struct
> dma_chan *chan)
> 
>  	sdmac->event_id0 = 0;
>  	sdmac->event_id1 = 0;
> +	sdmac->context_loaded = false;
Martin, thanks for you patch, sorry for the issue left in kernel for so long, because my below patch set has been pending from last year. I would like revert commit ad0d92d: "dmaengine: imx-sdma: refine to load context only once" since some drivers may change.
context during two transfer like spi did. I would pick up this patch set this week anyway. 
https://lore.kernel.org/patchwork/patch/1086454/ 
> 
>  	sdma_set_channel_priority(sdmac, 0);
> 
> --
> 1.9.1


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

end of thread, other threads:[~2020-03-02  7:36 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-29 13:40 [PATCH] dmaengine: imx-sdma: fix context cache Martin Fuzzey
2020-01-29 20:19 ` Fabio Estevam
2020-02-24 16:57   ` Vinod Koul
2020-02-28 12:44 ` Richard Leitner
2020-03-02  7:36 ` Robin Gong

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