Re: [PATCH 04/20] mtd: nand: ecc-bch: Stop exporting the private structure

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Sep 29, 2020 at 6:09 PM Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
>
> The NAND BCH control structure has nothing to do outside of this
> driver, all users of the nand_bch_init/free() functions just save it
> to chip->ecc.priv so do it in this driver directly and return a
> regular error code instead.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
> ---

Starting with this commit:  3c0fe36abebe, the kernel either doesn't
build or returns errors on some omap2plus devices with the following
error:

    nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xbc
    nand: Micron MT29F4G16ABBDA3W
    nand: 512 MiB, SLC, erase size: 128 KiB, page size: 2048, OOB size: 64
    nand: using OMAP_ECC_BCH8_CODE_HW_DETECTION_SW
    Invalid ECC layout
    omap2-nand 30000000.nand: unable to use BCH library
    omap2-nand: probe of 30000000.nand failed with error -22
    8<--- cut here ---

There are few commits using git bisect that have build errors, so it
wasn't possible for me to determine the exact commit that broke the
ECC.  If the build failed, I marked it as 'bad' to git bisect.

Newer commits have remedied the build issue, but the Invalid ECC
layout error still exists as of 5.11-RC2.

Do you have any suggestions on what I can do to remedy this?  I am
willing to try and test.



