lkml.org 
[lkml]   [2015]   [Jul]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC PATCH 1/2] mtd: nand: add nand_check_erased helper functions
From
Date

Dear Boris,

thanks for pointing this out again.

I'm on the same topic too, using iMX6 (I'll try to test you patch on the
next days, if I found some spare time, unfortunately I got a 3.10
kernel, so I think the patch will not apply cleanly :-( ).

See my comment below (and on the next mail too)

Il 31/07/2015 09:10, Boris Brezillon ha scritto:
> On Thu, 30 Jul 2015 19:34:53 +0200
> Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
>
>> Add two helper functions to help NAND controller drivers test whether a
>> specific NAND region is erased or not.
>>
>> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
>> ---
>> drivers/mtd/nand/nand_base.c | 104 +++++++++++++++++++++++++++++++++++++++++++
>> include/linux/mtd/nand.h | 8 ++++
>> 2 files changed, 112 insertions(+)
>>
>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
>> index ceb68ca..1542ea7 100644
>> --- a/drivers/mtd/nand/nand_base.c
>> +++ b/drivers/mtd/nand/nand_base.c
>> @@ -1101,6 +1101,110 @@ out:
>> EXPORT_SYMBOL(nand_lock);
>>
>> /**
>> + * nand_check_erased_buf - check if a buffer contains (almost) only 0xff data
>> + * @buf: buffer to test
>> + * @len: buffer length
>> + * @bitflips_threshold:maximum number of bitflips
>> + *
>> + * Check if a buffer contains only 0xff, which means the underlying region
>> + * has been erased and is ready to be programmed.
>> + * The bitflips_threshold specify the maximum number of bitflips before
>> + * considering the region is not erased.
>> + * Note: The logic of this function has been extracted from the memweight
>> + * implementation, except that nand_check_erased_buf function exit before
>> + * testing the whole buffer if the number of bitflips exceed the
>> + * bitflips_threshold value.
>> + *
>> + * Returns a positive number of bitflips or -ERROR_CODE.
>> + */
>> +int nand_check_erased_buf(void *buf, int len, int bitflips_threshold)
>> +{
>> + const unsigned char *bitmap = buf;
>> + int bitflips = 0;
>> + int weight;
>> + int longs;
>> +
>> + for (; len && ((unsigned long)bitmap) % sizeof(long);
>> + len--, bitmap++) {
>> + weight = hweight8(*bitmap);
>> +
>> + bitflips += sizeof(u8) - weight;
>> + if (bitflips > bitflips_threshold)
>> + return -EINVAL;

I think it's better to do something like:

if (UNLIKELY(bitflips > bitflips_threshold))
return -EINVAL;

isn't it? :-)
(the same for the other if)


>> + }
>> +
>> +
>> + for (longs = len / sizeof(long); longs;
>> + longs--, bitmap += sizeof(long)) {
>> + BUG_ON(longs >= INT_MAX / BITS_PER_LONG);
>> + weight = hweight_long(*((unsigned long *)bitmap));
>> +
>> + bitflips += sizeof(long) - weight;
>> + if (bitflips > bitflips_threshold)
>> + return -EINVAL;
>> + }
>> +
>> + len %= sizeof(long);
>> +
>> + for (; len > 0; len--, bitmap++) {
>> + weight = hweight8(*bitmap);
>> + bitflips += sizeof(u8) - weight;
>> + }
>> +
>> + return bitflips;
>> +}
>> +EXPORT_SYMBOL(nand_check_erased_buf);
>> +
>> +/**
>> + * nand_check_erased_ecc_chunk - check if an ECC chunk contains (almost) only
>> + * 0xff data
>> + * @data: data buffer to test
>> + * @datalen: data length
>> + * @ecc: ECC buffer
>> + * @ecclen: ECC length
>> + * @extraoob: extra OOB buffer
>> + * @extraooblen: extra OOB length
>> + * @bitflips_threshold: maximum number of bitflips
>> + *
>> + * Check if a data buffer and its associated ECC and OOB data contains only
>> + * 0xff pattern, which means the underlying region has been erased and is
>> + * ready to be programmed.
>> + * The bitflips_threshold specify the maximum number of bitflips before
>> + * considering the region as not erased.
>> + *
>> + * Returns a positive number of bitflips or -ERROR_CODE.
>> + */
>> +int nand_check_erased_ecc_chunk(void *data, int datalen,
>> + void *ecc, int ecclen,
>> + void *extraoob, int extraooblen,
>> + int bitflips_threshold)
>> +{
>> + int bitflips = 0;
>> + int ret;
>> +
>> + ret = nand_check_erased_buf(data, datalen, bitflips_threshold);
>> + if (ret < 0)
>> + return ret;
>> +
>> + bitflips += ret;
>> + bitflips_threshold -= ret;
>> +
>> + ret = nand_check_erased_buf(ecc, ecclen, bitflips_threshold);
>> + if (ret < 0)
>> + return ret;
>> +
>> + bitflips += ret;
>> + bitflips_threshold -= ret;
>> +
>> + ret = nand_check_erased_buf(extraoob, extraooblen, bitflips_threshold);
>> + if (ret < 0)
>> + return ret;
>> +
>
> Forgot the memset operations here:
>
> memset(data, 0xff, datalen);
> memset(ecc, 0xff, ecclen);
> memset(extraoob, 0xff, extraooblen);

Yes, you're right.. I did the same mistake on my first implementation
too ;-)

As additional optimization you may also check if the lower layer already
did the check for you (e.g. if you have an iMXQP as we saw in latest
days), but I think it's a minor one, because you'll face this situation
very very unlikely.

--

Andrea SCIAN

DAVE Embedded Systems


\
 
 \ /
  Last update: 2015-07-31 12:21    [W:0.152 / U:0.536 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site