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

Submitted by Vignesh R on Feb. 27, 2017, 12:08 p.m.

Details

Message ID 20170227120839.16545-2-vigneshr@ti.com
State New
Delegated to: Cyrille Pitchen
Headers show

Commit Message

Vignesh R Feb. 27, 2017, 12:08 p.m.
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(-)

Comments

Richard Weinberger Feb. 28, 2017, 9:39 p.m.
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
Vignesh R March 1, 2017, 5:13 a.m.
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.
Cyrille Pitchen March 1, 2017, 10:09 a.m.
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
>
Boris Brezillon March 1, 2017, 10:18 a.m.
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 :))
Frode Isaksen March 1, 2017, 11:18 a.m.
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
>
>
>
Vignesh R March 1, 2017, 11:50 a.m.
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
>>
>
Boris Brezillon March 1, 2017, 12:12 p.m.
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.

Patch hide | download patch | download mbox

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;
 };