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
