Message-ID: <20170829154144.25f9d0ac@bbrezillon>
Date:   Tue, 29 Aug 2017 15:41:44 +0200
From:   Boris Brezillon <boris.brezillon@...e-electrons.com>
To:     Lothar Waßmann <LW@...O-electronics.de>
Cc:     Brian Norris <computersforpeace@...il.com>,
        Cyrille Pitchen <cyrille.pitchen@...ev4u.fr>,
        David Woodhouse <dwmw2@...radead.org>,
        Marek Vasut <marek.vasut@...il.com>,
        Richard Weinberger <richard@....at>,
        linux-kernel@...r.kernel.org, linux-mtd@...ts.infradead.org
Subject: Re: [PATCH 1/2] mtd: nand: make Samsung SLC NAND usable again

On Tue, 29 Aug 2017 15:18:07 +0200
Lothar Waßmann <LW@...O-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@...O-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@...O-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

Powered by blists - more mailing lists