[2/2] dmaengine: sh: rcar-dmac: Fix glitch in dmaengine_tx_status

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Dirk,

Thank you for the patch.

On Mon, Mar 04, 2019 at 08:56:10AM +0100, Dirk Behme wrote:
> From: Achim Dahlhoff <Achim.Dahlhoff@de.bosch.com>
> 
> The tx_status poll in the rcar_dmac driver reads the status register
> which indicates which chunk is busy (DMACHCRB). Afterwards the point
> inside the chunk is read from DMATCRB. It is possible that the chunk
> has changed between the two reads. The result is a non-monotonous
> increase of the residue. Fix this by introducing a 'safe read' logic.
> 
> Fixes: 73a47bd0da66 ("dmaengine: rcar-dmac: use TCRB instead of TCR for residue")
> Signed-off-by: Achim Dahlhoff <Achim.Dahlhoff@de.bosch.com>
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> Cc: <stable@vger.kernel.org> # v4.16+
> ---
> Note: Patch done against mainline v5.0
> 
>  drivers/dma/sh/rcar-dmac.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
> index 2ea59229d7f5..b8afd7f4e07c 100644
> --- a/drivers/dma/sh/rcar-dmac.c
> +++ b/drivers/dma/sh/rcar-dmac.c
> @@ -1273,6 +1273,13 @@ static int rcar_dmac_chan_terminate_all(struct dma_chan *chan)
>  	return 0;
>  }
>  
> +static inline
> +unsigned int rcar_dmac_read_chcrb_mask(struct rcar_dmac_chan *chan)
> +{
> +	return rcar_dmac_chan_read(chan, RCAR_DMACHCRB) &
> +				   RCAR_DMACHCRB_DPTR_MASK;
> +}
> +
>  static unsigned int rcar_dmac_chan_get_residue(struct rcar_dmac_chan *chan,
>  					       dma_cookie_t cookie)
>  {
> @@ -1282,6 +1289,7 @@ static unsigned int rcar_dmac_chan_get_residue(struct rcar_dmac_chan *chan,
>  	enum dma_status status;
>  	unsigned int residue = 0;
>  	unsigned int dptr = 0;
> +	unsigned int tcrb, chcrb;

We tend to use one variable per line in the rcar-dmac driver.

>  
>  	if (!desc)
>  		return 0;
> @@ -1329,6 +1337,17 @@ static unsigned int rcar_dmac_chan_get_residue(struct rcar_dmac_chan *chan,
>  		return 0;
>  	}
>  
> +	/*
> +	 * We need to read two registers.
> +	 * Make sure the control register does not skip to next chunk
> +	 * while reading the counter.
> +	 */
> + retry:
> +	chcrb = rcar_dmac_read_chcrb_mask(chan);
> +	tcrb = rcar_dmac_chan_read(chan, RCAR_DMATCRB);
> +	if (chcrb != rcar_dmac_read_chcrb_mask(chan)) /* Still the same? */
> +		goto retry;

How about a loop instead of a goto ?

	unsigned int chcrb = ~0;
	...

	while (1) {
		chcrb = rcar_dmac_chan_read(chan, RCAR_DMACHCRB)
		      & RCAR_DMACHCRB_DPTR_MASK;
		if (chcrb == prev_chcrb)
			break;

		tcrb = rcar_dmac_chan_read(chan, RCAR_DMATCRB);
		prev_chcrb = chcrb;
	}

I would even turn this into a for loop to cap the maximum number of
iterations, just in case.

> +
>  	/*
>  	 * In descriptor mode the descriptor running pointer is not maintained
>  	 * by the interrupt handler, find the running descriptor from the
> @@ -1336,8 +1355,7 @@ static unsigned int rcar_dmac_chan_get_residue(struct rcar_dmac_chan *chan,
>  	 * mode just use the running descriptor pointer.
>  	 */
>  	if (desc->hwdescs.use) {
> -		dptr = (rcar_dmac_chan_read(chan, RCAR_DMACHCRB) &
> -			RCAR_DMACHCRB_DPTR_MASK) >> RCAR_DMACHCRB_DPTR_SHIFT;
> +		dptr = chcrb >> RCAR_DMACHCRB_DPTR_SHIFT;
>  		if (dptr == 0)
>  			dptr = desc->nchunks;
>  		dptr--;
> @@ -1355,7 +1373,7 @@ static unsigned int rcar_dmac_chan_get_residue(struct rcar_dmac_chan *chan,
>  	}
>  
>  	/* Add the residue for the current chunk. */
> -	residue += rcar_dmac_chan_read(chan, RCAR_DMATCRB) << desc->xfer_shift;
> +	residue += tcrb << desc->xfer_shift;
>  
>  	return residue;
>  }