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

login
register
mail settings
Submitter Dirk Behme
Date Jan. 15, 2019, 12:44 p.m.
Message ID <22f3bf50-6241-5beb-0d6d-c556b6cfebb9@de.bosch.com>
Download mbox | patch
Permalink /patch/700255/
State New
Headers show

Comments

Dirk Behme - Jan. 15, 2019, 12:44 p.m.
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
Yoshihiro Shimoda - Jan. 21, 2019, 3:16 a.m.
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

Patch

--- 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;