| Message ID | 1504001833-18097-2-git-send-email-LW@KARO-electronics.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series | mtd: nand: fix regression introduced by splitting off manufacturer dependent code | expand |
Hi Lothar, On Tue, 29 Aug 2017 12:17:12 +0200 Lothar Waßmann <LW@KARO-electronics.de> wrote: > commit c51d0ac59f24 ("mtd: nand: Move Samsung specific init/detection > logic in nand_samsung.c") introduced a regression for Samsung SLC NAND > chips. Prior to this commit chip->bits_per_cell was initialized by calling > nand_get_bits_per_cell() before using nand_is_slc(). > With the offending commit this call is skipped, leaving > chip->bits_per_cell cleared to zero when the manufacturer specific > '.detect' function calls nand_is_slc() which in turn interprets > bits_per_cell != 1 as indication for an MLC chip. > The effect is that e.g. a K9F1G08U0F NAND chip is falsely detected as > MLC NAND with 4KiB page size rather than SLC with 2KiB page size. Oops, sorry for this regression. > > Add a call to nand_get_bits_per_cell() before calling the .detect hook > function in nand_manufacturer_detect(), so that the nand_is_slc() > calls in the manufacturer specific code will return correct results. I'd prefer a different solution (see below). > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > --- > drivers/mtd/nand/nand_base.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > index 9900476..bcc8cef1 100644 > --- a/drivers/mtd/nand/nand_base.c > +++ b/drivers/mtd/nand/nand_base.c > @@ -3820,10 +3820,13 @@ static void nand_manufacturer_detect(struct nand_chip *chip) > * nand_decode_ext_id() otherwise. > */ > if (chip->manufacturer.desc && chip->manufacturer.desc->ops && > - chip->manufacturer.desc->ops->detect) > + chip->manufacturer.desc->ops->detect) { > + /* The 3rd id byte holds MLC / multichip data */ > + chip->bits_per_cell = nand_get_bits_per_cell(chip->id.data[2]); I'd prefer not to force this bit_per_cell detection here. How about explicitly calling nand_decode_ext_id() from the samsung and hynix ->detect() hooks (see proposed diff below)? Regards, Boris > chip->manufacturer.desc->ops->detect(chip); > - else > + } else { > nand_decode_ext_id(chip); > + } > } > > /* --->8--- diff --git a/drivers/mtd/nand/nand_hynix.c b/drivers/mtd/nand/nand_hynix.c index b12dc7325378..d3d41ebc2164 100644 --- a/drivers/mtd/nand/nand_hynix.c +++ b/drivers/mtd/nand/nand_hynix.c @@ -547,6 +547,8 @@ static void hynix_nand_decode_id(struct nand_chip *chip) bool valid_jedecid; u8 tmp; + nand_decode_ext_id(chip); + /* * Exclude all SLC NANDs from this advanced detection scheme. * According to the ranges defined in several datasheets, it might @@ -554,10 +556,8 @@ static void hynix_nand_decode_id(struct nand_chip *chip) * If that the case rework the test to let SLC NANDs go through the * detection process. */ - if (chip->id.len < 6 || nand_is_slc(chip)) { - nand_decode_ext_id(chip); + if (chip->id.len < 6 || nand_is_slc(chip)) return; - } /* Extract pagesize */ mtd->writesize = 2048 << (chip->id.data[3] & 0x03); diff --git a/drivers/mtd/nand/nand_samsung.c b/drivers/mtd/nand/nand_samsung.c index 1e0755997762..b009b47ac552 100644 --- a/drivers/mtd/nand/nand_samsung.c +++ b/drivers/mtd/nand/nand_samsung.c @@ -21,6 +21,8 @@ static void samsung_nand_decode_id(struct nand_chip *chip) { struct mtd_info *mtd = nand_to_mtd(chip); + nand_decode_ext_id(chip); + /* New Samsung (6 byte ID): Samsung K9GAG08U0F (p.44) */ if (chip->id.len == 6 && !nand_is_slc(chip) && chip->id.data[5] != 0x00) { @@ -89,8 +91,6 @@ static void samsung_nand_decode_id(struct nand_chip *chip) chip->ecc_step_ds = 0; } } - } else { - nand_decode_ext_id(chip); } }
Hi, On Tue, 29 Aug 2017 14:16:58 +0200 Boris Brezillon wrote: > Hi Lothar, > > On Tue, 29 Aug 2017 12:17:12 +0200 > Lothar Waßmann <LW@KARO-electronics.de> wrote: > > > commit c51d0ac59f24 ("mtd: nand: Move Samsung specific init/detection > > logic in nand_samsung.c") introduced a regression for Samsung SLC NAND > > chips. Prior to this commit chip->bits_per_cell was initialized by calling > > nand_get_bits_per_cell() before using nand_is_slc(). > > With the offending commit this call is skipped, leaving > > chip->bits_per_cell cleared to zero when the manufacturer specific > > '.detect' function calls nand_is_slc() which in turn interprets > > bits_per_cell != 1 as indication for an MLC chip. > > The effect is that e.g. a K9F1G08U0F NAND chip is falsely detected as > > MLC NAND with 4KiB page size rather than SLC with 2KiB page size. > > Oops, sorry for this regression. > > > > > Add a call to nand_get_bits_per_cell() before calling the .detect hook > > function in nand_manufacturer_detect(), so that the nand_is_slc() > > calls in the manufacturer specific code will return correct results. > > I'd prefer a different solution (see below). > > > > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > > --- > > drivers/mtd/nand/nand_base.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > > index 9900476..bcc8cef1 100644 > > --- a/drivers/mtd/nand/nand_base.c > > +++ b/drivers/mtd/nand/nand_base.c > > @@ -3820,10 +3820,13 @@ static void nand_manufacturer_detect(struct nand_chip *chip) > > * nand_decode_ext_id() otherwise. > > */ > > if (chip->manufacturer.desc && chip->manufacturer.desc->ops && > > - chip->manufacturer.desc->ops->detect) > > + chip->manufacturer.desc->ops->detect) { > > + /* The 3rd id byte holds MLC / multichip data */ > > + chip->bits_per_cell = nand_get_bits_per_cell(chip->id.data[2]); > > I'd prefer not to force this bit_per_cell detection here. How about > explicitly calling nand_decode_ext_id() from the samsung and hynix > ->detect() hooks (see proposed diff below)? > I chose the same place in the code flow where this initialization had been before. And it does only that portion of nand_decode_ext_id() that was executed prior to the vendor specific code in the old code. A call to nand_decode_ext_id() would do more than has been done previously. I prefer not to have to rely on every single manufacturer dependent code calling this function on its own. But you are the maintainer and have to decide finally. With my second patch it should be easy to spot when the call is missing though. Another alternative were to let nand_is_slc() do the initialization from id_data when it is first called (bits_per_cell == 0). Lothar Waßmann
On Tue, 29 Aug 2017 15:18:07 +0200 Lothar Waßmann <LW@KARO-electronics.de> wrote: > Hi, > > On Tue, 29 Aug 2017 14:16:58 +0200 Boris Brezillon wrote: > > Hi Lothar, > > > > On Tue, 29 Aug 2017 12:17:12 +0200 > > Lothar Waßmann <LW@KARO-electronics.de> wrote: > > > > > commit c51d0ac59f24 ("mtd: nand: Move Samsung specific init/detection > > > logic in nand_samsung.c") introduced a regression for Samsung SLC NAND > > > chips. Prior to this commit chip->bits_per_cell was initialized by calling > > > nand_get_bits_per_cell() before using nand_is_slc(). > > > With the offending commit this call is skipped, leaving > > > chip->bits_per_cell cleared to zero when the manufacturer specific > > > '.detect' function calls nand_is_slc() which in turn interprets > > > bits_per_cell != 1 as indication for an MLC chip. > > > The effect is that e.g. a K9F1G08U0F NAND chip is falsely detected as > > > MLC NAND with 4KiB page size rather than SLC with 2KiB page size. > > > > Oops, sorry for this regression. > > > > > > > > Add a call to nand_get_bits_per_cell() before calling the .detect hook > > > function in nand_manufacturer_detect(), so that the nand_is_slc() > > > calls in the manufacturer specific code will return correct results. > > > > I'd prefer a different solution (see below). > > > > > > > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > > > --- > > > drivers/mtd/nand/nand_base.c | 7 +++++-- > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > > > index 9900476..bcc8cef1 100644 > > > --- a/drivers/mtd/nand/nand_base.c > > > +++ b/drivers/mtd/nand/nand_base.c > > > @@ -3820,10 +3820,13 @@ static void nand_manufacturer_detect(struct nand_chip *chip) > > > * nand_decode_ext_id() otherwise. > > > */ > > > if (chip->manufacturer.desc && chip->manufacturer.desc->ops && > > > - chip->manufacturer.desc->ops->detect) > > > + chip->manufacturer.desc->ops->detect) { > > > + /* The 3rd id byte holds MLC / multichip data */ > > > + chip->bits_per_cell = nand_get_bits_per_cell(chip->id.data[2]); > > > > I'd prefer not to force this bit_per_cell detection here. How about > > explicitly calling nand_decode_ext_id() from the samsung and hynix > > ->detect() hooks (see proposed diff below)? > > > I chose the same place in the code flow where this initialization had > been before. And it does only that portion of nand_decode_ext_id() that > was executed prior to the vendor specific code in the old code. > A call to nand_decode_ext_id() would do more than has been done > previously. My main concern is, can we be sure this portion of the 3rd byte is always used to encode the bits-per-cell information? NAND vendors tend to take liberties with the NAND ids fields and I fear we'll someday have a NAND that does not follow this encoding scheme. This being said, find_full_id_nand() also calls nand_get_bits_per_cell() even though it's using full-id information for other characteristics, which tend to confirm noone ever had a NAND abusing this bits-per-cell field. I'll just take your patch as is and add Cc-stable a Fixes tags. Note that I'm planning to rework the NAND detection logic a bit to let manufacturer code tweak the characteristics even if the NAND is ONFI or JEDEC compliant (see below). > > I prefer not to have to rely on every single manufacturer dependent > code calling this function on its own. But you are the maintainer and > have to decide finally. > With my second patch it should be easy to spot when the call is missing > though. Yep, your second patch is fine. > > Another alternative were to let nand_is_slc() do the initialization > from id_data when it is first called (bits_per_cell == 0). Well, you could do return bits_per_cell <= 1 but I think your WARN() is appropriate (though I'd put it somewhere else, like just after the detection logic). --->8--- diff --git a/drivers/mtd/nand/nand_amd.c b/drivers/mtd/nand/nand_amd.c index 170403a3bfa8..95342862fd31 100644 --- a/drivers/mtd/nand/nand_amd.c +++ b/drivers/mtd/nand/nand_amd.c @@ -21,7 +21,9 @@ static void amd_nand_decode_id(struct nand_chip *chip) { struct mtd_info *mtd = nand_to_mtd(chip); - nand_decode_ext_id(chip); + /* Skip ID decoding for JEDEC or ONFI chips. */ + if (chip->onfi_version || chip->jedec_version) + return; /* * Check for Spansion/AMD ID + repeating 5th, 6th byte since diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index c6c18b82f8f4..0640687b50c2 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -3982,9 +3982,9 @@ static bool find_full_id_nand(struct nand_chip *chip, } /* - * Manufacturer detection. Only used when the NAND is not ONFI or JEDEC - * compliant and does not have a full-id or legacy-id entry in the nand_ids - * table. + * Manufacturer detection. This follows the regular ONFI, JEDEC or ID decoding + * detection and allows driver specific code to tweak the NAND chip + * characteristics. */ static void nand_manufacturer_detect(struct nand_chip *chip) { @@ -3995,8 +3995,6 @@ static void nand_manufacturer_detect(struct nand_chip *chip) if (chip->manufacturer.desc && chip->manufacturer.desc->ops && chip->manufacturer.desc->ops->detect) chip->manufacturer.desc->ops->detect(chip); - else - nand_decode_ext_id(chip); } /* @@ -4128,7 +4126,7 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type) chip->chipsize = (uint64_t)type->chipsize << 20; if (!type->pagesize) - nand_manufacturer_detect(chip); + nand_decode_ext_id(chip) else nand_decode_id(chip, type); @@ -4136,6 +4134,7 @@ static int nand_detect(struct nand_chip *chip, struct nand_flash_dev *type) chip->options |= type->options; ident_done: + nand_manufacturer_detect(chip); if (chip->options & NAND_BUSWIDTH_AUTO) { WARN_ON(busw & NAND_BUSWIDTH_16); diff --git a/drivers/mtd/nand/nand_hynix.c b/drivers/mtd/nand/nand_hynix.c index b12dc7325378..b77b45c4ed03 100644 --- a/drivers/mtd/nand/nand_hynix.c +++ b/drivers/mtd/nand/nand_hynix.c @@ -547,6 +547,10 @@ static void hynix_nand_decode_id(struct nand_chip *chip) bool valid_jedecid; u8 tmp; + /* Skip ID decoding for JEDEC or ONFI chips. */ + if (chip->onfi_version || chip->jedec_version) + return; + /* * Exclude all SLC NANDs from this advanced detection scheme. * According to the ranges defined in several datasheets, it might @@ -554,10 +558,8 @@ static void hynix_nand_decode_id(struct nand_chip *chip) * If that the case rework the test to let SLC NANDs go through the * detection process. */ - if (chip->id.len < 6 || nand_is_slc(chip)) { - nand_decode_ext_id(chip); + if (chip->id.len < 6 || nand_is_slc(chip)) return; - } /* Extract pagesize */ mtd->writesize = 2048 << (chip->id.data[3] & 0x03); diff --git a/drivers/mtd/nand/nand_samsung.c b/drivers/mtd/nand/nand_samsung.c index 1e0755997762..9da0a2101d63 100644 --- a/drivers/mtd/nand/nand_samsung.c +++ b/drivers/mtd/nand/nand_samsung.c @@ -21,6 +21,10 @@ static void samsung_nand_decode_id(struct nand_chip *chip) { struct mtd_info *mtd = nand_to_mtd(chip); + /* Skip ID decoding for JEDEC or ONFI chips. */ + if (chip->onfi_version || chip->jedec_version) + return; + /* New Samsung (6 byte ID): Samsung K9GAG08U0F (p.44) */ if (chip->id.len == 6 && !nand_is_slc(chip) && chip->id.data[5] != 0x00) { @@ -89,8 +93,6 @@ static void samsung_nand_decode_id(struct nand_chip *chip) chip->ecc_step_ds = 0; } } - } else { - nand_decode_ext_id(chip); } } diff --git a/drivers/mtd/nand/nand_toshiba.c b/drivers/mtd/nand/nand_toshiba.c index fa787ba38dcd..1aa56363bc16 100644 --- a/drivers/mtd/nand/nand_toshiba.c +++ b/drivers/mtd/nand/nand_toshiba.c @@ -21,7 +21,9 @@ static void toshiba_nand_decode_id(struct nand_chip *chip) { struct mtd_info *mtd = nand_to_mtd(chip); - nand_decode_ext_id(chip); + /* Skip ID decoding for JEDEC or ONFI chips. */ + if (chip->onfi_version || chip->jedec_version) + return; /* * Toshiba 24nm raw SLC (i.e., not BENAND) have 32B OOB per
Hi, On Tue, 29 Aug 2017 15:41:44 +0200 Boris Brezillon wrote: > On Tue, 29 Aug 2017 15:18:07 +0200 > Lothar Waßmann <LW@KARO-electronics.de> wrote: > > > Hi, > > > > On Tue, 29 Aug 2017 14:16:58 +0200 Boris Brezillon wrote: > > > Hi Lothar, > > > > > > On Tue, 29 Aug 2017 12:17:12 +0200 > > > Lothar Waßmann <LW@KARO-electronics.de> wrote: > > > > > > > commit c51d0ac59f24 ("mtd: nand: Move Samsung specific init/detection > > > > logic in nand_samsung.c") introduced a regression for Samsung SLC NAND > > > > chips. Prior to this commit chip->bits_per_cell was initialized by calling > > > > nand_get_bits_per_cell() before using nand_is_slc(). > > > > With the offending commit this call is skipped, leaving > > > > chip->bits_per_cell cleared to zero when the manufacturer specific > > > > '.detect' function calls nand_is_slc() which in turn interprets > > > > bits_per_cell != 1 as indication for an MLC chip. > > > > The effect is that e.g. a K9F1G08U0F NAND chip is falsely detected as > > > > MLC NAND with 4KiB page size rather than SLC with 2KiB page size. > > > > > > Oops, sorry for this regression. > > > > > > > > > > > Add a call to nand_get_bits_per_cell() before calling the .detect hook > > > > function in nand_manufacturer_detect(), so that the nand_is_slc() > > > > calls in the manufacturer specific code will return correct results. > > > > > > I'd prefer a different solution (see below). > > > > > > > > > > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > > > > --- > > > > drivers/mtd/nand/nand_base.c | 7 +++++-- > > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > > > > index 9900476..bcc8cef1 100644 > > > > --- a/drivers/mtd/nand/nand_base.c > > > > +++ b/drivers/mtd/nand/nand_base.c > > > > @@ -3820,10 +3820,13 @@ static void nand_manufacturer_detect(struct nand_chip *chip) > > > > * nand_decode_ext_id() otherwise. > > > > */ > > > > if (chip->manufacturer.desc && chip->manufacturer.desc->ops && > > > > - chip->manufacturer.desc->ops->detect) > > > > + chip->manufacturer.desc->ops->detect) { > > > > + /* The 3rd id byte holds MLC / multichip data */ > > > > + chip->bits_per_cell = nand_get_bits_per_cell(chip->id.data[2]); > > > > > > I'd prefer not to force this bit_per_cell detection here. How about > > > explicitly calling nand_decode_ext_id() from the samsung and hynix > > > ->detect() hooks (see proposed diff below)? > > > > > I chose the same place in the code flow where this initialization had > > been before. And it does only that portion of nand_decode_ext_id() that > > was executed prior to the vendor specific code in the old code. > > A call to nand_decode_ext_id() would do more than has been done > > previously. > > My main concern is, can we be sure this portion of the 3rd byte is > always used to encode the bits-per-cell information? NAND vendors tend > to take liberties with the NAND ids fields and I fear we'll someday have > a NAND that does not follow this encoding scheme. > This being said, find_full_id_nand() also calls > nand_get_bits_per_cell() even though it's using full-id information for > other characteristics, which tend to confirm noone ever had a NAND > abusing this bits-per-cell field. > > I'll just take your patch as is and add Cc-stable a Fixes tags. > OK. > Note that I'm planning to rework the NAND detection logic a bit to let > manufacturer code tweak the characteristics even if the NAND is ONFI or > JEDEC compliant (see below). > > > > > I prefer not to have to rely on every single manufacturer dependent > > code calling this function on its own. But you are the maintainer and > > have to decide finally. > > With my second patch it should be easy to spot when the call is missing > > though. > > Yep, your second patch is fine. > > > > > Another alternative were to let nand_is_slc() do the initialization > > from id_data when it is first called (bits_per_cell == 0). > > Well, you could do > > return bits_per_cell <= 1 > That would just cover up the fact, that noone cared for correctly initializing the bits_per_cell field. > but I think your WARN() is appropriate (though I'd put it somewhere > else, like just after the detection logic). > IMO it is in exactly the right place, where the variable in question is to be used. Lothar Waßmann
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index 9900476..bcc8cef1 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -3820,10 +3820,13 @@ static void nand_manufacturer_detect(struct nand_chip *chip) * nand_decode_ext_id() otherwise. */ if (chip->manufacturer.desc && chip->manufacturer.desc->ops && - chip->manufacturer.desc->ops->detect) + chip->manufacturer.desc->ops->detect) { + /* The 3rd id byte holds MLC / multichip data */ + chip->bits_per_cell = nand_get_bits_per_cell(chip->id.data[2]); chip->manufacturer.desc->ops->detect(chip); - else + } else { nand_decode_ext_id(chip); + } } /*
commit c51d0ac59f24 ("mtd: nand: Move Samsung specific init/detection logic in nand_samsung.c") introduced a regression for Samsung SLC NAND chips. Prior to this commit chip->bits_per_cell was initialized by calling nand_get_bits_per_cell() before using nand_is_slc(). With the offending commit this call is skipped, leaving chip->bits_per_cell cleared to zero when the manufacturer specific '.detect' function calls nand_is_slc() which in turn interprets bits_per_cell != 1 as indication for an MLC chip. The effect is that e.g. a K9F1G08U0F NAND chip is falsely detected as MLC NAND with 4KiB page size rather than SLC with 2KiB page size. Add a call to nand_get_bits_per_cell() before calling the .detect hook function in nand_manufacturer_detect(), so that the nand_is_slc() calls in the manufacturer specific code will return correct results. Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> --- drivers/mtd/nand/nand_base.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)