Re: [PATCH v6 3/3] drivers: iio: adc: add support for ad777x family

[Jonathan Cameron]

On Thu, 10 Oct 2024 21:25:00 +0300
Andy Shevchenko <andy@xxxxxxxxxx> wrote:

> On Thu, Oct 10, 2024 at 06:45:16PM +0100, Jonathan Cameron wrote:
> > On Thu, 10 Oct 2024 14:32:49 +0000
> > "Nechita, Ramona" <Ramona.Nechita@xxxxxxxxxx> wrote:  
> 
> ...
> 
> > > >> +	/*
> > > >> +	 * DMA (thus cache coherency maintenance) requires the
> > > >> +	 * transfer buffers to live in their own cache lines.
> > > >> +	 */
> > > >> +	struct {
> > > >> +		u32 chans[8];
> > > >> +		s64 timestamp;    
> > > >
> > > >	aligned_s64 timestamp;
> > > >
> > > >while it makes no difference in this case, this makes code aligned inside the IIO subsystem.    
> > > 
> > > I might be missing something but I can't find the aligned_s64 data type, should I define it myself
> > > in the driver?  
> > 
> > Recent addition to the iio tree so it is in linux-next but not in mainline yet.
> > https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/?h=togreg&id=e4ca0e59c39442546866f3dd514a3a5956577daf
> > It just missed last cycle.
> >   
> > > >> +	} data __aligned(IIO_DMA_MINALIGN);    
> > > >
> > > >Note, this is different alignment to the above. And isn't the buffer below should have it instead?  
> > 
> > While I'm here:  No to this one.  The s64 alignment is about
> > performance of CPU access + consistency across CPU architectures.
> > This one (which happens to always be 8 or more) is about DMA safety.  
> 
> Right, but shouldn't...
> 
> > > >> +	u32			spidata_tx[8];  
> 
> > > >> +	u8			reg_rx_buf[3];
> > > >> +	u8			reg_tx_buf[3];  
> 
> ...one of these also be cache aligned for DMA?
No need as long as driver doesn't do anything bad like
write to these whilst another dma transaction is in flight.
(I haven't checked though, but typical drivers don't)
All you have to do is ensure that any DMA buffer doesn't share
a cacheline with an unrelated bit of data (i.e. a separate allocation or some
state tracking etc). It is fine for multiple DMA buffers  (say rx and tx)
to be in the same cacheline.  Any DMA device that is stupid enough
to stomp it itself is broken (and would be fairly hard to build!). Such
a device would need to be worked around in the controller driver.

They are allowed to write back stale data, but not garbage data to unrelated
parts of the cacheline.  I.e. a tx buffer that was changed whilst
the SPI transaction was going on, might be overwritten with the old value
when the SPI controller DMAs back an updated version of the cacheline
containing the rx data + a cached copy of the early tx data.
The risk we are defending against with this alignment isn't this, it's
unrelated (and typically not protected by lock) fields in the structure
being overwritten with stale data.  The really nasty one being when
the allocator gives the next bit of the cacheline to another allocation.
(avoided here because the structure is sized as a multiple of the maximum
 alignment).

Now, the code that bounces unaligned dma buffers on arm64 will probably
bounce these because it can't tell they are safe :( That's not incorrect
it's just less than optimal.

Jonathan
> 
> > > >> +	u8			reset_buf[8];  
>