Hi Frieder,
Frieder Schrempf <frieder@xxxxxxx> wrote on Mon, 30 Aug 2021 09:21:07
+0200:
> From: Frieder Schrempf <frieder.schrempf@xxxxxxxxxx>
>
> The new generic NAND ECC framework stores the configuration and requirements
> in separate places since commit 93ef92f6f422 (" mtd: nand: Use the new generic ECC object ").
> In 5.10.x The SPI NAND layer still uses only the requirements to track the ECC
> properties. This mismatch leads to values of zero being used for ECC strength
> and step_size in the SPI NAND layer wherever nanddev_get_ecc_conf() is used and
> therefore breaks the SPI NAND on-die ECC support in 5.10.x.
>
> By using nanddev_get_ecc_requirements() instead of nanddev_get_ecc_conf() for
> SPI NAND, we make sure that the correct parameters for the detected chip are
> used. In later versions (5.11.x) this is fixed anyway with the implementation
> of the SPI NAND on-die ECC engine.
>
> Cc: stable@xxxxxxxxxxxxxxx # 5.10.x
> Reported-by: voice INTER connect GmbH <developer@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Frieder Schrempf <frieder.schrempf@xxxxxxxxxx>
Why not just reverting 9a333a72c1d0 ("mtd: spinand: Use
nanddev_get_ecc_conf() when relevant")? I think using this "new"
nanddev_get_ecc_requirements() helper because it fits the purpose even
if it is wrong [1] doesn't bring the right information. I know it is
strictly equivalent but I would personally prefer keeping the old
writing "nand->eccreq.xxxx".
[1] We don't want the requirements but the actual current configuration
here, which was the primary purpose of the initial patch which ended
being a mistake at that point in time because the SPI-NAND core was not
ready yet.
Thanks,
Miquèl
> ---
> Resending this with an improved subject prefix and because the
> previous mail wasn't delivered to some of the lists.
> ---
> drivers/mtd/nand/spi/core.c | 6 +++---
> drivers/mtd/nand/spi/macronix.c | 6 +++---
> drivers/mtd/nand/spi/toshiba.c | 6 +++---
> 3 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index 558d8a14810b..8794a1f6eacd 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -419,7 +419,7 @@ static int spinand_check_ecc_status(struct
> spinand_device *spinand, u8 status)
> * fixed, so let's return the maximum possible value
> so that
> * wear-leveling layers move the data immediately.
> */
> - return nanddev_get_ecc_conf(nand)->strength;
> + return nanddev_get_ecc_requirements(nand)->strength;
>
> case STATUS_ECC_UNCOR_ERROR:
> return -EBADMSG;
> @@ -1090,8 +1090,8 @@ static int spinand_init(struct spinand_device
> *spinand) mtd->oobavail = ret;
>
> /* Propagate ECC information to mtd_info */
> - mtd->ecc_strength = nanddev_get_ecc_conf(nand)->strength;
> - mtd->ecc_step_size = nanddev_get_ecc_conf(nand)->step_size;
> + mtd->ecc_strength =
> nanddev_get_ecc_requirements(nand)->strength;
> + mtd->ecc_step_size =
> nanddev_get_ecc_requirements(nand)->step_size;
> return 0;
>
> diff --git a/drivers/mtd/nand/spi/macronix.c
> b/drivers/mtd/nand/spi/macronix.c index 8e801e4c3a00..cd7a9cacc3fb
> 100644 --- a/drivers/mtd/nand/spi/macronix.c
> +++ b/drivers/mtd/nand/spi/macronix.c
> @@ -84,11 +84,11 @@ static int mx35lf1ge4ab_ecc_get_status(struct
> spinand_device *spinand,
> * data around if it's not necessary.
> */
> if (mx35lf1ge4ab_get_eccsr(spinand, &eccsr))
> - return nanddev_get_ecc_conf(nand)->strength;
> + return
> nanddev_get_ecc_requirements(nand)->strength;
> - if (WARN_ON(eccsr >
> nanddev_get_ecc_conf(nand)->strength ||
> + if (WARN_ON(eccsr >
> nanddev_get_ecc_requirements(nand)->strength || !eccsr))
> - return nanddev_get_ecc_conf(nand)->strength;
> + return
> nanddev_get_ecc_requirements(nand)->strength;
> return eccsr;
>
> diff --git a/drivers/mtd/nand/spi/toshiba.c
> b/drivers/mtd/nand/spi/toshiba.c index 21fde2875674..6fe7bd2a94d2
> 100644 --- a/drivers/mtd/nand/spi/toshiba.c
> +++ b/drivers/mtd/nand/spi/toshiba.c
> @@ -90,12 +90,12 @@ static int tx58cxgxsxraix_ecc_get_status(struct
> spinand_device *spinand,
> * data around if it's not necessary.
> */
> if (spi_mem_exec_op(spinand->spimem, &op))
> - return nanddev_get_ecc_conf(nand)->strength;
> + return
> nanddev_get_ecc_requirements(nand)->strength;
> mbf >>= 4;
>
> - if (WARN_ON(mbf >
> nanddev_get_ecc_conf(nand)->strength || !mbf))
> - return nanddev_get_ecc_conf(nand)->strength;
> + if (WARN_ON(mbf >
> nanddev_get_ecc_requirements(nand)->strength || !mbf))
> + return
> nanddev_get_ecc_requirements(nand)->strength;
> return mbf;
>
