From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from exsmtp03.microchip.com ([198.175.253.49] helo=email.microchip.com) by bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux)) id 1cj1DI-0003wI-2c for linux-mtd@lists.infradead.org; Wed, 01 Mar 2017 10:10:30 +0000 Subject: Re: [RFC PATCH 1/2] mtd: spi-nor: Introduce bounce buffer to handle vmalloc'd buffers To: Richard Weinberger , Vignesh R , David Woodhouse , Brian Norris , Boris Brezillon , Marek Vasut References: <20170227120839.16545-1-vigneshr@ti.com> <20170227120839.16545-2-vigneshr@ti.com> <8e441369-5c15-d711-1789-b55eadf33c8f@nod.at> CC: , , , Frode Isaksen , From: Cyrille Pitchen Message-ID: <65b4a156-f2a8-4c5e-4399-2024a147d5ec@atmel.com> Date: Wed, 1 Mar 2017 11:09:57 +0100 MIME-Version: 1.0 In-Reply-To: <8e441369-5c15-d711-1789-b55eadf33c8f@nod.at> Content-Type: text/plain; charset="iso-8859-15" Content-Transfer-Encoding: 8bit List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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 >> --- >> 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 >> #include >> #include >> +#include >> >> #include >> #include >> @@ -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 >