Patchwork [7/8] mtd: nand_bbt: use string library

login
register
mail settings
Submitter Brian Norris
Date June 22, 2012, 11:35 p.m.
Message ID <1340408145-24531-8-git-send-email-computersforpeace@gmail.com>
Download mbox | patch
Permalink /patch/166686/
State New
Headers show

Comments

Brian Norris - June 22, 2012, 11:35 p.m.
Some nand_bbt code can be shortened by using memcmp() and memchr_inv().
As an added bonus, there is a possbile performance benefit.

Borrowed some code from Akinobu Mita.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
Cc: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/mtd/nand/nand_bbt.c |   27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)
Shmulik Ladkani - June 26, 2012, 1:37 p.m.
Hi Brian,

On Fri, 22 Jun 2012 16:35:44 -0700 Brian Norris <computersforpeace@gmail.com> wrote:
>  static int check_pattern(uint8_t *buf, int len, int paglen, struct nand_bbt_descr *td)
>  {
> -	int i, end = 0;
> +	int end = 0;
>  	uint8_t *p = buf;
>  
>  	if (td->options & NAND_BBT_NO_OOB)
>  		return check_pattern_no_oob(buf, td);
>  
>  	end = paglen + td->offs;
> -	if (td->options & NAND_BBT_SCANEMPTY) {
> -		for (i = 0; i < end; i++) {
> -			if (p[i] != 0xff)
> -				return -1;
> -		}
> -	}
> +	if (td->options & NAND_BBT_SCANEMPTY)
> +		if (memchr_inv(p, 0xff, end))
> +			return -1;
>  	p += end;
>  

A question regarding the original code:
Why the region validated for 0xff is [0 , paglen+td->of) ?
I assume this buffer region contains the inband data. Why verify inband
data is all 0xff?
Shouldn't the region validated be [paglen , paglen+td->of) ?

Regards,
Shmulik
Brian Norris - July 16, 2012, 6:06 a.m.
Add Ivan, as omap2.c is one of the only NAND_BBT_SCANEMPTY users.

On Tue, Jun 26, 2012 at 6:37 AM, Shmulik Ladkani
<shmulik.ladkani@gmail.com> wrote:
> On Fri, 22 Jun 2012 16:35:44 -0700 Brian Norris <computersforpeace@gmail.com> wrote:
>>  static int check_pattern(uint8_t *buf, int len, int paglen, struct nand_bbt_descr *td)
>>  {
>> -     int i, end = 0;
>> +     int end = 0;
>>       uint8_t *p = buf;
>>
>>       if (td->options & NAND_BBT_NO_OOB)
>>               return check_pattern_no_oob(buf, td);
>>
>>       end = paglen + td->offs;
>> -     if (td->options & NAND_BBT_SCANEMPTY) {
>> -             for (i = 0; i < end; i++) {
>> -                     if (p[i] != 0xff)
>> -                             return -1;
>> -             }
>> -     }
>> +     if (td->options & NAND_BBT_SCANEMPTY)
>> +             if (memchr_inv(p, 0xff, end))
>> +                     return -1;
>>       p += end;
>>
>
> A question regarding the original code:

This is another instance of a NAND_BBT_* option that I don't really
understand, but I'll provide my thoughts...

> Why the region validated for 0xff is [0 , paglen+td->of) ?
> I assume this buffer region contains the inband data. Why verify inband
> data is all 0xff?

If used on a flash-based BBT page, then it checks for an empty table
(why would you need this?), and if used for factory-marked bad blocks,
then it checks that all the non-marker locations are 0xff. I guess the
latter is at least more reasonable, but that still doesn't really
answer "why?" So I'm guessing this another ill-conceived option.

> Shouldn't the region validated be [paglen , paglen+td->of) ?

Dunno.

Anyway, it's only used in a single driver (omap2.c) in
"bb_descrip_flashbased" which, despite its name, does not use a
flash-based BBT - also used in "agand_flashbased". So the option is
*only* used for OOB bad block markers, ensuring that the non-marker
positions are 0xff. But I don't see how it's valid to assume that the
data will be 0xff, nor do I know why someone would want to check that.

Finally, I think that some of the "use" of NAND_BBT_SCANEMPTY is
obscured by the fact that omap2.c's main codepath involves
(permanently) clearing NAND_BBT_SCANEMPTY in nand_memory_bbt(). See:
bd->options &= ~NAND_BBT_SCANEMPTY;

So, is this another candidate for removal, as it is practically unused
and unmaintained? Or any comments on its purpose, Ivan?

Brian
Ivan Djelic - July 16, 2012, 11:57 p.m.
On Mon, Jul 16, 2012 at 08:06:31AM +0200, Brian Norris wrote:
> Add Ivan, as omap2.c is one of the only NAND_BBT_SCANEMPTY users.
> 
> On Tue, Jun 26, 2012 at 6:37 AM, Shmulik Ladkani
> <shmulik.ladkani@gmail.com> wrote:
> > On Fri, 22 Jun 2012 16:35:44 -0700 Brian Norris <computersforpeace@gmail.com> wrote:
> >>  static int check_pattern(uint8_t *buf, int len, int paglen, struct nand_bbt_descr *td)
> >>  {
> >> -     int i, end = 0;
> >> +     int end = 0;
> >>       uint8_t *p = buf;
> >>
> >>       if (td->options & NAND_BBT_NO_OOB)
> >>               return check_pattern_no_oob(buf, td);
> >>
> >>       end = paglen + td->offs;
> >> -     if (td->options & NAND_BBT_SCANEMPTY) {
> >> -             for (i = 0; i < end; i++) {
> >> -                     if (p[i] != 0xff)
> >> -                             return -1;
> >> -             }
> >> -     }
> >> +     if (td->options & NAND_BBT_SCANEMPTY)
> >> +             if (memchr_inv(p, 0xff, end))
> >> +                     return -1;
> >>       p += end;
> >>
> >
> > A question regarding the original code:
> 
> This is another instance of a NAND_BBT_* option that I don't really
> understand, but I'll provide my thoughts...
> 
> > Why the region validated for 0xff is [0 , paglen+td->of) ?
> > I assume this buffer region contains the inband data. Why verify inband
> > data is all 0xff?
> 
> If used on a flash-based BBT page, then it checks for an empty table
> (why would you need this?), and if used for factory-marked bad blocks,
> then it checks that all the non-marker locations are 0xff. I guess the
> latter is at least more reasonable, but that still doesn't really
> answer "why?" So I'm guessing this another ill-conceived option.
> 
> > Shouldn't the region validated be [paglen , paglen+td->of) ?
> 
> Dunno.
> 
> Anyway, it's only used in a single driver (omap2.c) in
> "bb_descrip_flashbased" which, despite its name, does not use a
> flash-based BBT - also used in "agand_flashbased". So the option is
> *only* used for OOB bad block markers, ensuring that the non-marker
> positions are 0xff. But I don't see how it's valid to assume that the
> data will be 0xff, nor do I know why someone would want to check that.
> 
> Finally, I think that some of the "use" of NAND_BBT_SCANEMPTY is
> obscured by the fact that omap2.c's main codepath involves
> (permanently) clearing NAND_BBT_SCANEMPTY in nand_memory_bbt(). See:
> bd->options &= ~NAND_BBT_SCANEMPTY;
> 
> So, is this another candidate for removal, as it is practically unused
> and unmaintained? Or any comments on its purpose, Ivan?

Hi Brian,

I haven't really had a detailed look at this bbt structure while writing my ECC patches.
I think it is indeed a candidate for removal.

My guess is that a specific OOB layout compatible with the OMAP ROM ECC was added at some point,
and this legacy bbt structure found its way into the same commit... ?

Since omap2.c also disables bbt scan in omap_nand_probe():
      info->nand.options    |= NAND_SKIP_BBTSCAN;
I guess it does not really make a difference ?

Adding Sukumar Ghorai in CC, as he introduced this in f040d33253b2daf6f305fc35fca2399047ced028:

    omap3: nand: making ecc layout as compatible with romcode ecc
    
    This patch overrides nand ecc layout and bad block descriptor (for 8-bit
    device) to support hw ecc in romcode layout. So as to have in sync with ecc
    layout throughout; i.e. x-loader, u-boot and kernel.
    
    This enables to flash x-loader, u-boot, kernel, FS images from kernel itself
    and compatiable with other tools.
    
    This patch does not enables this feature by default and need to pass from
    board file to enable for any board.

Ghorai, do you remember why option NAND_BBT_SCANEMPTY is used ?
Thanks,

BR,
Artem Bityutskiy - Aug. 15, 2012, 11:53 a.m.
On Fri, 2012-06-22 at 16:35 -0700, Brian Norris wrote:
> Some nand_bbt code can be shortened by using memcmp() and memchr_inv().
> As an added bonus, there is a possbile performance benefit.
> 
> Borrowed some code from Akinobu Mita.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> Cc: Akinobu Mita <akinobu.mita@gmail.com>

Pushed to l2-mtd.git, thanks!

Patch

diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
index ca07230..45cdb97 100644
--- a/drivers/mtd/nand/nand_bbt.c
+++ b/drivers/mtd/nand/nand_bbt.c
@@ -68,6 +68,7 @@ 
 #include <linux/delay.h>
 #include <linux/vmalloc.h>
 #include <linux/export.h>
+#include <linux/string.h>
 
 static int check_pattern_no_oob(uint8_t *buf, struct nand_bbt_descr *td)
 {
@@ -89,19 +90,16 @@  static int check_pattern_no_oob(uint8_t *buf, struct nand_bbt_descr *td)
  */
 static int check_pattern(uint8_t *buf, int len, int paglen, struct nand_bbt_descr *td)
 {
-	int i, end = 0;
+	int end = 0;
 	uint8_t *p = buf;
 
 	if (td->options & NAND_BBT_NO_OOB)
 		return check_pattern_no_oob(buf, td);
 
 	end = paglen + td->offs;
-	if (td->options & NAND_BBT_SCANEMPTY) {
-		for (i = 0; i < end; i++) {
-			if (p[i] != 0xff)
-				return -1;
-		}
-	}
+	if (td->options & NAND_BBT_SCANEMPTY)
+		if (memchr_inv(p, 0xff, end))
+			return -1;
 	p += end;
 
 	/* Compare the pattern */
@@ -111,10 +109,8 @@  static int check_pattern(uint8_t *buf, int len, int paglen, struct nand_bbt_desc
 	if (td->options & NAND_BBT_SCANEMPTY) {
 		p += td->len;
 		end += td->len;
-		for (i = end; i < len; i++) {
-			if (*p++ != 0xff)
-				return -1;
-		}
+		if (memchr_inv(p, 0xff, len - end))
+			return -1;
 	}
 	return 0;
 }
@@ -130,14 +126,9 @@  static int check_pattern(uint8_t *buf, int len, int paglen, struct nand_bbt_desc
  */
 static int check_short_pattern(uint8_t *buf, struct nand_bbt_descr *td)
 {
-	int i;
-	uint8_t *p = buf;
-
 	/* Compare the pattern */
-	for (i = 0; i < td->len; i++) {
-		if (p[td->offs + i] != td->pattern[i])
-			return -1;
-	}
+	if (memcmp(buf + td->offs, td->pattern, td->len))
+		return -1;
 	return 0;
 }