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

["Arnd Bergmann" <arnd@xxxxxxxx>]

On Thu, Mar 20, 2025, at 13:29, Mark Brown wrote:
> 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.

Thanks for adding me!

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

I saw the same thing looked at it the other day and got confused
about why 'local_buf' is allocated with GFP_DMA and 'buf'
uses plain GFP_KERNEL when they are both used in the same place.

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.

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