Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 19 Mar 2018 13:26:09 +0530
From:   Vinod Koul <vinod.koul@intel.com>
To:     Ludovic Desroches <ludovic.desroches@microchip.com>
Cc:     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
Message-ID: <20180319075608.GM15443@localhost>
References: <20180222113955.4043-1-maxime.jayat@mobile-devices.fr>
 <20180301082516.GL22457@rfolt0960.corp.atmel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20180301082516.GL22457@rfolt0960.corp.atmel.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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