[PATCH 0/2] mtd: nand: fix regression introduced by splitting off manufacturer dependent code

[PATCH 0/2] mtd: nand: fix regression introduced by splitting off manufacturer dependent code

[Lothar Waßmann]

commit c51d0ac59f24 ("mtd: nand: Move Samsung specific init/detection
logic in nand_samsung.c") introduced a regression for Samsung SLC NAND
chips by skipping the initialization of chip->bits_per_cell that is
done in nand_decode_ext_id() from which the manufacturer dependent
code was extracted.
The regression should also affect Hynix and Macronix chips whose code
was separated out in further commits but which I cannot test.
AMD/Spansion and Toshiba NAND are not affected, since they are calling 
nand_decode_ext_id() (which initializes bhip->bits_per_cell) in their
.detect function.

Fix the regression and add a warning to nand_is_slc() to prevent
further regressions of this kind.

[PATCH 1/2] mtd: nand: make Samsung SLC NAND usable again

[Lothar Waßmann]

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(-)

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);
+	}
 }
 
 /*
-- 
2.1.4

[PATCH 2/2] mtd: nand: complain loudly when chip->bits_per_cell is not correctly initialized

[Lothar Waßmann]

chip->bits_per_cell which is used to determine the NAND cell type
(SLC/MLC) should always have a value != 0.
Complain loudly if the value is 0 in nand_is_slc() to catch use before
correct initialization.

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
---
 include/linux/mtd/rawnand.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
index 94feece..2b05f42 100644
--- a/include/linux/mtd/rawnand.h
+++ b/include/linux/mtd/rawnand.h
@@ -1234,6 +1234,8 @@ int onfi_init_data_interface(struct nand_chip *chip,
  */
 static inline bool nand_is_slc(struct nand_chip *chip)
 {
+	WARN(chip->bits_per_cell == 0,
+	     "chip->bits_per_cell is used uninitialized\n");
 	return chip->bits_per_cell == 1;
 }
 
-- 
2.1.4

Re: [PATCH 1/2] mtd: nand: make Samsung SLC NAND usable again

[Boris Brezillon]

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);
 	}
 }

Re: [PATCH 1/2] mtd: nand: make Samsung SLC NAND usable again

[Lothar Waßmann]

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

Re: [PATCH 1/2] mtd: nand: make Samsung SLC NAND usable again

[Boris Brezillon]

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

Re: [PATCH 1/2] mtd: nand: make Samsung SLC NAND usable again

[Lothar Waßmann]

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

Re: [PATCH 0/2] mtd: nand: fix regression introduced by splitting off manufacturer dependent code

[Boris Brezillon]

On Tue, 29 Aug 2017 12:17:11 +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 by skipping the initialization of chip->bits_per_cell that is
> done in nand_decode_ext_id() from which the manufacturer dependent
> code was extracted.
> The regression should also affect Hynix and Macronix chips whose code
> was separated out in further commits but which I cannot test.
> AMD/Spansion and Toshiba NAND are not affected, since they are calling 
> nand_decode_ext_id() (which initializes bhip->bits_per_cell) in their
> .detect function.
> 
> Fix the regression and add a warning to nand_is_slc() to prevent
> further regressions of this kind.

Applied both to nand/next and generated a new PR to push this in 4.14.
Also added the Fixes and Cc-stable tag to the first patch.

Thanks,

Boris