Messages in this thread | | | Subject | Re: [RFC PATCH 1/2] mtd: nand: add nand_check_erased helper functions | From | Andrea Scian <> | Date | Fri, 31 Jul 2015 12:06:32 +0200 |
| |
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
| |