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 <[email protected]>
>> ---
>> 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
>
On Wed, 1 Mar 2017 11:09:57 +0100
Cyrille Pitchen <[email protected]> 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 <[email protected]>
> >> ---
> >> 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 :))
On 01/03/2017 11:18, Boris Brezillon wrote:
> On Wed, 1 Mar 2017 11:09:57 +0100
> Cyrille Pitchen <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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
>
>
>
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 <[email protected]>
>>> ---
>>> 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
On Wed, 1 Mar 2017 12:18:30 +0100
Frode Isaksen <[email protected]> wrote:
> On 01/03/2017 11:18, Boris Brezillon wrote:
> > On Wed, 1 Mar 2017 11:09:57 +0100
> > Cyrille Pitchen <[email protected]> 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 <[email protected]>
> >>>> ---
> >>>> 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.