All of lore.kernel.org
 help / color / mirror / Atom feed
* dmaengine: at_xdmac: fix rare residue corruption
@ 2018-02-22 11:39 ` Maxime Jayat
  0 siblings, 0 replies; 10+ messages in thread
From: Maxime Jayat @ 2018-02-22 11:39 UTC (permalink / raw)
  To: Ludovic Desroches, Vinod Koul
  Cc: Nicolas Ferre, Dan Williams, dmaengine, linux-kernel,
	Maxime Jayat, stable

Despite the efforts made to correctly read the NDA and CUBC registers,
the order in which the registers are read could sometimes lead to an
inconsistent state.

Re-using the timeline from the comments, this following timing of
registers reads could lead to reading NDA with value "@desc2" and
CUBC with value "MAX desc1":

 INITD --------                    ------------
              |____________________|
       _______________________  _______________
 NDA       @desc2             \/   @desc3
       _______________________/\_______________
       __________  ___________  _______________
 CUBC       0    \/ MAX desc1 \/  MAX desc2
       __________/\___________/\_______________
        |  |          |  |
Events:(1)(2)        (3)(4)

(1) check_nda = @desc2
(2) initd = 1
(3) cur_ubc = MAX desc1
(4) cur_nda = @desc2

This is allowed by the condition ((check_nda == cur_nda) && initd),
despite cur_ubc and cur_nda being in the precise state we don't want.

This error leads to incorrect residue computation.

Fix it by inversing the order in which CUBC and INITD are read. This
makes sure that NDA and CUBC are always read together either _before_
INITD goes to 0 or _after_ it is back at 1.
The case where NDA is read before INITD is at 0 and CUBC is read after
INITD is back at 1 will be rejected by check_nda and cur_nda being
different.

Fixes: 53398f488821 ("dmaengine: at_xdmac: fix residue corruption")
Cc: stable@vger.kernel.org
Signed-off-by: Maxime Jayat <maxime.jayat@mobile-devices.fr>
---
Hi,

I had a bug where the serial ports on the Atmel SAMA5D2 were sometimes
returning the same data twice, for up to 4096 bytes.

After investigation, I noticed that the ring buffer used in
atmel_serial (in rx dma mode) had sometimes a incorrect "head" value,
which made the ring buffer do a complete extraneous loop of data
pushed to the tty layer.

I tracked it down to the residue of the dma being wrong, and after
more head scratching, I found this bug in the reading of the
registers.

Before fixing this, I was able to reproduce the bug reliably in a few
minutes. With this patch applied, the bug did not reappear after
several hours in testing.


 drivers/dma/at_xdmac.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c
index c00e3923d7d8..94236ec9d410 100644
--- a/drivers/dma/at_xdmac.c
+++ b/drivers/dma/at_xdmac.c
@@ -1471,10 +1471,10 @@ at_xdmac_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
 	for (retry = 0; retry < AT_XDMAC_RESIDUE_MAX_RETRIES; retry++) {
 		check_nda = at_xdmac_chan_read(atchan, AT_XDMAC_CNDA) & 0xfffffffc;
 		rmb();
-		initd = !!(at_xdmac_chan_read(atchan, AT_XDMAC_CC) & AT_XDMAC_CC_INITD);
-		rmb();
 		cur_ubc = at_xdmac_chan_read(atchan, AT_XDMAC_CUBC);
 		rmb();
+		initd = !!(at_xdmac_chan_read(atchan, AT_XDMAC_CC) & AT_XDMAC_CC_INITD);
+		rmb();
 		cur_nda = at_xdmac_chan_read(atchan, AT_XDMAC_CNDA) & 0xfffffffc;
 		rmb();
 

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH] dmaengine: at_xdmac: fix rare residue corruption
@ 2018-02-22 11:39 ` Maxime Jayat
  0 siblings, 0 replies; 10+ messages in thread
From: Maxime Jayat @ 2018-02-22 11:39 UTC (permalink / raw)
  To: Ludovic Desroches, Vinod Koul
  Cc: Nicolas Ferre, Dan Williams, dmaengine, linux-kernel,
	Maxime Jayat, stable

Despite the efforts made to correctly read the NDA and CUBC registers,
the order in which the registers are read could sometimes lead to an
inconsistent state.

Re-using the timeline from the comments, this following timing of
registers reads could lead to reading NDA with value "@desc2" and
CUBC with value "MAX desc1":

 INITD --------                    ------------
              |____________________|
       _______________________  _______________
 NDA       @desc2             \/   @desc3
       _______________________/\_______________
       __________  ___________  _______________
 CUBC       0    \/ MAX desc1 \/  MAX desc2
       __________/\___________/\_______________
        |  |          |  |
Events:(1)(2)        (3)(4)

(1) check_nda = @desc2
(2) initd = 1
(3) cur_ubc = MAX desc1
(4) cur_nda = @desc2

This is allowed by the condition ((check_nda == cur_nda) && initd),
despite cur_ubc and cur_nda being in the precise state we don't want.

This error leads to incorrect residue computation.

