Re: [PATCH] mtd: spinand: confirm ready state on SkyHigh S35ML chips

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] mtd: spinand: false -ETIMEDOUT in spinand_wait_ready_confirmed()
- [Medium] mtd: spinand: unpaced confirmation read in spinand_wait_ready_confirmed()
- [Low] mtd: spinand: spinand_wait() bypasses SPINAND_PREMATURE_READY_QUIRK
--

commit 0d677bd9a2d2905d97cdc33fcc8ccfcf7d8a7cc7
Author: Vitaliy Sochnev <sochnev.v.74@gmail.com>

mtd: spinand: confirm ready state on SkyHigh S35ML chips

This commit adds a SPINAND_PREMATURE_READY_QUIRK flag for SkyHigh S35ML
SPI-NAND chips and introduces spinand_wait_ready_confirmed() to perform a
follow-up status read after spinand_wait() reports ready, repeating the
wait if STATUS_BUSY is set again.

> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index 133776ba53da0..b051ff1d6be5c 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -710,6 +710,53 @@ int spinand_wait(struct spinand_device *spinand, unsigned long initial_delay_us,
>  	return status & STATUS_BUSY ? -ETIMEDOUT : 0;
>  }
>  
> +/**
> + * spinand_wait_ready_confirmed() - Wait for the device to be ready
> + * @spinand: the spinand device
> + * @initial_delay_us: delay in us before starting to poll
> + * @poll_delay_us: time to sleep between reads in us
> + * @s: the pointer to variable to store the value of REG_STATUS
> + *
> + * Same as spinand_wait(), except for devices flagged with
> + * SPINAND_PREMATURE_READY_QUIRK, whose ready bit can flip back to busy
> + * shortly after first reporting ready. For those, a ready result from
> + * spinand_wait() is only trusted once a follow-up status read still shows
> + * the device ready, and spinand_wait() is called again otherwise. Each
> + * spinand_wait() gets its own SPINAND_WAITRDY_TIMEOUT_MS budget and no
> + * new one is started once that span has elapsed, so such a device can
> + * spend up to roughly twice that time here.
> + *
> + * Return: 0 on success, a negative error code otherwise.
> + */
> +static int spinand_wait_ready_confirmed(struct spinand_device *spinand,
> +					unsigned long initial_delay_us,
> +					unsigned long poll_delay_us, u8 *s)
> +{
> +	unsigned long timeo;
> +	u8 status;
> +	int ret;
> +
> +	if (!(spinand->flags & SPINAND_PREMATURE_READY_QUIRK))
> +		return spinand_wait(spinand, initial_delay_us, poll_delay_us, s);
> +
> +	timeo = jiffies + msecs_to_jiffies(SPINAND_WAITRDY_TIMEOUT_MS);
> +	do {
> +		ret = spinand_wait(spinand, initial_delay_us, poll_delay_us,
> +				   &status);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = spinand_read_status(spinand, &status);
> +		if (ret)
> +			return ret;

[Severity: Medium]
In spinand_wait_ready_confirmed() in drivers/mtd/nand/spi/core.c,
spinand_read_status() is called immediately after spinand_wait() returns 0
without any delay between the two reads.

Inside spinand_wait(), spi_mem_poll_status() waits initial_delay_us before
the first check and poll_delay_us between subsequent software polls, or
uses hardware status-polling offload (spi_controller_mem_ops.poll_status)
which completes as soon as STATUS_BUSY first clears. A 3-byte GET_FEATURE
transaction on a fast SPI bus takes only a fraction of a microsecond on the
wire. On a low-overhead SPI controller or one using poll_status offload,
can the back-to-back spinand_read_status() call execute before the
premature-ready glitch window has ended, allowing spinand_read_page(),
spinand_write_page(), spinand_mtd_continuous_page_read(), or
spinand_erase() to proceed while the chip is still busy?

Also, when spinand_read_status() does observe STATUS_BUSY and the do-while
loop repeats, spinand_wait() is called again with the original
initial_delay_us (such as 250 us for erase or 75 us for write) even though
initial_delay_us already elapsed on the first iteration. Should there be a
delay (such as poll_delay_us or a dedicated settling delay) before the
confirmation spinand_read_status(), with initial_delay_us cleared to 0 for
subsequent loop iterations?

> +	} while ((status & STATUS_BUSY) && time_before(jiffies, timeo));
> +
> +	if (s)
> +		*s = status;
> +
> +	return status & STATUS_BUSY ? -ETIMEDOUT : 0;
> +}

[Severity: Medium]
Can spinand_wait_ready_confirmed() in drivers/mtd/nand/spi/core.c return a
spurious -ETIMEDOUT if the calling task is preempted or delayed?

First, timeo is computed before the do-while loop as:

	timeo = jiffies + msecs_to_jiffies(SPINAND_WAITRDY_TIMEOUT_MS);

On the first iteration, if spinand_wait() observes the premature ready
sample (for example via its post-timeout fallback spinand_read_status()
after scheduling delay) or if the task is preempted between spinand_wait()
and spinand_read_status() such that jiffies is now at or past timeo when
spinand_read_status() sees STATUS_BUSY set again, time_before(jiffies,
timeo) evaluates to false. The loop then exits after the first iteration
and returns -ETIMEDOUT without ever calling spinand_wait() a second time to
wait for the actual flash operation to finish.

Second, if the task is preempted right after spinand_read_status() reads
STATUS_BUSY and before time_before(jiffies, timeo) is evaluated, the loop
exits and returns -ETIMEDOUT based on the stale STATUS_BUSY value read
before the preemption, without performing a final status re-read after the
timeout expires (unlike spinand_wait()). Should spinand_read_status() be
checked one last time if the loop terminates with STATUS_BUSY set?

[ ... ]

> @@ -733,10 +780,10 @@ static int spinand_reset_op(struct spinand_device *spinand)
>  	if (ret)
>  		return ret;
>  
> -	return spinand_wait(spinand,
> -			    SPINAND_RESET_INITIAL_DELAY_US,
> -			    SPINAND_RESET_POLL_DELAY_US,
> -			    NULL);
> +	return spinand_wait_ready_confirmed(spinand,
> +					    SPINAND_RESET_INITIAL_DELAY_US,
> +					    SPINAND_RESET_POLL_DELAY_US,
> +					    NULL);
>  }

[Severity: Low]
This isn't a bug, but spinand_wait() is declared in
include/linux/mtd/spinand.h and called by vendor drivers (such as
f50l1g41lb_otp_lock() in drivers/mtd/nand/spi/esmt.c and
mt29f2g01abagd_otp_lock() in drivers/mtd/nand/spi/micron.c) to wait for
device readiness.

Instead of adding spinand_wait_ready_confirmed() and updating all five
callers in drivers/mtd/nand/spi/core.c (spinand_reset_op(),
spinand_read_page(), spinand_write_page(),
spinand_mtd_continuous_page_read(), and spinand_erase()) while leaving
spinand_wait() unconfirmed, would it be cleaner to move the single-wait
body into a static helper and handle SPINAND_PREMATURE_READY_QUIRK directly
inside spinand_wait()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030203342.91172-1-sochnev.v.74@gmail.com?part=1

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