Re: [PATCH v3] MTD: NAND: add OOB layout without ECC bytes

[Boris Brezillon]

Le Sat, 19 Aug 2017 11:41:59 +0200,
Miquel RAYNAL <miquel.raynal@xxxxxxxxxxxxxxxxxx> a Ãcrit :

> > 
> > Le Wed, 16 Aug 2017 09:39:06 +0200,
> > Miquel Raynal <miquel.raynal@xxxxxxxxxxxxxxxxxx> a Ãcrit :
> >   
> > > Add layout functions for small and large pages with mainly free
> > > bytes plus reserved space for Bad Block Markers. This may be useful
> > > for specific chip that use on-die ECC feature, ie. the TOSHIBA
> > > BENAND (Built-in ECC NAND). The ECC bytes are stored in an isolated
> > > area inaccessible by the driver and thus the OOB layout must only
> > > reserve space for Bad Block Markers.    
> > 
> > Yes, that's true, but as said below, I expect the toshiba driver to
> > explicitly set this layout.  
> 
> I will change:
> -for specific chips that use on-die ECC feature, ie. the [...].
> +for configurations that does set ECC_NONE. Specific chips that use
> on-die ECC feature, ie. the [...], might also take advantage of it.

Sounds better.

> 
> >   
> > > 
> > > Bad Block Markers position was extracted from the existing OOB
> > > layouts by assigning as free all the bytes marked as ECC.
> > > 
> > > The layout functions added by this patch were supposed to be used
> > > with ECC_NONE mode but it would break existing drivers that do not
> > > rely on an implementation of this layout by the core, so set it as
> > > a fallthrough when no other layout matches.    
> > 
> > Well, it's still supposed to be used when ECC_NONE is set, except we
> > can only use it for cases that were not already falling in the
> > supported large/small page with ECC cases.  
> 
> I'll edit the commit message, what about:
> 
> The layout functions added by this patch intend to be used when ECC_NONE
> is set, except it is only usable for cases that were not already falling
> in the supported large/small page with ECC cases. Not breaking
> existing drivers needs these functions to be set as a fallthrough when
> no other layout matches and ECC_NONE is actually set.

Hm, still a bit hard to follow. How about:

"
Normally, if NAND_ECC_NONE is chosen the MTD user can use the whole OOB
area (except the bytes reserved for the bad block markers) to store
private data, but the NAND framework has been using the same OOB layout
(the one reserving ECC bytes) no matter whether ECC is enabled or not.
In order to not break existing MTD users which may rely on this layout
to work properly, we keep things unchanged and only use the 'no_ecc'
layouts for previously unsupported configurations.
"

