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

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

[Vitaliy Sochnev]

On a Nokia XG-040G-MD with a SkyHigh 256 MiB SPI-NAND, UBI reports
fixable bit-flips during rootfs mount without this workaround:

  ubi0: fixable bit-flip detected at PEB 1979
  ubi0: schedule PEB 1979 for scrubbing
  ubi0: scrubbed PEB 1979 (LEB 5:53), data moved to PEB 645

Other reboots also report fixable bit-flips. A tester reports that after
applying the workaround, sysupgrade, multiple reboots and an active
workload produced no such warnings. Long-term operation is unverified.

spinand_wait() accepts a single non-busy status sample. On these chips,
the ready bit can briefly clear before the device is actually ready,
allowing a premature cache read or a new program/erase command. The
fallback status read for controllers with poll_status offload does not
provide a second ready confirmation in the usual case.

Mark the affected SkyHigh parts with a quirk and require a second ready
status sample after spinand_wait() for those parts. If the chip becomes
busy again, repeat the wait while the outer timeout has not elapsed.
Each inner wait has its own timeout, so the quirk can wait up to roughly
twice as long. Leave other SPI-NAND parts unchanged.

Suggested-by: Mikhail Zhilkin <csharper2005@gmail.com>
Signed-off-by: Vitaliy Sochnev <sochnev.v.74@gmail.com>
---
Notes for reviewers:

The two ready samples are taken in separate SPI transactions, without an
explicit delay. The duration of the premature-ready interval has not been
measured. If both samples fall within that interval, the wait can still
return too early. Would a measured, explicit delay be preferable here?

The on-device result comes from a Nokia XG-040G-MD with a 256 MiB SkyHigh
chip running OpenWrt with this workaround. The linux-next port has only
been compile-tested; the other S35ML entries flagged by this patch have
not been tested on hardware with this workaround.

 drivers/mtd/nand/spi/core.c    | 85 +++++++++++++++++++++++++++-------
 drivers/mtd/nand/spi/skyhigh.c | 12 +++--
 include/linux/mtd/spinand.h    |  1 +
 3 files changed, 76 insertions(+), 22 deletions(-)

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;
+	} while ((status & STATUS_BUSY) && time_before(jiffies, timeo));
+
+	if (s)
+		*s = status;
+
+	return status & STATUS_BUSY ? -ETIMEDOUT : 0;
+}
+
 static int spinand_read_id_op(struct spinand_device *spinand, u8 naddr,
 			      u8 ndummy, u8 *buf)
 {
@@ -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);
 }
 
 static int spinand_lock_block(struct spinand_device *spinand, u8 lock)
