On Thu, Mar 20, 2025 at 12:43:30PM +0100, Petr Tesarik wrote:
> Grant Likely <grant.likely@xxxxxxxxxxxx> wrote:
> > On Sun, 27 Jan 2013 14:35:04 +0800, Mark Brown <broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
> > > Use GFP_DMA in order to ensure that the memory we allocate for transfers
> > > in spi_write_then_read() can be DMAed. On most platforms this will have
> > > no effect.
> > Applied, thanks.
> I'm sorry to revive such an old thread, but I'm trying to clean up DMA
> zone use in preparation of killing the need for that zone entirely, and
> this use looks fishy to me. I'm curious if it solves a real-world issue.
Copying in Arnd who was muttering about this stuff the other day. Since
the original patch was over a decade ago I have absolutely no
recollection of the circumstances around the change. I imagine I was
running into issues on some customer platform.
> First, the semantics of GFP_DMA can be confusing. FWIW allocating with
> GFP_DMA does *not* mean you get a buffer that can be directly passed to
> a DMA controller (think of cache coherency on arm, or memory encryption
> with confidential computing).
I'm not sure what you're thinking of with cache coherency there? The
usual issues are around the DMA controller not being able to address the
memory.
> Second, this code path is taken only if transfer size is greater than
> SPI_BUFSIZ, or if there is contention over the pre-allocated buffer,
> which is initialized in spi_init() without GFP_DMA:
> buf = kmalloc(SPI_BUFSIZ, GFP_KERNEL);
> IIUC most transfers use this buffer, and they have apparently worked
> fine for the last 10+ years...
On a lot of systems most transfers are short and won't be DMAed at all
since PIO ends up being more efficient, and most hardware is perfectly
happy to DMA to/from wherever so *shrug*. SPI_BUFSIZ is a maximum of 32
bytes which is going to be under the copybreak limit for quite a few
controllers, though admittedly 16 is also a popular number, so a lot of
the time we don't actually DMA out of it at all.
> What about reverting commit 2cd94c8a1b41 ("spi: Ensure memory used for
> spi_write_then_read() is DMA safe"), unless you have strong evidence
> that it is needed?
The whole goal there is to try to avoid triggering another copy to do
the DMA so just reverting rather than replacing with some other
construct that achieves the same goal doesn't seem great. I think
possibly we should just not do the copy at all any more and trust the
core to DTRT with any buffers that are passed in, I think we've got
enough stuff in the core though I can't remember if it'll copy with
things allocated on the stack well. I'd need to page the status back
in.
Attachment:
signature.asc
Description: PGP signature
