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

[Jonathan Cameron]

On Thu, 23 Jun 2022 19:33:45 +0200
Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:

> On Thu, Jun 23, 2022 at 6:14 PM Cosmin Tanislav <demonsingur@xxxxxxxxx> wrote:
> > On 6/23/22 18:39, Andy Shevchenko wrote:  
> > > On Thu, Jun 23, 2022 at 5:27 PM Cosmin Tanislav <demonsingur@xxxxxxxxx> wrote:  
> > >> On 6/20/22 21:29, Andy Shevchenko wrote:  
> > >>> On Mon, Jun 20, 2022 at 6:27 PM Cosmin Tanislav <demonsingur@xxxxxxxxx> 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.)
> 
Oddly I thought I replied to this before lunch, but not seeing my own message.
My understanding is that any device that trashes it's own buffers during
DMA is broken and needs to implement bounce buffers within the SPI master
driver.  That shouldn't happen anyway because the race conditions around
this are about caching and stale data write back. During a transfer, only
the device should touch the cacheline with the DMA buffers in it, so any
write back that might return stale data should be pushing back the correct
data (unchanged) or data the device wants to update..

My suggestion is an expanded comment (I'm fine adding this whilst applying
and fixing the docs version numbering highlighted above). The key here
is that we don't have one bit of the code changing the buffers whilst
DMA is in progress via a different path.  They all need to be covered
by the same lock.

	 * DMA (thus cache coherency maintenance) requires any
	 * transfer buffers to live in their own cache lines.
	 * As the use of these buffers is synchronous, all of the
	 * buffers used for DMA in this driver may share a cache
	 * line.

Note I'm waiting on a DT review though before taking this. Complex
binding but I see it's in their patchwork as needs review so hopefully
they'll get to it soon. 

https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20220620162059.1097264-2-cosmin.tanislav@xxxxxxxxxx/


Jonathan