[PATCH v4 03/23] mtd: nand: add generic helpers to check, match, maximize ECC settings
Masahiro Yamada
yamada.masahiro at socionext.com
Tue Jun 6 18:48:33 PDT 2017
2017-06-07 6:47 GMT+09:00 Boris Brezillon <boris.brezillon at free-electrons.com>:
> On Tue, 6 Jun 2017 08:21:42 +0900
> Masahiro Yamada <yamada.masahiro at socionext.com> 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 at socionext.com>
>> ---
>>
>> 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
More information about the linux-mtd
mailing list