linux-kernel - Re: [PATCH] dmaengine: rcar-dmac: use DMATCRB when xxx_TO

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 16 Oct 2017 11:49:53 +0300
From:   Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:     Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>
Cc:     Geert Uytterhoeven <geert@...ux-m68k.org>,
        Dan Williams <dan.j.williams@...el.com>,
        Vinod Koul <vinod.koul@...el.com>,
        Niklas Söderlund 
        <niklas.soderlund+renesas@...natech.se>, dmaengine@...r.kernel.org,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Hiroyuki Yokoyama <hiroyuki.yokoyama.vx@...esas.com>
Subject: Re: [PATCH] dmaengine: rcar-dmac: use DMATCRB when xxx_TO_MEM direction

Hi Morimoto-san,

Thank you for the patch.

(By the way the subject line should have mentioned v2)

On Monday, 16 October 2017 10:28:35 EEST Kuninori Morimoto wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>
> 
> SYS/RT/Audio DMAC have both TCR/TCRB.
> Its difference is transfer counter value of read (= TCR)
> or write (= TCRB). The relationship is like below.
> 
>          TCR       TCRB
>  [SOURCE] -> [DMAC] -> [DESTINATION]
> 
> Thus, for residue calculation, we want to read
> TCRB when MEM_TO_MEM/DEV_TO_MEM
> TCR  when MEM_TO_DEV
> Otherwise, Sound Capture has noise after PluseAudio support
> (= 07b7acb51d2 ("ASoC: rsnd: update pointer more accurate"))

In the MEM_TO_DEV direction, what really matters is how much data has been 
written to memory or to the device. If the DMA is interrupted between read and 
write, the data read but not written doesn't matter. It doesn't end up in the 
destination, so shouldn't be counted. TCRB is thus the register we should use 
in this cases.

In the DEV_TO_MEM direction, the situation is more complex. Both the read and 
write side are important. What matters from a data consumer point of view is 
how much data has been written to memory. On the other hand, if the transfer 
is interrupted between read and write, we'll end up losing data (read from the 
device but never written to memory), which can also be important to report.

In practice, however, I see why DMA could be interrupted between read and 
write in the MEM_TO_DEV case if the device doesn't acknowledge a write for 
whatever reason. Interruptions in the DEV_TO_MEM case would surprise me, as 
the write side shouldn't prevent data from reaching memory (or we'd have much 
worse problems than DMA). Both TCR and TCRB should thus be equivalent in this 
case.

Similarly the MEM_TO_MEM case should be fine with both TCR and TCRB.

There could be problems I'm not aware of, so the explanation above might not 
be correct, but if it is you could use TCRB unconditionally as you did in v1. 
In any case, this patch uses TCR for MEM_TO_DEV, while I think the correct 
register in that case is TCRB.

I believe it would be worth capturing the above explanation in the commit 
message and/or comment.

> Signed-off-by: Hiroyuki Yokoyama <hiroyuki.yokoyama.vx@...esas.com>
> [Kuninori: add detail info on log, care direction]
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>
> ---
> v1 -> v2
> 
>  - fixup Subject
>  - care direction
>  - From Kuninori Morimoto
> 
>  drivers/dma/sh/rcar-dmac.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
> index 2b2c7db..f5d7bb7 100644
> --- a/drivers/dma/sh/rcar-dmac.c
> +++ b/drivers/dma/sh/rcar-dmac.c
> @@ -1246,6 +1246,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;
> +	u32 reg;
> 
>  	if (!desc)
>  		return 0;
> @@ -1309,8 +1310,17 @@ static unsigned int rcar_dmac_chan_get_residue(struct
> rcar_dmac_chan *chan, residue += chunk->size;
>  	}
> 
> -	/* Add the residue for the current chunk. */
> -	residue += rcar_dmac_chan_read(chan, RCAR_DMATCR) << desc->xfer_shift;
> +	/*
> +	 * Add the residue for the current chunk
> +	 *
> +	 *         TCR       TCRB
> +	 * [SOURCE] -> [DMAC] -> [DESTINATION]
> +	 *
> +	 * MEM_TO_xxx : TCR
> +	 * xxx_TO_MEM : TCRB
> +	 */
> +	reg = (desc->direction == DMA_MEM_TO_DEV) ? RCAR_DMATCR : RCAR_DMATCRB;
> +	residue += rcar_dmac_chan_read(chan, reg) << desc->xfer_shift;
> 
>  	return residue;
>  }

-- 
Regards,

Laurent Pinchart