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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
> > > > ---
> > > >  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