Re: [PATCH 1/3] mtd: spi-nor: add optional DMA-safe bounce buffer for data transfer

[Trent Piepho <tpiepho@impinj.com>]

On Thu, 2017-12-28 at 11:39 +0100, Cyrille Pitchen wrote:
> Le 26/12/2017 à 20:43, Trent Piepho a écrit :
> > On Sun, 2017-12-24 at 05:36 +0100, Cyrille Pitchen wrote:
> > > 
> > > Then the patch adds two hardware capabilities for SPI flash controllers,
> > > SNOR_HWCAPS_WR_BOUNCE and SNOR_HWCAPS_RD_BOUNCE.
> > 
> > Are there any drivers for which a bounce buffer is NOT needed when the
> > tx/rx buffer is not in DMA safe memory?  Maybe it would make more sense
> > to invert the sense of these flags, so that they indicate the driver
> > does not need DMA safe buffers, if that is the uncommon/non-existent
> > case, so that fewer drivers need to be modified to to be fixed?
> > 
> 
> It doesn't sound safe for a first step. I don't know if some of the
> spi-flash controllers are embedded inside systems with small memory and
> don't care about DMA transfers. Maybe they don't plan to use anything else
> but PIO transfers. Then why taking the risk to exhaust the memory on systems
> that would not use the bounce buffer anyway?

This would certainly be the case when the driver does not even support
DMA in the first place.

This also makes me wonder, how inefficient does this become when it
uses a bounce buffer for small transfer that would not use DMA anyway? 
 In the spi_flash_read() interface for spi masters, there is a master
method spi_flash_can_dma() callback used by the spi core to tell if
each transfer can be DMAed.

Should something like that be used here, to ask the master if it would
use dma on the buffer?

This might also prevent allocation of the bounce buffer if the only DMA
unsafe transfers are tiny control ops with stack variables that
wouldn't use DMA, e.g. the stuff spi_nor_read_sfdp_dma_unsafe() does.


> About the memory loss when forcing the SNOR_HWCAPS_*_BOUNCE in m25p80.c, I
> justify it because the m25p80 has to be compliant with the SPI sub-system
> requirements but currently is not. However I've taken care not to allocate
> the bounce buffer as long as we use only DMA-safe buffers.

Another possibility to reduce memory usage: make the buffer smaller
when first allocated by being just enough for the needed space.  Grow
it (by powers of two?) until it reaches the max allowed size.  No
reason to use a 256 kB buffer if DMA unsafe operations never get that
big.


> Here at the MTD side, the main (only ?) source of DMA-unsafe buffers is
> the UBIFS (JFFS2 too ?) layer. Then I've assumed that systems using such a
> file-system should already have enough system memory.

I saw a note in one of the existing DMA fixes that JFFS2 was the source
of the unsafe buffers, so probably there too.


> 
> Vignesh has suggested to call virt_addr_valid() instead.
> I think Boris has also told me about this function.
> So it might be the right solution. What do you think about their proposal?

Not sure what exactly the differences are between these methods.  The
fact that each of the many existing DMA fixes uses slightly different
code to detect what is unsafe speaks to the difficulty of this problem!
 virt_addr_valid() is already used by spi-ti-qspi.  spi core uses for
the buffer map helper, but that code path is for buffers which are NOT
vmalloc or highmem, but are still not virt_addr_valid() for some other
reason.

> > > +static int spi_nor_get_bounce_buffer(struct spi_nor *nor,
> > > +				     u_char **buffer,
> > > +				     size_t *buffer_size)
> > > +{
> > > +
> > > +	*buffer = nor->bounce_buffer;
> > > +	*buffer_size = size;
> > 
> > So the buffer is returned via the parameter, and also via a field
> > inside nor.  Seems redundant.  Consider address could be returned via
> > the function return value coupled with PTR_ERR() for the error cases. 
> > Or not return address at all since it's available via nor-
> > > bounce_buffer.
> 
> Why not. It would require more lines though. I guess it's purely a matter of taste.

Well, also consider that you don't need to even return the buffer
pointer at all, since it's available as nor->bounce_buffer.  Which it
is used as in spi_nor_write() and spi_nor_read().

> > This pattern, check if bounce is enabled, check if address is dma-
> > unsafe, get bounce buffer, seems to be very common.  Could it be
> > refactored into one helper?
> > 
> > u_char *buffer = spi_nor_check_bounce(nor, buf, len, &buffer_size);
> 
> The conditions that define the value of 'use_bounce' also depend on the type
> of operation, read or write, hence on the two different flags
> SNOR_F_USE_RD_BOUNCE and SNOR_F_USE_WR_BOUNCE.

Just pass one of those flags as an argument to tell what direction it
is in.  Though I wonder if using a bounce buffer for only one direction
will ever be used.

> 
> Besides, 'use_bounce' is also tested later in spi_nor_read(), sst_write()
> and sst_write().
> 
> So I don't really see how the spi_nor_check_bounce() function you propose
> could be that different from spi_nor_get_bounce_buffer().

> 
> > if (IS_ERR(buffer)) 
> >     return PTR_ERR(buffer);
> > // buffer = nor->bounce_buffer or buf, whichever is correct
> > // buffer_size = len or bounce buffer size, whichever is correct
> > 
> > Could spi_nor_read_sfdp_dma_unsafe() also use this buffer?
> > 
> 
> I didn't use the bounce buffer in spi_nor_read_sfdp_dma_unsafe() on
> purpose: the bounce buffer, when needed, is allocated once for all to limit
> performance loss. However, to avoid increasing the memory footprint, if not
> absolutely needed the bounce buffer is not allocated at all.

spi-nor tries to provide a common implementation of DMA bounce buffers,
 yet spi-nor itself has two different DMA bounce buffer
implementations.

I think the real answer for spi_nor_read_sfdp_dma_unsafe() is that it
shouldn't be written that way and the function should go away.  The two
call sites should just kmalloc the struct they read into instead of
placing it on the stack.  The dma_unsafe wrapper kmallocs a buffer
anyway, so it's not like there is any more allocation going on.