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

["Arnd Bergmann" <arnd@xxxxxxxx>]

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:
>
>> drivers/spi/atmel-quadspi.c:#define ATMEL_QSPI_DMA_MIN_BYTES    16
>> drivers/spi/spi-at91-usart.c:#define US_DMA_MIN_BYTES       16
>> drivers/spi/spi-atmel.c:#define DMA_MIN_BYTES   16
>> drivers/spi/spi-davinci.c:#define DMA_MIN_BYTES 16
>> drivers/spi/spi-stm32.c:#define SPI_DMA_MIN_BYTES       16
>> drivers/spi/spi-imx.c:  .fifo_size = 8,
>
>> so any transfers from 17 to 32 bytes would try to use the
>> non-GFP_DMA 'buf' and then try to map that.
>
> 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.

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

>> > I suspect that practically speaking almost everything will be under the
>> > copybreak limit.  Probably we should just make regmap use spi_sync()
>> > with the supplied buffers here and avoid spi_write_then_read().
>
>> I'm a bit lost in that code, but doesn't spi_sync() require
>> a buffer that can be passed into dma_map_sg() and (in theory
>> at least) GFP_DMA?
>
> Yes, it does - the API is in general that the memory be something we
> could DMA, in case the controller wants to.  You'll probably get away
> with just passing whatever for small enough transfers, it's much more
> common to get a controller that won't DMA than one that must DMA.
>
> I think I'd been under the impression that dma_map_sg() would handle
> things similarly to dma_map_single() (it's a bit of a landmine for it
> not to...).  It's a very long time since I looked at any of this stuff.

Yes, dma_map_{single,page,sg} all do the same thing
internally.

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

>> >> I don't know what a good replacement interface would be, but
>> >> ideally there should never be more than one copy here,
>> >> which means that any temporary buffer would need to be
>> >> allocated according to the dma_mask of the underlying
>> >> bus master (dmaengine, spi controller, ...).
>
>> > Which is a pain because even if you've got the device for the SPI
>> > controller there's no way to discover if it does it's own DMA.
>
>> __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.

      Arnd