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

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] [day] [month] [year] [list]

Message-ID: <Zw5NQM6Z9ATOx6SX@smile.fi.intel.com>
Date: Tue, 15 Oct 2024 14:08:48 +0300
From: Andy Shevchenko <andy@...nel.org>
To: Jonathan Cameron <jic23@...nel.org>
Cc: "Nechita, Ramona" <Ramona.Nechita@...log.com>,
	Lars-Peter Clausen <lars@...afoo.de>,
	"Hennerich, Michael" <Michael.Hennerich@...log.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, "Sa, Nuno" <Nuno.Sa@...log.com>,
	David Lechner <dlechner@...libre.com>,
	"Schmitt, Marcelo" <Marcelo.Schmitt@...log.com>,
	Olivier Moysan <olivier.moysan@...s.st.com>,
	Dumitru Ceclan <mitrutzceclan@...il.com>,
	Matteo Martelli <matteomartelli3@...il.com>,
	João Paulo Gonçalves <joao.goncalves@...adex.com>,
	Alisa-Dariana Roman <alisadariana@...il.com>,
	"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [PATCH v6 3/3] drivers: iio: adc: add support for ad777x family

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@...nel.org> 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@...log.com> 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