Re: [RFC PATCH 1/2] mtd: nand: add nand_check_erased helper functions

Andrea Scian
2015-07-31T10:07:04Z

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