| Message ID | 1449268528-18154-1-git-send-email-antoine.tenart@free-electrons.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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)? 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? > 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"; > + > + 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)/ ? Also could use a comment either here or above nand_flash_jedec_read_param() as to what the match criteria are. > + 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); } Or even better, convert to a for loop with a max # of iterations. > + if (ret) { > pr_err("Could not find valid JEDEC parameter page; aborting\n"); > return 0; > } Brian
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
On Fri, Dec 11, 2015 at 09:16:20AM +0100, Antoine Tenart wrote: > 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. Well, that's a fair statement to some extent. But generally, it is *much* preferred to code to real practice, rather than theoretical standards. And it's also much preferred not to code in potentially infinite loops. That's why we have timeouts, for instance, on most I/O. > 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. Yes, please put some kind of limit. Probably a low one (after some number of damaged parameter pages, what's the likelihood that there will be any more correct ones?). If you make it a macro with a reasonable name, then it'll be more obvious what it does, in case someone needs to increase it. > > 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. OK. Well, I guess having the code might be better than nothing. I assume you at least faked the parameter pages so you could exercise all the code paths? Brian
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"; + + for (i = 0; i < sizeof(*p); i++) + ((uint8_t *)p)[i] = chip->read_byte(mtd); + + for (i = 0; i < 4; i++) + 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); + if (ret) { pr_err("Could not find valid JEDEC parameter page; aborting\n"); return 0; }
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. Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com> --- drivers/mtd/nand/nand_base.c | 44 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 10 deletions(-)