Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1588636 > unrolled thread

[RFC PATCH 0/2] mtd: spi-nor: Handle vmalloc'd buffers

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH 0/2] mtd: spi-nor: Handle vmalloc'd buffers Vignesh R <vigneshr@ti.com> - 2017-02-27 13:20 +0100
    [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce buffer support Vignesh R <vigneshr@ti.com> - 2017-02-27 13:20 +0100
      Re: [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce  buffer support Richard Weinberger <richard@nod.at> - 2017-02-28 22:50 +0100
        Re: [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce  buffer support Vignesh R <vigneshr@ti.com> - 2017-03-01 09:00 +0100
          Re: [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce  buffer support Cyrille Pitchen <cyrille.pitchen@atmel.com> - 2017-03-01 12:10 +0100
            Re: [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce  buffer support Frode Isaksen <fisaksen@baylibre.com> - 2017-03-01 12:50 +0100
            Re: [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce  buffer support Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-03-01 13:40 +0100
            Re: [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce  buffer support Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-03-01 15:30 +0100
              Re: [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce  buffer support Cyrille Pitchen <cyrille.pitchen@atmel.com> - 2017-03-01 15:40 +0100
            Re: [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce  buffer support Cyrille Pitchen <cyrille.pitchen@atmel.com> - 2017-03-01 15:30 +0100
              Re: [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce  buffer support Mark Brown <broonie@kernel.org> - 2017-03-01 17:00 +0100
            Re: [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce  buffer support Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-03-01 17:20 +0100
            Re: [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce  buffer support Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-03-01 18:00 +0100
              Re: [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce  buffer support Frode Isaksen <fisaksen@baylibre.com> - 2017-03-02 10:10 +0100
                Re: [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce  buffer support Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-03-02 16:00 +0100
                  Re: [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce  buffer support Frode Isaksen <fisaksen@baylibre.com> - 2017-03-02 16:20 +0100
                    Re: [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce  buffer support Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-03-02 17:00 +0100
                      Re: [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce  buffer support Frode Isaksen <fisaksen@baylibre.com> - 2017-03-03 10:10 +0100
                  Re: [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce  buffer support Cyrille Pitchen <cyrille.pitchen@atmel.com> - 2017-03-02 17:50 +0100
                  Re: [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce  buffer support Mark Brown <broonie@kernel.org> - 2017-03-02 19:20 +0100
                    Re: [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce  buffer support Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-03-03 04:20 +0100
                      Re: [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce  buffer support Mark Brown <broonie@kernel.org> - 2017-03-03 14:20 +0100
                  Re: [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce  buffer support Vignesh R <vigneshr@ti.com> - 2017-03-06 12:50 +0100
                Re: [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce  buffer support Vignesh R <vigneshr@ti.com> - 2017-03-03 23:10 +0100
    [RFC PATCH 1/2] mtd: spi-nor: Introduce bounce buffer to handle vmalloc'd buffers Vignesh R <vigneshr@ti.com> - 2017-02-27 13:20 +0100
      Re: [RFC PATCH 1/2] mtd: spi-nor: Introduce bounce buffer to handle  vmalloc'd buffers Richard Weinberger <richard@nod.at> - 2017-03-01 02:50 +0100
        Re: [RFC PATCH 1/2] mtd: spi-nor: Introduce bounce buffer to handle  vmalloc'd buffers Vignesh R <vigneshr@ti.com> - 2017-03-01 09:00 +0100
        Re: [RFC PATCH 1/2] mtd: spi-nor: Introduce bounce buffer to handle  vmalloc'd buffers Cyrille Pitchen <cyrille.pitchen@atmel.com> - 2017-03-01 11:20 +0100
          Re: [RFC PATCH 1/2] mtd: spi-nor: Introduce bounce buffer to handle  vmalloc'd buffers Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-03-01 12:00 +0100
            Re: [RFC PATCH 1/2] mtd: spi-nor: Introduce bounce buffer to handle  vmalloc'd buffers Frode Isaksen <fisaksen@baylibre.com> - 2017-03-01 12:30 +0100
              Re: [RFC PATCH 1/2] mtd: spi-nor: Introduce bounce buffer to handle  vmalloc'd buffers Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-03-01 13:40 +0100
          Re: [RFC PATCH 1/2] mtd: spi-nor: Introduce bounce buffer to handle  vmalloc'd buffers Vignesh R <vigneshr@ti.com> - 2017-03-01 13:00 +0100
    Re: [RFC PATCH 0/2] mtd: spi-nor: Handle vmalloc'd buffers Frode Isaksen <fisaksen@baylibre.com> - 2017-02-27 17:00 +0100

Page 2 of 2 — ← Prev page 1 [2]

#1591648 — Re: [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce buffer support

On Thu, 2 Mar 2017 17:00:41 +0000
Mark Brown <broonie@kernel.org> wrote:

> On Thu, Mar 02, 2017 at 03:29:21PM +0100, Boris Brezillon wrote:
> > Vignesh R <vigneshr@ti.com> wrote:  
> 
> > > Or SPI core can be extended in a way similar to this RFC. That is, SPI
> > > master driver will set a flag to request SPI core to use of bounce
> > > buffer for vmalloc'd buffers. And spi_map_buf() just uses bounce buffer
> > > in case buf does not belong to kmalloc region based on the flag.  
> 
> > That's a better approach IMHO. Note that the decision should not only  
> 
> I don't understand how the driver is supposed to tell if it might need a
> bounce buffer due to where the memory is allocated and the caches used
> by the particular system it is used on?

That's true, but if the SPI controller driver can't decide that, how
could a SPI device driver guess?

We could patch dma_map_sg() to create a bounce buffer when it's given a
vmalloc-ed buffer and we are running on a system using VIVT or VIPT
caches (it's already allocating bounce buffers when the peripheral
device cannot access the memory region, so why not in this case).

This still leaves 2 problems:
1/ for big transfers, dynamically allocating a bounce buffer on demand
   (and freeing it after the DMA operation) might fail, or might induce
   some latency, especially when the system is under high mem pressure.
   Allocating these bounce buffers once during the SPI device driver
   ->probe() guarantees that the bounce buffer will always be available
   when needed, but OTOH, we don't know if it's really needed.
2/ only the SPI and/or DMA engine know when using DMA with a bounce
   buffer is better than using PIO mode. The limit is probably
   different from the DMA vs PIO mode (dma_min_len <
   dma_bounce_min_len). Thanks to ->can_dma() we can let drivers decide
   when preparing the buffer for a DMA transfer is needed.
3/ if the DMA engine does not support chaining DMA descriptor, and the
   vmalloc-ed buffer spans several non-contiguous pages, doing DMA
   is simply not possible. That one can probably handled with the
   ->can_dma() hook too.

>  The suggestion to pass via
> scatterlists seems a bit more likely to work but even then I'm not clear
> that drivers doing PIO would play well.

You mean that SPI device drivers would directly pass an sg list instead
of a virtual pointer? Not sure that would help, we're just moving the
decision one level up without providing more information to help decide
what to do.

> 
> > be based on the buffer type, but also on the transfer length and/or
> > whether the controller supports transferring non physically contiguous
> > buffers.  
> 
> The reason most drivers only look at the transfer length when deciding
> that they can DMA is that most controllers are paired with DMA
> controllers that are sensibly implemented, the only factor they're
> selecting on is the copybreak for performance.

Of course, the checks I mentioned (especially the physically contiguous
one) are SPI controller and/or DMA engine dependent. Some of them might
be irrelevant.

[toc] | [prev] | [next] | [standalone]

#1591935 — Re: [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce buffer support

[Multipart message — attachments visible in raw view] — view raw

On Thu, Mar 02, 2017 at 08:49:00PM +0100, Boris Brezillon wrote:

> 1/ for big transfers, dynamically allocating a bounce buffer on demand
>    (and freeing it after the DMA operation) might fail, or might induce
>    some latency, especially when the system is under high mem pressure.
>    Allocating these bounce buffers once during the SPI device driver
>    ->probe() guarantees that the bounce buffer will always be available
>    when needed, but OTOH, we don't know if it's really needed.

Yeah, but then the bounces are going to slow things down anyway.  This
does seem fixable if we do something with caching the buffer once we
create it, and if it's implemented elsewhere the same problem will
exist.  We can't just allocate the maximum possible buffer size because
some devices have effectively unlimited transfer sizes so you could
waste huge amounts of memory, especially in the common case where we
don't use vmalloc() at all.

> 2/ only the SPI and/or DMA engine know when using DMA with a bounce
>    buffer is better than using PIO mode. The limit is probably
>    different from the DMA vs PIO mode (dma_min_len <
>    dma_bounce_min_len). Thanks to ->can_dma() we can let drivers decide
>    when preparing the buffer for a DMA transfer is needed.

I'm not so worried about that, the numbers are basically an educated
guess anyway.  It's a concern though, yes.

> 3/ if the DMA engine does not support chaining DMA descriptor, and the
>    vmalloc-ed buffer spans several non-contiguous pages, doing DMA
>    is simply not possible. That one can probably handled with the
>    ->can_dma() hook too.

Anything that doesn't support chaining is in trouble already (or should
be soon hopefully), but mostly the controllers will have no idea about
that as they're just asking their DMA controller to do things.  We'd be
better off having the core query the capabilities of the DMA controllers
directly.

> >  The suggestion to pass via
> > scatterlists seems a bit more likely to work but even then I'm not clear
> > that drivers doing PIO would play well.

> You mean that SPI device drivers would directly pass an sg list instead
> of a virtual pointer? Not sure that would help, we're just moving the
> decision one level up without providing more information to help decide
> what to do.

I think the idea was to ensure you only use one mapping type at once.

[toc] | [prev] | [next] | [standalone]

#1593239 — Re: [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce buffer support

On Thursday 02 March 2017 07:59 PM, Boris Brezillon wrote:
> On Thu, 2 Mar 2017 19:24:43 +0530
> Vignesh R <vigneshr@ti.com> wrote:
> 
>>>>>>     
>>>>> Not really, I am debugging another issue with UBIFS on DRA74 EVM (ARM
>>>>> cortex-a15) wherein pages allocated by vmalloc are in highmem region
>>>>> that are not addressable using 32 bit addresses and is backed by LPAE.
>>>>> So, a 32 bit DMA cannot access these buffers at all.
>>>>> When dma_map_sg() is called to map these pages by spi_map_buf() the
>>>>> physical address is just truncated to 32 bit in pfn_to_dma() (as part of
>>>>> dma_map_sg() call). This results in random crashes as DMA starts
>>>>> accessing random memory during SPI read.
>>>>>
>>>>> IMO, there may be more undiscovered caveat with using dma_map_sg() for
>>>>> non kmalloc'd buffers and its better that spi-nor starts handling these
>>>>> buffers instead of relying on spi_map_msg() and working around every
>>>>> time something pops up.
>>>>>  
>>>> Ok, I had a closer look at the SPI framework, and it seems there's a
>>>> way to tell to the core that a specific transfer cannot use DMA
>>>> (->can_dam()). The first thing you should do is fix the spi-davinci
>>>> driver:
>>>>
>>>> 1/ implement ->can_dma()
>>>> 2/ patch davinci_spi_bufs() to take the decision to do DMA or not on a
>>>>    per-xfer basis and not on a per-device basis
>>>>  
>>
>> This would lead to poor perf defeating entire purpose of using DMA.
> 
> Hm, that's not really true. For all cases where you have a DMA-able
> buffer it would still use DMA. For other cases (like the UBI+SPI-NOR
> case we're talking about here), yes, it will be slower, but slower is
> still better than buggy.
> So, in any case, I think the fixes pointed by Frode are needed.
> 
Yes, but still bounce buffer does help in perf improvement over PIO.

>>
>>>> Then we can start thinking about how to improve perfs by using a bounce
>>>> buffer for large transfers, but I'm still not sure this should be done
>>>> at the MTD level...  
>>
>> If its at SPI level, then I guess each individual drivers which cannot
>> handle vmalloc'd buffers will have to implement bounce buffer logic.
> 
> Well, that's my opinion. The only one that can decide when to do
> PIO, when to use DMA or when to use a bounce buffer+DMA is the SPI
> controller.
> If you move this logic to the SPI NOR layer, you'll have to guess what
> is the best approach, and I fear the decision will be wrong on some
> platforms (leading to perf degradation).
> 
> You're mentioning code duplication in each SPI controller, I agree,
> this is far from ideal, but what you're suggesting is not necessarily
> better. What if another SPI user starts passing vmalloc-ed buffers to
> the SPI controller? You'll have to duplicate the bounce-buffer logic in
> this user as well.
> 

Hmm... Yes, there are ways to by pass SPI core.

>>
>> Or SPI core can be extended in a way similar to this RFC. That is, SPI
>> master driver will set a flag to request SPI core to use of bounce
>> buffer for vmalloc'd buffers. And spi_map_buf() just uses bounce buffer
>> in case buf does not belong to kmalloc region based on the flag.
> 
> That's a better approach IMHO. Note that the decision should not only
> be based on the buffer type, but also on the transfer length and/or
> whether the controller supports transferring non physically contiguous
> buffers.
> 
> Maybe we should just extend ->can_dma() to let the core know if it
> should use a bounce buffer.
> 

Yes, this is definitely needed. ->can_dma() currently returns bool. We
need a better interface that returns different error codes for
restriction on buffer length vs buffer type (I dont see any appropriate
error codes) or make ->can_dma() return flag asking for bounce buffer.
SPI controller drivers may use cache_is_*() and virt_addr_valid() to
decide whether or not request bounce buffer.

> Regarding the bounce buffer allocation logic, I'm not sure how it
> should be done. The SPI user should be able to determine a max transfer
> len (at least this is the case for SPI NORs) and inform the SPI layer
> about this boundary so that the SPI core can allocate a bounce buffer
> of this size. But we also have limitations at the SPI master level
> (->max_transfer_size(), ->max_message_size()).
> 

Again, I guess only SPI controller can suggest the appropriate size of
bounce buffer based on its internal constraints and use cases that its
known to support.



-- 
Regards
Vignesh

[toc] | [prev] | [next] | [standalone]

#1592312 — Re: [RFC PATCH 2/2] mtd: devices: m25p80: Enable spi-nor bounce buffer support

>>>>   
>>> Not really, I am debugging another issue with UBIFS on DRA74 EVM (ARM
>>> cortex-a15) wherein pages allocated by vmalloc are in highmem region
>>> that are not addressable using 32 bit addresses and is backed by LPAE.
>>> So, a 32 bit DMA cannot access these buffers at all.
>>> When dma_map_sg() is called to map these pages by spi_map_buf() the
>>> physical address is just truncated to 32 bit in pfn_to_dma() (as part of
>>> dma_map_sg() call). This results in random crashes as DMA starts
>>> accessing random memory during SPI read.
>>>
>>> IMO, there may be more undiscovered caveat with using dma_map_sg() for
>>> non kmalloc'd buffers and its better that spi-nor starts handling these
>>> buffers instead of relying on spi_map_msg() and working around every
>>> time something pops up.
>>>
>> Ok, I had a closer look at the SPI framework, and it seems there's a
>> way to tell to the core that a specific transfer cannot use DMA
>> (->can_dam()). The first thing you should do is fix the spi-davinci
>> driver:
>>
>> 1/ implement ->can_dma()
>> 2/ patch davinci_spi_bufs() to take the decision to do DMA or not on a
>>    per-xfer basis and not on a per-device basis
>>

This would lead to poor perf defeating entire purpose of using DMA.

>> Then we can start thinking about how to improve perfs by using a bounce
>> buffer for large transfers, but I'm still not sure this should be done
>> at the MTD level...

If its at SPI level, then I guess each individual drivers which cannot
handle vmalloc'd buffers will have to implement bounce buffer logic.

Or SPI core can be extended in a way similar to this RFC. That is, SPI
master driver will set a flag to request SPI core to use of bounce
buffer for vmalloc'd buffers. And spi_map_buf() just uses bounce buffer
in case buf does not belong to kmalloc region based on the flag.

Mark, Cyrille, Is that what you prefer?

-- 
Regards
Vignesh

[toc] | [prev] | [next] | [standalone]

#1588640 — [RFC PATCH 1/2] mtd: spi-nor: Introduce bounce buffer to handle vmalloc'd buffers

Filesystems like UBIFS may pass vmalloc'd buffers to SPI NOR layer which
will end up in SPI layer. SPI core does try to handle such buffers (see
spi_map_buf()) by doing vmalloc_to_page() and creating scatterlist. But,
its known that this does not work well with VIVT/aliasing cache
architectures.
This also fails when buffers are addressed using LPAE (buffers in region
higher than 32 bit addressable region), if DMA is 32bit only.

Introduce bounce buffers support in SPI NOR framework to handle
vmalloc'd buffers. Use a pre-allocated per flash bounce buffer equal to
the sector size of the flash. Flash drivers can enable this feature by
setting SNOR_F_USE_BOUNCE_BUFFER flag.
This would also enable SPI NOR drivers to safely use DMA in their
read/write callbacks.

Signed-off-by: Vignesh R <vigneshr@ti.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 30 +++++++++++++++++++++++++++---
 include/linux/mtd/spi-nor.h   |  4 ++++
 2 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 747645c74134..c241fefa5aff 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -17,6 +17,7 @@
 #include <linux/mutex.h>
 #include <linux/math64.h>
 #include <linux/sizes.h>
+#include <linux/mm.h>
 
 #include <linux/mtd/mtd.h>
 #include <linux/of_platform.h>
@@ -1205,11 +1206,21 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
 
 	while (len) {
 		loff_t addr = from;
+		bool use_bb = false;
+		u_char *dst_buf = buf;
+		size_t buf_len = len;
 
 		if (nor->flags & SNOR_F_S3AN_ADDR_DEFAULT)
 			addr = spi_nor_s3an_addr_convert(nor, addr);
 
-		ret = nor->read(nor, addr, len, buf);
+		if (!virt_addr_valid(buf) && nor->bounce_buf) {
+			use_bb = true;
+			dst_buf = nor->bounce_buf;
+			if (len > mtd->erasesize)
+				buf_len = mtd->erasesize;
+		}
+
+		ret = nor->read(nor, from, buf_len, dst_buf);
 		if (ret == 0) {
 			/* We shouldn't see 0-length reads */
 			ret = -EIO;
@@ -1217,7 +1228,8 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
 		}
 		if (ret < 0)
 			goto read_err;
-
+		if (use_bb)
+			memcpy(buf, dst_buf, ret);
 		WARN_ON(ret > len);
 		*retlen += ret;
 		buf += ret;
@@ -1329,6 +1341,7 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
 		return ret;
 
 	for (i = 0; i < len; ) {
+		const u_char *src_buf = buf + i;
 		ssize_t written;
 		loff_t addr = to + i;
 
@@ -1354,8 +1367,13 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
 		if (nor->flags & SNOR_F_S3AN_ADDR_DEFAULT)
 			addr = spi_nor_s3an_addr_convert(nor, addr);
 
+		if (!virt_addr_valid(buf) && nor->bounce_buf) {
+			memcpy(nor->bounce_buf, buf + i, page_remain);
+			src_buf = nor->bounce_buf;
+		}
+
 		write_enable(nor);
-		ret = nor->write(nor, addr, page_remain, buf + i);
+		ret = nor->write(nor, addr, page_remain, src_buf);
 		if (ret < 0)
 			goto write_err;
 		written = ret;
@@ -1720,6 +1738,12 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 		return -EINVAL;
 	}
 
+	if (nor->flags & SNOR_F_USE_BOUNCE_BUFFER) {
+		nor->bounce_buf = devm_kmalloc(dev, mtd->erasesize, GFP_KERNEL);
+		if (!nor->bounce_buf)
+			dev_err(dev, "unable to allocated bounce buffer\n");
+	}
+
 	nor->read_dummy = spi_nor_read_dummy_cycles(nor);
 
 	if (info->flags & SPI_S3AN) {
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index f2a718030476..3846ff5fa011 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -141,6 +141,7 @@ enum spi_nor_option_flags {
 	SNOR_F_NO_OP_CHIP_ERASE	= BIT(2),
 	SNOR_F_S3AN_ADDR_DEFAULT = BIT(3),
 	SNOR_F_READY_XSR_RDY	= BIT(4),
+	SNOR_F_USE_BOUNCE_BUFFER = BIT(5),
 };
 
 /**
@@ -173,6 +174,8 @@ enum spi_nor_option_flags {
  * @flash_unlock:	[FLASH-SPECIFIC] unlock a region of the SPI NOR
  * @flash_is_locked:	[FLASH-SPECIFIC] check if a region of the SPI NOR is
  *			completely locked
+ * @bounce_buf		bounce buffer to use incase of vmalloc'd buffers,
+ *			for drivers that may use DMA.
  * @priv:		the private data
  */
 struct spi_nor {
@@ -205,6 +208,7 @@ struct spi_nor {
 	int (*flash_unlock)(struct spi_nor *nor, loff_t ofs, uint64_t len);
 	int (*flash_is_locked)(struct spi_nor *nor, loff_t ofs, uint64_t len);
 
+	void *bounce_buf;
 	void *priv;
 };
 
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1589937 — Re: [RFC PATCH 1/2] mtd: spi-nor: Introduce bounce buffer to handle vmalloc'd buffers

Vignesh,

Am 27.02.2017 um 13:08 schrieb Vignesh R:
> Filesystems like UBIFS may pass vmalloc'd buffers to SPI NOR layer which
> will end up in SPI layer. SPI core does try to handle such buffers (see
> spi_map_buf()) by doing vmalloc_to_page() and creating scatterlist. But,
> its known that this does not work well with VIVT/aliasing cache
> architectures.
> This also fails when buffers are addressed using LPAE (buffers in region
> higher than 32 bit addressable region), if DMA is 32bit only.
> 
> Introduce bounce buffers support in SPI NOR framework to handle
> vmalloc'd buffers. Use a pre-allocated per flash bounce buffer equal to
> the sector size of the flash. Flash drivers can enable this feature by
> setting SNOR_F_USE_BOUNCE_BUFFER flag.
> This would also enable SPI NOR drivers to safely use DMA in their
> read/write callbacks.
> 
> Signed-off-by: Vignesh R <vigneshr@ti.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 30 +++++++++++++++++++++++++++---
>  include/linux/mtd/spi-nor.h   |  4 ++++
>  2 files changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 747645c74134..c241fefa5aff 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -17,6 +17,7 @@
>  #include <linux/mutex.h>
>  #include <linux/math64.h>
>  #include <linux/sizes.h>
> +#include <linux/mm.h>
>  
>  #include <linux/mtd/mtd.h>
>  #include <linux/of_platform.h>
> @@ -1205,11 +1206,21 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
>  
>  	while (len) {
>  		loff_t addr = from;
> +		bool use_bb = false;
> +		u_char *dst_buf = buf;
> +		size_t buf_len = len;
>  
>  		if (nor->flags & SNOR_F_S3AN_ADDR_DEFAULT)
>  			addr = spi_nor_s3an_addr_convert(nor, addr);
>  
> -		ret = nor->read(nor, addr, len, buf);
> +		if (!virt_addr_valid(buf) && nor->bounce_buf) {
> +			use_bb = true;
> +			dst_buf = nor->bounce_buf;
> +			if (len > mtd->erasesize)
> +				buf_len = mtd->erasesize;

Doesn't this degrade the read operation to a short read?
Not sure whether this is harmless or not.
Cyrille?

> +		}
> +
> +		ret = nor->read(nor, from, buf_len, dst_buf);
>  		if (ret == 0) {
>  			/* We shouldn't see 0-length reads */
>  			ret = -EIO;
> @@ -1217,7 +1228,8 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
>  		}
>  		if (ret < 0)
>  			goto read_err;
> -
> +		if (use_bb)
> +			memcpy(buf, dst_buf, ret);
>  		WARN_ON(ret > len);
>  		*retlen += ret;
>  		buf += ret;
> @@ -1329,6 +1341,7 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>  		return ret;
>  
>  	for (i = 0; i < len; ) {
> +		const u_char *src_buf = buf + i;
>  		ssize_t written;
>  		loff_t addr = to + i;
>  
> @@ -1354,8 +1367,13 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>  		if (nor->flags & SNOR_F_S3AN_ADDR_DEFAULT)
>  			addr = spi_nor_s3an_addr_convert(nor, addr);
>  
> +		if (!virt_addr_valid(buf) && nor->bounce_buf) {
> +			memcpy(nor->bounce_buf, buf + i, page_remain);
> +			src_buf = nor->bounce_buf;
> +		}
> +
>  		write_enable(nor);
> -		ret = nor->write(nor, addr, page_remain, buf + i);
> +		ret = nor->write(nor, addr, page_remain, src_buf);
>  		if (ret < 0)
>  			goto write_err;
>  		written = ret;
> @@ -1720,6 +1738,12 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>  		return -EINVAL;
>  	}
>  
> +	if (nor->flags & SNOR_F_USE_BOUNCE_BUFFER) {
> +		nor->bounce_buf = devm_kmalloc(dev, mtd->erasesize, GFP_KERNEL);
> +		if (!nor->bounce_buf)
> +			dev_err(dev, "unable to allocated bounce buffer\n");

I think we should return here and not continue.

Thanks,
//richard

[toc] | [prev] | [next] | [standalone]

#1590071 — Re: [RFC PATCH 1/2] mtd: spi-nor: Introduce bounce buffer to handle vmalloc'd buffers

On Wednesday 01 March 2017 03:09 AM, Richard Weinberger wrote:
> Vignesh,
> 
> Am 27.02.2017 um 13:08 schrieb Vignesh R:
>> Filesystems like UBIFS may pass vmalloc'd buffers to SPI NOR layer which
>> will end up in SPI layer. SPI core does try to handle such buffers (see
>> spi_map_buf()) by doing vmalloc_to_page() and creating scatterlist. But,
>> its known that this does not work well with VIVT/aliasing cache
>> architectures.
>> This also fails when buffers are addressed using LPAE (buffers in region
>> higher than 32 bit addressable region), if DMA is 32bit only.
>>
>> Introduce bounce buffers support in SPI NOR framework to handle
>> vmalloc'd buffers. Use a pre-allocated per flash bounce buffer equal to
>> the sector size of the flash. Flash drivers can enable this feature by
>> setting SNOR_F_USE_BOUNCE_BUFFER flag.
>> This would also enable SPI NOR drivers to safely use DMA in their
>> read/write callbacks.
>>
>> Signed-off-by: Vignesh R <vigneshr@ti.com>
>> ---
>>  drivers/mtd/spi-nor/spi-nor.c | 30 +++++++++++++++++++++++++++---
>>  include/linux/mtd/spi-nor.h   |  4 ++++
>>  2 files changed, 31 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index 747645c74134..c241fefa5aff 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -17,6 +17,7 @@
>>  #include <linux/mutex.h>
>>  #include <linux/math64.h>
>>  #include <linux/sizes.h>
>> +#include <linux/mm.h>
>>  
>>  #include <linux/mtd/mtd.h>
>>  #include <linux/of_platform.h>
>> @@ -1205,11 +1206,21 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
>>  
>>  	while (len) {
>>  		loff_t addr = from;
>> +		bool use_bb = false;
>> +		u_char *dst_buf = buf;
>> +		size_t buf_len = len;
>>  
>>  		if (nor->flags & SNOR_F_S3AN_ADDR_DEFAULT)
>>  			addr = spi_nor_s3an_addr_convert(nor, addr);
>>  
>> -		ret = nor->read(nor, addr, len, buf);
>> +		if (!virt_addr_valid(buf) && nor->bounce_buf) {
>> +			use_bb = true;
>> +			dst_buf = nor->bounce_buf;
>> +			if (len > mtd->erasesize)
>> +				buf_len = mtd->erasesize;
> 
> Doesn't this degrade the read operation to a short read?
> Not sure whether this is harmless or not.

It would not be practical to allocate very large buffer using kmalloc as
that migh just take away significant chunk of contiguous kernel memory
(which may be idle when flash is not in use in case of pre-allocated
buffer). Also allocating and deallocating on demand might lead for
fragmentation.
I am trying to find a balance between performance and memory usage.
Whats the max read size we can expect here? Could you provide some
insight from UBIFS perspective?

From my experiments of reading 20MB file from JFFS2/UBIFS, I haven't
seen read requests larger than mtd->erasesize that use vmalloc'd buffers.


> Cyrille?
> 
>> +		}
>> +
>> +		ret = nor->read(nor, from, buf_len, dst_buf);
>>  		if (ret == 0) {
>>  			/* We shouldn't see 0-length reads */
>>  			ret = -EIO;
>> @@ -1217,7 +1228,8 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
>>  		}
>>  		if (ret < 0)
>>  			goto read_err;
>> -
>> +		if (use_bb)
>> +			memcpy(buf, dst_buf, ret);
>>  		WARN_ON(ret > len);
>>  		*retlen += ret;
>>  		buf += ret;
>> @@ -1329,6 +1341,7 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>>  		return ret;
>>  
>>  	for (i = 0; i < len; ) {
>> +		const u_char *src_buf = buf + i;
>>  		ssize_t written;
>>  		loff_t addr = to + i;
>>  
>> @@ -1354,8 +1367,13 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>>  		if (nor->flags & SNOR_F_S3AN_ADDR_DEFAULT)
>>  			addr = spi_nor_s3an_addr_convert(nor, addr);
>>  
>> +		if (!virt_addr_valid(buf) && nor->bounce_buf) {
>> +			memcpy(nor->bounce_buf, buf + i, page_remain);
>> +			src_buf = nor->bounce_buf;
>> +		}
>> +
>>  		write_enable(nor);
>> -		ret = nor->write(nor, addr, page_remain, buf + i);
>> +		ret = nor->write(nor, addr, page_remain, src_buf);
>>  		if (ret < 0)
>>  			goto write_err;
>>  		written = ret;
>> @@ -1720,6 +1738,12 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>  		return -EINVAL;
>>  	}
>>  
>> +	if (nor->flags & SNOR_F_USE_BOUNCE_BUFFER) {
>> +		nor->bounce_buf = devm_kmalloc(dev, mtd->erasesize, GFP_KERNEL);
>> +		if (!nor->bounce_buf)
>> +			dev_err(dev, "unable to allocated bounce buffer\n");
> 
> I think we should return here and not continue.
> 

Agreed.

-- 
Regards
Vignesh

[toc] | [prev] | [next] | [standalone]

#1590174 — Re: [RFC PATCH 1/2] mtd: spi-nor: Introduce bounce buffer to handle vmalloc'd buffers

Le 28/02/2017 à 22:39, Richard Weinberger a écrit :
> Vignesh,
> 
> Am 27.02.2017 um 13:08 schrieb Vignesh R:
>> Filesystems like UBIFS may pass vmalloc'd buffers to SPI NOR layer which
>> will end up in SPI layer. SPI core does try to handle such buffers (see
>> spi_map_buf()) by doing vmalloc_to_page() and creating scatterlist. But,
>> its known that this does not work well with VIVT/aliasing cache
>> architectures.
>> This also fails when buffers are addressed using LPAE (buffers in region
>> higher than 32 bit addressable region), if DMA is 32bit only.
>>
>> Introduce bounce buffers support in SPI NOR framework to handle
>> vmalloc'd buffers. Use a pre-allocated per flash bounce buffer equal to
>> the sector size of the flash. Flash drivers can enable this feature by
>> setting SNOR_F_USE_BOUNCE_BUFFER flag.
>> This would also enable SPI NOR drivers to safely use DMA in their
>> read/write callbacks.
>>
>> Signed-off-by: Vignesh R <vigneshr@ti.com>
>> ---
>>  drivers/mtd/spi-nor/spi-nor.c | 30 +++++++++++++++++++++++++++---
>>  include/linux/mtd/spi-nor.h   |  4 ++++
>>  2 files changed, 31 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index 747645c74134..c241fefa5aff 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -17,6 +17,7 @@
>>  #include <linux/mutex.h>
>>  #include <linux/math64.h>
>>  #include <linux/sizes.h>
>> +#include <linux/mm.h>
>>  
>>  #include <linux/mtd/mtd.h>
>>  #include <linux/of_platform.h>
>> @@ -1205,11 +1206,21 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
>>  
>>  	while (len) {
>>  		loff_t addr = from;
>> +		bool use_bb = false;
>> +		u_char *dst_buf = buf;
>> +		size_t buf_len = len;
>>  
>>  		if (nor->flags & SNOR_F_S3AN_ADDR_DEFAULT)
>>  			addr = spi_nor_s3an_addr_convert(nor, addr);
>>  
>> -		ret = nor->read(nor, addr, len, buf);
>> +		if (!virt_addr_valid(buf) && nor->bounce_buf) {

Should we use is_vmalloc_addr() instead of virt_addr_valid() ?

I guess virt_addr_valid() returns true even for kmalloc'ed buffers
however the copy into the bounce buffer should be avoided for kmalloc'ed
memory.

>> +			use_bb = true;
>> +			dst_buf = nor->bounce_buf;
>> +			if (len > mtd->erasesize)
>> +				buf_len = mtd->erasesize;
> 
> Doesn't this degrade the read operation to a short read?
> Not sure whether this is harmless or not.
> Cyrille?
>

Currently in spi-nor, mtd->erasesize can be either 4KB or 64KB.
Later other values will be supported such as 32KB or 128KB so I guess we
can assume the minimum value for mtd->erasesize is 4KB.
So I don't expect a noticeable impact on the read performances.

Anyway, we can also add a nor->bounce_buf_size and set it to
max_t(size_t, mtd->erasesize, MIN_BOUNCE_BUF_SIZE) if we want to
guarantee a minimum size for this bounce buffer hence limiting the
performance loss.


>> +		}
>> +
>> +		ret = nor->read(nor, from, buf_len, dst_buf);
>>  		if (ret == 0) {
>>  			/* We shouldn't see 0-length reads */
>>  			ret = -EIO;
>> @@ -1217,7 +1228,8 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
>>  		}
>>  		if (ret < 0)
>>  			goto read_err;
>> -
>> +		if (use_bb)
>> +			memcpy(buf, dst_buf, ret);
>>  		WARN_ON(ret > len);
>>  		*retlen += ret;
>>  		buf += ret;
>> @@ -1329,6 +1341,7 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>>  		return ret;
>>  
>>  	for (i = 0; i < len; ) {
>> +		const u_char *src_buf = buf + i;
>>  		ssize_t written;
>>  		loff_t addr = to + i;
>>  
>> @@ -1354,8 +1367,13 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>>  		if (nor->flags & SNOR_F_S3AN_ADDR_DEFAULT)
>>  			addr = spi_nor_s3an_addr_convert(nor, addr);
>>  
>> +		if (!virt_addr_valid(buf) && nor->bounce_buf) {
>> +			memcpy(nor->bounce_buf, buf + i, page_remain);
>> +			src_buf = nor->bounce_buf;
>> +		}
>> +
>>  		write_enable(nor);
>> -		ret = nor->write(nor, addr, page_remain, buf + i);
>> +		ret = nor->write(nor, addr, page_remain, src_buf);
>>  		if (ret < 0)
>>  			goto write_err;
>>  		written = ret;
>> @@ -1720,6 +1738,12 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>  		return -EINVAL;
>>  	}
>>  
>> +	if (nor->flags & SNOR_F_USE_BOUNCE_BUFFER) {
>> +		nor->bounce_buf = devm_kmalloc(dev, mtd->erasesize, GFP_KERNEL);
>> +		if (!nor->bounce_buf)
>> +			dev_err(dev, "unable to allocated bounce buffer\n");
> 
> I think we should return here and not continue.
> 
> Thanks,
> //richard
> 

[toc] | [prev] | [next] | [standalone]

#1590244 — Re: [RFC PATCH 1/2] mtd: spi-nor: Introduce bounce buffer to handle vmalloc'd buffers

On Wed, 1 Mar 2017 11:09:57 +0100
Cyrille Pitchen <cyrille.pitchen@atmel.com> wrote:

> Le 28/02/2017 à 22:39, Richard Weinberger a écrit :
> > Vignesh,
> > 
> > Am 27.02.2017 um 13:08 schrieb Vignesh R:  
> >> Filesystems like UBIFS may pass vmalloc'd buffers to SPI NOR layer which
> >> will end up in SPI layer. SPI core does try to handle such buffers (see
> >> spi_map_buf()) by doing vmalloc_to_page() and creating scatterlist. But,
> >> its known that this does not work well with VIVT/aliasing cache
> >> architectures.
> >> This also fails when buffers are addressed using LPAE (buffers in region
> >> higher than 32 bit addressable region), if DMA is 32bit only.
> >>
> >> Introduce bounce buffers support in SPI NOR framework to handle
> >> vmalloc'd buffers. Use a pre-allocated per flash bounce buffer equal to
> >> the sector size of the flash. Flash drivers can enable this feature by
> >> setting SNOR_F_USE_BOUNCE_BUFFER flag.
> >> This would also enable SPI NOR drivers to safely use DMA in their
> >> read/write callbacks.
> >>
> >> Signed-off-by: Vignesh R <vigneshr@ti.com>
> >> ---
> >>  drivers/mtd/spi-nor/spi-nor.c | 30 +++++++++++++++++++++++++++---
> >>  include/linux/mtd/spi-nor.h   |  4 ++++
> >>  2 files changed, 31 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> >> index 747645c74134..c241fefa5aff 100644
> >> --- a/drivers/mtd/spi-nor/spi-nor.c
> >> +++ b/drivers/mtd/spi-nor/spi-nor.c
> >> @@ -17,6 +17,7 @@
> >>  #include <linux/mutex.h>
> >>  #include <linux/math64.h>
> >>  #include <linux/sizes.h>
> >> +#include <linux/mm.h>
> >>  
> >>  #include <linux/mtd/mtd.h>
> >>  #include <linux/of_platform.h>
> >> @@ -1205,11 +1206,21 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
> >>  
> >>  	while (len) {
> >>  		loff_t addr = from;
> >> +		bool use_bb = false;
> >> +		u_char *dst_buf = buf;
> >> +		size_t buf_len = len;
> >>  
> >>  		if (nor->flags & SNOR_F_S3AN_ADDR_DEFAULT)
> >>  			addr = spi_nor_s3an_addr_convert(nor, addr);
> >>  
> >> -		ret = nor->read(nor, addr, len, buf);
> >> +		if (!virt_addr_valid(buf) && nor->bounce_buf) {  
> 
> Should we use is_vmalloc_addr() instead of virt_addr_valid() ?
> 
> I guess virt_addr_valid() returns true even for kmalloc'ed buffers
> however the copy into the bounce buffer should be avoided for kmalloc'ed
> memory.

The test is !virt_addr_valid(), so we won't use the bounce buffer for
kmalloc-ed regions. I don't remember why we use virt_addr_valid()
instead of is_vmalloc_addr() in the NAND framework, but there was a
good reason (virt_addr_valid() is more restrictive, but I don't
remember why it's safer :))

[toc] | [prev] | [next] | [standalone]

#1590267 — Re: [RFC PATCH 1/2] mtd: spi-nor: Introduce bounce buffer to handle vmalloc'd buffers

On 01/03/2017 11:18, Boris Brezillon wrote:
> On Wed, 1 Mar 2017 11:09:57 +0100
> Cyrille Pitchen <cyrille.pitchen@atmel.com> wrote:
>
>> Le 28/02/2017 à 22:39, Richard Weinberger a écrit :
>>> Vignesh,
>>>
>>> Am 27.02.2017 um 13:08 schrieb Vignesh R:  
>>>> Filesystems like UBIFS may pass vmalloc'd buffers to SPI NOR layer which
>>>> will end up in SPI layer. SPI core does try to handle such buffers (see
>>>> spi_map_buf()) by doing vmalloc_to_page() and creating scatterlist. But,
>>>> its known that this does not work well with VIVT/aliasing cache
>>>> architectures.
>>>> This also fails when buffers are addressed using LPAE (buffers in region
>>>> higher than 32 bit addressable region), if DMA is 32bit only.
>>>>
>>>> Introduce bounce buffers support in SPI NOR framework to handle
>>>> vmalloc'd buffers. Use a pre-allocated per flash bounce buffer equal to
>>>> the sector size of the flash. Flash drivers can enable this feature by
>>>> setting SNOR_F_USE_BOUNCE_BUFFER flag.
>>>> This would also enable SPI NOR drivers to safely use DMA in their
>>>> read/write callbacks.
>>>>
>>>> Signed-off-by: Vignesh R <vigneshr@ti.com>
>>>> ---
>>>>  drivers/mtd/spi-nor/spi-nor.c | 30 +++++++++++++++++++++++++++---
>>>>  include/linux/mtd/spi-nor.h   |  4 ++++
>>>>  2 files changed, 31 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>>>> index 747645c74134..c241fefa5aff 100644
>>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>>> @@ -17,6 +17,7 @@
>>>>  #include <linux/mutex.h>
>>>>  #include <linux/math64.h>
>>>>  #include <linux/sizes.h>
>>>> +#include <linux/mm.h>
>>>>  
>>>>  #include <linux/mtd/mtd.h>
>>>>  #include <linux/of_platform.h>
>>>> @@ -1205,11 +1206,21 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
>>>>  
>>>>  	while (len) {
>>>>  		loff_t addr = from;
>>>> +		bool use_bb = false;
>>>> +		u_char *dst_buf = buf;
>>>> +		size_t buf_len = len;
>>>>  
>>>>  		if (nor->flags & SNOR_F_S3AN_ADDR_DEFAULT)
>>>>  			addr = spi_nor_s3an_addr_convert(nor, addr);
>>>>  
>>>> -		ret = nor->read(nor, addr, len, buf);
>>>> +		if (!virt_addr_valid(buf) && nor->bounce_buf) {  
>> Should we use is_vmalloc_addr() instead of virt_addr_valid() ?
>>
>> I guess virt_addr_valid() returns true even for kmalloc'ed buffers
>> however the copy into the bounce buffer should be avoided for kmalloc'ed
>> memory.
> The test is !virt_addr_valid(), so we won't use the bounce buffer for
> kmalloc-ed regions. I don't remember why we use virt_addr_valid()
> instead of is_vmalloc_addr() in the NAND framework, but there was a
> good reason (virt_addr_valid() is more restrictive, but I don't
> remember why it's safer :))
I think virt_addr_valid() picks up both kmap'ed and vmalloc'ed pages as not valid...

Frode
>
>
>

[toc] | [prev] | [next] | [standalone]

#1590300 — Re: [RFC PATCH 1/2] mtd: spi-nor: Introduce bounce buffer to handle vmalloc'd buffers

On Wed, 1 Mar 2017 12:18:30 +0100
Frode Isaksen <fisaksen@baylibre.com> wrote:

> On 01/03/2017 11:18, Boris Brezillon wrote:
> > On Wed, 1 Mar 2017 11:09:57 +0100
> > Cyrille Pitchen <cyrille.pitchen@atmel.com> wrote:
> >  
> >> Le 28/02/2017 à 22:39, Richard Weinberger a écrit :  
> >>> Vignesh,
> >>>
> >>> Am 27.02.2017 um 13:08 schrieb Vignesh R:    
> >>>> Filesystems like UBIFS may pass vmalloc'd buffers to SPI NOR layer which
> >>>> will end up in SPI layer. SPI core does try to handle such buffers (see
> >>>> spi_map_buf()) by doing vmalloc_to_page() and creating scatterlist. But,
> >>>> its known that this does not work well with VIVT/aliasing cache
> >>>> architectures.
> >>>> This also fails when buffers are addressed using LPAE (buffers in region
> >>>> higher than 32 bit addressable region), if DMA is 32bit only.
> >>>>
> >>>> Introduce bounce buffers support in SPI NOR framework to handle
> >>>> vmalloc'd buffers. Use a pre-allocated per flash bounce buffer equal to
> >>>> the sector size of the flash. Flash drivers can enable this feature by
> >>>> setting SNOR_F_USE_BOUNCE_BUFFER flag.
> >>>> This would also enable SPI NOR drivers to safely use DMA in their
> >>>> read/write callbacks.
> >>>>
> >>>> Signed-off-by: Vignesh R <vigneshr@ti.com>
> >>>> ---
> >>>>  drivers/mtd/spi-nor/spi-nor.c | 30 +++++++++++++++++++++++++++---
> >>>>  include/linux/mtd/spi-nor.h   |  4 ++++
> >>>>  2 files changed, 31 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> >>>> index 747645c74134..c241fefa5aff 100644
> >>>> --- a/drivers/mtd/spi-nor/spi-nor.c
> >>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
> >>>> @@ -17,6 +17,7 @@
> >>>>  #include <linux/mutex.h>
> >>>>  #include <linux/math64.h>
> >>>>  #include <linux/sizes.h>
> >>>> +#include <linux/mm.h>
> >>>>  
> >>>>  #include <linux/mtd/mtd.h>
> >>>>  #include <linux/of_platform.h>
> >>>> @@ -1205,11 +1206,21 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
> >>>>  
> >>>>  	while (len) {
> >>>>  		loff_t addr = from;
> >>>> +		bool use_bb = false;
> >>>> +		u_char *dst_buf = buf;
> >>>> +		size_t buf_len = len;
> >>>>  
> >>>>  		if (nor->flags & SNOR_F_S3AN_ADDR_DEFAULT)
> >>>>  			addr = spi_nor_s3an_addr_convert(nor, addr);
> >>>>  
> >>>> -		ret = nor->read(nor, addr, len, buf);
> >>>> +		if (!virt_addr_valid(buf) && nor->bounce_buf) {    
> >> Should we use is_vmalloc_addr() instead of virt_addr_valid() ?
> >>
> >> I guess virt_addr_valid() returns true even for kmalloc'ed buffers
> >> however the copy into the bounce buffer should be avoided for kmalloc'ed
> >> memory.  
> > The test is !virt_addr_valid(), so we won't use the bounce buffer for
> > kmalloc-ed regions. I don't remember why we use virt_addr_valid()
> > instead of is_vmalloc_addr() in the NAND framework, but there was a
> > good reason (virt_addr_valid() is more restrictive, but I don't
> > remember why it's safer :))  
> I think virt_addr_valid() picks up both kmap'ed and vmalloc'ed pages as not valid...

Thanks for the remainder. So we definitely want to use
virt_addr_valid() here.

[toc] | [prev] | [next] | [standalone]

#1590280 — Re: [RFC PATCH 1/2] mtd: spi-nor: Introduce bounce buffer to handle vmalloc'd buffers

On Wednesday 01 March 2017 03:39 PM, Cyrille Pitchen wrote:
> Le 28/02/2017 à 22:39, Richard Weinberger a écrit :
>> Vignesh,
>>
>> Am 27.02.2017 um 13:08 schrieb Vignesh R:
>>> Filesystems like UBIFS may pass vmalloc'd buffers to SPI NOR layer which
>>> will end up in SPI layer. SPI core does try to handle such buffers (see
>>> spi_map_buf()) by doing vmalloc_to_page() and creating scatterlist. But,
>>> its known that this does not work well with VIVT/aliasing cache
>>> architectures.
>>> This also fails when buffers are addressed using LPAE (buffers in region
>>> higher than 32 bit addressable region), if DMA is 32bit only.
>>>
>>> Introduce bounce buffers support in SPI NOR framework to handle
>>> vmalloc'd buffers. Use a pre-allocated per flash bounce buffer equal to
>>> the sector size of the flash. Flash drivers can enable this feature by
>>> setting SNOR_F_USE_BOUNCE_BUFFER flag.
>>> This would also enable SPI NOR drivers to safely use DMA in their
>>> read/write callbacks.
>>>
>>> Signed-off-by: Vignesh R <vigneshr@ti.com>
>>> ---
>>>  drivers/mtd/spi-nor/spi-nor.c | 30 +++++++++++++++++++++++++++---
>>>  include/linux/mtd/spi-nor.h   |  4 ++++
>>>  2 files changed, 31 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>>> index 747645c74134..c241fefa5aff 100644
>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>> @@ -17,6 +17,7 @@
>>>  #include <linux/mutex.h>
>>>  #include <linux/math64.h>
>>>  #include <linux/sizes.h>
>>> +#include <linux/mm.h>
>>>  
>>>  #include <linux/mtd/mtd.h>
>>>  #include <linux/of_platform.h>
>>> @@ -1205,11 +1206,21 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
>>>  
>>>  	while (len) {
>>>  		loff_t addr = from;
>>> +		bool use_bb = false;
>>> +		u_char *dst_buf = buf;
>>> +		size_t buf_len = len;
>>>  
>>>  		if (nor->flags & SNOR_F_S3AN_ADDR_DEFAULT)
>>>  			addr = spi_nor_s3an_addr_convert(nor, addr);
>>>  
>>> -		ret = nor->read(nor, addr, len, buf);
>>> +		if (!virt_addr_valid(buf) && nor->bounce_buf) {
> 
> Should we use is_vmalloc_addr() instead of virt_addr_valid() ?
> 
> I guess virt_addr_valid() returns true even for kmalloc'ed buffers
> however the copy into the bounce buffer should be avoided for kmalloc'ed
> memory.
> 

Its !virt_addr_valid(), so that both vmap and kmap'd buffers are taken
care of.

>>> +			use_bb = true;
>>> +			dst_buf = nor->bounce_buf;
>>> +			if (len > mtd->erasesize)
>>> +				buf_len = mtd->erasesize;
>>
>> Doesn't this degrade the read operation to a short read?
>> Not sure whether this is harmless or not.
>> Cyrille?
>>
> 
> Currently in spi-nor, mtd->erasesize can be either 4KB or 64KB.
> Later other values will be supported such as 32KB or 128KB so I guess we
> can assume the minimum value for mtd->erasesize is 4KB.
> So I don't expect a noticeable impact on the read performances.
> 
> Anyway, we can also add a nor->bounce_buf_size and set it to
> max_t(size_t, mtd->erasesize, MIN_BOUNCE_BUF_SIZE) if we want to
> guarantee a minimum size for this bounce buffer hence limiting the
> performance loss.
> 

yeah, I can do that if you insist. Any suggestion for
MIN_BOUNCE_BUF_SIZE? 64KB?

> 
>>> +		}
>>> +
>>> +		ret = nor->read(nor, from, buf_len, dst_buf);
>>>  		if (ret == 0) {
>>>  			/* We shouldn't see 0-length reads */
>>>  			ret = -EIO;
>>> @@ -1217,7 +1228,8 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
>>>  		}
>>>  		if (ret < 0)
>>>  			goto read_err;
>>> -
>>> +		if (use_bb)
>>> +			memcpy(buf, dst_buf, ret);
>>>  		WARN_ON(ret > len);
>>>  		*retlen += ret;
>>>  		buf += ret;
>>> @@ -1329,6 +1341,7 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>>>  		return ret;
>>>  
>>>  	for (i = 0; i < len; ) {
>>> +		const u_char *src_buf = buf + i;
>>>  		ssize_t written;
>>>  		loff_t addr = to + i;
>>>  
>>> @@ -1354,8 +1367,13 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>>>  		if (nor->flags & SNOR_F_S3AN_ADDR_DEFAULT)
>>>  			addr = spi_nor_s3an_addr_convert(nor, addr);
>>>  
>>> +		if (!virt_addr_valid(buf) && nor->bounce_buf) {
>>> +			memcpy(nor->bounce_buf, buf + i, page_remain);
>>> +			src_buf = nor->bounce_buf;
>>> +		}
>>> +
>>>  		write_enable(nor);
>>> -		ret = nor->write(nor, addr, page_remain, buf + i);
>>> +		ret = nor->write(nor, addr, page_remain, src_buf);
>>>  		if (ret < 0)
>>>  			goto write_err;
>>>  		written = ret;
>>> @@ -1720,6 +1738,12 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>>  		return -EINVAL;
>>>  	}
>>>  
>>> +	if (nor->flags & SNOR_F_USE_BOUNCE_BUFFER) {
>>> +		nor->bounce_buf = devm_kmalloc(dev, mtd->erasesize, GFP_KERNEL);
>>> +		if (!nor->bounce_buf)
>>> +			dev_err(dev, "unable to allocated bounce buffer\n");
>>
>> I think we should return here and not continue.
>>
>> Thanks,
>> //richard
>>
> 

-- 
Regards
Vignesh

[toc] | [prev] | [next] | [standalone]

#1588779

On 27/02/2017 13:08, Vignesh R wrote:
> This series implements bounce buffer support to handle vmalloc'd buffers
> in case of drivers use DMA, similar to what is done in MTD NAND
> framework.
>
> I have tested this on two platform:
> DRA74 SoC (cortex a15 @1GHz) with s25fl256s1 QSPI (SPI bus frequency
> 76.8 MHz) and I don't see any performance degradation with UBIFS
> read/write(10MB file).
> AM437x SoC (cortex a9 @1GHz) with macronix mx66l51235l QSPI flash (SPI bus
> frequency 48MHz) and performance degradation is <4% for UBIFS.
>
>
> Vignesh R (2):
>   mtd: spi-nor: Introduce bounce buffer to handle vmalloc'd buffers
>   mtd: devices: m25p80: Enable spi-nor bounce buffer support
>
>  drivers/mtd/devices/m25p80.c  |  1 +
>  drivers/mtd/spi-nor/spi-nor.c | 30 +++++++++++++++++++++++++++---
>  include/linux/mtd/spi-nor.h   |  4 ++++
>  3 files changed, 32 insertions(+), 3 deletions(-)
>
I have tested this on a daVinci EVM board with SPI DMA enabled and the UBIFS tests all passed.

Thanks,
Frode

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web