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

[Vignesh R]

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

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