From: Dirk Behme <dirk.behme@de.bosch.com>
To: "Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>,
linux-renesas-soc@vger.kernel.org,
laurent.pinchart+renesas@ideasonboard.com
Cc: <dmaengine@vger.kernel.org>, <vinod.koul@intel.com>,
<geert+renesas@glider.be>, <mfarooq@visteon.com>,
Achim Dahlhoff <Achim.Dahlhoff@de.bosch.com>
Subject: Re: [PATCHv2 1/4] dmaengine: rcar-dmac: use result of updated get_residue in tx_status
Date: Tue, 15 Jan 2019 13:44:23 +0100 [thread overview]
Message-ID: <22f3bf50-6241-5beb-0d6d-c556b6cfebb9@de.bosch.com> (raw)
In-Reply-To: <20160630151518.32329-2-niklas.soderlund+renesas@ragnatech.se>
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
next prev parent reply other threads:[~2019-01-15 12:51 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-06-30 15:15 [PATCHv2 0/4] Residue patches for rcar-dmac from renesas-drivers Niklas Söderlund
2016-06-30 15:15 ` [PATCHv2 1/4] dmaengine: rcar-dmac: use result of updated get_residue in tx_status Niklas Söderlund
2019-01-15 12:44 ` Dirk Behme [this message]
2019-01-21 3:16 ` Yoshihiro Shimoda
2016-06-30 15:15 ` [PATCHv2 2/4] dmaengine: rcar-dmac: warn if transfer cannot start as TE = 1 Niklas Söderlund
2016-06-30 15:15 ` [PATCHv2 3/4] dmaengine: rcar-dmac: Fixed active descriptor initializing Niklas Söderlund
2016-06-30 15:15 ` [PATCHv2 4/4] dmaengine: rcar-dmac: Fix residue reporting for pending descriptors Niklas Söderlund
2016-07-08 5:39 ` [PATCHv2 0/4] Residue patches for rcar-dmac from renesas-drivers Vinod Koul
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=22f3bf50-6241-5beb-0d6d-c556b6cfebb9@de.bosch.com \
--to=dirk.behme@de.bosch.com \
--cc=Achim.Dahlhoff@de.bosch.com \
--cc=dmaengine@vger.kernel.org \
--cc=geert+renesas@glider.be \
--cc=laurent.pinchart+renesas@ideasonboard.com \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=mfarooq@visteon.com \
--cc=niklas.soderlund+renesas@ragnatech.se \
--cc=vinod.koul@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).