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

From: Cyrille Pitchen
Date: Wed Mar 01 2017 - 05:13:16 EST


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@xxxxxx>
>> ---
>> 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
>