Re: [PATCH v0] mtd: rawnand: marvell: fix 8bit ECC layouts

From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Aryan Srivastava <aryan.srivastava@alliedtelesis.co.nz>
Cc: Richard Weinberger <richard@nod.at>,
	 Vignesh Raghavendra <vigneshr@ti.com>,
	 Elad Nachman <enachman@marvell.com>,
	linux-mtd@lists.infradead.org,  linux-kernel@vger.kernel.org
Subject: Re: [PATCH v0] mtd: rawnand: marvell: fix 8bit ECC layouts
Date: Wed, 22 Oct 2025 15:39:27 +0200	[thread overview]
Message-ID: <87qzuvrr74.fsf@bootlin.com> (raw)
In-Reply-To: <20251022013752.2381694-1-aryan.srivastava@alliedtelesis.co.nz> (Aryan Srivastava's message of "Wed, 22 Oct 2025 14:37:52 +1300")

On 22/10/2025 at 14:37:52 +13, Aryan Srivastava <aryan.srivastava@alliedtelesis.co.nz> wrote:

> These were modified to appease a check in nand_scan_tail. This change led
> to the last spare bytes never being written to or read from.
>
> Modify the fix by restoring the layouts and setting ecc->steps to the
> full_chunk_cnt value in the 8 bit ECC cases (4k and 8k page size). This
> allows the driver to continue reading/writing all chunks while also
> passing the check in nand_scan_tail.
>
> Fixes: e6a30d0c48a1 ("mtd: rawnand: marvell: fix layouts")
> Signed-off-by: Aryan Srivastava <aryan.srivastava@alliedtelesis.co.nz>
> ---
>  drivers/mtd/nand/raw/marvell_nand.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
> index 303b3016a070..c20feacbaca8 100644
> --- a/drivers/mtd/nand/raw/marvell_nand.c
> +++ b/drivers/mtd/nand/raw/marvell_nand.c
> @@ -292,10 +292,10 @@ static const struct marvell_hw_ecc_layout marvell_nfc_layouts[] = {
>  	MARVELL_LAYOUT( 2048,   512,  8,  2,  1, 1024,  0, 30,1024,64, 30),
>  	MARVELL_LAYOUT( 2048,   512,  16, 4,  4, 512,   0, 30,  0, 32, 30),
>  	MARVELL_LAYOUT( 4096,   512,  4,  2,  2, 2048, 32, 30,  0,  0,  0),
> -	MARVELL_LAYOUT( 4096,   512,  8,  4,  4, 1024,  0, 30,  0, 64, 30),
> +	MARVELL_LAYOUT( 4096,   512,  8,  5,  4, 1024,  0, 30,  0, 64, 30),
>  	MARVELL_LAYOUT( 4096,   512,  16, 8,  8, 512,   0, 30,  0, 32, 30),
>  	MARVELL_LAYOUT( 8192,   512,  4,  4,  4, 2048,  0, 30,  0,  0,  0),
> -	MARVELL_LAYOUT( 8192,   512,  8,  8,  8, 1024,  0, 30,  0, 160, 30),
> +	MARVELL_LAYOUT( 8192,   512,  8,  9,  8, 1024,  0, 30,  0, 160, 30),
>  	MARVELL_LAYOUT( 8192,   512,  16, 16, 16, 512,  0, 30,  0,  32,
> 30),
>  };
>  
> @@ -2286,7 +2286,16 @@ static int marvell_nand_hw_ecc_controller_init(struct mtd_info *mtd,
>  	}
>  
>  	mtd_set_ooblayout(mtd, &marvell_nand_ooblayout_ops);
> -	ecc->steps = l->nchunks;
> +
> +	/* Validity checks in nand_scan_tail assume even sized chunks, but in the case of 8bit
> +	 * ECC with 4k/8k page size the last chunk is spare data, which is not sized to the data
> +	 * chunks. Overwrite the ecc->steps to pass this validity check, while maintaining the
> +	 * correct number of chunks in-driver.
> +	 */

I am not a big fan of this approach, I wonder whether we should relax
the check in nand_scan_tail() or not. Maybe we could avoid the error in
the core by just printing a warning, this way the drivers would not need
to lie to the core.

What I would suggest, now that I properly understood the problem, is to:
1. Revert Elad's patchset *entirely* (including the revert of
   existing/valid but with little use layouts).
2. Relax the check in the core.

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/