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

login
register
mail settings
Submitter Dirk Behme
Date March 4, 2019, 7:56 a.m.
Message ID <20190304075610.10810-2-dirk.behme@de.bosch.com>
Download mbox | patch
Permalink /patch/740465/
State New
Headers show

Comments

Dirk Behme - March 4, 2019, 7:56 a.m.
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(-)
Laurent Pinchart - March 4, 2019, 10:37 a.m.
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;
>  }

Patch

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;
 
 	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;
+
 	/*
 	 * 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;
 }