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

[Andy Shevchenko]

On Sat, Oct 12, 2024 at 11:42:02AM +0100, Jonathan Cameron wrote:
> 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.

Thanks for the really good elaboration! I will try hard to not forget it.

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

-- 
With Best Regards,
Andy Shevchenko