Re: [PATCH v4 03/23] mtd: nand: add generic helpers to check, match, maximize ECC settings

[Masahiro Yamada]

2017-06-07 6:47 GMT+09:00 Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>:
> On Tue,  6 Jun 2017 08:21:42 +0900
> Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx> wrote:
>
>> Driver are responsible for setting up ECC parameters correctly.
>> Those include:
>>   - Check if ECC parameters specified (usually by DT) are valid
>>   - Meet the chip's ECC requirement
>>   - Maximize ECC strength if NAND_ECC_MAXIMIZE flag is set
>>
>> The logic can be generalized by factoring out common code.
>>
>> This commit adds 3 helpers to the NAND framework:
>> nand_check_ecc_caps - Check if preset step_size and strength are valid
>> nand_match_ecc_req - Match the chip's requirement
>> nand_maximize_ecc - Maximize the ECC strength
>>
>> To use the helpers above, a driver needs to provide:
>>   - Data array of supported ECC step size and strength
>>   - A hook that calculates ECC bytes from the combination of
>>     step_size and strength.
>>
>> By using those helpers, code duplication among drivers will be
>> reduced.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx>
>> ---
>>
>> Changes since the previous version:
>>
>>  - Step size info holds an array of associated strengths
>>  - nand_match_ecc_req() does not take care of the case
>>    where ecc_size/strength is already set
>>  - Reflect more comments from Boris
>>
>> Previous version:
>> http://patchwork.ozlabs.org/patch/752107/
>>
>>
>> Changes in v4: None
>> Changes in v3: None
>> Changes in v2: None
>>
>>  drivers/mtd/nand/nand_base.c | 219 +++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/mtd/nand.h     |  35 +++++++
>>  2 files changed, 254 insertions(+)
>>
>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
>> index bdfa903..f2da4f2 100644
>> --- a/drivers/mtd/nand/nand_base.c
>> +++ b/drivers/mtd/nand/nand_base.c
>> @@ -4509,6 +4509,225 @@ static int nand_set_ecc_soft_ops(struct mtd_info *mtd)
>>       }
>>  }
>>
>> +/**
>> + * nand_check_ecc_caps - check the sanity of preset ECC settings
>> + * @mtd: mtd info structure
>> + * @chip: nand chip info structure
>> + * @caps: ECC caps info structure
>> + *
>> + * When ECC step size and strength are already set, check if they are supported
>> + * by the controller and the calculated ECC bytes fit within the chip's OOB.
>> + * On success, the calculated ECC bytes is set.
>> + */
>> +int nand_check_ecc_caps(struct mtd_info *mtd, struct nand_chip *chip,
>> +                     const struct nand_ecc_caps *caps)
>> +{
>> +     const struct nand_ecc_step_info *stepinfo;
>> +     int avail_oobsize = mtd->oobsize - caps->oob_reserve_bytes;
>> +     int preset_step = chip->ecc.size;
>> +     int preset_strength = chip->ecc.strength;
>> +     int ecc_bytes;
>> +     int i, j;
>> +
>> +     if (WARN_ON(avail_oobsize < 0))
>> +             return -EINVAL;
>> +
>> +     if (!preset_step || !preset_strength)
>> +             return -ENODATA;
>> +
>> +     for (i = 0; i < caps->nstepinfos; i++) {
>> +             stepinfo = &caps->stepinfos[i];
>> +
>> +             if (stepinfo->stepsize != preset_step)
>> +                     continue;
>> +
>> +             for (j = 0; j < stepinfo->nstrengths; j++) {
>> +                     if (stepinfo->strengths[j] == preset_strength)
>> +                             goto found;
>> +             }
>> +     }
>> +
>> +     pr_err("ECC (step, strength) = (%d, %d) not supported on this controller",
>> +            preset_step, preset_strength);
>> +
>> +     return -ENOTSUPP;
>> +
>> +found:
>
> I prefer something like:
>
>         if (i == caps->nstepinfos) {
>                 pr_err(...);
>                 return -ENOTSUPP;
>         }
>
>         ...
>
> instead of this 'found' label.


I want to bail-out if (step, strength) matches.
In this version, the for-loop is double-nested by "step" and "strength".
In C language, it is not possible to bail-out from multi-nested loop
with a single "break;" statement.  That is why I used "found:" label to do it.

In my first version where there was a single for-loop,
I did not use the goto label.
http://patchwork.ozlabs.org/patch/752107/

Do you have any suggestion for cleaner implementation?






>> +     ecc_bytes = caps->calc_ecc_bytes(preset_step, preset_strength);
>> +     if (WARN_ON_ONCE(ecc_bytes < 0))
>> +             return ecc_bytes;
>> +
>> +     if (ecc_bytes * mtd->writesize / preset_step > avail_oobsize) {
>> +             pr_err("ECC (step, strength) = (%d, %d) does not fit in OOB",
>> +                    preset_step, preset_strength);
>> +             return -ENOSPC;
>> +     }
>> +
>> +     chip->ecc.bytes = ecc_bytes;
>> +
>> +     return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(nand_check_ecc_caps);
>> +
>> +/**
>> + * nand_match_ecc_req - meet the chip's requirement with least ECC bytes
>> + * @mtd: mtd info structure
>> + * @chip: nand chip info structure
>> + * @caps: ECC engine caps info structure
>> + *
>> + * If a chip's ECC requirement is provided, try to meet it with the least
>> + * number of ECC bytes (i.e. with the largest number of OOB-free bytes).
>> + * On success, the chosen ECC settings are set.
>> + */
>> +int nand_match_ecc_req(struct mtd_info *mtd, struct nand_chip *chip,
>> +                    const struct nand_ecc_caps *caps)
>> +{
>> +     const struct nand_ecc_step_info *stepinfo;
>> +     int avail_oobsize = mtd->oobsize - caps->oob_reserve_bytes;
>> +     int req_step = chip->ecc_step_ds;
>> +     int req_strength = chip->ecc_strength_ds;
>> +     int req_corr, step_size, strength, steps, ecc_bytes, ecc_bytes_total;
>> +     int best_step, best_strength, best_ecc_bytes;
>> +     int best_ecc_bytes_total = INT_MAX;
>
> Just nitpicking, but why not -1 instead of INT_MAX?

Because nand_match_ecc_req() prefers a smaller ecc_bytes_total.
So I chose the largest int number as an init value.
If we started from -1, the following if-conditional would have no effect.

     /*
      * We assume the best is to meet the chip's requrement
      * with the least number of ECC bytes.
      */
     if (ecc_bytes_total < best_ecc_bytes_total) {
                best_ecc_bytes_total = ecc_bytes_total;
                best_step = step_size;
                best_strength = strength;
                best_ecc_bytes = ecc_bytes;
     }






>> +     int i, j;
>> +
>> +     if (WARN_ON(avail_oobsize < 0))
>> +             return -EINVAL;
>> +
>> +     /* No information provided by the NAND chip */
>> +     if (!req_step || !req_strength)
>> +             return -ENOTSUPP;
>> +
>> +     /* number of correctable bits the chip requires in a page */
>> +     req_corr = mtd->writesize / req_step * req_strength;
>> +
>> +     for (i = 0; i < caps->nstepinfos; i++) {
>> +             stepinfo = &caps->stepinfos[i];
>> +             step_size = stepinfo->stepsize;
>> +
>> +             for (j = 0; j < stepinfo->nstrengths; j++) {
>> +                     strength = stepinfo->strengths[j];
>> +
>> +                     /*
>> +                      * If both step size and strength are smaller than the
>> +                      * chip's requirement, it is not easy to compare the
>> +                      * resulted reliability.
>> +                      */
>> +                     if (step_size < req_step && strength < req_strength)
>> +                             continue;
>> +
>> +                     if (mtd->writesize % step_size)
>> +                             continue;
>> +
>> +                     steps = mtd->writesize / step_size;
>> +
>> +                     ecc_bytes = caps->calc_ecc_bytes(step_size, strength);
>> +                     if (WARN_ON_ONCE(ecc_bytes < 0))
>> +                             continue;
>> +                     ecc_bytes_total = ecc_bytes * steps;
>> +
>> +                     if (ecc_bytes_total > avail_oobsize ||
>> +                         strength * steps < req_corr)
>> +                             continue;
>> +
>> +                     /*
>> +                      * We assume the best is to meet the chip's requrement
>> +                      * with the least number of ECC bytes.
>> +                      */
>> +                     if (ecc_bytes_total < best_ecc_bytes_total) {
>> +                             best_ecc_bytes_total = ecc_bytes_total;
>> +                             best_step = step_size;
>> +                             best_strength = strength;
>> +                             best_ecc_bytes = ecc_bytes;
>> +                     }
>> +             }
>> +     }
>> +
>> +     if (best_ecc_bytes_total == INT_MAX)
>> +             return -ENOTSUPP;
>> +
>> +     chip->ecc.size = best_step;
>> +     chip->ecc.strength = best_strength;
>> +     chip->ecc.bytes = best_ecc_bytes;
>> +
>> +     return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(nand_match_ecc_req);
>> +
>> +/**
>> + * nand_maximize_ecc - choose the max ECC strength available
>> + * @mtd: mtd info structure
>> + * @chip: nand chip info structure
>> + * @caps: ECC engine caps info structure
>> + *
>> + * Choose the max ECC strength that is supported on the controller, and can fit
>> + * within the chip's OOB.  On success, the chosen ECC settings are set.
>> + */
>> +int nand_maximize_ecc(struct mtd_info *mtd, struct nand_chip *chip,
>> +                   const struct nand_ecc_caps *caps)
>> +{
>> +     const struct nand_ecc_step_info *stepinfo;
>> +     int avail_oobsize = mtd->oobsize - caps->oob_reserve_bytes;
>> +     int step_size, strength, steps, ecc_bytes, corr;
>> +     int best_corr = 0;
>> +     int best_step = 0;
>> +     int best_strength, best_ecc_bytes;
>> +     int i, j;
>> +
>> +     if (WARN_ON(avail_oobsize < 0))
>> +             return -EINVAL;
>> +
>> +     for (i = 0; i < caps->nstepinfos; i++) {
>> +             stepinfo = &caps->stepinfos[i];
>> +             step_size = stepinfo->stepsize;
>> +
>> +
>
> Extra blank line here.

OK. I will remove it.



>> +
>> +/**
>> + * struct nand_ecc_caps - capability of ECC engine
>> + * @stepinfos: array of ECC step information
>> + * @nstepinfos: number of ECC step information
>> + * @calc_ecc_bytes: driver's hook to calculate ECC bytes per step
>> + * @oob_reserve_bytes: number of bytes in OOB that must be reserved
>> + */
>> +struct nand_ecc_caps {
>> +     const struct nand_ecc_step_info *stepinfos;
>> +     int nstepinfos;
>> +     int (*calc_ecc_bytes)(int step_size, int strength);
>> +     int oob_reserve_bytes;
>
> Why is this needed? I thought we agreed on passing oobavail as an
> argument to these helper funcs. If a driver needs to reserve a few OOB
> bytes, then doing mtd->oobsize - rsvd_bytes is not such a big deal.


oobavail is really chip-dependent, so I agreed
that it can not be included in the caps struct.

Then, I flipped the logic.
The number of reserved bytes will be more chip-independent.
But, oob_reserve_bytes may not necessarily a fixed value.

I can pass oobavail as a function argument.


-- 
Best Regards
Masahiro Yamada