| Message ID | 20190304075610.10810-2-dirk.behme@de.bosch.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [1/2] dmaengine: sh: rcar-dmac: With cyclic DMA residue 0 is valid | expand |
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; > }
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; }