From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from down.free-electrons.com ([37.187.137.238] helo=mail.free-electrons.com) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1ZXDkI-0004uT-M3 for linux-mtd@lists.infradead.org; Wed, 02 Sep 2015 19:31:00 +0000 Date: Wed, 2 Sep 2015 21:30:34 +0200 From: Boris Brezillon To: Brian Norris Cc: David Woodhouse , linux-mtd@lists.infradead.org, Andrea Scian , Richard Weinberger Subject: Re: [PATCH v2 1/2] mtd: nand: add nand_check_erased helper functions Message-ID: <20150902213034.76d095f0@bbrezillon> In-Reply-To: <20150902184143.GR81844@google.com> References: <1440409642-5495-1-git-send-email-boris.brezillon@free-electrons.com> <1440409642-5495-2-git-send-email-boris.brezillon@free-electrons.com> <20150902184143.GR81844@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Hi, On Wed, 2 Sep 2015 11:41:43 -0700 Brian Norris 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 > > --- > > 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