[PATCH 2/4] MTD: flash drivers set ecc strength

[Brian Norris]

Hi Mike, Artem, David,

Sorry for the late review here. It looks like this is already queued
up for merging soon by David...
(BTW, patch 1 and 2 seem to do nothing by themselves; should they
really be merged in the 3.4 window?)

On Sun, Mar 11, 2012 at 2:21 PM, Mike Dunn <mikedunn at newsguy.com> wrote:
> Flash device drivers initialize 'ecc_strength' in struct mtd_info, which is the
> maximum number of bit errors that can be corrected in one writesize region.
>
> Drivers using the nand interface intitialize 'strength' in struct nand_ecc_ctrl,
> which is the maximum number of bit errors that can be corrected in one ecc step.
> Nand infrastructure code translates this to 'ecc_strength'.
>
> Also for nand drivers, the nand infrastructure code sets ecc.strength for ecc
> modes NAND_ECC_SOFT, NAND_ECC_SOFT_BCH, and NAND_ECC_NONE.  It is set in the
> driver for all other modes.

I agree that the other modes should be set by the driver, but is there
any kind of sanity check? We've seen problems with the addition of the
mtd->writebufsize parameter that didn't get caught as uninitialized in
a large number of drivers. I don't think your later patches handle the
case that mtd->ecc_strength is not initialized or is 0.

> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 1e907dc..8008853 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -3350,6 +3350,7 @@ int nand_scan_tail(struct mtd_info *mtd)
>                if (!chip->ecc.size)
>                        chip->ecc.size = 256;
>                chip->ecc.bytes = 3;
> +               chip->ecc.strength = 1;
>                break;
>
>        case NAND_ECC_SOFT_BCH:
> @@ -3384,6 +3385,8 @@ int nand_scan_tail(struct mtd_info *mtd)
>                        pr_warn("BCH ECC initialization failed!\n");
>                        BUG();
>                }
> +               chip->ecc.strength =
> +                       chip->ecc.bytes*8 / fls(8*chip->ecc.size);

Isn't this spacing against coding style? I'd suggest spaces around the
'*'. Also, after a few minutes, I have no idea where this calculation
comes from. But I'm not familiar with SOFT_BCH. Maybe a comment is in
order?

> @@ -3397,6 +3400,7 @@ int nand_scan_tail(struct mtd_info *mtd)
>                chip->ecc.write_oob = nand_write_oob_std;
>                chip->ecc.size = mtd->writesize;
>                chip->ecc.bytes = 0;
> +               chip->ecc.strength = 0;
>                break;

This will cause problems with your patch 4, I think. I'll comment on
the other email.

Brian