From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752120AbdCAKNM (ORCPT ); Wed, 1 Mar 2017 05:13:12 -0500 Received: from exsmtp03.microchip.com ([198.175.253.49]:57783 "EHLO email.microchip.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751971AbdCAKNH (ORCPT ); Wed, 1 Mar 2017 05:13:07 -0500 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 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 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 X-Brightmail-Tracker: =?iso-8859-15?Q?H4sIAAAAAAAAC+NgFvrBKsWRWlGSWpSXmKPExsXCxeXDovtn7rYIgydnD?= =?iso-8859-15?Q?tzktTjwYiGLxZELa5ktJq6czGzx9NI/VovLu+awWexuWsZuMXtJP4tF48?= =?iso-8859-15?Q?eb7BZH99xjtpi88w2jxf+zH9gduD3e32hl93iy6SKjx85Zd9k9Nq/Q8ti?= =?iso-8859-15?Q?8pN7j5rxCj+M3tjN5fN4kF8ARxZqZl5RfkcCacenBQ/aCOyoVu8/+YW5g?= =?iso-8859-15?Q?/CnTxcjFISSwjFHi8KXnrF2MHBzCAgkSz98YgMRFBH4xSnz72cIGUdTPK?= =?iso-8859-15?Q?DFx4kRGEIdZYCGjxJ1JE5m7GDk52AQMJd4+OMoKYvMK2Eh8+XmRCcRmEV?= =?iso-8859-15?Q?CRWLqwgw3EFhWIkJj/dBUTRI2gxMmZT1hAbE4BK4kjfzsZQWxmAX2J1Q0?= =?iso-8859-15?Q?H2CBseYnmrbPB5gsJqEksbFkBZksIBEq8+/OKDcJ2klh6ZQMLhG0ncXj6?= =?iso-8859-15?Q?RXYI216ie+l2RpiaD7/fQtnaEttf7WOFsHUkth3sh+q1ldgzYyIThO0u8?= =?iso-8859-15?Q?eDRcijbV2LWwwaomiiJDftes09glJyF5IVZSM6eheTsBYzMqxilkzPydI?= =?iso-8859-15?Q?vLdFMrkjMMjPVykzMKdHMTM/P0kvNzNzFC0oDhDsadv8MPMUpyMCmJ8l7?= =?iso-8859-15?Q?i2hYhxJeUn1KZkVicEV9UmpNafIhRhoNDSYI3fzZQTrAoNT21Ii0zB5iQ?= =?iso-8859-15?Q?YNJMHJyHGCU4eJREeLnmANXwFhck5hZnpkPkTzFKSonznpwFlBAASWSU5?= =?iso-8859-15?Q?sH1XmIUlRLmjZ0OlOMpSC3KzSyBiN9iFON4yMTxmEmIJS8/L1UK6FQGID?= =?iso-8859-15?Q?BgfMUozsGoJMw7B+QWnsy8Erg1r4AuYAK64IXKVpALShIRUlINjEcTDNc?= =?iso-8859-15?Q?3TFmqX+256v7/Bcwd17S2br9iW1IrvLYk58biXO2GY0ITV+6aZCT6LTz4?= =?iso-8859-15?Q?TetX7omXfHV6Xty8zJLBxP7J2brF3Pj91FeFWZ1G8x+a52w68fJDiZ30i?= =?iso-8859-15?Q?ag1RodX/GN5O2ePueYslzefpZZxPnTvWxNT+3BCXrZe0Gl+8fZq5q/vJO?= =?iso-8859-15?Q?OfmSmxFGckGmoxFxUnAgBNWoaKiwMAAA=3D=3D?= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >