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.ten...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
