Re: [PATCH 2/3] dmaengine: at_xdmac: fix residue corruption

[Nicolas Ferre <nicolas.ferre@atmel.com>]

Le 12/05/2016 16:54, Ludovic Desroches a écrit :
> An unexpected value of CUBC can lead to a corrupted residue. A more
> complex sequence is needed to detect an inaccurate value for NCA or CUBC.
> 
> Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> Fixes: e1f7c9eee707 ("dmaengine: at_xdmac: creation of the atmel
> eXtended DMA Controller driver")
> Cc: stable@vger.kernel.org #v4.1 and later

Reviewed-by: Nicolas Ferre <nicolas.ferre@atmel.com>

> ---
>  drivers/dma/at_xdmac.c | 54 ++++++++++++++++++++++++++++++--------------------
>  1 file changed, 32 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c
> index ba9b0b7..b02494e 100644
> --- a/drivers/dma/at_xdmac.c
> +++ b/drivers/dma/at_xdmac.c
> @@ -1400,6 +1400,7 @@ at_xdmac_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
>  	u32			cur_nda, check_nda, cur_ubc, mask, value;
>  	u8			dwidth = 0;
>  	unsigned long		flags;
> +	bool			initd;
>  
>  	ret = dma_cookie_status(chan, cookie, txstate);
>  	if (ret == DMA_COMPLETE)
> @@ -1435,34 +1436,43 @@ at_xdmac_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
>  	}
>  
>  	/*
> -	 * When processing the residue, we need to read two registers but we
> -	 * can't do it in an atomic way. AT_XDMAC_CNDA is used to find where
> -	 * we stand in the descriptor list and AT_XDMAC_CUBC is used
> -	 * to know how many data are remaining for the current descriptor.
> -	 * Since the dma channel is not paused to not loose data, between the
> -	 * AT_XDMAC_CNDA and AT_XDMAC_CUBC read, we may have change of
> -	 * descriptor.
> -	 * For that reason, after reading AT_XDMAC_CUBC, we check if we are
> -	 * still using the same descriptor by reading a second time
> -	 * AT_XDMAC_CNDA. If AT_XDMAC_CNDA has changed, it means we have to
> -	 * read again AT_XDMAC_CUBC.
> +	 * The easiest way to compute the residue should be to pause the DMA
> +	 * but doing this can lead to miss some data as some devices don't
> +	 * have FIFO.
> +	 * We need to read several registers because:
> +	 * - DMA is running therefore a descriptor change is possible while
> +	 * reading these registers
> +	 * - When the block transfer is done, the value of the CUBC register
> +	 * is set to its initial value until the fetch of the next descriptor.
> +	 * This value will corrupt the residue calculation so we have to skip
> +	 * it.
> +	 *
> +	 * INITD --------                    ------------
> +	 *              |____________________|
> +	 *       _______________________  _______________
> +	 * NDA       @desc2             \/   @desc3
> +	 *       _______________________/\_______________
> +	 *       __________  ___________  _______________
> +	 * CUBC       0    \/ MAX desc1 \/  MAX desc2
> +	 *       __________/\___________/\_______________
> +	 *
> +	 * Since descriptors are aligned on 64 bits, we can assume that
> +	 * the update of NDA and CUBC is atomic.
>  	 * Memory barriers are used to ensure the read order of the registers.
> -	 * A max number of retries is set because unlikely it can never ends if
> -	 * we are transferring a lot of data with small buffers.
> +	 * A max number of retries is set because unlikely it could never ends.
>  	 */
> -	cur_nda = at_xdmac_chan_read(atchan, AT_XDMAC_CNDA) & 0xfffffffc;
> -	rmb();
> -	cur_ubc = at_xdmac_chan_read(atchan, AT_XDMAC_CUBC);
>  	for (retry = 0; retry < AT_XDMAC_RESIDUE_MAX_RETRIES; retry++) {
> -		rmb();
>  		check_nda = at_xdmac_chan_read(atchan, AT_XDMAC_CNDA) & 0xfffffffc;
> -
> -		if (likely(cur_nda == check_nda))
> -			break;
> -
> -		cur_nda = check_nda;
> +		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();
> +		cur_nda = at_xdmac_chan_read(atchan, AT_XDMAC_CNDA) & 0xfffffffc;
> +		rmb();
> +
> +		if ((check_nda == cur_nda) && initd)
> +			break;
>  	}
>  
>  	if (unlikely(retry >= AT_XDMAC_RESIDUE_MAX_RETRIES)) {
> 


-- 
Nicolas Ferre