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

Message-ID: <20241012114202.425f1b74@jic23-huawei>
Date: Sat, 12 Oct 2024 11:42:02 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Andy Shevchenko <andy@...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 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.

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