On Thu, Mar 20, 2025 at 05:30:01PM +0100, Arnd Bergmann wrote: > On Thu, Mar 20, 2025, at 15:42, Mark Brown wrote: > > Really we just don't expect the small buffer to be DMAed. > I looked at a couple of drivers and found many have a copybreak > limit less than SPI_BUFSIZE > #define SPI_BUFSIZ max(32, SMP_CACHE_BYTES) > 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. > >> 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 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. > - 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. > >> 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.
Attachment:
signature.asc
Description: PGP signature
