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

Wed Sep 2 12:30:34 PDT 2015

Hi,

On Wed, 2 Sep 2015 11:41:43 -0700
Brian Norris <computersforpeace at gmail.com> wrote:

> Hi Boris,
> 
> First of all, thanks for the persistence.

Hehe, you're not done with me: I still have a bunch of NAND related
patches to post ;-). But I need this one to be accepted first.

> 
> On Mon, Aug 24, 2015 at 11:47:21AM +0200, Boris Brezillon wrote:
> > Add two helper functions to help NAND controller drivers test whether a
> > specific NAND region is erased or not.
> 
> What sort of tests (if any) did you run for this?

As I said, I have a series depending on this patch (see this branch [1]
if you want more details), which means I successfully tested it on a
sunxi platform.
BTW, I tested it with the nandflipbits tool [2] I posted a while ago.
Could you consider taking it (or a similar tool) in mtd-utils?

Andrea, did you test this patch on your imx platform(s)?

> 
> > Signed-off-by: Boris Brezillon <boris.brezillon at free-electrons.com>
> > ---
> >  drivers/mtd/nand/nand_base.c | 108 +++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/mtd/nand.h     |   8 ++++
> >  2 files changed, 116 insertions(+)
> > 
> > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > index ceb68ca..4d2ef65 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > @@ -1101,6 +1101,114 @@ 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.
> 
> Small clarification:
> 
>   "Returns a positive number of bitflips less than or equal to
>   bitflips_threshold, or -ERROR_CODE for bitflips in excess of the
>   threshold."
> 

Sure, I'll change this comment.

> > + */
> > +int nand_check_erased_buf(void *buf, int len, int bitflips_threshold)
> 
> I don't have much problem with this function as a helper, but does it
> really need to be exported? This is exactly the form I *don't* want to
> encourage -- checking a data buffer while ignoring the OOB / ECC. Or did
> you have a good reason to export this one? I see you don't use it
> outside of this file.

I currently have no reason to export this function, but if we want to
reuse part of the logic for non standard drivers we might need to
export it afterwards.
The example I have in mind is the GPMI driver, which also need to count
bitflips, with the exception that ECC buffers are not necessarily
aligned on a byte. So exposing this function would allow reusing a
common logic for aligned buffers, and then dealing with unaligned parts
in driver specific code.

> 
> And especially if we don't end up exporting this one, I'd really like
> some extra explanatory text next to the public interface
> (nand_check_erased_ecc_chunk()) to explain some of the pitfalls.
> Particularly, why we must check both the data and spare area, and how we
> highly recommend (if not require) that new drivers use the existing
> methods rather than roll their own.

This is something I can add even if we keep exporting the previous
function ;-).

> 
> > +{
> > +	const unsigned char *bitmap = buf;
> > +	int bitflips = 0;
> > +	int weight;
> > +	int longs;
> > +
> 
> Does it make any sense to have a short-circuit condition for the
> threshold == 0 case? We could expect to see that for common 1-bit ECC or
> similar.

Hm, I'm not sure I understand your suggestion. How would you shortcut
this case? You still need to detect one bitflip before returning, right?
Are you suggesting to use memweight() in this case? Because if you are,
then I don't see any benefit since I have copied the memweight()
implementation to implement the nand_check_erased_buf() logic.
The only thing that could be avoided are the extra
'bitflips > bitflips_threshold' checks, but I'm not sure it makes such a
big difference for small/medium NAND pages (and 1-bit ECC will
definitely be used on relatively small pages).

> 
> > +	for (; len && ((unsigned long)bitmap) % sizeof(long);
> 
> Cast pointer to unsigned long? I'd expect uintptr_t, but then it sees
> those are the same often enough that types.h says they're identical...

As I said in the header note, I blindly copied memweight()
implementation, sightly adapting it for my need, so there might be room
for improvement.

> 
> > +	     len--, bitmap++) {
> > +		weight = hweight8(*bitmap);
> > +		bitflips += BITS_PER_BYTE - weight;
> > +		if (unlikely(bitflips > bitflips_threshold))
> > +			return -EINVAL;
> 
> I'm not sure EINVAL is the right thing. This isn't a bad argument, it's
> just a bad flash. EUCLEAN would fit typical usage. Same comment applies
> elsewhere.

Yep, I was considering replacing EINVAL by EIO, but EUCLEAN is better.
I'll fix that.

> 
> > +	}
> > +
> > +
> > +	for (longs = len / sizeof(long); longs;
> > +	     longs--, bitmap += sizeof(long)) {
> > +		BUG_ON(longs >= INT_MAX / BITS_PER_LONG);
> 
> Please don't add BUG_ON(). The language around it has gotten even
> stronger to discourage its use. And in this instance I don't think it's
> even helpful; because 'longs' is derived from 'len' (which is an 'int'),
> this condition is mathematically impossible, no?

Ditto: everything was blindly copied from the memweight implementation.

> 
> Also (feel free to shoot me down), wouldn't the loop bounds be a little
> more straightforward (and consistent with the rest of the function), and
> not require the 'longs' variable, when written as the following?
> 
> 	for (; len >= sizeof(long); len -= sizeof(long), bitmap += sizeof(long)) {
> 		...
> 	}

Definitely better.

> 
> > +		weight = hweight_long(*((unsigned long *)bitmap));
> > +		bitflips += BITS_PER_LONG - weight;
> > +		if (unlikely(bitflips > bitflips_threshold))
> > +			return -EINVAL;
> > +	}
> > +
> > +	len %= sizeof(long);
> 
> If you did the above, then you shouldn't need this line.

Yep.

> 
> > +
> > +	for (; len > 0; len--, bitmap++) {
> > +		weight = hweight8(*bitmap);
> > +		bitflips += BITS_PER_BYTE - weight;
> > +		if (unlikely(bitflips > bitflips_threshold))
> > +			return -EINVAL;
> > +	}
> > +
> > +	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.
> 
> Add similar language from above? Also, it's very important to note that
> this function wipes the buffers in the nearly-erased case!

Yes, I'll add that in the comment, in addition to a lot more precise
description of how to use it and the importance of the ecc and extraoob
fields (even if they are optionals).

Thanks for your review.

Best Regards,

Boris

[1]https://github.com/bbrezillon/linux-sunxi/tree/sunxi/mtd-next
[2]http://patchwork.ozlabs.org/patch/415517/

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com