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

On Friday 29 December 2017 12:24 AM, Trent Piepho wrote:

On Thu, 2017-12-28 at 11:39 +0100, Cyrille Pitchen wrote:

quoted

Le 26/12/2017 à 20:43, Trent Piepho a écrit :

quoted

On Sun, 2017-12-24 at 05:36 +0100, Cyrille Pitchen wrote:

quoted

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.

quoted

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.

quoted

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.

quoted

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!

My understanding based on Documentation/DMA-API-HOWTO.txt and
Documentation/arm/memory.txt is that
virt_addr_valid() will guarantee that address is in range of
PAGE_OFFSET to high_memory-1 (Kernel direct-mapped RAM region) which is
address range of buffers that are DMA'able.

 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.

	if (vmalloced_buf || kmap_buf) {
		/* Handle vmalloc'd or kmap'd buffers */
		...
        } else if (virt_addr_valid(buf)) {
		/* Handle kmalloc'd and such buffers */
                ...
	} else {
		/* Error if none of the above */
		return -EINVAL;
	}



-- 
Regards
Vignesh
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html