| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <b19ba370-8d8f-2cad-292b-6a910ed48e27@ti.com>
Date: Wed, 1 Mar 2017 17:20:35 +0530
From: Vignesh R <vigneshr@...com>
To: Cyrille Pitchen <cyrille.pitchen@...el.com>,
Richard Weinberger <richard@....at>,
David Woodhouse <dwmw2@...radead.org>,
Brian Norris <computersforpeace@...il.com>,
Boris Brezillon <boris.brezillon@...e-electrons.com>,
Marek Vasut <marek.vasut@...il.com>
CC: <linux-mtd@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
<linux-omap@...r.kernel.org>,
Frode Isaksen <fisaksen@...libre.com>,
<linux-spi@...r.kernel.org>
Subject: Re: [RFC PATCH 1/2] mtd: spi-nor: Introduce bounce buffer to handle
vmalloc'd buffers
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@...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
>>
>
--
Regards
Vignesh
Powered by blists - more mailing lists