>  drivers/mtd/nand/ecc-sw-bch.c       | 36 ++++++++++++++++-------------
>  drivers/mtd/nand/raw/fsmc_nand.c    |  2 +-
>  drivers/mtd/nand/raw/nand_base.c    | 12 ++++++----
>  drivers/mtd/nand/raw/omap2.c        | 16 ++++++-------
>  include/linux/mtd/nand-ecc-sw-bch.h | 11 ++++-----
>  5 files changed, 41 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/mtd/nand/ecc-sw-bch.c b/drivers/mtd/nand/ecc-sw-bch.c
> index fe080a0837d8..97221ec3876e 100644
> --- a/drivers/mtd/nand/ecc-sw-bch.c
> +++ b/drivers/mtd/nand/ecc-sw-bch.c
> @@ -90,7 +90,7 @@ EXPORT_SYMBOL(nand_bch_correct_data);
>
>  /**
>   * nand_bch_init - Initialize software BCH ECC engine
> - * @mtd: MTD device
> + * @chip: NAND chip object
>   *
>   * Returns: a pointer to a new NAND BCH control structure, or NULL upon failure
>   *
> @@ -105,24 +105,24 @@ EXPORT_SYMBOL(nand_bch_correct_data);
>   * @eccsize = 512 (thus, m = 13 is the smallest integer such that 2^m - 1 > 512 * 8)
>   * @eccbytes = 7 (7 bytes are required to store m * t = 13 * 4 = 52 bits)
>   */
> -struct nand_bch_control *nand_bch_init(struct mtd_info *mtd)
> +int nand_bch_init(struct nand_chip *chip)
>  {
> -       struct nand_chip *nand = mtd_to_nand(mtd);
> +       struct mtd_info *mtd = nand_to_mtd(chip);
>         unsigned int m, t, eccsteps, i;
>         struct nand_bch_control *nbc = NULL;
>         unsigned char *erased_page;
> -       unsigned int eccsize = nand->ecc.size;
> -       unsigned int eccbytes = nand->ecc.bytes;
> -       unsigned int eccstrength = nand->ecc.strength;
> +       unsigned int eccsize = chip->ecc.size;
> +       unsigned int eccbytes = chip->ecc.bytes;
> +       unsigned int eccstrength = chip->ecc.strength;
>
>         if (!eccbytes && eccstrength) {
>                 eccbytes = DIV_ROUND_UP(eccstrength * fls(8 * eccsize), 8);
> -               nand->ecc.bytes = eccbytes;
> +               chip->ecc.bytes = eccbytes;
>         }
>
>         if (!eccsize || !eccbytes) {
>                 pr_warn("ecc parameters not supplied\n");
> -               goto fail;
> +               return -EINVAL;
>         }
>
>         m = fls(1+8*eccsize);
> @@ -130,7 +130,9 @@ struct nand_bch_control *nand_bch_init(struct mtd_info *mtd)
>
>         nbc = kzalloc(sizeof(*nbc), GFP_KERNEL);
>         if (!nbc)
> -               goto fail;
> +               return -ENOMEM;
> +
> +       chip->ecc.priv = nbc;
>
>         nbc->bch = bch_init(m, t, 0, false);
>         if (!nbc->bch)
> @@ -165,8 +167,8 @@ struct nand_bch_control *nand_bch_init(struct mtd_info *mtd)
>          * FIXME: we should probably rework the sequencing in nand_scan_tail()
>          * to avoid setting those fields twice.
>          */
> -       nand->ecc.steps = eccsteps;
> -       nand->ecc.total = eccsteps * eccbytes;
> +       chip->ecc.steps = eccsteps;
> +       chip->ecc.total = eccsteps * eccbytes;
>         if (mtd_ooblayout_count_eccbytes(mtd) != (eccsteps*eccbytes)) {
>                 pr_warn("invalid ecc layout\n");
>                 goto fail;
> @@ -192,12 +194,12 @@ struct nand_bch_control *nand_bch_init(struct mtd_info *mtd)
>                 nbc->eccmask[i] ^= 0xff;
>
>         if (!eccstrength)
> -               nand->ecc.strength = (eccbytes * 8) / fls(8 * eccsize);
> +               chip->ecc.strength = (eccbytes * 8) / fls(8 * eccsize);
>
> -       return nbc;
> +       return 0;
>  fail:
> -       nand_bch_free(nbc);
> -       return NULL;
> +       nand_bch_free(chip);
> +       return -EINVAL;
>  }
>  EXPORT_SYMBOL(nand_bch_init);
>
> @@ -205,8 +207,10 @@ EXPORT_SYMBOL(nand_bch_init);
>   * nand_bch_free - Release NAND BCH ECC resources
>   * @nbc: NAND BCH control structure
>   */
> -void nand_bch_free(struct nand_bch_control *nbc)
> +void nand_bch_free(struct nand_chip *chip)
>  {
> +       struct nand_bch_control *nbc = chip->ecc.priv;
> +
>         if (nbc) {
>                 bch_free(nbc->bch);
>                 kfree(nbc->errloc);
> diff --git a/drivers/mtd/nand/raw/fsmc_nand.c b/drivers/mtd/nand/raw/fsmc_nand.c
> index 4191831df182..1bc2462efeab 100644
> --- a/drivers/mtd/nand/raw/fsmc_nand.c
> +++ b/drivers/mtd/nand/raw/fsmc_nand.c
> @@ -927,7 +927,7 @@ static int fsmc_nand_attach_chip(struct nand_chip *nand)
>
>         /*
>          * Don't set layout for BCH4 SW ECC. This will be
> -        * generated later in nand_bch_init() later.
> +        * generated later during BCH initialization.
>          */
>         if (nand->ecc.engine_type == NAND_ECC_ENGINE_TYPE_ON_HOST) {
>                 switch (mtd->oobsize) {
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 189d3a301a38..c47441ddc5cf 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -5208,6 +5208,7 @@ static int nand_set_ecc_soft_ops(struct nand_chip *chip)
>         struct mtd_info *mtd = nand_to_mtd(chip);
>         struct nand_device *nanddev = mtd_to_nanddev(mtd);
>         struct nand_ecc_ctrl *ecc = &chip->ecc;
> +       int ret;
>
>         if (WARN_ON(ecc->engine_type != NAND_ECC_ENGINE_TYPE_SOFT))
>                 return -EINVAL;
> @@ -5294,13 +5295,14 @@ static int nand_set_ecc_soft_ops(struct nand_chip *chip)
>                         ecc->strength = bytes * 8 / fls(8 * ecc->size);
>                 }
>
> -               /* See nand_bch_init() for details. */
> +               /* See the software BCH ECC initialization for details */
>                 ecc->bytes = 0;
> -               ecc->priv = nand_bch_init(mtd);
> -               if (!ecc->priv) {
> +               ret = nand_bch_init(chip);
> +               if (ret) {
>                         WARN(1, "BCH ECC initialization failed!\n");
> -                       return -EINVAL;
> +                       return ret;
>                 }
> +
>                 return 0;
>         default:
>                 WARN(1, "Unsupported ECC algorithm!\n");
> @@ -5960,7 +5962,7 @@ void nand_cleanup(struct nand_chip *chip)
>  {
>         if (chip->ecc.engine_type == NAND_ECC_ENGINE_TYPE_SOFT &&
>             chip->ecc.algo == NAND_ECC_ALGO_BCH)
> -               nand_bch_free((struct nand_bch_control *)chip->ecc.priv);
> +               nand_bch_free(chip);
>
>         nanddev_cleanup(&chip->base);
>
> diff --git a/drivers/mtd/nand/raw/omap2.c b/drivers/mtd/nand/raw/omap2.c
> index 0ef209e1cd87..6aab57336690 100644
> --- a/drivers/mtd/nand/raw/omap2.c
> +++ b/drivers/mtd/nand/raw/omap2.c
> @@ -2047,10 +2047,10 @@ static int omap_nand_attach_chip(struct nand_chip *chip)
>                 /* Reserve one byte for the OMAP marker */
>                 oobbytes_per_step       = chip->ecc.bytes + 1;
>                 /* Software BCH library is used for locating errors */
> -               chip->ecc.priv          = nand_bch_init(mtd);
> -               if (!chip->ecc.priv) {
> +               err = nand_bch_init(chip);
> +               if (err) {
>                         dev_err(dev, "Unable to use BCH library\n");
> -                       return -EINVAL;
> +                       return err;
>                 }
>                 break;
>
> @@ -2089,10 +2089,10 @@ static int omap_nand_attach_chip(struct nand_chip *chip)
>                 /* Reserve one byte for the OMAP marker */
>                 oobbytes_per_step       = chip->ecc.bytes + 1;
>                 /* Software BCH library is used for locating errors */
> -               chip->ecc.priv          = nand_bch_init(mtd);
> -               if (!chip->ecc.priv) {
> +               err = nand_bch_init(chip);
> +               if (err) {
>                         dev_err(dev, "unable to use BCH library\n");
> -                       return -EINVAL;
> +                       return err;
>                 }
>                 break;
>
> @@ -2272,7 +2272,7 @@ static int omap_nand_probe(struct platform_device *pdev)
>         if (!IS_ERR_OR_NULL(info->dma))
>                 dma_release_channel(info->dma);
>         if (nand_chip->ecc.priv) {
> -               nand_bch_free(nand_chip->ecc.priv);
> +               nand_bch_free(nand_chip);
>                 nand_chip->ecc.priv = NULL;
>         }
>         return err;
> @@ -2286,7 +2286,7 @@ static int omap_nand_remove(struct platform_device *pdev)
>         int ret;
>
>         if (nand_chip->ecc.priv) {
> -               nand_bch_free(nand_chip->ecc.priv);
> +               nand_bch_free(nand_chip);
>                 nand_chip->ecc.priv = NULL;
>         }
>         if (info->dma)
> diff --git a/include/linux/mtd/nand-ecc-sw-bch.h b/include/linux/mtd/nand-ecc-sw-bch.h
> index 1e1ee3af82b1..b62b8bd4669f 100644
> --- a/include/linux/mtd/nand-ecc-sw-bch.h
> +++ b/include/linux/mtd/nand-ecc-sw-bch.h
> @@ -10,7 +10,6 @@
>
>  struct mtd_info;
>  struct nand_chip;
> -struct nand_bch_control;
>
>  #if IS_ENABLED(CONFIG_MTD_NAND_ECC_SW_BCH)
>
> @@ -30,11 +29,11 @@ int nand_bch_correct_data(struct nand_chip *chip, u_char *dat,
>  /*
>   * Initialize BCH encoder/decoder
>   */
> -struct nand_bch_control *nand_bch_init(struct mtd_info *mtd);
> +int nand_bch_init(struct nand_chip *chip);
>  /*
>   * Release BCH encoder/decoder resources
>   */
> -void nand_bch_free(struct nand_bch_control *nbc);
> +void nand_bch_free(struct nand_chip *chip);
>
>  #else /* !CONFIG_MTD_NAND_ECC_SW_BCH */
>
> @@ -54,12 +53,12 @@ nand_bch_correct_data(struct nand_chip *chip, unsigned char *buf,
>         return -ENOTSUPP;
>  }
>
> -static inline struct nand_bch_control *nand_bch_init(struct mtd_info *mtd)
> +static inline int nand_bch_init(struct nand_chip *chip)
>  {
> -       return NULL;
> +       return -ENOTSUPP;
>  }
>
> -static inline void nand_bch_free(struct nand_bch_control *nbc) {}
> +static inline void nand_bch_free(struct nand_chip *chip) {}
>
>  #endif /* CONFIG_MTD_NAND_ECC_SW_BCH */
>
> --
> 2.20.1
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/



[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux