From: Ludovic Desroches <ludovic.desroches@microchip.com> To: Vinod Koul <vinod.koul@intel.com> Cc: Ludovic Desroches <ludovic.desroches@microchip.com>, Maxime Jayat <maxime.jayat@mobile-devices.fr>, Nicolas Ferre <nicolas.ferre@microchip.com>, Dan Williams <dan.j.williams@intel.com>, dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org Subject: dmaengine: at_xdmac: fix rare residue corruption Date: Mon, 19 Mar 2018 08:59:16 +0100 [thread overview] Message-ID: <20180319075916.GA2939@rfolt0960.corp.atmel.com> (raw) 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
WARNING: multiple messages have this Message-ID (diff)
From: Ludovic Desroches <ludovic.desroches@microchip.com> To: Vinod Koul <vinod.koul@intel.com> Cc: Ludovic Desroches <ludovic.desroches@microchip.com>, Maxime Jayat <maxime.jayat@mobile-devices.fr>, Nicolas Ferre <nicolas.ferre@microchip.com>, Dan Williams <dan.j.williams@intel.com>, <dmaengine@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <stable@vger.kernel.org> Subject: Re: [PATCH] dmaengine: at_xdmac: fix rare residue corruption Date: Mon, 19 Mar 2018 08:59:16 +0100 [thread overview] Message-ID: <20180319075916.GA2939@rfolt0960.corp.atmel.com> (raw) In-Reply-To: <20180319075608.GM15443@localhost> 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
next reply other threads:[~2018-03-19 7:59 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-03-19 7:59 Ludovic Desroches [this message] 2018-03-19 7:59 ` [PATCH] dmaengine: at_xdmac: fix rare residue corruption 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:56 Vinod Koul 2018-03-19 7:56 ` [PATCH] " Vinod Koul 2018-03-01 8:25 Ludovic Desroches 2018-03-01 8:25 ` [PATCH] " Ludovic Desroches 2018-02-22 11:39 Maxime Jayat 2018-02-22 11:39 ` [PATCH] " Maxime Jayat
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20180319075916.GA2939@rfolt0960.corp.atmel.com \ --to=ludovic.desroches@microchip.com \ --cc=dan.j.williams@intel.com \ --cc=dmaengine@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=maxime.jayat@mobile-devices.fr \ --cc=nicolas.ferre@microchip.com \ --cc=stable@vger.kernel.org \ --cc=vinod.koul@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.