[PATCHv2 0/4] Residue patches for rcar-dmac from renesas-drivers

[PATCHv2 0/4] Residue patches for rcar-dmac from renesas-drivers

[Niklas Söderlund]

Hi all,

I have looked over the DMA residue branches in renesas-drivers tree
(topic/rcar-dmac-hamza-v3 and topic/rcar-dmac-residue-v1) hoping to
bring them to upstream. The original author for the bulk of the patches
Hamza Farooq have not shown any activity since autumn 2015.

In this cover letter I talk about all the patches in the above branches
but for obvious reasons only patches I think should be forwarded to
upstream are included in the series. All patches are based on top of
v4.7-rc1 and are tested on Koelsch using Geerts sertest tool to generate
DMA traffic. The highest baud I could get to work at was 921600 bps.
>From reading the original mail threads my conclusion is that this is
good enough since all errors reported by Hamza Farooq where related to
the serial sh-sci driver.

* Patches from topic/rcar-dmac-hamza-v3

- dma: rcar-dma: add wait after stopping dma engine
    In a perfect world one should check that RCAR_DMACHCR_DE is read
    back as 0 after it have been cleared. The documentation clearly
    states that one should do so.

    In the real world the worst case time for this register to be
    cleared as Morimoto-san checked with HW is 700us. Laurent later
    pointed out this is too long to busy loop over since this is done
    from both interrupt context and user context with a spinlock hold.

    Also in the real world the rcar-dmac driver WARN_ON_ONCE() that
    RCAR_DMACHCR_DE is not set before it attempts to start an transferee
    in rcar_dmac_chan_start_xfer(). So if this ever becomes a problem we
    will know about it. At least I have never seen this warning while
    using DMA. Whit this in mind I have not tried to implement a fix for
    this and I think the original patch can be dropped.

- dma: rcar-dma: Added dma_pause operation to rcar_dma driver
    This patch adds support for device pause operation, but not the
    resume operation. As pointed out in the original mail thread the
    pausing must not destroy any already started transaction which it do
    in its current form according to its author.

    Since the feature is incomplete and the use case never documented I
    propose we drop this patch and if we can find the use case create a
    separate task to add pause/resume functionality to the driver later.

- dma: rcar-dma: check if complete DMA packet received but not processed
    This patch is already addressed by Laurent in the original mail
    thread:

    "I don't think this will work. In particular rcar_dmac_tx_status()
    can be called with a cookie corresponding to a transfer not yet
    scheduled. A pending transfer complete interrupt doesn't mean that
    that cookie is complete.

    Furthermore I'm not sure this will really improve performances. The
    dma_cookie_status() call below just compares cookie values, I don't
    expect it to be much slower than a register read."

    I can see no reason to disagree with Laurent here and I propose this
    patch is dropped.

- dma: rcar-dma: use result of updated get_residue in tx_status
    Patch is IMHO good but needs a explanation in commit message to
    address the questions raised in the original mail thread. I have
    added en explanation and included the original patch.

- dma: rcar-dma: warn if transfer cannot start as TE = 1
    Patch is a bit oddly implemented and uses new functionality from
    patch 'dma: rcar-dma: add wait after stopping dma engine' above. I
    do however think the patch have merit. One should make sure
    according to the documentation that RCAR_DMACHCR_TE is not set
    before starting a transfer. I have reworked the check and proposed a
    different solution.

- dma: rcar-dma: Fixed active descriptor initializing
    Patch is clearly a bugfix and looks good. Laurent had a good comment
    in the original mail thread about how it can be made differently by
    reworking rcar_dmac_free_chan_resources() to call
    rcar_dmac_chan_reinit() which would correctly set
    rchan->desc.running to NULL.

    I have tried to implement this but failed. In my attempts to
    implement Laurents proposal which I thought would be trivial the
    kernel locks-up after a while with 'rcu_sched self-detected stall on
    CPU'. And I'm not smart enough to figure out why so I have kept the
    original patch so the bug fix part can be merged since that in
    itself is good.