@@ -767,10 +814,10 @@ int spinand_read_page(struct spinand_device *spinand,
 	if (ret)
 		return ret;
 
-	ret = spinand_wait(spinand,
-			   SPINAND_READ_INITIAL_DELAY_US,
-			   SPINAND_READ_POLL_DELAY_US,
-			   &status);
+	ret = spinand_wait_ready_confirmed(spinand,
+					   SPINAND_READ_INITIAL_DELAY_US,
+					   SPINAND_READ_POLL_DELAY_US,
+					   &status);
 	if (ret < 0)
 		return ret;
 
@@ -814,10 +861,10 @@ int spinand_write_page(struct spinand_device *spinand,
 	if (ret)
 		return ret;
 
-	ret = spinand_wait(spinand,
-			   SPINAND_WRITE_INITIAL_DELAY_US,
-			   SPINAND_WRITE_POLL_DELAY_US,
-			   &status);
+	ret = spinand_wait_ready_confirmed(spinand,
+					   SPINAND_WRITE_INITIAL_DELAY_US,
+					   SPINAND_WRITE_POLL_DELAY_US,
+					   &status);
 	if (ret)
 		return ret;
 
@@ -939,8 +986,10 @@ static int spinand_mtd_continuous_page_read(struct mtd_info *mtd, loff_t from,
 		if (ret)
 			goto end_cont_read;
 
-		ret = spinand_wait(spinand, SPINAND_READ_INITIAL_DELAY_US,
-				   SPINAND_READ_POLL_DELAY_US, NULL);
+		ret = spinand_wait_ready_confirmed(spinand,
+						   SPINAND_READ_INITIAL_DELAY_US,
+						   SPINAND_READ_POLL_DELAY_US,
+						   NULL);
 		if (ret < 0)
 			goto end_cont_read;
 
@@ -1196,10 +1245,10 @@ static int spinand_erase(struct nand_device *nand, const struct nand_pos *pos)
 	if (ret)
 		return ret;
 
-	ret = spinand_wait(spinand,
-			   SPINAND_ERASE_INITIAL_DELAY_US,
-			   SPINAND_ERASE_POLL_DELAY_US,
-			   &status);
+	ret = spinand_wait_ready_confirmed(spinand,
+					   SPINAND_ERASE_INITIAL_DELAY_US,
+					   SPINAND_ERASE_POLL_DELAY_US,
+					   &status);
 
 	if (!ret && (status & STATUS_ERASE_FAILED))
 		ret = -EIO;
diff --git a/drivers/mtd/nand/spi/skyhigh.c b/drivers/mtd/nand/spi/skyhigh.c
index bf9ce163e6a7b..19f2380b0427a 100644
--- a/drivers/mtd/nand/spi/skyhigh.c
+++ b/drivers/mtd/nand/spi/skyhigh.c
@@ -88,7 +88,8 @@ static const struct spinand_info skyhigh_spinand_table[] = {
 		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
 					      &write_cache_variants,
 					      &update_cache_variants),
-		     SPINAND_NO_RAW_ACCESS,
+		     SPINAND_NO_RAW_ACCESS |
+		     SPINAND_PREMATURE_READY_QUIRK,
 		     SPINAND_ECCINFO(&skyhigh_spinand_ooblayout,
 				     skyhigh_spinand_ecc_get_status)),
 	SPINAND_INFO("S35ML01G300",
@@ -98,7 +99,8 @@ static const struct spinand_info skyhigh_spinand_table[] = {
 		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
 					      &write_cache_variants,
 					      &update_cache_variants),
-		     SPINAND_NO_RAW_ACCESS,
+		     SPINAND_NO_RAW_ACCESS |
+		     SPINAND_PREMATURE_READY_QUIRK,
 		     SPINAND_ECCINFO(&skyhigh_spinand_ooblayout,
 				     skyhigh_spinand_ecc_get_status)),
 	SPINAND_INFO("S35ML02G300",
@@ -108,7 +110,8 @@ static const struct spinand_info skyhigh_spinand_table[] = {
 		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
 					      &write_cache_variants,
 					      &update_cache_variants),
-		     SPINAND_NO_RAW_ACCESS,
+		     SPINAND_NO_RAW_ACCESS |
+		     SPINAND_PREMATURE_READY_QUIRK,
 		     SPINAND_ECCINFO(&skyhigh_spinand_ooblayout,
 				     skyhigh_spinand_ecc_get_status)),
 	SPINAND_INFO("S35ML04G300",
@@ -118,7 +121,8 @@ static const struct spinand_info skyhigh_spinand_table[] = {
 		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
 					      &write_cache_variants,
 					      &update_cache_variants),
-		     SPINAND_NO_RAW_ACCESS,
+		     SPINAND_NO_RAW_ACCESS |
+		     SPINAND_PREMATURE_READY_QUIRK,
 		     SPINAND_ECCINFO(&skyhigh_spinand_ooblayout,
 				     skyhigh_spinand_ecc_get_status)),
 };
diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
index d7c99fcfb85ef..0767964275cf2 100644
--- a/include/linux/mtd/spinand.h
+++ b/include/linux/mtd/spinand.h
@@ -492,6 +492,7 @@ struct spinand_ecc_info {
 #define SPINAND_HAS_READ_PLANE_SELECT_BIT		BIT(3)
 #define SPINAND_NO_RAW_ACCESS				BIT(4)
 #define SPINAND_ODTR_PACKED_PAGE_READ			BIT(5)
+#define SPINAND_PREMATURE_READY_QUIRK			BIT(6)
 
 /**
  * struct spinand_ondie_ecc_conf - private SPI-NAND on-die ECC engine structure

base-commit: 7649ba73f9e7e1e04d0b46213e39c79ae09dd55c

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

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

[sashiko-bot]

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/