Re: [PATCH v2 2/7] mtd: spi-nor: Wait for NOR ready with poll_timeout_us()

From: Paul Cercueil <paul.cercueil@bootlin.com>
To: sashiko-reviews@lists.linux.dev,
	Pratyush Yadav <pratyush@kernel.org>,
	 Michael Walle <mwalle@kernel.org>,
	Takahiro Kuwano <takahiro.kuwano@infineon.com>,
	Miquel Raynal	 <miquel.raynal@bootlin.com>,
	Richard Weinberger <richard@nod.at>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Mark Brown <broonie@kernel.org>,
	Thomas Petazzoni	 <thomas.petazzoni@bootlin.com>
Cc: Kees Cook <kees@kernel.org>, Tony Luck <tony.luck@intel.com>,
	"Guilherme G . Piccoli" <gpiccoli@igalia.com>,
	linux-mtd@lists.infradead.org,
	Bharat Bhushan <bbhushan2@marvell.com>,
	 Sunil Kovvuri Goutham	 <sgoutham@marvell.com>,
	Tanmay Jagdale <tanmay@marvell.com>
Subject: Re: [PATCH v2 2/7] mtd: spi-nor: Wait for NOR ready with poll_timeout_us()
Date: Tue, 13 Oct 2026 17:29:36 +0200	[thread overview]
Message-ID: <5ce5ffcd7c38e4fe9f972937954dd177fb631bce.camel@bootlin.com> (raw)
In-Reply-To: <sashiko-outbox-169912@kernel.org>

Hi Sashiko,

Le mardi 13 octobre 2026 à 14:30 +0000, sashiko-bot@kernel.org a écrit :
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s)
> to consider:
> - [High] mtd: spi-nor: 32-bit integer overflow in timeout_ms * 1000
> --
> 
> commit d88f329aca4b57db5e4ba0a0fc748ee4d7440aba
> Author: Tanmay Jagdale <tanmay@marvell.com>
> 
> mtd: spi-nor: Wait for NOR ready with poll_timeout_us()
> 
> Instead of polling for the spi_nor_ready() with a custom loop, use
> poll_timeout_us(). The sleep period between two polls is set to 100 us
> which I think should be fine.
> 
> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > index 8bc117b46e028..2509fb983d4f2 100644
> > --- a/drivers/mtd/spi-nor/core.c
> > +++ b/drivers/mtd/spi-nor/core.c
> [ ... ]
> > @@ -712,29 +713,21 @@ static int spi_nor_ready(struct spi_nor *nor)
> >   * Return: 0 on success, -errno otherwise.
> >   */
> >  static int spi_nor_wait_till_ready_with_timeout(struct spi_nor *nor,
> > -						unsigned long
> > timeout_jiffies)
> > +						unsigned long timeout_ms)
> >  {
> > -	unsigned long deadline;
> > -	int timeout = 0, ret;
> > +	int ret, status;
> >  
> > -	deadline = jiffies + timeout_jiffies;
> > -
> > -	while (!timeout) {
> > -		if (time_after_eq(jiffies, deadline))
> > -			timeout = 1;
> > -
> > -		ret = spi_nor_ready(nor);
> > -		if (ret < 0)
> > -			return ret;
> > -		if (ret)
> > -			return 0;
> > -
> > -		cond_resched();
> > +	ret = poll_timeout_us(status = spi_nor_ready(nor), status,
> > +			      100, timeout_ms * 1000, false);
>                                    ^^^^^^^^^^^^^^^^^^^
> [Severity: High]
> Can this multiplication overflow on 32-bit systems?
> 
> In spi_nor_erase_dice(), the timeout scales with the flash size. For a 256MB
> flash, timeout_ms evaluates to 40000UL * 128 = 5,120,000.
> 
> Because both operands are 32-bit types on 32-bit platforms, the arithmetic
> could overflow ULONG_MAX (4,294,967,295), wrapping to 825,032,704 before being
> implicitly promoted to u64 inside the poll_timeout_us macro.
> 
> This truncates the wait timeout from ~85 minutes to ~13.7 minutes, which could
> cause legitimately slow erase operations on large flashes to prematurely abort
> with -ETIMEDOUT.
> 
> > +	if (ret < 0) {
> > -	dev_dbg(nor->dev, "flash operation timed out\n");
> > +		dev_dbg(nor->dev, "flash operation timed out\n");
> > +		return ret;
> > +	}

Technically you're not wrong, but to overflow the multiplication here, you'd
need a NOR size of at least (UINT32_MAX / 40000 * 2MiB) ~= 210 GiB, good luck
finding one of these.

I guess I need a v3 anyway, so I'll switch 'timeout_ms' to a u64 there.

Cheers,
-Paul

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