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();
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();
initd = !!(at_xdmac_chan_read(atchan, AT_XDMAC_CC) & AT_XDMAC_CC_INITD);rmb();2.14.1
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();
initd = !!(at_xdmac_chan_read(atchan, AT_XDMAC_CC) & AT_XDMAC_CC_INITD);rmb();2.14.1
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();
initd = !!(at_xdmac_chan_read(atchan, AT_XDMAC_CC) & AT_XDMAC_CC_INITD);rmb();2.14.1
-- ~Vinod
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
linux-stable-mirror@lists.linaro.org
-
Ludovic Desroches -
Maxime Jayat -
Vinod Koul