> 
> >   
> > > 
> > > Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxxxxxxxxx>
> > > ---
> > >  drivers/mtd/nand/nand_base.c | 65
> > > ++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 60
> > > insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/nand/nand_base.c
> > > b/drivers/mtd/nand/nand_base.c index c5221795a1e8..7c0ae674e50a
> > > 100644 --- a/drivers/mtd/nand/nand_base.c
> > > +++ b/drivers/mtd/nand/nand_base.c
> > > @@ -53,7 +53,38 @@ static int nand_get_device(struct mtd_info *mtd,
> > > int new_state); static int nand_do_write_oob(struct mtd_info *mtd,
> > > loff_t to, struct mtd_oob_ops *ops);
> > >  
> > > -/* Define default oob placement schemes for large and small page
> > > devices */ +/*
> > > + * Define default OOB placement schemes for:
> > > + *   - no ECC or software ECC
> > > + *   - small or large page devices
> > > + */
> > > +static int nand_ooblayout_free_sp_no_ecc(struct mtd_info *mtd, int
> > > section,
> > > +					   struct mtd_oob_region
> > > *oobregion) +{
> > > +	if (section > 1)
> > > +		return -ERANGE;
> > > +
> > > +	if (!section) {
> > > +		if (mtd->oobsize == 16) {
> > > +			oobregion->offset = 0;
> > > +			oobregion->length = 4;
> > > +		} else {
> > > +			oobregion->offset = 0;
> > > +			oobregion->length = 5;
> > > +		}
> > > +	} else {
> > > +		oobregion->offset = 6;
> > > +		oobregion->length = mtd->oobsize -
> > > oobregion->offset;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +const struct mtd_ooblayout_ops nand_ooblayout_sp_no_ecc_ops = {
> > > +	.free = nand_ooblayout_free_sp_no_ecc,
> > > +};
> > > +EXPORT_SYMBOL_GPL(nand_ooblayout_sp_no_ecc_ops);
> > > +
> > >  static int nand_ooblayout_ecc_sp(struct mtd_info *mtd, int section,
> > >  				 struct mtd_oob_region *oobregion)
> > >  {
> > > @@ -109,6 +140,23 @@ const struct mtd_ooblayout_ops
> > > nand_ooblayout_sp_ops = { };
> > >  EXPORT_SYMBOL_GPL(nand_ooblayout_sp_ops);
> > >  
> > > +static int nand_ooblayout_free_lp_no_ecc(struct mtd_info *mtd, int
> > > section,
> > > +					   struct mtd_oob_region
> > > *oobregion) +{
> > > +	if (section)
> > > +		return -ERANGE;
> > > +
> > > +	oobregion->offset = 2;
> > > +	oobregion->length = mtd->oobsize - oobregion->offset;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +const struct mtd_ooblayout_ops nand_ooblayout_lp_no_ecc_ops = {
> > > +	.free = nand_ooblayout_free_lp_no_ecc,
> > > +};
> > > +EXPORT_SYMBOL_GPL(nand_ooblayout_lp_no_ecc_ops);    
> > 
> > No need to export those symbols until someone really needs them.
> > BTW, they are exported but not declared in nand.h, so right now
> > nobody can use them.  
> 
> I though the EXPORT_* was expected as it was below every layout
> function. I'll remove both of the exports, but as you said, while
> nobody use them, I won't add the prototypes in nand.h neither, right ?

Right.

> 
> >   
> > > +
> > >  static int nand_ooblayout_ecc_lp(struct mtd_info *mtd, int section,
> > >  				 struct mtd_oob_region *oobregion)
> > >  {
> > > @@ -4649,10 +4697,17 @@ int nand_scan_tail(struct mtd_info *mtd)
> > >  			mtd_set_ooblayout(mtd,
> > > &nand_ooblayout_lp_hamming_ops); break;
> > >  		default:
> > > -			WARN(1, "No oob scheme defined for oobsize
> > > %d\n",
> > > -				mtd->oobsize);
> > > -			ret = -EINVAL;
> > > -			goto err_free;
> > > +			/*
> > > +			 * In the case of on-die ECC (when ECC are
> > > stored in an
> > > +			 * isolated area inaccessible by the
> > > driver), the OOB
> > > +			 * layout must only reserve space for Bad
> > > Block Markers.
> > > +			 */    
> > 
> > This kind of on-die ECC is not yet supported and I'd expect NAND
> > manufacturer drivers to set the appropriate layout before reaching
> > this point, so that's not really relevant here.  
> 
> To be removed then.
> 
> >   
> > > +			if (mtd->writesize <= 512)
> > > +				mtd_set_ooblayout(mtd,
> > > +
> > > &nand_ooblayout_sp_no_ecc_ops);
> > > +			else
> > > +				mtd_set_ooblayout(mtd,
> > > +
> > > &nand_ooblayout_lp_no_ecc_ops);    
> > 
> > That's not exactly what I was suggesting. What I had in mind was:
> > 
> > 			if (ecc->mode == NAND_ECC_NONE) {
> > 				/*
> > 				 * Expose the whole OOB area to users
> > if ECC_NONE
> > 				 * is passed. We could do that for
> > all kind of
> > 				 * ->oobsize, but we must keep the old
> > 				 * large/small page with ECC layout
> > for
> > 				 * compatibility reasons.
> > 				 */
> > 				if (mtd->writesize <= 512)
> > 					ze(mtd,
> > 							  &nand_ooblayout_sp_no_ecc_ops);
> > 				else
> > 					mtd_set_ooblayout(mtd,
> > 							  &nand_ooblayout_lp_no_ecc_ops);
> > 
> > 				break;
> > 			}
> > 
> > 			WARN(1, "No oob scheme defined for oobsize
> > %d\n", mtd->oobsize);
> > 			ret = -EINVAL;
> > 			goto err_free;  
> 
> Ok, I clearly misunderstood your previous explanation, sorry about
> that. I'll use your version as is.

Hm, I also realize nand_ooblayout_sp_no_ecc_ops will actually never
be used because small page NANDs always have ->oobsize set to 8 or
16 bytes, and these settings are already using nand_ooblayout_sp_ops.

You can just simplify the code like this:

			/*
			 * Expose the whole OOB area to users if ECC_NONE
			 * is passed. We could do that for all kind of
			 * ->oobsize, but we must keep the old large/small
			 * page with ECC layout when ->oobsize <= 128 for
			 * compatibility reasons.
			 */
			if (ecc->mode == NAND_ECC_NONE)
				mtd_set_ooblayout(mtd,
						  &nand_ooblayout_lp_no_ecc_ops);

Regards,

Boris