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@xxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxx>
> Signed-off-by: Dirk Behme <dirk.behme@xxxxxxxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx> # 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;
> }
--
Regards,
Laurent Pinchart