* Patch from topic/rcar-dmac-residue-v1

- dmaengine: rcar-dmac: Fix residue reporting for pending descriptors
    Patch is good AFIK. There where one report on the mail list by
    Hamza Farooq that state he saw lots of warnings coming from the
    WARN() introduced in this patch. I have run lots of testes using
    Geerts nice sertest tool and have not once seen the warning once. The
    patch is included in this series.


Changes since v1:
- Remove extra blank line from 4/4
- Fix commit title to be consistent with 'dmaengine: rcar-dmac:'
- Collect Ack-by from Laurent

Laurent Pinchart (1):
  dmaengine: rcar-dmac: Fix residue reporting for pending descriptors

Muhammad Hamza Farooq (2):
  dmaengine: rcar-dmac: use result of updated get_residue in tx_status
  dmaengine: rcar-dmac: Fixed active descriptor initializing

Niklas Söderlund (1):
  dmaengine: rcar-dmac: warn if transfer cannot start as TE = 1

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

-- 
2.9.0

[PATCHv2 1/4] dmaengine: rcar-dmac: use result of updated get_residue in tx_status

[Niklas Söderlund]

From: Muhammad Hamza Farooq <mfarooq@visteon.com>

The hardware might have complete the transfer but the interrupt handler
might not have had a chance to run. If rcar_dmac_chan_get_residue()
which reads HW registers finds that there is no residue return
DMA_COMPLETE.

