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

[Richard Weinberger]

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) {
> +			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