Fix it by inversing the order in which CUBC and INITD are read. This
makes sure that NDA and CUBC are always read together either _before_
INITD goes to 0 or _after_ it is back at 1.
The case where NDA is read before INITD is at 0 and CUBC is read after
INITD is back at 1 will be rejected by check_nda and cur_nda being
different.

Fixes: 53398f488821 ("dmaengine: at_xdmac: fix residue corruption")
Cc: stable@vger.kernel.org
Signed-off-by: Maxime Jayat <maxime.jayat@mobile-devices.fr>
---
Hi,

I had a bug where the serial ports on the Atmel SAMA5D2 were sometimes
returning the same data twice, for up to 4096 bytes.

After investigation, I noticed that the ring buffer used in
atmel_serial (in rx dma mode) had sometimes a incorrect "head" value,
which made the ring buffer do a complete extraneous loop of data
pushed to the tty layer.

I tracked it down to the residue of the dma being wrong, and after
more head scratching, I found this bug in the reading of the
registers.

Before fixing this, I was able to reproduce the bug reliably in a few
minutes. With this patch applied, the bug did not reappear after
several hours in testing.


 drivers/dma/at_xdmac.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c
index c00e3923d7d8..94236ec9d410 100644
--- a/drivers/dma/at_xdmac.c
+++ b/drivers/dma/at_xdmac.c
@@ -1471,10 +1471,10 @@ at_xdmac_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
 	for (retry = 0; retry < AT_XDMAC_RESIDUE_MAX_RETRIES; retry++) {
 		check_nda = at_xdmac_chan_read(atchan, AT_XDMAC_CNDA) & 0xfffffffc;
 		rmb();
-		initd = !!(at_xdmac_chan_read(atchan, AT_XDMAC_CC) & AT_XDMAC_CC_INITD);
-		rmb();
 		cur_ubc = at_xdmac_chan_read(atchan, AT_XDMAC_CUBC);
 		rmb();
+		initd = !!(at_xdmac_chan_read(atchan, AT_XDMAC_CC) & AT_XDMAC_CC_INITD);
+		rmb();
 		cur_nda = at_xdmac_chan_read(atchan, AT_XDMAC_CNDA) & 0xfffffffc;
 		rmb();
 
-- 
2.14.1

^ permalink raw reply related	[flat|nested] 10+ messages in thread
* dmaengine: at_xdmac: fix rare residue corruption
  2018-02-22 11:39 ` [PATCH] " Maxime Jayat
@ 2018-03-01  8:25 ` Ludovic Desroches
  -1 siblings, 0 replies; 10+ messages in thread
From: Ludovic Desroches @ 2018-03-01  8:25 UTC (permalink / raw)
  To: Maxime Jayat
  Cc: Ludovic Desroches, Vinod Koul, Nicolas Ferre, Dan Williams,
	dmaengine, linux-kernel, stable

Hi Maxime,

On Thu, Feb 22, 2018 at 12:39:55PM +0100, Maxime Jayat wrote:
> Despite the efforts made to correctly read the NDA and CUBC registers,
> the order in which the registers are read could sometimes lead to an
> inconsistent state.
> 
> Re-using the timeline from the comments, this following timing of
> registers reads could lead to reading NDA with value "@desc2" and
> CUBC with value "MAX desc1":
> 
>  INITD --------                    ------------
>               |____________________|
>        _______________________  _______________
>  NDA       @desc2             \/   @desc3
>        _______________________/\_______________
>        __________  ___________  _______________
>  CUBC       0    \/ MAX desc1 \/  MAX desc2
>        __________/\___________/\_______________
>         |  |          |  |
> Events:(1)(2)        (3)(4)
> 
> (1) check_nda = @desc2
> (2) initd = 1
> (3) cur_ubc = MAX desc1
> (4) cur_nda = @desc2
> 
> This is allowed by the condition ((check_nda == cur_nda) && initd),
> despite cur_ubc and cur_nda being in the precise state we don't want.
> 
> This error leads to incorrect residue computation.
> 
> Fix it by inversing the order in which CUBC and INITD are read. This
> makes sure that NDA and CUBC are always read together either _before_
> INITD goes to 0 or _after_ it is back at 1.
> The case where NDA is read before INITD is at 0 and CUBC is read after
> INITD is back at 1 will be rejected by check_nda and cur_nda being
> different.
> 
> Fixes: 53398f488821 ("dmaengine: at_xdmac: fix residue corruption")
> Cc: stable@vger.kernel.org
> Signed-off-by: Maxime Jayat <maxime.jayat@mobile-devices.fr>

Nice work! I agree with the change you propose.

I am disappointed we didn't spot this case so I would like to double-check with
the hardware guy there is no issue with the sequence you propose. That's
why I am waiting a bit before giving my ack.

Regards

Ludovic

> ---
> Hi,
> 
> I had a bug where the serial ports on the Atmel SAMA5D2 were sometimes
> returning the same data twice, for up to 4096 bytes.
> 
> After investigation, I noticed that the ring buffer used in
> atmel_serial (in rx dma mode) had sometimes a incorrect "head" value,
> which made the ring buffer do a complete extraneous loop of data
> pushed to the tty layer.
> 
> I tracked it down to the residue of the dma being wrong, and after
> more head scratching, I found this bug in the reading of the
> registers.
> 
> Before fixing this, I was able to reproduce the bug reliably in a few
> minutes. With this patch applied, the bug did not reappear after
> several hours in testing.
> 
> 
>  drivers/dma/at_xdmac.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c
> index c00e3923d7d8..94236ec9d410 100644
> --- a/drivers/dma/at_xdmac.c
> +++ b/drivers/dma/at_xdmac.c
> @@ -1471,10 +1471,10 @@ at_xdmac_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
>  	for (retry = 0; retry < AT_XDMAC_RESIDUE_MAX_RETRIES; retry++) {
>  		check_nda = at_xdmac_chan_read(atchan, AT_XDMAC_CNDA) & 0xfffffffc;
>  		rmb();
> -		initd = !!(at_xdmac_chan_read(atchan, AT_XDMAC_CC) & AT_XDMAC_CC_INITD);
> -		rmb();
>  		cur_ubc = at_xdmac_chan_read(atchan, AT_XDMAC_CUBC);
>  		rmb();
> +		initd = !!(at_xdmac_chan_read(atchan, AT_XDMAC_CC) & AT_XDMAC_CC_INITD);
> +		rmb();
>  		cur_nda = at_xdmac_chan_read(atchan, AT_XDMAC_CNDA) & 0xfffffffc;
>  		rmb();
>  
> -- 
> 2.14.1
>
---
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] dmaengine: at_xdmac: fix rare residue corruption
@ 2018-03-01  8:25 ` Ludovic Desroches
  0 siblings, 0 replies; 10+ messages in thread
From: Ludovic Desroches @ 2018-03-01  8:25 UTC (permalink / raw)
  To: Maxime Jayat
  Cc: Ludovic Desroches, Vinod Koul, Nicolas Ferre, Dan Williams,
	dmaengine, linux-kernel, stable

Hi Maxime,

On Thu, Feb 22, 2018 at 12:39:55PM +0100, Maxime Jayat wrote:
> Despite the efforts made to correctly read the NDA and CUBC registers,
> the order in which the registers are read could sometimes lead to an
> inconsistent state.
> 
> Re-using the timeline from the comments, this following timing of
> registers reads could lead to reading NDA with value "@desc2" and
> CUBC with value "MAX desc1":
> 
>  INITD --------                    ------------
>               |____________________|
>        _______________________  _______________
>  NDA       @desc2             \/   @desc3
>        _______________________/\_______________
>        __________  ___________  _______________
>  CUBC       0    \/ MAX desc1 \/  MAX desc2
>        __________/\___________/\_______________
>         |  |          |  |
> Events:(1)(2)        (3)(4)
> 
> (1) check_nda = @desc2
> (2) initd = 1
> (3) cur_ubc = MAX desc1
> (4) cur_nda = @desc2
> 
> This is allowed by the condition ((check_nda == cur_nda) && initd),
> despite cur_ubc and cur_nda being in the precise state we don't want.
> 
> This error leads to incorrect residue computation.
> 
> Fix it by inversing the order in which CUBC and INITD are read. This
> makes sure that NDA and CUBC are always read together either _before_
> INITD goes to 0 or _after_ it is back at 1.
> The case where NDA is read before INITD is at 0 and CUBC is read after
> INITD is back at 1 will be rejected by check_nda and cur_nda being
> different.
> 
> Fixes: 53398f488821 ("dmaengine: at_xdmac: fix residue corruption")
> Cc: stable@vger.kernel.org
> Signed-off-by: Maxime Jayat <maxime.jayat@mobile-devices.fr>

Nice work! I agree with the change you propose.

I am disappointed we didn't spot this case so I would like to double-check with
the hardware guy there is no issue with the sequence you propose. That's
why I am waiting a bit before giving my ack.

Regards

Ludovic

> ---
> Hi,
> 
> I had a bug where the serial ports on the Atmel SAMA5D2 were sometimes
> returning the same data twice, for up to 4096 bytes.
> 
> After investigation, I noticed that the ring buffer used in
> atmel_serial (in rx dma mode) had sometimes a incorrect "head" value,
> which made the ring buffer do a complete extraneous loop of data
> pushed to the tty layer.
> 
> I tracked it down to the residue of the dma being wrong, and after
> more head scratching, I found this bug in the reading of the
> registers.
> 
> Before fixing this, I was able to reproduce the bug reliably in a few
> minutes. With this patch applied, the bug did not reappear after
> several hours in testing.
> 
> 
>  drivers/dma/at_xdmac.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c
> index c00e3923d7d8..94236ec9d410 100644
> --- a/drivers/dma/at_xdmac.c
> +++ b/drivers/dma/at_xdmac.c
> @@ -1471,10 +1471,10 @@ at_xdmac_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
>  	for (retry = 0; retry < AT_XDMAC_RESIDUE_MAX_RETRIES; retry++) {
>  		check_nda = at_xdmac_chan_read(atchan, AT_XDMAC_CNDA) & 0xfffffffc;
>  		rmb();
> -		initd = !!(at_xdmac_chan_read(atchan, AT_XDMAC_CC) & AT_XDMAC_CC_INITD);
> -		rmb();
>  		cur_ubc = at_xdmac_chan_read(atchan, AT_XDMAC_CUBC);
>  		rmb();
> +		initd = !!(at_xdmac_chan_read(atchan, AT_XDMAC_CC) & AT_XDMAC_CC_INITD);
> +		rmb();
>  		cur_nda = at_xdmac_chan_read(atchan, AT_XDMAC_CNDA) & 0xfffffffc;
>  		rmb();
>  
> -- 
> 2.14.1
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread
* dmaengine: at_xdmac: fix rare residue corruption
  2018-03-01  8:25 ` [PATCH] " Ludovic Desroches
@ 2018-03-19  7:56 ` Vinod Koul
  -1 siblings, 0 replies; 10+ messages in thread
From: Vinod Koul @ 2018-03-19  7:56 UTC (permalink / raw)
  To: Ludovic Desroches
  Cc: Maxime Jayat, Nicolas Ferre, Dan Williams, dmaengine,
	linux-kernel, stable

On Thu, Mar 01, 2018 at 09:25:16AM +0100, Ludovic Desroches wrote:
> Hi Maxime,
> 
> On Thu, Feb 22, 2018 at 12:39:55PM +0100, Maxime Jayat wrote:
> > Despite the efforts made to correctly read the NDA and CUBC registers,
> > the order in which the registers are read could sometimes lead to an
> > inconsistent state.
> > 
> > Re-using the timeline from the comments, this following timing of
> > registers reads could lead to reading NDA with value "@desc2" and
> > CUBC with value "MAX desc1":
> > 
> >  INITD --------                    ------------
> >               |____________________|
> >        _______________________  _______________
> >  NDA       @desc2             \/   @desc3
> >        _______________________/\_______________
> >        __________  ___________  _______________
> >  CUBC       0    \/ MAX desc1 \/  MAX desc2
> >        __________/\___________/\_______________
> >         |  |          |  |
> > Events:(1)(2)        (3)(4)
> > 
> > (1) check_nda = @desc2
> > (2) initd = 1
> > (3) cur_ubc = MAX desc1
> > (4) cur_nda = @desc2
> > 
> > This is allowed by the condition ((check_nda == cur_nda) && initd),
> > despite cur_ubc and cur_nda being in the precise state we don't want.
> > 
> > This error leads to incorrect residue computation.
> > 
> > Fix it by inversing the order in which CUBC and INITD are read. This
> > makes sure that NDA and CUBC are always read together either _before_
> > INITD goes to 0 or _after_ it is back at 1.
> > The case where NDA is read before INITD is at 0 and CUBC is read after
> > INITD is back at 1 will be rejected by check_nda and cur_nda being
> > different.
> > 
> > Fixes: 53398f488821 ("dmaengine: at_xdmac: fix residue corruption")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Maxime Jayat <maxime.jayat@mobile-devices.fr>
> 
> Nice work! I agree with the change you propose.
> 
> I am disappointed we didn't spot this case so I would like to double-check with
> the hardware guy there is no issue with the sequence you propose. That's
> why I am waiting a bit before giving my ack.

any update on that? This has been pending for a while...

> 
> Regards
> 
> Ludovic
> 
> > ---
> > Hi,
> > 
> > I had a bug where the serial ports on the Atmel SAMA5D2 were sometimes
> > returning the same data twice, for up to 4096 bytes.
> > 
> > After investigation, I noticed that the ring buffer used in
> > atmel_serial (in rx dma mode) had sometimes a incorrect "head" value,
> > which made the ring buffer do a complete extraneous loop of data
> > pushed to the tty layer.
> > 
> > I tracked it down to the residue of the dma being wrong, and after
> > more head scratching, I found this bug in the reading of the
> > registers.
> > 
> > Before fixing this, I was able to reproduce the bug reliably in a few
> > minutes. With this patch applied, the bug did not reappear after
> > several hours in testing.
> > 
> > 
> >  drivers/dma/at_xdmac.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c
> > index c00e3923d7d8..94236ec9d410 100644
> > --- a/drivers/dma/at_xdmac.c
> > +++ b/drivers/dma/at_xdmac.c
> > @@ -1471,10 +1471,10 @@ at_xdmac_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
> >  	for (retry = 0; retry < AT_XDMAC_RESIDUE_MAX_RETRIES; retry++) {
> >  		check_nda = at_xdmac_chan_read(atchan, AT_XDMAC_CNDA) & 0xfffffffc;
> >  		rmb();
> > -		initd = !!(at_xdmac_chan_read(atchan, AT_XDMAC_CC) & AT_XDMAC_CC_INITD);
> > -		rmb();
> >  		cur_ubc = at_xdmac_chan_read(atchan, AT_XDMAC_CUBC);
> >  		rmb();
> > +		initd = !!(at_xdmac_chan_read(atchan, AT_XDMAC_CC) & AT_XDMAC_CC_INITD);
> > +		rmb();
> >  		cur_nda = at_xdmac_chan_read(atchan, AT_XDMAC_CNDA) & 0xfffffffc;
> >  		rmb();
> >  
> > -- 
> > 2.14.1
> >

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] dmaengine: at_xdmac: fix rare residue corruption
@ 2018-03-19  7:56 ` Vinod Koul
  0 siblings, 0 replies; 10+ messages in thread
From: Vinod Koul @ 2018-03-19  7:56 UTC (permalink / raw)
  To: Ludovic Desroches
  Cc: Maxime Jayat, Nicolas Ferre, Dan Williams, dmaengine,
	linux-kernel, stable

On Thu, Mar 01, 2018 at 09:25:16AM +0100, Ludovic Desroches wrote:
> Hi Maxime,
> 
> On Thu, Feb 22, 2018 at 12:39:55PM +0100, Maxime Jayat wrote:
> > Despite the efforts made to correctly read the NDA and CUBC registers,
> > the order in which the registers are read could sometimes lead to an
> > inconsistent state.
> > 
> > Re-using the timeline from the comments, this following timing of
> > registers reads could lead to reading NDA with value "@desc2" and
> > CUBC with value "MAX desc1":
> > 
> >  INITD --------                    ------------
> >               |____________________|
> >        _______________________  _______________
> >  NDA       @desc2             \/   @desc3
> >        _______________________/\_______________
> >        __________  ___________  _______________
> >  CUBC       0    \/ MAX desc1 \/  MAX desc2
> >        __________/\___________/\_______________
> >         |  |          |  |
> > Events:(1)(2)        (3)(4)
> > 
> > (1) check_nda = @desc2
> > (2) initd = 1
> > (3) cur_ubc = MAX desc1
> > (4) cur_nda = @desc2
> > 
> > This is allowed by the condition ((check_nda == cur_nda) && initd),
> > despite cur_ubc and cur_nda being in the precise state we don't want.
> > 
> > This error leads to incorrect residue computation.
> > 
> > Fix it by inversing the order in which CUBC and INITD are read. This
> > makes sure that NDA and CUBC are always read together either _before_
> > INITD goes to 0 or _after_ it is back at 1.
> > The case where NDA is read before INITD is at 0 and CUBC is read after
> > INITD is back at 1 will be rejected by check_nda and cur_nda being
> > different.
> > 
> > Fixes: 53398f488821 ("dmaengine: at_xdmac: fix residue corruption")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Maxime Jayat <maxime.jayat@mobile-devices.fr>
> 
> Nice work! I agree with the change you propose.
> 
> I am disappointed we didn't spot this case so I would like to double-check with
> the hardware guy there is no issue with the sequence you propose. That's
> why I am waiting a bit before giving my ack.

any update on that? This has been pending for a while...

> 
> Regards
> 
> Ludovic
> 
> > ---
> > Hi,
> > 
> > I had a bug where the serial ports on the Atmel SAMA5D2 were sometimes
> > returning the same data twice, for up to 4096 bytes.
> > 
> > After investigation, I noticed that the ring buffer used in
> > atmel_serial (in rx dma mode) had sometimes a incorrect "head" value,
> > which made the ring buffer do a complete extraneous loop of data
> > pushed to the tty layer.
> > 
> > I tracked it down to the residue of the dma being wrong, and after
> > more head scratching, I found this bug in the reading of the
> > registers.
> > 
> > Before fixing this, I was able to reproduce the bug reliably in a few
> > minutes. With this patch applied, the bug did not reappear after
> > several hours in testing.
> > 
> > 
> >  drivers/dma/at_xdmac.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c
> > index c00e3923d7d8..94236ec9d410 100644
> > --- a/drivers/dma/at_xdmac.c
> > +++ b/drivers/dma/at_xdmac.c
> > @@ -1471,10 +1471,10 @@ at_xdmac_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
> >  	for (retry = 0; retry < AT_XDMAC_RESIDUE_MAX_RETRIES; retry++) {
> >  		check_nda = at_xdmac_chan_read(atchan, AT_XDMAC_CNDA) & 0xfffffffc;
> >  		rmb();
> > -		initd = !!(at_xdmac_chan_read(atchan, AT_XDMAC_CC) & AT_XDMAC_CC_INITD);
> > -		rmb();
> >  		cur_ubc = at_xdmac_chan_read(atchan, AT_XDMAC_CUBC);
> >  		rmb();
> > +		initd = !!(at_xdmac_chan_read(atchan, AT_XDMAC_CC) & AT_XDMAC_CC_INITD);
> > +		rmb();
> >  		cur_nda = at_xdmac_chan_read(atchan, AT_XDMAC_CNDA) & 0xfffffffc;
> >  		rmb();
> >  
> > -- 
> > 2.14.1
> > 

-- 
~Vinod

^ permalink raw reply	[flat|nested] 10+ messages in thread
* dmaengine: at_xdmac: fix rare residue corruption
  2018-03-19  7:56 ` [PATCH] " Vinod Koul
@ 2018-03-19  7:59 ` Ludovic Desroches
  -1 siblings, 0 replies; 10+ messages in thread
From: Ludovic Desroches @ 2018-03-19  7:59 UTC (permalink / raw)
  To: Vinod Koul
  Cc: Ludovic Desroches, Maxime Jayat, Nicolas Ferre, Dan Williams,
	dmaengine, linux-kernel, stable

On Mon, Mar 19, 2018 at 01:26:09PM +0530, Vinod Koul wrote:
> On Thu, Mar 01, 2018 at 09:25:16AM +0100, Ludovic Desroches wrote:
> > Hi Maxime,
> > 
> > On Thu, Feb 22, 2018 at 12:39:55PM +0100, Maxime Jayat wrote:
> > > Despite the efforts made to correctly read the NDA and CUBC registers,
> > > the order in which the registers are read could sometimes lead to an
> > > inconsistent state.
> > > 
> > > Re-using the timeline from the comments, this following timing of
> > > registers reads could lead to reading NDA with value "@desc2" and
> > > CUBC with value "MAX desc1":
> > > 
> > >  INITD --------                    ------------
> > >               |____________________|
> > >        _______________________  _______________
> > >  NDA       @desc2             \/   @desc3
> > >        _______________________/\_______________
> > >        __________  ___________  _______________
> > >  CUBC       0    \/ MAX desc1 \/  MAX desc2
> > >        __________/\___________/\_______________
> > >         |  |          |  |
> > > Events:(1)(2)        (3)(4)
> > > 
> > > (1) check_nda = @desc2
> > > (2) initd = 1
> > > (3) cur_ubc = MAX desc1
> > > (4) cur_nda = @desc2
> > > 
> > > This is allowed by the condition ((check_nda == cur_nda) && initd),
> > > despite cur_ubc and cur_nda being in the precise state we don't want.
> > > 
> > > This error leads to incorrect residue computation.
> > > 
> > > Fix it by inversing the order in which CUBC and INITD are read. This
> > > makes sure that NDA and CUBC are always read together either _before_
> > > INITD goes to 0 or _after_ it is back at 1.
> > > The case where NDA is read before INITD is at 0 and CUBC is read after
> > > INITD is back at 1 will be rejected by check_nda and cur_nda being
> > > different.
> > > 
> > > Fixes: 53398f488821 ("dmaengine: at_xdmac: fix residue corruption")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Maxime Jayat <maxime.jayat@mobile-devices.fr>
> > 
> > Nice work! I agree with the change you propose.
> > 
> > I am disappointed we didn't spot this case so I would like to double-check with
> > the hardware guy there is no issue with the sequence you propose. That's
> > why I am waiting a bit before giving my ack.
> 
> any update on that? This has been pending for a while...

Unfortunately not. As I am pretty confident in Maxime patch:
Acked-by: Ludovic Desroches <ludovic.desroches@microchip.com>

> 
> > 
> > Regards
> > 
> > Ludovic
> > 
> > > ---
> > > Hi,
> > > 
> > > I had a bug where the serial ports on the Atmel SAMA5D2 were sometimes
> > > returning the same data twice, for up to 4096 bytes.
> > > 
> > > After investigation, I noticed that the ring buffer used in
> > > atmel_serial (in rx dma mode) had sometimes a incorrect "head" value,
> > > which made the ring buffer do a complete extraneous loop of data
> > > pushed to the tty layer.
> > > 
> > > I tracked it down to the residue of the dma being wrong, and after
> > > more head scratching, I found this bug in the reading of the
> > > registers.
> > > 
> > > Before fixing this, I was able to reproduce the bug reliably in a few
> > > minutes. With this patch applied, the bug did not reappear after
> > > several hours in testing.
> > > 
> > > 
> > >  drivers/dma/at_xdmac.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c
> > > index c00e3923d7d8..94236ec9d410 100644
> > > --- a/drivers/dma/at_xdmac.c
> > > +++ b/drivers/dma/at_xdmac.c
> > > @@ -1471,10 +1471,10 @@ at_xdmac_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
> > >  	for (retry = 0; retry < AT_XDMAC_RESIDUE_MAX_RETRIES; retry++) {
> > >  		check_nda = at_xdmac_chan_read(atchan, AT_XDMAC_CNDA) & 0xfffffffc;
> > >  		rmb();
> > > -		initd = !!(at_xdmac_chan_read(atchan, AT_XDMAC_CC) & AT_XDMAC_CC_INITD);
> > > -		rmb();
> > >  		cur_ubc = at_xdmac_chan_read(atchan, AT_XDMAC_CUBC);
> > >  		rmb();
> > > +		initd = !!(at_xdmac_chan_read(atchan, AT_XDMAC_CC) & AT_XDMAC_CC_INITD);
> > > +		rmb();
> > >  		cur_nda = at_xdmac_chan_read(atchan, AT_XDMAC_CNDA) & 0xfffffffc;
> > >  		rmb();
> > >  
> > > -- 
> > > 2.14.1
> > > 
> 
> -- 
> ~Vinod
---
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] dmaengine: at_xdmac: fix rare residue corruption
@ 2018-03-19  7:59 ` Ludovic Desroches
  0 siblings, 0 replies; 10+ messages in thread
From: Ludovic Desroches @ 2018-03-19  7:59 UTC (permalink / raw)
  To: Vinod Koul
  Cc: Ludovic Desroches, Maxime Jayat, Nicolas Ferre, Dan Williams,
	dmaengine, linux-kernel, stable

On Mon, Mar 19, 2018 at 01:26:09PM +0530, Vinod Koul wrote:
> On Thu, Mar 01, 2018 at 09:25:16AM +0100, Ludovic Desroches wrote:
> > Hi Maxime,
> > 
> > On Thu, Feb 22, 2018 at 12:39:55PM +0100, Maxime Jayat wrote:
> > > Despite the efforts made to correctly read the NDA and CUBC registers,
> > > the order in which the registers are read could sometimes lead to an
> > > inconsistent state.
> > > 
> > > Re-using the timeline from the comments, this following timing of
> > > registers reads could lead to reading NDA with value "@desc2" and
> > > CUBC with value "MAX desc1":
> > > 
> > >  INITD --------                    ------------
> > >               |____________________|
> > >        _______________________  _______________
> > >  NDA       @desc2             \/   @desc3
> > >        _______________________/\_______________
> > >        __________  ___________  _______________
> > >  CUBC       0    \/ MAX desc1 \/  MAX desc2
> > >        __________/\___________/\_______________
> > >         |  |          |  |
> > > Events:(1)(2)        (3)(4)
> > > 
> > > (1) check_nda = @desc2
> > > (2) initd = 1
> > > (3) cur_ubc = MAX desc1
> > > (4) cur_nda = @desc2
> > > 
> > > This is allowed by the condition ((check_nda == cur_nda) && initd),
> > > despite cur_ubc and cur_nda being in the precise state we don't want.
> > > 
> > > This error leads to incorrect residue computation.
> > > 
> > > Fix it by inversing the order in which CUBC and INITD are read. This
> > > makes sure that NDA and CUBC are always read together either _before_
> > > INITD goes to 0 or _after_ it is back at 1.
> > > The case where NDA is read before INITD is at 0 and CUBC is read after
> > > INITD is back at 1 will be rejected by check_nda and cur_nda being
> > > different.
> > > 
> > > Fixes: 53398f488821 ("dmaengine: at_xdmac: fix residue corruption")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Maxime Jayat <maxime.jayat@mobile-devices.fr>
> > 
> > Nice work! I agree with the change you propose.
> > 
> > I am disappointed we didn't spot this case so I would like to double-check with
> > the hardware guy there is no issue with the sequence you propose. That's
> > why I am waiting a bit before giving my ack.
> 
> any update on that? This has been pending for a while...

Unfortunately not. As I am pretty confident in Maxime patch:
Acked-by: Ludovic Desroches <ludovic.desroches@microchip.com>

> 
> > 
> > Regards
> > 
> > Ludovic
> > 
> > > ---
> > > Hi,
> > > 
> > > I had a bug where the serial ports on the Atmel SAMA5D2 were sometimes
> > > returning the same data twice, for up to 4096 bytes.
> > > 
> > > After investigation, I noticed that the ring buffer used in
> > > atmel_serial (in rx dma mode) had sometimes a incorrect "head" value,
> > > which made the ring buffer do a complete extraneous loop of data
> > > pushed to the tty layer.
> > > 
> > > I tracked it down to the residue of the dma being wrong, and after
> > > more head scratching, I found this bug in the reading of the
> > > registers.
> > > 
> > > Before fixing this, I was able to reproduce the bug reliably in a few
> > > minutes. With this patch applied, the bug did not reappear after
> > > several hours in testing.
> > > 
> > > 
> > >  drivers/dma/at_xdmac.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c
> > > index c00e3923d7d8..94236ec9d410 100644
> > > --- a/drivers/dma/at_xdmac.c
> > > +++ b/drivers/dma/at_xdmac.c
> > > @@ -1471,10 +1471,10 @@ at_xdmac_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
> > >  	for (retry = 0; retry < AT_XDMAC_RESIDUE_MAX_RETRIES; retry++) {
> > >  		check_nda = at_xdmac_chan_read(atchan, AT_XDMAC_CNDA) & 0xfffffffc;
> > >  		rmb();
> > > -		initd = !!(at_xdmac_chan_read(atchan, AT_XDMAC_CC) & AT_XDMAC_CC_INITD);
> > > -		rmb();
> > >  		cur_ubc = at_xdmac_chan_read(atchan, AT_XDMAC_CUBC);
> > >  		rmb();
> > > +		initd = !!(at_xdmac_chan_read(atchan, AT_XDMAC_CC) & AT_XDMAC_CC_INITD);
> > > +		rmb();
> > >  		cur_nda = at_xdmac_chan_read(atchan, AT_XDMAC_CNDA) & 0xfffffffc;
> > >  		rmb();
> > >  
> > > -- 
> > > 2.14.1
> > > 
> 
> -- 
> ~Vinod

^ permalink raw reply	[flat|nested] 10+ messages in thread
* dmaengine: at_xdmac: fix rare residue corruption
  2018-02-22 11:39 ` [PATCH] " Maxime Jayat
@ 2018-03-27 12:04 ` Vinod Koul
  -1 siblings, 0 replies; 10+ messages in thread
From: Vinod Koul @ 2018-03-27 12:04 UTC (permalink / raw)
  To: Maxime Jayat
  Cc: Ludovic Desroches, Nicolas Ferre, Dan Williams, dmaengine,
	linux-kernel, stable

On Thu, Feb 22, 2018 at 12:39:55PM +0100, Maxime Jayat wrote:
> Despite the efforts made to correctly read the NDA and CUBC registers,
> the order in which the registers are read could sometimes lead to an
> inconsistent state.
> 
> Re-using the timeline from the comments, this following timing of
> registers reads could lead to reading NDA with value "@desc2" and
> CUBC with value "MAX desc1":
> 
>  INITD --------                    ------------
>               |____________________|
>        _______________________  _______________
>  NDA       @desc2             \/   @desc3
>        _______________________/\_______________
>        __________  ___________  _______________
>  CUBC       0    \/ MAX desc1 \/  MAX desc2
>        __________/\___________/\_______________
>         |  |          |  |
> Events:(1)(2)        (3)(4)
> 
> (1) check_nda = @desc2
> (2) initd = 1
> (3) cur_ubc = MAX desc1
> (4) cur_nda = @desc2
> 
> This is allowed by the condition ((check_nda == cur_nda) && initd),
> despite cur_ubc and cur_nda being in the precise state we don't want.
> 
> This error leads to incorrect residue computation.
> 
> Fix it by inversing the order in which CUBC and INITD are read. This
> makes sure that NDA and CUBC are always read together either _before_
> INITD goes to 0 or _after_ it is back at 1.
> The case where NDA is read before INITD is at 0 and CUBC is read after
> INITD is back at 1 will be rejected by check_nda and cur_nda being
> different.

Applied, thanks

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH] dmaengine: at_xdmac: fix rare residue corruption
@ 2018-03-27 12:04 ` Vinod Koul
  0 siblings, 0 replies; 10+ messages in thread
From: Vinod Koul @ 2018-03-27 12:04 UTC (permalink / raw)
  To: Maxime Jayat
  Cc: Ludovic Desroches, Nicolas Ferre, Dan Williams, dmaengine,
	linux-kernel, stable

On Thu, Feb 22, 2018 at 12:39:55PM +0100, Maxime Jayat wrote:
> Despite the efforts made to correctly read the NDA and CUBC registers,
> the order in which the registers are read could sometimes lead to an
> inconsistent state.
> 
> Re-using the timeline from the comments, this following timing of
> registers reads could lead to reading NDA with value "@desc2" and
> CUBC with value "MAX desc1":
> 
>  INITD --------                    ------------
>               |____________________|
>        _______________________  _______________
>  NDA       @desc2             \/   @desc3
>        _______________________/\_______________
>        __________  ___________  _______________
>  CUBC       0    \/ MAX desc1 \/  MAX desc2
>        __________/\___________/\_______________
>         |  |          |  |
> Events:(1)(2)        (3)(4)
> 
> (1) check_nda = @desc2
> (2) initd = 1
> (3) cur_ubc = MAX desc1
> (4) cur_nda = @desc2
> 
> This is allowed by the condition ((check_nda == cur_nda) && initd),
> despite cur_ubc and cur_nda being in the precise state we don't want.
> 
> This error leads to incorrect residue computation.
> 
> Fix it by inversing the order in which CUBC and INITD are read. This
> makes sure that NDA and CUBC are always read together either _before_
> INITD goes to 0 or _after_ it is back at 1.
> The case where NDA is read before INITD is at 0 and CUBC is read after
> INITD is back at 1 will be rejected by check_nda and cur_nda being
> different.

Applied, thanks

-- 
~Vinod

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-03-27 12:04 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-01  8:25 dmaengine: at_xdmac: fix rare residue corruption Ludovic Desroches
2018-03-01  8:25 ` [PATCH] " Ludovic Desroches
  -- strict thread matches above, loose matches on Subject: below --
2018-03-27 12:04 Vinod Koul
2018-03-27 12:04 ` [PATCH] " Vinod Koul
2018-03-19  7:59 Ludovic Desroches
2018-03-19  7:59 ` [PATCH] " Ludovic Desroches
2018-03-19  7:56 Vinod Koul
2018-03-19  7:56 ` [PATCH] " Vinod Koul
2018-02-22 11:39 Maxime Jayat
2018-02-22 11:39 ` [PATCH] " Maxime Jayat

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.