Signed-off-by: Muhammad Hamza Farooq <mfarooq@visteon.com>
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
[Niklas: add explanation in commit message]
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/dma/sh/rcar-dmac.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
index dfb1792..791a064 100644
--- a/drivers/dma/sh/rcar-dmac.c
+++ b/drivers/dma/sh/rcar-dmac.c
@@ -1202,6 +1202,10 @@ static enum dma_status rcar_dmac_tx_status(struct dma_chan *chan,
 	residue = rcar_dmac_chan_get_residue(rchan, cookie);
 	spin_unlock_irqrestore(&rchan->lock, flags);
 
+	/* if there's no residue, the cookie is complete */
+	if (!residue)
+		return DMA_COMPLETE;
+
 	dma_set_residue(txstate, residue);
 
 	return status;
-- 
2.9.0

[PATCHv2 2/4] dmaengine: rcar-dmac: warn if transfer cannot start as TE = 1

[Niklas Söderlund]

The documentation states one should make sure both DE and TE are cleared
before starting a transaction. This patch extends the current warning to
look at both DE and TE.

Based on previous work from Muhammad Hamza Farooq.

Suggested-by: Muhammad Hamza Farooq <mfarooq@visteon.com>
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/dma/sh/rcar-dmac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
index 791a064..7f26576 100644
--- a/drivers/dma/sh/rcar-dmac.c
+++ b/drivers/dma/sh/rcar-dmac.c
@@ -311,7 +311,7 @@ static bool rcar_dmac_chan_is_busy(struct rcar_dmac_chan *chan)
 {
 	u32 chcr = rcar_dmac_chan_read(chan, RCAR_DMACHCR);
 
-	return (chcr & (RCAR_DMACHCR_DE | RCAR_DMACHCR_TE)) == RCAR_DMACHCR_DE;
+	return !!(chcr & (RCAR_DMACHCR_DE | RCAR_DMACHCR_TE));
 }
 
 static void rcar_dmac_chan_start_xfer(struct rcar_dmac_chan *chan)
-- 
2.9.0

[PATCHv2 3/4] dmaengine: rcar-dmac: Fixed active descriptor initializing

[Niklas Söderlund]

From: Muhammad Hamza Farooq <mfarooq@visteon.com>

Running descriptor pointer is set to NULL upon freeing resources. Other-
wise, rcar_dmac_issue_pending might not start new transfers

Signed-off-by: Muhammad Hamza Farooq <mfarooq@visteon.com>
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/dma/sh/rcar-dmac.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
index 7f26576..59951fb 100644
--- a/drivers/dma/sh/rcar-dmac.c
+++ b/drivers/dma/sh/rcar-dmac.c
@@ -990,6 +990,8 @@ static void rcar_dmac_free_chan_resources(struct dma_chan *chan)
 	list_splice_init(&rchan->desc.done, &list);
 	list_splice_init(&rchan->desc.wait, &list);
 
+	rchan->desc.running = NULL;
+
 	list_for_each_entry(desc, &list, node)
 		rcar_dmac_realloc_hwdesc(rchan, desc, 0);
 
-- 
2.9.0

[PATCHv2 4/4] dmaengine: rcar-dmac: Fix residue reporting for pending descriptors

[Niklas Söderlund]

From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Cookies corresponding to pending transfers have a residue value equal to
the full size of the corresponding descriptor. The driver miscomputes
that and uses the size of the active descriptor instead. Fix it.

Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
[geert: Also check desc.active list]
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/dma/sh/rcar-dmac.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
index 59951fb..d536dbf 100644
--- a/drivers/dma/sh/rcar-dmac.c
+++ b/drivers/dma/sh/rcar-dmac.c
@@ -1145,6 +1145,7 @@ static unsigned int rcar_dmac_chan_get_residue(struct rcar_dmac_chan *chan,
 	struct rcar_dmac_desc *desc = chan->desc.running;
 	struct rcar_dmac_xfer_chunk *running = NULL;
 	struct rcar_dmac_xfer_chunk *chunk;
+	enum dma_status status;
 	unsigned int residue = 0;
 	unsigned int dptr = 0;
 
@@ -1152,12 +1153,38 @@ static unsigned int rcar_dmac_chan_get_residue(struct rcar_dmac_chan *chan,
 		return 0;
 
 	/*
+	 * If the cookie corresponds to a descriptor that has been completed
+	 * there is no residue. The same check has already been performed by the
+	 * caller but without holding the channel lock, so the descriptor could
+	 * now be complete.
+	 */
+	status = dma_cookie_status(&chan->chan, cookie, NULL);
+	if (status == DMA_COMPLETE)
+		return 0;
+
+	/*
 	 * If the cookie doesn't correspond to the currently running transfer
 	 * then the descriptor hasn't been processed yet, and the residue is
 	 * equal to the full descriptor size.
 	 */
-	if (cookie != desc->async_tx.cookie)
-		return desc->size;
+	if (cookie != desc->async_tx.cookie) {
+		list_for_each_entry(desc, &chan->desc.pending, node) {
+			if (cookie == desc->async_tx.cookie)
+				return desc->size;
+		}
+		list_for_each_entry(desc, &chan->desc.active, node) {
+			if (cookie == desc->async_tx.cookie)
+				return desc->size;
+		}
+
+		/*
+		 * No descriptor found for the cookie, there's thus no residue.
+		 * This shouldn't happen if the calling driver passes a correct
+		 * cookie value.
+		 */
+		WARN(1, "No descriptor for cookie!");
+		return 0;
+	}
 
 	/*
 	 * In descriptor mode the descriptor running pointer is not maintained
-- 
2.9.0

Re: [PATCHv2 0/4] Residue patches for rcar-dmac from renesas-drivers

[Vinod Koul]

On Thu, Jun 30, 2016 at 05:15:14PM +0200, Niklas S�derlund wrote:
> Hi all,
> 
> I have looked over the DMA residue branches in renesas-drivers tree
> (topic/rcar-dmac-hamza-v3 and topic/rcar-dmac-residue-v1) hoping to
> bring them to upstream. The original author for the bulk of the patches
> Hamza Farooq have not shown any activity since autumn 2015.
> 
> In this cover letter I talk about all the patches in the above branches
> but for obvious reasons only patches I think should be forwarded to
> upstream are included in the series. All patches are based on top of
> v4.7-rc1 and are tested on Koelsch using Geerts sertest tool to generate
> DMA traffic. The highest baud I could get to work at was 921600 bps.
> From reading the original mail threads my conclusion is that this is
> good enough since all errors reported by Hamza Farooq where related to
> the serial sh-sci driver.

Applied all, thanks

-- 
~Vinod

[PATCHv2,1/4] dmaengine: rcar-dmac: use result of updated get_residue in tx_status

[Dirk Behme]

On 30.06.2016 17:15, Niklas Söderlund wrote:
> From: Muhammad Hamza Farooq <mfarooq@visteon.com>
> 
> The hardware might have complete the transfer but the interrupt handler
> might not have had a chance to run. If rcar_dmac_chan_get_residue()
> which reads HW registers finds that there is no residue return
> DMA_COMPLETE.
> 
> Signed-off-by: Muhammad Hamza Farooq <mfarooq@visteon.com>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> [Niklas: add explanation in commit message]
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>   drivers/dma/sh/rcar-dmac.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
> index dfb1792..791a064 100644
> --- a/drivers/dma/sh/rcar-dmac.c
> +++ b/drivers/dma/sh/rcar-dmac.c
> @@ -1202,6 +1202,10 @@ static enum dma_status rcar_dmac_tx_status(struct dma_chan *chan,
>   	residue = rcar_dmac_chan_get_residue(rchan, cookie);
>   	spin_unlock_irqrestore(&rchan->lock, flags);
>   
> +	/* if there's no residue, the cookie is complete */
> +	if (!residue)
> +		return DMA_COMPLETE;
> +
>   	dma_set_residue(txstate, residue);
>   
>   	return status;


We have some doubts about this change (mainline commit [1]). Not being a 
DMA expert, let me try to explain:

We are configuring a cyclic, never ending DMA 
(dmaengine_prep_dma_cyclic()). For this cyclic DMA we continuously poll 
the residue (txstate->residue) via rcar_dmac_tx_status(). Having a 
cyclic, never ending DMA we think that residue == 0 is a valid value. 
However, with above change, a residue value of 0 is 'dropped' and never 
written via dma_set_residue() to txstate. So in case we continuously 
poll, this value is lost, resulting in wrong behavior of the caller.

In our case with cyclic, never ending DMA, changing this to

         dma_set_residue(txstate, residue);

?

Opinions?

Best regards

Dirk

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/dma/sh/rcar-dmac.c?h=v5.0-rc2&id=3544d2878817bd139dda238cdd86a15e1c03d037

--- a/drivers/dma/sh/rcar-dmac.c
+++ b/drivers/dma/sh/rcar-dmac.c
@@ -1388,7 +1388,7 @@ static enum dma_status rcar_dmac_tx_status(struct 
dma_chan *chan,

         /* if there's no residue, the cookie is complete */
         if (!residue)
-               return DMA_COMPLETE;
+               status = DMA_COMPLETE;

         dma_set_residue(txstate, residue);

seems to help. If we ignore the still wrong DMA_COMPLETE status (which 
in case of cyclic DMA doesn't make any sense?) the caller get the 
correct txstate->residue values (not dropping the 0).

So maybe we need anything like

--- a/drivers/dma/sh/rcar-dmac.c
+++ b/drivers/dma/sh/rcar-dmac.c
@@ -1387,7 +1387,7 @@ static enum dma_status rcar_dmac_tx_status(struct 
dma_chan *chan,
         spin_unlock_irqrestore(&rchan->lock, flags);

         /* if there's no residue, the cookie is complete */
-       if (!residue)
+       if (!residue && !chan->desc.running->cyclic)
                 return DMA_COMPLETE;

Re: [PATCHv2 1/4] dmaengine: rcar-dmac: use result of updated get_residue in tx_status

[Dirk Behme]

On 30.06.2016 17:15, Niklas Söderlund wrote:
> From: Muhammad Hamza Farooq <mfarooq@visteon.com>
> 
> The hardware might have complete the transfer but the interrupt handler
> might not have had a chance to run. If rcar_dmac_chan_get_residue()
> which reads HW registers finds that there is no residue return
> DMA_COMPLETE.
> 
> Signed-off-by: Muhammad Hamza Farooq <mfarooq@visteon.com>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> [Niklas: add explanation in commit message]
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>   drivers/dma/sh/rcar-dmac.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
> index dfb1792..791a064 100644
> --- a/drivers/dma/sh/rcar-dmac.c
> +++ b/drivers/dma/sh/rcar-dmac.c
> @@ -1202,6 +1202,10 @@ static enum dma_status rcar_dmac_tx_status(struct dma_chan *chan,
>   	residue = rcar_dmac_chan_get_residue(rchan, cookie);
>   	spin_unlock_irqrestore(&rchan->lock, flags);
>   
> +	/* if there's no residue, the cookie is complete */
> +	if (!residue)
> +		return DMA_COMPLETE;
> +
>   	dma_set_residue(txstate, residue);
>   
>   	return status;


We have some doubts about this change (mainline commit [1]). Not being a 
DMA expert, let me try to explain:

We are configuring a cyclic, never ending DMA 
(dmaengine_prep_dma_cyclic()). For this cyclic DMA we continuously poll 
the residue (txstate->residue) via rcar_dmac_tx_status(). Having a 
cyclic, never ending DMA we think that residue == 0 is a valid value. 
However, with above change, a residue value of 0 is 'dropped' and never 
written via dma_set_residue() to txstate. So in case we continuously 
poll, this value is lost, resulting in wrong behavior of the caller.

In our case with cyclic, never ending DMA, changing this to

--- a/drivers/dma/sh/rcar-dmac.c
+++ b/drivers/dma/sh/rcar-dmac.c
@@ -1388,7 +1388,7 @@ static enum dma_status rcar_dmac_tx_status(struct 
dma_chan *chan,

         /* if there's no residue, the cookie is complete */
         if (!residue)
-               return DMA_COMPLETE;
+               status = DMA_COMPLETE;

         dma_set_residue(txstate, residue);

seems to help. If we ignore the still wrong DMA_COMPLETE status (which 
in case of cyclic DMA doesn't make any sense?) the caller get the 
correct txstate->residue values (not dropping the 0).

So maybe we need anything like

--- a/drivers/dma/sh/rcar-dmac.c
+++ b/drivers/dma/sh/rcar-dmac.c
@@ -1387,7 +1387,7 @@ static enum dma_status rcar_dmac_tx_status(struct 
dma_chan *chan,
         spin_unlock_irqrestore(&rchan->lock, flags);

         /* if there's no residue, the cookie is complete */
-       if (!residue)
+       if (!residue && !chan->desc.running->cyclic)
                 return DMA_COMPLETE;

         dma_set_residue(txstate, residue);

?

Opinions?

Best regards

Dirk

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/dma/sh/rcar-dmac.c?h=v5.0-rc2&id=3544d2878817bd139dda238cdd86a15e1c03d037

[PATCHv2,1/4] dmaengine: rcar-dmac: use result of updated get_residue in tx_status

[Yoshihiro Shimoda]

Hi Dirk,
(revised Vinod's email address)

> From: Dirk Behme, Sent: Tuesday, January 15, 2019 9:44 PM
> 
> On 30.06.2016 17:15, Niklas Söderlund wrote:
> > From: Muhammad Hamza Farooq <mfarooq@visteon.com>
> >
> > The hardware might have complete the transfer but the interrupt handler
> > might not have had a chance to run. If rcar_dmac_chan_get_residue()
> > which reads HW registers finds that there is no residue return
> > DMA_COMPLETE.
> >
> > Signed-off-by: Muhammad Hamza Farooq <mfarooq@visteon.com>
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > [Niklas: add explanation in commit message]
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >   drivers/dma/sh/rcar-dmac.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
> > index dfb1792..791a064 100644
> > --- a/drivers/dma/sh/rcar-dmac.c
> > +++ b/drivers/dma/sh/rcar-dmac.c
> > @@ -1202,6 +1202,10 @@ static enum dma_status rcar_dmac_tx_status(struct dma_chan *chan,
> >   	residue = rcar_dmac_chan_get_residue(rchan, cookie);
> >   	spin_unlock_irqrestore(&rchan->lock, flags);
> >
> > +	/* if there's no residue, the cookie is complete */
> > +	if (!residue)
> > +		return DMA_COMPLETE;
> > +
> >   	dma_set_residue(txstate, residue);
> >
> >   	return status;
> 
> 
> We have some doubts about this change (mainline commit [1]). Not being a
> DMA expert, let me try to explain:
> 
> We are configuring a cyclic, never ending DMA
> (dmaengine_prep_dma_cyclic()). For this cyclic DMA we continuously poll
> the residue (txstate->residue) via rcar_dmac_tx_status(). Having a
> cyclic, never ending DMA we think that residue == 0 is a valid value.
> However, with above change, a residue value of 0 is 'dropped' and never
> written via dma_set_residue() to txstate. So in case we continuously
> poll, this value is lost, resulting in wrong behavior of the caller.

According to the Documentation/driver-api/dmaengine/provider.rst [1],
device_tx_status should report the bytes left. So, we should fix
the current implementation as you said.
---
[1]
``device_tx_status``

  - Should report the bytes left to go over on the given channel
  <snip>
  - In the case of a cyclic transfer, it should only take into
    account the current period.
---

> In our case with cyclic, never ending DMA, changing this to
> 
> --- a/drivers/dma/sh/rcar-dmac.c
> +++ b/drivers/dma/sh/rcar-dmac.c
> @@ -1388,7 +1388,7 @@ static enum dma_status rcar_dmac_tx_status(struct
> dma_chan *chan,
> 
>          /* if there's no residue, the cookie is complete */
>          if (!residue)
> -               return DMA_COMPLETE;
> +               status = DMA_COMPLETE;
> 
>          dma_set_residue(txstate, residue);
> 
> seems to help.

So, at least, we should apply this code above.

>           If we ignore the still wrong DMA_COMPLETE status (which
> in case of cyclic DMA doesn't make any sense?) the caller get the
> correct txstate->residue values (not dropping the 0).
> 
> So maybe we need anything like
> 
> --- a/drivers/dma/sh/rcar-dmac.c
> +++ b/drivers/dma/sh/rcar-dmac.c
> @@ -1387,7 +1387,7 @@ static enum dma_status rcar_dmac_tx_status(struct
> dma_chan *chan,
>          spin_unlock_irqrestore(&rchan->lock, flags);
> 
>          /* if there's no residue, the cookie is complete */
> -       if (!residue)
> +       if (!residue && !chan->desc.running->cyclic)
>                  return DMA_COMPLETE;
> 
>          dma_set_residue(txstate, residue);
> 
> ?

I could not find whether a cyclic transfer should not return DMA_COMPLETE on
Documentations/. I only found the following in the include/linux/dmaengine.h:

/**
 * enum dma_status - DMA transaction status
 * @DMA_COMPLETE: transaction completed

Best regards,
Yoshihiro Shimoda

> Opinions?
> 
> Best regards
> 
> Dirk
> 
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/dma/sh/rcar-dmac.c?h=v5.0-rc2&id=3
> 544d2878817bd139dda238cdd86a15e1c03d037

RE: [PATCHv2 1/4] dmaengine: rcar-dmac: use result of updated get_residue in tx_status

[Yoshihiro Shimoda]

Hi Dirk,
(revised Vinod's email address)

> From: Dirk Behme, Sent: Tuesday, January 15, 2019 9:44 PM
> 
> On 30.06.2016 17:15, Niklas Söderlund wrote:
> > From: Muhammad Hamza Farooq <mfarooq@visteon.com>
> >
> > The hardware might have complete the transfer but the interrupt handler
> > might not have had a chance to run. If rcar_dmac_chan_get_residue()
> > which reads HW registers finds that there is no residue return
> > DMA_COMPLETE.
> >
> > Signed-off-by: Muhammad Hamza Farooq <mfarooq@visteon.com>
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > [Niklas: add explanation in commit message]
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >   drivers/dma/sh/rcar-dmac.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
> > index dfb1792..791a064 100644
> > --- a/drivers/dma/sh/rcar-dmac.c
> > +++ b/drivers/dma/sh/rcar-dmac.c
> > @@ -1202,6 +1202,10 @@ static enum dma_status rcar_dmac_tx_status(struct dma_chan *chan,
> >   	residue = rcar_dmac_chan_get_residue(rchan, cookie);
> >   	spin_unlock_irqrestore(&rchan->lock, flags);
> >
> > +	/* if there's no residue, the cookie is complete */
> > +	if (!residue)
> > +		return DMA_COMPLETE;
> > +
> >   	dma_set_residue(txstate, residue);
> >
> >   	return status;
> 
> 
> We have some doubts about this change (mainline commit [1]). Not being a
> DMA expert, let me try to explain:
> 
> We are configuring a cyclic, never ending DMA
> (dmaengine_prep_dma_cyclic()). For this cyclic DMA we continuously poll
> the residue (txstate->residue) via rcar_dmac_tx_status(). Having a
> cyclic, never ending DMA we think that residue == 0 is a valid value.
> However, with above change, a residue value of 0 is 'dropped' and never
> written via dma_set_residue() to txstate. So in case we continuously
> poll, this value is lost, resulting in wrong behavior of the caller.

According to the Documentation/driver-api/dmaengine/provider.rst [1],
device_tx_status should report the bytes left. So, we should fix
the current implementation as you said.
---
[1]
``device_tx_status``

  - Should report the bytes left to go over on the given channel
  <snip>
  - In the case of a cyclic transfer, it should only take into
    account the current period.
---

> In our case with cyclic, never ending DMA, changing this to
> 
> --- a/drivers/dma/sh/rcar-dmac.c
> +++ b/drivers/dma/sh/rcar-dmac.c
> @@ -1388,7 +1388,7 @@ static enum dma_status rcar_dmac_tx_status(struct
> dma_chan *chan,
> 
>          /* if there's no residue, the cookie is complete */
>          if (!residue)
> -               return DMA_COMPLETE;
> +               status = DMA_COMPLETE;
> 
>          dma_set_residue(txstate, residue);
> 
> seems to help.

So, at least, we should apply this code above.

>           If we ignore the still wrong DMA_COMPLETE status (which
> in case of cyclic DMA doesn't make any sense?) the caller get the
> correct txstate->residue values (not dropping the 0).
> 
> So maybe we need anything like
> 
> --- a/drivers/dma/sh/rcar-dmac.c
> +++ b/drivers/dma/sh/rcar-dmac.c
> @@ -1387,7 +1387,7 @@ static enum dma_status rcar_dmac_tx_status(struct
> dma_chan *chan,
>          spin_unlock_irqrestore(&rchan->lock, flags);
> 
>          /* if there's no residue, the cookie is complete */
> -       if (!residue)
> +       if (!residue && !chan->desc.running->cyclic)
>                  return DMA_COMPLETE;
> 
>          dma_set_residue(txstate, residue);
> 
> ?

I could not find whether a cyclic transfer should not return DMA_COMPLETE on
Documentations/. I only found the following in the include/linux/dmaengine.h:

/**
 * enum dma_status - DMA transaction status
 * @DMA_COMPLETE: transaction completed

Best regards,
Yoshihiro Shimoda

> Opinions?
> 
> Best regards
> 
> Dirk
> 
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/dma/sh/rcar-dmac.c?h=v5.0-rc2&id=3
> 544d2878817bd139dda238cdd86a15e1c03d037