[v3,1/2] dmaengine: sh: rcar-dmac: With cyclic DMA residue 0 is valid

[v3,1/2] dmaengine: sh: rcar-dmac: With cyclic DMA residue 0 is valid

[Dirk Behme]

Having a cyclic DMA, a residue 0 is not an indication of a completed
DMA. In case of cyclic DMA make sure that dma_set_residue() is called
and with this a residue of 0 is forwarded correctly to the caller.

Fixes: 3544d2878817 ("dmaengine: rcar-dmac: use result of updated get_residue in tx_status")
Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
Signed-off-by: Achim Dahlhoff <Achim.Dahlhoff@de.bosch.com>
Signed-off-by: Hiroyuki Yokoyama <hiroyuki.yokoyama.vx@renesas.com>
Signed-off-by: Yao Lihua <ylhuajnu@outlook.com>
Cc: <stable@vger.kernel.org> # v4.8+
---
Note: Patch done against mainline v5.0

Changes in v2: None

Changes in v3: Move reading rchan into the spin lock protection.

 drivers/dma/sh/rcar-dmac.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
index 2b4f25698169..54810ffd95e2 100644
--- a/drivers/dma/sh/rcar-dmac.c
+++ b/drivers/dma/sh/rcar-dmac.c
@@ -1368,6 +1368,7 @@ static enum dma_status rcar_dmac_tx_status(struct dma_chan *chan,
 	enum dma_status status;
 	unsigned long flags;
 	unsigned int residue;
+	bool cyclic;
 
 	status = dma_cookie_status(chan, cookie, txstate);
 	if (status == DMA_COMPLETE || !txstate)
@@ -1375,10 +1376,11 @@ static enum dma_status rcar_dmac_tx_status(struct dma_chan *chan,
 
 	spin_lock_irqsave(&rchan->lock, flags);
 	residue = rcar_dmac_chan_get_residue(rchan, cookie);
+	cyclic = rchan->desc.running ? rchan->desc.running->cyclic : false;
 	spin_unlock_irqrestore(&rchan->lock, flags);
 
 	/* if there's no residue, the cookie is complete */
-	if (!residue)
+	if (!residue && !cyclic)
 		return DMA_COMPLETE;
 
 	dma_set_residue(txstate, residue);

[PATCH v3 1/2] dmaengine: sh: rcar-dmac: With cyclic DMA residue 0 is valid

[Dirk Behme]

Having a cyclic DMA, a residue 0 is not an indication of a completed
DMA. In case of cyclic DMA make sure that dma_set_residue() is called
and with this a residue of 0 is forwarded correctly to the caller.

Fixes: 3544d2878817 ("dmaengine: rcar-dmac: use result of updated get_residue in tx_status")
Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
Signed-off-by: Achim Dahlhoff <Achim.Dahlhoff@de.bosch.com>
Signed-off-by: Hiroyuki Yokoyama <hiroyuki.yokoyama.vx@renesas.com>
Signed-off-by: Yao Lihua <ylhuajnu@outlook.com>
Cc: <stable@vger.kernel.org> # v4.8+
---
Note: Patch done against mainline v5.0

Changes in v2: None

Changes in v3: Move reading rchan into the spin lock protection.

 drivers/dma/sh/rcar-dmac.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
index 2b4f25698169..54810ffd95e2 100644
--- a/drivers/dma/sh/rcar-dmac.c
+++ b/drivers/dma/sh/rcar-dmac.c
@@ -1368,6 +1368,7 @@ static enum dma_status rcar_dmac_tx_status(struct dma_chan *chan,
 	enum dma_status status;
 	unsigned long flags;
 	unsigned int residue;
+	bool cyclic;
 
 	status = dma_cookie_status(chan, cookie, txstate);
 	if (status == DMA_COMPLETE || !txstate)
@@ -1375,10 +1376,11 @@ static enum dma_status rcar_dmac_tx_status(struct dma_chan *chan,
 
 	spin_lock_irqsave(&rchan->lock, flags);
 	residue = rcar_dmac_chan_get_residue(rchan, cookie);
+	cyclic = rchan->desc.running ? rchan->desc.running->cyclic : false;
 	spin_unlock_irqrestore(&rchan->lock, flags);
 
 	/* if there's no residue, the cookie is complete */
-	if (!residue)
+	if (!residue && !cyclic)
 		return DMA_COMPLETE;
 
 	dma_set_residue(txstate, residue);
-- 
2.20.0

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

[Dirk Behme]

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

Changes in v2: Switch goto/retry to for loop

Changes in v3: None

 drivers/dma/sh/rcar-dmac.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
index 54810ffd95e2..e2a5398f89b5 100644
--- a/drivers/dma/sh/rcar-dmac.c
+++ b/drivers/dma/sh/rcar-dmac.c
@@ -1282,6 +1282,9 @@ 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 chcrb;
+	unsigned int tcrb;
+	unsigned int i;
 
 	if (!desc)
 		return 0;
@@ -1329,6 +1332,24 @@ 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.
+	 * Trying it 3 times should be enough: Initial read, retry, retry
+	 * for the paranoid.
+	 */
+	for (i = 0; i < 3; i++) {
+		chcrb = rcar_dmac_chan_read(chan, RCAR_DMACHCRB) &
+					    RCAR_DMACHCRB_DPTR_MASK;
+		tcrb = rcar_dmac_chan_read(chan, RCAR_DMATCRB);
+		/* Still the same? */
+		if (chcrb == (rcar_dmac_chan_read(chan, RCAR_DMACHCRB) &
+			      RCAR_DMACHCRB_DPTR_MASK))
+			break;
+	}
+	WARN_ONCE(i >= 3, "residue might be not continuous!");
+
 	/*
 	 * In descriptor mode the descriptor running pointer is not maintained
 	 * by the interrupt handler, find the running descriptor from the
@@ -1336,8 +1357,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 +1375,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 v3 2/2] dmaengine: sh: rcar-dmac: Fix glitch in dmaengine_tx_status

[Dirk Behme]

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

Changes in v2: Switch goto/retry to for loop

Changes in v3: None

 drivers/dma/sh/rcar-dmac.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
index 54810ffd95e2..e2a5398f89b5 100644
--- a/drivers/dma/sh/rcar-dmac.c
+++ b/drivers/dma/sh/rcar-dmac.c
@@ -1282,6 +1282,9 @@ 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 chcrb;
+	unsigned int tcrb;
+	unsigned int i;
 
 	if (!desc)
 		return 0;
@@ -1329,6 +1332,24 @@ 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.
+	 * Trying it 3 times should be enough: Initial read, retry, retry
+	 * for the paranoid.
+	 */
+	for (i = 0; i < 3; i++) {
+		chcrb = rcar_dmac_chan_read(chan, RCAR_DMACHCRB) &
+					    RCAR_DMACHCRB_DPTR_MASK;
+		tcrb = rcar_dmac_chan_read(chan, RCAR_DMATCRB);
+		/* Still the same? */
+		if (chcrb == (rcar_dmac_chan_read(chan, RCAR_DMACHCRB) &
+			      RCAR_DMACHCRB_DPTR_MASK))
+			break;
+	}
+	WARN_ONCE(i >= 3, "residue might be not continuous!");
+
 	/*
 	 * In descriptor mode the descriptor running pointer is not maintained
 	 * by the interrupt handler, find the running descriptor from the
@@ -1336,8 +1357,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 +1375,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;
 }
-- 
2.20.0

[v3,1/2] dmaengine: sh: rcar-dmac: With cyclic DMA residue 0 is valid

[Yoshihiro Shimoda]

Hi Dirk-san,

> From: Dirk Behme, Sent: Friday, April 12, 2019 2:29 PM
> 
> Having a cyclic DMA, a residue 0 is not an indication of a completed
> DMA. In case of cyclic DMA make sure that dma_set_residue() is called
> and with this a residue of 0 is forwarded correctly to the caller.
> 
> Fixes: 3544d2878817 ("dmaengine: rcar-dmac: use result of updated get_residue in tx_status")
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> Signed-off-by: Achim Dahlhoff <Achim.Dahlhoff@de.bosch.com>
> Signed-off-by: Hiroyuki Yokoyama <hiroyuki.yokoyama.vx@renesas.com>
> Signed-off-by: Yao Lihua <ylhuajnu@outlook.com>
> Cc: <stable@vger.kernel.org> # v4.8+
> ---

Thank you for the patch! Since cyclic transfers will not stop even if
the residue is 0, I agree this driver should not return DMA_COMPLETE
(transaction completed). So,

Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Best regards,
Yoshihiro Shimoda

RE: [PATCH v3 1/2] dmaengine: sh: rcar-dmac: With cyclic DMA residue 0 is valid

[Yoshihiro Shimoda]

Hi Dirk-san,

> From: Dirk Behme, Sent: Friday, April 12, 2019 2:29 PM
> 
> Having a cyclic DMA, a residue 0 is not an indication of a completed
> DMA. In case of cyclic DMA make sure that dma_set_residue() is called
> and with this a residue of 0 is forwarded correctly to the caller.
> 
> Fixes: 3544d2878817 ("dmaengine: rcar-dmac: use result of updated get_residue in tx_status")
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> Signed-off-by: Achim Dahlhoff <Achim.Dahlhoff@de.bosch.com>
> Signed-off-by: Hiroyuki Yokoyama <hiroyuki.yokoyama.vx@renesas.com>
> Signed-off-by: Yao Lihua <ylhuajnu@outlook.com>
> Cc: <stable@vger.kernel.org> # v4.8+
> ---

Thank you for the patch! Since cyclic transfers will not stop even if
the residue is 0, I agree this driver should not return DMA_COMPLETE
(transaction completed). So,

Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Best regards,
Yoshihiro Shimoda

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

[Yoshihiro Shimoda]

Hi Dirk-san,

> From: Dirk Behme, Sent: Friday, April 12, 2019 2:29 PM
> 
> 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")

I guess this driver has this issue potencially from previous of the commit
(maybe commit ccadee9b1e9 ("dmaengine: rcar-dmac: Implement support for hardware descriptor lists"?).

But, this patch seems good to fix the issue.
So,

Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Best regards,
Yoshihiro Shimoda

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

[Yoshihiro Shimoda]

Hi Dirk-san,

> From: Dirk Behme, Sent: Friday, April 12, 2019 2:29 PM
> 
> 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")

I guess this driver has this issue potencially from previous of the commit
(maybe commit ccadee9b1e9 ("dmaengine: rcar-dmac: Implement support for hardware descriptor lists"?).

But, this patch seems good to fix the issue.
So,

Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Best regards,
Yoshihiro Shimoda

[v3,1/2] dmaengine: sh: rcar-dmac: With cyclic DMA residue 0 is valid

[Laurent Pinchart]

Hi Dirk,

Thank you for the patch.

On Fri, Apr 12, 2019 at 07:29:13AM +0200, Dirk Behme wrote:
> Having a cyclic DMA, a residue 0 is not an indication of a completed
> DMA. In case of cyclic DMA make sure that dma_set_residue() is called
> and with this a residue of 0 is forwarded correctly to the caller.
> 
> Fixes: 3544d2878817 ("dmaengine: rcar-dmac: use result of updated get_residue in tx_status")
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> Signed-off-by: Achim Dahlhoff <Achim.Dahlhoff@de.bosch.com>
> Signed-off-by: Hiroyuki Yokoyama <hiroyuki.yokoyama.vx@renesas.com>
> Signed-off-by: Yao Lihua <ylhuajnu@outlook.com>
> Cc: <stable@vger.kernel.org> # v4.8+

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> Note: Patch done against mainline v5.0
> 
> Changes in v2: None
> 
> Changes in v3: Move reading rchan into the spin lock protection.
> 
>  drivers/dma/sh/rcar-dmac.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
> index 2b4f25698169..54810ffd95e2 100644
> --- a/drivers/dma/sh/rcar-dmac.c
> +++ b/drivers/dma/sh/rcar-dmac.c
> @@ -1368,6 +1368,7 @@ static enum dma_status rcar_dmac_tx_status(struct dma_chan *chan,
>  	enum dma_status status;
>  	unsigned long flags;
>  	unsigned int residue;
> +	bool cyclic;
>  
>  	status = dma_cookie_status(chan, cookie, txstate);
>  	if (status == DMA_COMPLETE || !txstate)
> @@ -1375,10 +1376,11 @@ static enum dma_status rcar_dmac_tx_status(struct dma_chan *chan,
>  
>  	spin_lock_irqsave(&rchan->lock, flags);
>  	residue = rcar_dmac_chan_get_residue(rchan, cookie);
> +	cyclic = rchan->desc.running ? rchan->desc.running->cyclic : false;
>  	spin_unlock_irqrestore(&rchan->lock, flags);
>  
>  	/* if there's no residue, the cookie is complete */
> -	if (!residue)
> +	if (!residue && !cyclic)
>  		return DMA_COMPLETE;
>  
>  	dma_set_residue(txstate, residue);

Re: [PATCH v3 1/2] dmaengine: sh: rcar-dmac: With cyclic DMA residue 0 is valid

[Laurent Pinchart]

Hi Dirk,

Thank you for the patch.

On Fri, Apr 12, 2019 at 07:29:13AM +0200, Dirk Behme wrote:
> Having a cyclic DMA, a residue 0 is not an indication of a completed
> DMA. In case of cyclic DMA make sure that dma_set_residue() is called
> and with this a residue of 0 is forwarded correctly to the caller.
> 
> Fixes: 3544d2878817 ("dmaengine: rcar-dmac: use result of updated get_residue in tx_status")
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> Signed-off-by: Achim Dahlhoff <Achim.Dahlhoff@de.bosch.com>
> Signed-off-by: Hiroyuki Yokoyama <hiroyuki.yokoyama.vx@renesas.com>
> Signed-off-by: Yao Lihua <ylhuajnu@outlook.com>
> Cc: <stable@vger.kernel.org> # v4.8+

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> Note: Patch done against mainline v5.0
> 
> Changes in v2: None
> 
> Changes in v3: Move reading rchan into the spin lock protection.
> 
>  drivers/dma/sh/rcar-dmac.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
> index 2b4f25698169..54810ffd95e2 100644
> --- a/drivers/dma/sh/rcar-dmac.c
> +++ b/drivers/dma/sh/rcar-dmac.c
> @@ -1368,6 +1368,7 @@ static enum dma_status rcar_dmac_tx_status(struct dma_chan *chan,
>  	enum dma_status status;
>  	unsigned long flags;
>  	unsigned int residue;
> +	bool cyclic;
>  
>  	status = dma_cookie_status(chan, cookie, txstate);
>  	if (status == DMA_COMPLETE || !txstate)
> @@ -1375,10 +1376,11 @@ static enum dma_status rcar_dmac_tx_status(struct dma_chan *chan,
>  
>  	spin_lock_irqsave(&rchan->lock, flags);
>  	residue = rcar_dmac_chan_get_residue(rchan, cookie);
> +	cyclic = rchan->desc.running ? rchan->desc.running->cyclic : false;
>  	spin_unlock_irqrestore(&rchan->lock, flags);
>  
>  	/* if there's no residue, the cookie is complete */
> -	if (!residue)
> +	if (!residue && !cyclic)
>  		return DMA_COMPLETE;
>  
>  	dma_set_residue(txstate, residue);

-- 
Regards,

Laurent Pinchart

[v3,1/2] dmaengine: sh: rcar-dmac: With cyclic DMA residue 0 is valid

[Vinod Koul]

On 12-04-19, 07:29, Dirk Behme wrote:
> Having a cyclic DMA, a residue 0 is not an indication of a completed
> DMA. In case of cyclic DMA make sure that dma_set_residue() is called
> and with this a residue of 0 is forwarded correctly to the caller.

Applied both, thanks

Re: [PATCH v3 1/2] dmaengine: sh: rcar-dmac: With cyclic DMA residue 0 is valid

[Vinod Koul]

On 12-04-19, 07:29, Dirk Behme wrote:
> Having a cyclic DMA, a residue 0 is not an indication of a completed
> DMA. In case of cyclic DMA make sure that dma_set_residue() is called
> and with this a residue of 0 is forwarded correctly to the caller.

Applied both, thanks
-- 
~Vinod