| Message ID | 20170227120839.16545-2-vigneshr@ti.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Cyrille Pitchen |
| Headers | show |
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
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.
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 >
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 :))
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 > > >
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 >> >
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.
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; };
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(-)