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

[Mark Brown]

On Fri, Mar 21, 2025 at 01:41:52PM +0100, Arnd Bergmann wrote:
> On Thu, Mar 20, 2025, at 19:39, Mark Brown wrote:

> > Yes, like I said elsewhere in the thread 16 is a popular number but I
> > suspect that was the thining there.

> Ok, so do we assume we don't need the GFP_DMA then? That's
> fine with me, but in that case we should probably enable
> swiotlb on all arm32 platforms that may have ZONE_DMA smaller
> than ZONE_NORMAL.

I think that makes sense.

> >> - 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 

... (single/double register raw I/O from stack) ...

> which happens in a number of drivers but is harmless as long
> as the driver doesn't actually try to DMA into that buffer.

Hrm, right.  I think that usage is reasonable so we probably should
allow it rather than forcing things to do an allocation for a transfer
that ends up being PIOed anyway almost all the time, OTOH the same API
is also used for large transfers like firmware downloads where we don't
want to trigger a spurious copy.  Having different requirements at
different times would be miserable though so I think we need to just
accept any buffer and then figure it out inside the API, but I've not
fully thought that through yet.

> >> __spi_map_msg() already handles the case of an external
> >> DMA master through ctlr->dma_map_dev, so I think the same
> >> could be used to get a temporary buffer using
> >> dma_alloc_noncoherent() inside of spi_write_then_read()
> >> in place of the kmalloc(, GFP_DMA).

> > That only helps spi_write_then_read() though.

> Right, we'd need to mirror this in other interfaces, either changing
> the existing ones, or adding safer ones that can be used from regmap
> and from drivers that currently allocate their own GFP_DMA buffers
> for this purpose.

Yes, indeed.  I don't have a clear sense for what the best solution is
there yet.  Possibly some libary code for the "I want to DMA this random
memory" use case?

Attachment: signature.asc
Description: PGP signature