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

[Petr Tesarik <ptesarik@xxxxxxxx>]

On Tue, 05 Feb 2013 14:21:28 +0000
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.
> > 
> > Signed-off-by: Mark Brown <broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx>  
> 
> Applied, thanks.

Hi all,

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.

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

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

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?

Petr T

> g.
> 
> > ---
> >  drivers/spi/spi.c |    3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> > index 19ee901..14d0fba 100644
> > --- a/drivers/spi/spi.c
> > +++ b/drivers/spi/spi.c
> > @@ -1656,7 +1656,8 @@ int spi_write_then_read(struct spi_device *spi,
> >  	 * using the pre-allocated buffer or the transfer is too large.
> >  	 */
> >  	if ((n_tx + n_rx) > SPI_BUFSIZ || !mutex_trylock(&lock)) {
> > -		local_buf = kmalloc(max((unsigned)SPI_BUFSIZ, n_tx + n_rx), GFP_KERNEL);
> > +		local_buf = kmalloc(max((unsigned)SPI_BUFSIZ, n_tx + n_rx),
> > +				    GFP_KERNEL | GFP_DMA);
> >  		if (!local_buf)
> >  			return -ENOMEM;
> >  	} else {
> > -- 
> > 1.7.10.4
> >   
>