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

["Lothar Waßmann" <LW@KARO-electronics.de>]

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