linux-kernel - Re: [PATCH v5 2/2] iio: adc: ad4130: add AD4130 driver

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:   Thu, 23 Jun 2022 19:33:45 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Cosmin Tanislav <demonsingur@...il.com>
Cc:     Jonathan Cameron <jic23@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        linux-iio <linux-iio@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Cosmin Tanislav <cosmin.tanislav@...log.com>
Subject: Re: [PATCH v5 2/2] iio: adc: ad4130: add AD4130 driver

On Thu, Jun 23, 2022 at 6:14 PM Cosmin Tanislav <demonsingur@...il.com> wrote:
> On 6/23/22 18:39, Andy Shevchenko wrote:
> > On Thu, Jun 23, 2022 at 5:27 PM Cosmin Tanislav <demonsingur@...il.com> wrote:
> >> On 6/20/22 21:29, Andy Shevchenko wrote:
> >>> On Mon, Jun 20, 2022 at 6:27 PM Cosmin Tanislav <demonsingur@...il.com> wrote:

...

> >>>> +       /*
> >>>> +        * DMA (thus cache coherency maintenance) requires the
> >>>> +        * transfer buffers to live in their own cache lines.
> >>>> +        */
> >>>
> >>> This is a good comment, but what fields does it apply to?
> >>
> >> Whatever is below it, grouped together. This is not hard to
> >> understand.
> >
> > It's hard to understand what exactly is DMA-aware here. I see only one
> > buffer that is aligned properly for DMA, the rest are not, except the
> > case if all of them are going in one DMA transaction. Is this the case
> > here?
> >
> >>>> +       u8                      reset_buf[AD4130_RESET_BUF_SIZE] __aligned(IIO_DMA_MINALIGN);
> >
> > This is aligned.
> >
> >>>> +       u8                      reg_write_tx_buf[4];
> >
> > This one is aligned + offset (== AD4130_RESET_BUF_SIZE + 0).
> >
> >>>> +       u8                      reg_read_tx_buf[1];
> >
> > This one is aligned + offset (== AD4130_RESET_BUF_SIZE + 0 + 4).
> >
> >>>> +       u8                      reg_read_rx_buf[3];
> >
> > This one is aligned + offset (== AD4130_RESET_BUF_SIZE + 0 + 4 + 1).
> > And this is Rx.
> >
> >>>> +       u8                      fifo_tx_buf[2];
> >
> > Here is Tx again which is most likely is not aligned...
> >
> >>>> +       u8                      fifo_rx_buf[AD4130_FIFO_SIZE *
> >>>> +                                           AD4130_FIFO_MAX_SAMPLE_SIZE];
> >>>> +};
> >
>
> This has been mentioned before by Jonathan as a reply to V6 of my
> AD74413R driver.
>
>  > I'm surprised I didn't mention this before but you only need to
> ensure  > that any memory used for DMA is not in a cacheline with memory
> used
>  > for other things that might change concurrently.
>
> To my understanding, as long as the DMA buffers will all be accessed by
> the same DMA-compatible SPI controller, you only need to align them so
> they're not in the same cacheline with memory that will not be accessed
> by the SPI controller.

SPI is synchronous by nature, what will happen if the Tx and Rx
buffers are sharing the same cache line? Anybody to shed a light here?

(I.o.w. I'm not sure that we don't need to split the Rx and Tx buffers
of the same transfer.)

-- 
With Best Regards,
Andy Shevchenko