Re: [PATCH 1/3] mtd: rawnand: Fix and simplify again the continuous read derivations

From: Christophe Kerello <christophe.kerello@foss.st.com>
To: Miquel Raynal <miquel.raynal@bootlin.com>,
	Richard Weinberger <richard@nod.at>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Tudor Ambarus <tudor.ambarus@linaro.org>,
	Pratyush Yadav <pratyush@kernel.org>,
	Michael Walle <michael@walle.cc>, <linux-mtd@lists.infradead.org>
Cc: "Thomas Petazzoni" <thomas.petazzoni@bootlin.com>,
	"Julien Su" <juliensu@mxic.com.tw>,
	"Jaime Liao" <jaimeliao@mxic.com.tw>,
	"Jaime Liao" <jaimeliao.tw@gmail.com>,
	"Alvin Zhou" <alvinzhou@mxic.com.tw>,
	eagle.alexander923@gmail.com, mans@mansr.com, martin@geanix.com,
	"Sean Nyekjær" <sean@geanix.com>,
	stable@vger.kernel.org
Subject: Re: [PATCH 1/3] mtd: rawnand: Fix and simplify again the continuous read derivations
Date: Fri, 1 Mar 2024 18:48:31 +0100	[thread overview]
Message-ID: <ffdb0f72-5e49-4e41-9801-399035c0bdce@foss.st.com> (raw)
In-Reply-To: <20240223115545.354541-2-miquel.raynal@bootlin.com>

Hi Miquel,

On 2/23/24 12:55, Miquel Raynal wrote:
> We need to avoid the first page if we don't read it entirely.
> We need to avoid the last page if we don't read it entirely.
> While rather simple, this logic has been failed in the previous
> fix. This time I wrote about 30 unit tests locally to check each
> possible condition, hopefully I covered them all.
> 
> Reported-by: Christophe Kerello <christophe.kerello@foss.st.com>
> Closes: https://lore.kernel.org/linux-mtd/20240221175327.42f7076d@xps-13/T/#m399bacb10db8f58f6b1f0149a1df867ec086bb0a
> Suggested-by: Christophe Kerello <christophe.kerello@foss.st.com>
> Fixes: 828f6df1bcba ("mtd: rawnand: Clarify conditions to enable continuous reads")
> Cc: stable@vger.kernel.org
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Tested-by: Christophe Kerello <christophe.kerello@foss.st.com>

Regards,
Christophe Kerello.

> ---
>   drivers/mtd/nand/raw/nand_base.c | 38 ++++++++++++++++++--------------
>   1 file changed, 22 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 3b3ce2926f5d..bcfd99a1699f 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -3466,30 +3466,36 @@ static void rawnand_enable_cont_reads(struct nand_chip *chip, unsigned int page,
>   				      u32 readlen, int col)
>   {
>   	struct mtd_info *mtd = nand_to_mtd(chip);
> -	unsigned int end_page, end_col;
> +	unsigned int first_page, last_page;
>   
>   	chip->cont_read.ongoing = false;
>   
>   	if (!chip->controller->supported_op.cont_read)
>   		return;
>   
> -	end_page = DIV_ROUND_UP(col + readlen, mtd->writesize);
> -	end_col = (col + readlen) % mtd->writesize;
> +	/*
> +	 * Don't bother making any calculations if the length is too small.
> +	 * Side effect: avoids possible integer underflows below.
> +	 */
> +	if (readlen < (2 * mtd->writesize))
> +		return;
>   
> +	/* Derive the page where continuous read should start (the first full page read) */
> +	first_page = page;
>   	if (col)
> -		page++;
> -
> -	if (end_col && end_page)
> -		end_page--;
> -
> -	if (page + 1 > end_page)
> -		return;
> -
> -	chip->cont_read.first_page = page;
> -	chip->cont_read.last_page = end_page;
> -	chip->cont_read.ongoing = true;
> -
> -	rawnand_cap_cont_reads(chip);
> +		first_page++;
> +
> +	/* Derive the page where continuous read should stop (the last full page read) */
> +	last_page = page + ((col + readlen) / mtd->writesize) - 1;
> +
> +	/* Configure and enable continuous read when suitable */
> +	if (first_page < last_page) {
> +		chip->cont_read.first_page = first_page;
> +		chip->cont_read.last_page = last_page;
> +		chip->cont_read.ongoing = true;
> +		/* May reset the ongoing flag */
> +		rawnand_cap_cont_reads(chip);
> +	}
>   }
>   
>   static void rawnand_cont_read_skip_first_page(struct nand_chip *chip, unsigned int page)

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