Re: [RFC PATCH 20/27] mtd: nand: Let software ECC engines be retrieved from the NAND core

[Boris Brezillon <boris.brezillon@collabora.com>]

On Mon, 25 Feb 2019 16:49:33 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Boris,
> 
> Boris Brezillon <bbrezillon@kernel.org> wrote on Fri, 22 Feb 2019
> 15:29:57 +0100:
> 
> > On Thu, 21 Feb 2019 13:57:59 +0100
> > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >   
> > > Before making use of the ECC engines, we must retrieve them. Add the
> > > boilerplate for the ones already available: software engines (Hamming
> > > and BCH).
> > > 
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  drivers/mtd/nand/ecc/engine.c              | 14 ++++++++++++++
> > >  include/linux/mtd/nand-sw-bch-engine.h     |  3 +++
> > >  include/linux/mtd/nand-sw-hamming-engine.h |  3 +++
> > >  include/linux/mtd/nand.h                   |  3 +++
> > >  4 files changed, 23 insertions(+)
> > > 
> > > diff --git a/drivers/mtd/nand/ecc/engine.c b/drivers/mtd/nand/ecc/engine.c
> > > index 7dd3f9772698..318dbb2d56a2 100644
> > > --- a/drivers/mtd/nand/ecc/engine.c
> > > +++ b/drivers/mtd/nand/ecc/engine.c
> > > @@ -286,6 +286,20 @@ bool nand_ecc_correction_is_enough(struct nand_device *nand)
> > >  	return corr >= ds_corr && conf->strength >= reqs->strength;
> > >  }
> > >  
> > > +struct nand_ecc_engine *nand_ecc_sw_get_engine(struct nand_device *nand)    
> > 
> > What if you want to instantiate SW ECC with a custom layout? Can't we
> > instead have a function that create a new engine dynamically?
> > 
> > struct nand_ecc_engine *
> > nand_ecc_create_sw_engine(struct nand_device* nand,
> > 			  enum nand_ecc_algo algo,
> > 			  struct mtd_ooblayout *layout);
> > 
> >   
> 
> Right now, for both sw engines, a default layout is applied if there is
> none at engine initialization time.
> 
> Also, do we really need a "create" helper? I don't see what's created
> there. Maybe you had something else in mind, and the
> ecc_sw_xxx_get_engine() approach do not match what you expected, so
> please tell me more about your idea, otherwise I don't see what a
> nand_ecc_create_sw_engine() would bring.

The layout is part of the ECC engine properties, which I why I suggest
to create one engine instance per user and specify the layout to use
at instance creation time.

> 
> >   
> > > +{
> > > +	switch (nand->ecc.user_conf.algo) {    
> > 
> > Note that the conf is supposed to be passed afterwards, when the ctx is
> > created, so you should check nand->ecc.user_conf directly here.  
> 
> I think this is what I do so I suspect the above sentence is not what
> you actually meant?

Sorry, I meant "should not". My point is, the user_conf should only be
passed at context creation time, and should not be modified in-place by
the caller, unless proven necessary.

There's really 2 different steps that I think need to be isolated:

1/ retrieve/create an ECC engine instance (SW, HW-controller-side,
   on-die)
2/ ask this ECC engine instance to create a context out of a user conf

Your user_conf seems 2 mix the 2 concepts: the engine to use, the
strength/step-size you expect.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel