Re: [PATCH] spi: Ensure memory used for spi_write_then_read() is DMA safe

[Petr Tesarik <ptesarik@xxxxxxxx>]

On Fri, 21 Mar 2025 13:41:52 +0100
"Arnd Bergmann" <arnd@xxxxxxxx> wrote:

> On Thu, Mar 20, 2025, at 19:39, Mark Brown wrote:
> > On Thu, Mar 20, 2025 at 05:30:01PM +0100, Arnd Bergmann wrote:  
> >> On Thu, Mar 20, 2025, at 15:42, Mark Brown wrote:  
>[...]
> >> >> It also seems that the copy happens in the regmap_bulk_read()
> >> >> path but not the regmap_bulk_write(), which just passes down
> >> >> the original buffer without copying, as far as I can tell.  
> >  
> >> > Yes, writes don't need to do anything.  
> >  
> >> Can you explain why writes are different from reads here?  
> >
> > I think there may have been some context lost there while replying?  
> 
> I found the answer now: at least on common architectures (arm,
> powerpc, mips, ...), doing a write from an unaligned buffer
> on stack or in a shared data structure won't cause data corruption,
> but doing a read into that buffer can end up throwing away
> data that shares the same cacheline.

That's right. Additionally, any cache aliasing issues are irrelevant
for a write. Yes, there are (were?) some processors with a
virtual-indexed, virtual-tagged cache. Thank you, Arm...

>[...]
> >> - the way that spi_map_buf_attrs() is written, it actually
> >>   supports addresses from both kmap() and vmalloc() and
> >>   will attempt to correctly map those rather than reject
> >>   the buffer. While this sounds like a good idea, handling
> >>   vmalloc data like this risks stack data corruption
> >>   on non-coherent platforms when failing to map stack
> >>   buffers would be the better response.  
> >
> > IIRC that's there to support filesystems on SPI flashes or some other
> > application that uses vmalloc()ed buffers, it's definitely not intended
> > to support data on stack.  If it does anything for stack allocated data
> > that's accidental.  
> 
> Ok, then the question is what we should do about callers that pass
> in stack data. I can send a patch that adds a WARN_ONCE() or similar,
> but it would trigger on things like 
> 
> static int rt1711h_read16(struct rt1711h_chip *chip, unsigned int reg, u16 *val)
> {
>         return regmap_raw_read(chip->data.regmap, reg, val, sizeof(u16));
> }
> static int rt1711h_write16(struct rt1711h_chip *chip, unsigned int reg, u16 val)
> {
>         return regmap_raw_write(chip->data.regmap, reg, &val, sizeof(u16));
> }
> 
> which happens in a number of drivers but is harmless as long
> as the driver doesn't actually try to DMA into that buffer.

This sounds like we should push the WARN_ONCE() one level deeper, into
the DMA code. That's a good idea, actually, because it's always wrong
to do DMA to a stack address, not just when SPI does it.

Petr T