Re: [PATCH v3 01/16] mtd: rawnand: helper function for setting up ECC configuration

[Abhishek Sahu <absahu@codeaurora.org>]

On 2018-05-30 13:08, Masahiro Yamada wrote:
> 2018-05-30 15:21 GMT+09:00 Abhishek Sahu <absahu@codeaurora.org>:
>> On 2018-05-30 05:58, Masahiro Yamada wrote:
>>> 
>>> Hi.
>>> 
>>> 2018-05-30 4:30 GMT+09:00 Boris Brezillon 
>>> <boris.brezillon@bootlin.com>:
>>>> 
>>>> On Sat, 26 May 2018 10:42:47 +0200
>>>> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>>>> 
>>>>> Hi Abhishek,
>>>>> 
>>>>> On Fri, 25 May 2018 17:51:29 +0530, Abhishek Sahu
>>>>> <absahu@codeaurora.org> wrote:
>>>>> 
>>>>> > commit 2c8f8afa7f92 ("mtd: nand: add generic helpers to check,
>>>>> > match, maximize ECC settings") provides generic helpers which
>>>>> > drivers can use for setting up ECC parameters.
>>>>> >
>>>>> > Since same board can have different ECC strength nand chips so
>>>>> > following is the logic for setting up ECC strength and ECC step
>>>>> > size, which can be used by most of the drivers.
>>>>> >
>>>>> > 1. If both ECC step size and ECC strength are already set
>>>>> >    (usually by DT) then just check whether this setting
>>>>> >    is supported by NAND controller.
>>>>> > 2. If NAND_ECC_MAXIMIZE is set, then select maximum ECC strength
>>>>> >    supported by NAND controller.
>>>>> > 3. Otherwise, try to match the ECC step size and ECC strength closest
>>>>> >    to the chip's requirement. If available OOB size can't fit the chip
>>>>> >    requirement then select maximum ECC strength which can be fit with
>>>>> >    available OOB size.
>>>>> >
>>>>> > This patch introduces nand_ecc_choose_conf function which calls the
>>>>> > required helper functions for the above logic. The drivers can use
>>>>> > this single function instead of calling the 3 helper functions
>>>>> > individually.
>>>>> >
>>>>> > CC: Masahiro Yamada <yamada.masahiro@socionext.com>
>>>>> > Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
>>>>> > ---
>>>>> > * Changes from v2:
>>>>> >
>>>>> >   1. Renamed function to nand_ecc_choose_conf.
>>>>> >   2. Minor code reorganization to remove warning and 2 function calls
>>>>> >      for nand_maximize_ecc.
>>>>> >
>>>>> > * Changes from v1:
>>>>> >   NEW PATCH
>>>>> >
>>>>> >  drivers/mtd/nand/raw/nand_base.c | 42
>>>>> > ++++++++++++++++++++++++++++++++++++++++
>>>>> >  drivers/mtd/nand/raw/nand_base.c | 31 +++++++++++++++++++++++++++++++
>>>>> >  include/linux/mtd/rawnand.h      |  3 +++
>>>>> >  2 files changed, 34 insertions(+)
>>>>> >
>>>>> > diff --git a/drivers/mtd/nand/raw/nand_base.c
>>>>> > b/drivers/mtd/nand/raw/nand_base.c
>>>>> > index 72f3a89..e52019d 100644
>>>>> > --- a/drivers/mtd/nand/raw/nand_base.c
>>>>> > +++ b/drivers/mtd/nand/raw/nand_base.c
>>>>> > @@ -6249,6 +6249,37 @@ int nand_maximize_ecc(struct nand_chip *chip,
>>>>> >  }
>>>>> >  EXPORT_SYMBOL_GPL(nand_maximize_ecc);
>>>>> >
>>>>> > +/**
>>>>> > + * nand_ecc_choose_conf - Set the ECC strength and ECC step size
>>>>> > + * @chip: nand chip info structure
>>>>> > + * @caps: ECC engine caps info structure
>>>>> > + * @oobavail: OOB size that the ECC engine can use
>>>>> > + *
>>>>> > + * Choose the ECC configuration according to following logic
>>>>> > + *
>>>>> > + * 1. If both ECC step size and ECC strength are already set (usually
>>>>> > by DT)
>>>>> > + *    then check if it is supported by this controller.
>>>>> > + * 2. If NAND_ECC_MAXIMIZE is set, then select maximum ECC strength.
>>>>> > + * 3. Otherwise, try to match the ECC step size and ECC strength
>>>>> > closest
>>>>> > + *    to the chip's requirement. If available OOB size can't fit the
>>>>> > chip
>>>>> > + *    requirement then fallback to the maximum ECC step size and ECC
>>>>> > strength.
>>>>> > + *
>>>>> > + * On success, the chosen ECC settings are set.
>>>>> > + */
>>>>> > +int nand_ecc_choose_conf(struct nand_chip *chip,
>>>>> > +                    const struct nand_ecc_caps *caps, int oobavail)
>>>>> > +{
>>>>> > +   if (chip->ecc.size && chip->ecc.strength)
>>>>> > +           return nand_check_ecc_caps(chip, caps, oobavail);
>>>>> > +
>>>>> > +   if (!(chip->ecc.options & NAND_ECC_MAXIMIZE) &&
>>>>> > +       !nand_match_ecc_req(chip, caps, oobavail))
>>>>> > +           return 0;
>>>>> > +
>>>>> > +   return nand_maximize_ecc(chip, caps, oobavail);
>>>>> 
>>>>> I personally don't mind if nand_maximize_ecc() is called twice in
>>>>> the function if it clarifies the logic. Maybe the following will be
>>>>> more clear for the user?
>>>>> 
>>>>>       if (chip->ecc.size && chip->ecc.strength)
>>>>>               return nand_check_ecc_caps(chip, caps, oobavail);
>>>>> 
>>>>>       if (chip->ecc.options & NAND_ECC_MAXIMIZE)
>>>>>               return nand_maximize_ecc(chip, caps, oobavail);
>>>>> 
>>>>>       if (!nand_match_ecc_req(chip, caps, oobavail))
>>>>>               return 0;
>>>>> 
>>>>>       return nand_maximize_ecc(chip, caps, oobavail);
>>>> 
>>>> 
>>>> I personally don't mind, and it seems Masahiro wanted to keep the 
>>>> logic
>>>> he had used in the denali driver.
>>>> 
>>>>> 
>>>>> Also, I'm not sure we should just error out when 
>>>>> nand_check_ecc_caps()
>>>>> fails. What about something more robust, like:
>>>>> 
>>>>>       int ret;
>>>>> 
>>>>>       if (chip->ecc.size && chip->ecc.strength) {
>>>>>               ret = nand_check_ecc_caps(chip, caps, oobavail);
>>>>>               if (ret)
>>>>>                       goto maximize_ecc;
>>>> 
>>>> 
>>>> Nope. When someone asked for a specific ECC config by passing the
>>>> nand-ecc-xxx props we should apply it or return an erro if it's not
>>>> supported. People passing those props should now what the ECC engine
>>>> supports and pick one valid values.
>>>> 
>>>>> 
>>>>>               return 0;
>>>>>       }
>>>>> 
>>>>>       if (chip->ecc.options & NAND_ECC_MAXIMIZE)
>>>>>               goto maximize_ecc;
>>>>> 
>>>>>       ret = nand_match_ecc_req(chip, caps, oobavail);
>>>>>       if (ret)
>>>>>               goto maximize_ecc;
>>>>> 
>>>>>       return 0;
>>>>> 
>>>>> maximize_ecc:
>>>>>       return nand_maximize_ecc(chip, caps, oobavail);
>>>>> 
>>>> 
>>>> 
>>>> ______________________________________________________
>>>> Linux MTD discussion mailing list
>>>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>>> 
>>> 
>>> 
>>> 
>>> 
>>> 
>>> 
>>> This version looks good to me.
>>> 
>>> If you want to check the error code more precisely,
>>> how about something like follows?
>>> 
>>> 
>>> 
>>> int nand_ecc_choose_conf(struct nand_chip *chip,
>>>                         const struct nand_ecc_caps *caps, int 
>>> oobavail)
>>> {
>>>        int ret;
>>> 
>>>        if (chip->ecc.size && chip->ecc.strength)
>>>                return nand_check_ecc_caps(chip, caps, oobavail);
>>> 
>>>        if (!(chip->ecc.options & NAND_ECC_MAXIMIZE)) {
>>>                ret = nand_match_ecc_req(chip, caps, oobavail);
>>>                if (ret != -ENOTSUPP)
>>>                        return ret;
>>>        }
>>> 
>>>        return nand_maximize_ecc(chip, caps, oobavail);
>>> }
>>> 
>>> 
>>> Only the difference is the case
>>> where nand_match_ecc_req() returns a different error code
>>> than ENOTSUPP.
>>> (Currently, this happens only when insane 'oobavail' is passed.)
>>> 
>> 
>>  We can do that but to me, it will make the helper function
>>  more complicated. Currently, nand_match_ecc_req is returning
>>  other than ENOTSUPP 'oobavail < 0' is passed.
>>  and again in nand_maximize_ecc, we will check for validity
>>  of oobavail so nothing wrong will happen in calling
>>  nand_maximize_ecc.
> 
> 
> Right.  When I added those three helpers,
> I supposed they were independent APIs.
> That is why I added the 'oobavail < 0' sanity check
> in each of the three.
> 
> 
> If you make them internal sub-helpers
> (i.e. add 'static' instead of EXPORT_SYMBOL_GPL),
> you can check 'oobavail < 0'
> only in nand_ecc_choose_conf().
> 
> 

  I am not sure regarding making them static.

  Currently, Denali NAND driver is only using these functions.

  And Now, this nand_ecc_choose_conf will be help
  in all the cases.

  For nand_check_ecc_caps: call nand_ecc_choose_conf with
                           chip->ecc.size && chip->ecc.strength

  For nand_maximize_ecc: call nand_ecc_choose_conf with
                         NAND_ECC_MAXIMIZE

  So making them static also seems ok which will be
  easy to maintain in future.

  Thanks,
  Abhishek

> 
> 
> 
>>  Anyway we put this under WARN_ON condition
>> 
>>         if (WARN_ON(oobavail < 0))
>>                 return -EINVAL;
>> 
>>  so if this is being triggered, then it should be mostly
>>  programming error.
> 
> 
> Right.  Moreover,
> 
> WARN_ON(oobavail < 0 || oobavail > mtd->oobsize)
> 
> 
> 
> This is programming error, that is why WARN_ON() is used to
> make the log noisy.
> 
> 
>>  Thanks,
>>  Abhishek
>> 
>>> 
>>> ENOTSUPP means 'required ECC setting is not supported'.
>>> Other error code is more significant, so it is not a good reason
>>> to fall back to miximization, IMHO.
>> 
>> 
>> ______________________________________________________
>> Linux MTD discussion mailing list
>> http://lists.infradead.org/mailman/listinfo/linux-mtd/