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 <[email protected]> > > --- > > 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
signature.asc
Description: Digital signature
