linux-kernel - Re: [PATCH 2/2 v2] dmaengine: rcar-dmac: use TCRB instead of TCR for residue

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]

Message-ID: <87lgj5rcr5.wl%kuninori.morimoto.gx@renesas.com>
Date:   Fri, 17 Nov 2017 00:13:48 +0000
From:   Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        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 2/2 v2] dmaengine: rcar-dmac: use TCRB instead of TCR for residue


Hi Geert

Thank you for your review, and test.
But where is my brown paper bag for English spell ?

> 
> Hi Morimoto-san,
> 
> On Thu, Nov 16, 2017 at 5:34 AM, Kuninori Morimoto
> <kuninori.morimoto.gx@...esas.com> wrote:
> > From: Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>
> 
> Thanks for your patch!
> 
> > SYS/RT/Audio DMAC includes independent data buffers for reading
> > and writing. Therefore, the read transfer counter and write transfer
> > counter have different values.
> > TCR indicates read counter, and TCRB indicates write counter.
> > The relationship is like below.
> >
> >                 TCR       TCRB
> >         [SOURCE] -> [DMAC] -> [SINK]
> >
> > In the MEM_TO_DEV direction, what really matters is how much data has
> > been written to the device. If the DMA is interrupted between read and
> > write, then, the data 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. It can also be important to report.
> >
> > In the MEM_TO_MEM direction, what matters is of course how much data
> > has been written to memory from data consumer point of view.
> > Here, because read and write have independent data buffers, it will
> > take a while for TCR and TCRB to become equal. Thus we should check
> > TCRB in this case, too.
> >
> > Thus, all cases we should check TCRB instead of TCR.
> >
> > Without this patch, Sound Capture has noise after PluseAudio support
> 
> PulseAudio
> 
> > (= 07b7acb51d2 ("ASoC: rsnd: update pointer more accurate")), because
> > the recorder will use wrong residue counter which indicates transferred
> > from sound device, but in reality the data was not yet put to memory
> > and recorder will record it.
> >
> > However, because DMAC is buffering data until it can be transferable
> > size, TCRB might not be updated.
> > For example, if consumer doesn't know how much data can be receaved,
> 
> received
> 
> > it requests enough size to DMAC. But in reality, it might receave very
> 
> receive
> 
> > few data. In such case, DMAC just buffered it untile transferable size,
> 
> until
> 
> > and no TCRB updated.
> >
> > In such case, this buffered data will be transferred if CHCR::DE bit was
> > cleared, and this is happen if rcar_dmac_chan_halt(). In other word, it
> > happen when consumer called dmaengine_terminate_all().
> >
> > Because of this behavior, it need to flush buffered data when it returns
> > "residue" (= dmaengine_tx_status()).
> > Otherwise, consumer might calculate wrong things if it called
> > dmaengine_tx_status() and dmaengine_terminate_all() consecutively.
> >
> > Tested-by: Hiroyuki Yokoyama <hiroyuki.yokoyama.vx@...esas.com>
> > Tested-by: Ryo Kodama <ryo.kodama.vz@...esas.com>
> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>
> 
> My serial console is happy again (on Koelsch and Salvator-XS), so:
> Tested-by: Geert Uytterhoeven <geert+renesas@...der.be>
> 
> > --- a/drivers/dma/sh/rcar-dmac.c
> > +++ b/drivers/dma/sh/rcar-dmac.c
> > @@ -761,6 +761,23 @@ static void rcar_dmac_chcr_de_barrier(struct rcar_dmac_chan *chan)
> >         dev_err(chan->chan.device->dev, "CHCR DE check error\n");
> >  }
> >
> > +static void rcar_dmac_sync_tcr(struct rcar_dmac_chan *chan)
> > +{
> > +       u32 chcr = rcar_dmac_chan_read(chan, RCAR_DMACHCR);
> > +
> > +       if (!(chcr & RCAR_DMACHCR_DE))
> > +               return;
> > +
> > +       /* set DE=0 and flush remaining data */
> > +       rcar_dmac_chan_write(chan, RCAR_DMACHCR, (chcr & ~RCAR_DMACHCR_DE));
> > +
> > +       /* make sure all remaining data was fulshed */
> 
> flushed
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds