Re: [PATCH] mtd: nand: support JEDEC additional redundant parameter pages

[Antoine Tenart <antoine.tenart@free-electrons.com>]

[-- Attachment #1: Type: text/plain, Size: 5835 bytes --]

Brian,

On Thu, Dec 10, 2015 at 12:20:52PM -0800, Brian Norris wrote:
> On Fri, Dec 04, 2015 at 11:35:28PM +0100, Antoine Tenart wrote:
> > The JEDEC standard defines the JEDEC parameter page data structure.
> > One page plus two redundant pages are always there, in bits 0-1535.
> > Additionnal redundant parameter pages can be stored at bits 1536+.
> > Add support to read these pages.
> > 
> > The first 3 JEDEC parameter pages are always checked, and if none
> > is valid we try to read additional redundant pages following the
> > standard definition: we continue while at least two of the four bytes
> > of the parameter page signature match (stored in the first dword).
> > 
> > There is no limit to the number of additional redundant parameter
> > page.
> 
> Hmm, do we really want this to be unbounded? What if (for example) a
> driver is buggy and has some kind of wraparound, so that it keeps
> returning the same parameter page (or a sequence of a few pages)?

I would say buggy drivers need to be fixed. It's complicated to handle
all possible bugs a driver may have in the common code.

If you prefer we can put a limit to the tries the code make, but this
can also impact working drivers by not trying enough. I'm open to
suggestions.

> Also, is this actually solving any real problem? Have you seen flash
> that have more than the first 3 parameter pages? Have you tested
> this beyond the first 3?

This does not solve any real world problem I had. I had to look at the
JEDEC standard and I made this in order to test something. So I thought
this could be useful to others, as the current code does not fully
implement the standard.

> > Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> > ---
> >  drivers/mtd/nand/nand_base.c | 44 ++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 34 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > index cc74142938b0..31f4a6585703 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > @@ -3392,6 +3392,32 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
> >  	return 1;
> >  }
> >  
> > +static int nand_flash_jedec_read_param(struct mtd_info *mtd,
> > +				       struct nand_chip *chip,
> > +				       struct nand_jedec_params *p)
> > +{
> > +	int i, match = 0;
> > +	char sig[4] = "JESD";
> 
> sparse likes to complain about this:
> 
> drivers/mtd/nand/nand_base.c:3400:23: warning: too long initializer-string for array of char(no space for nul char) [sparse]
> 
> I'm not sure it has a real effect (though I haven't checked the C spec
> for what happens here), because we're not really using it like a
> 0-terminated string, but perhaps we can do something small to squash it?
> e.g., don't specify the [4], and just do this?
> 
> 	char sig[] = "JESD";

Sure.

> > +
> > +	for (i = 0; i < sizeof(*p); i++)
> > +		((uint8_t *)p)[i] = chip->read_byte(mtd);
> > +
> > +	for (i = 0; i < 4; i++)
> 
> Maybe s/4/ARRAY_SIZE(p->sig)/ ?

Yes, that's better.

> Also could use a comment either here or above
> nand_flash_jedec_read_param() as to what the match criteria are.

Good idea.

> > +		if (p->sig[i] == sig[i])
> > +			match++;
> > +
> > +	if (match < 2) {
> > +		pr_warn("Invalid JEDEC page\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 510) ==
> > +			le16_to_cpu(p->crc))
> > +		return 0;
> > +
> > +	return -EAGAIN;
> > +}
> > +
> >  /*
> >   * Check if the NAND chip is JEDEC compliant, returns 1 if it is, 0 otherwise.
> >   */
> > @@ -3400,8 +3426,7 @@ static int nand_flash_detect_jedec(struct mtd_info *mtd, struct nand_chip *chip,
> >  {
> >  	struct nand_jedec_params *p = &chip->jedec_params;
> >  	struct jedec_ecc_info *ecc;
> > -	int val;
> > -	int i, j;
> > +	int val, i, ret = 0;
> >  
> >  	/* Try JEDEC for unknown chip or LP */
> >  	chip->cmdfunc(mtd, NAND_CMD_READID, 0x40, -1);
> > @@ -3411,16 +3436,15 @@ static int nand_flash_detect_jedec(struct mtd_info *mtd, struct nand_chip *chip,
> >  		return 0;
> >  
> >  	chip->cmdfunc(mtd, NAND_CMD_PARAM, 0x40, -1);
> > -	for (i = 0; i < 3; i++) {
> > -		for (j = 0; j < sizeof(*p); j++)
> > -			((uint8_t *)p)[j] = chip->read_byte(mtd);
> > -
> > -		if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 510) ==
> > -				le16_to_cpu(p->crc))
> > +	for (i = 0; i < 3; i++)
> > +		if (!nand_flash_jedec_read_param(mtd, chip, p))
> >  			break;
> > -	}
> >  
> > -	if (i == 3) {
> > +	/* Try reading additional parameter pages */
> > +	if (i == 3)
> > +		while ((ret = nand_flash_jedec_read_param(mtd, chip, p)) ==
> > +				-EAGAIN);
> 
> This loop has a few problems aesthetically and functionally. As
> mentioned before, the unbounded loop is not very nice. I would suggest
> at least putting some kind of bound to it. Also, it's probably best not
> to try so hard to cram everything into one "line". And for a rare
> change, I agree with checkpatch.pl:
> 
> ERROR:TRAILING_STATEMENTS: trailing statements should be on next line
> #89: FILE: drivers/mtd/nand/nand_base.c:3445:
> +               while ((ret = nand_flash_jedec_read_param(mtd, chip, p)) ==
> +                               -EAGAIN);
> 
> In this case, I think it's saying the empty statement (;) should be on a new
> line.
> 
> So, it could more clearly be something like:
> 
> 	if (i == 3) {
> 		do {
> 			ret = nand_flash_jedec_read_param(mtd, chip, p);
> 		} while (ret == -EAGAIN);
> 	}

I agree, this is easier to read.

Thanks for the review!

Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]