mtd: spi-nor: take the flash lock around spi_nor_restore()

PATCH Superseded Itai Handler 3 participants 4 messages Sep 15 – Sep 18

Subsystems (3)

Stable branch Memory technology devices (MTD) SPI NOR

lore

Share & tools

lore

Itai Handler

author

2026-09-15 11:38 UTC #0 lore raw

PATCH v3 1/1 mtd: spi-nor: take the flash lock around spi_nor_restore()

spi_nor_shutdown() and spi_nor_remove() call spi_nor_restore() without nor->lock, which every other path to the chip takes through spi_nor_prep_and_lock(). Both run with the MTD device still registered, so another thread can be in the middle of an operation.

A busy flash ignores everything but status reads, so the restore is silently dropped and the chip is left in 4-byte mode. A restore landing between two chunks of a read switches the chip to 3-byte addressing while spi_nor_read() carries on sending four address bytes.

Take the lock, so the restore runs between operations instead of during one. This narrows the race rather than closing it: an operation starting afterwards still addresses a 3-byte chip with nor->addr_nbytes left at 4.

Fixes 59b356ffd0b0 ("mtd: m25p80: restore the status of SPI flash when exiting")

Cc [email protected]

Assisted-by LLM

Signed-off-by Itai Handler <[email protected]>

---
v2 1/3, the spi_nor_rww_start_exclusive() lock fix, is dropped from this series now that it is queued in spi-nor/next. This patch still depends on it: it adds the first ->shutdown and ->remove callers of the exclusive lock, so an RWW flash would deadlock without it. Please let it reach stable first.

Changes in v3:
- Fold the spi_nor_remove() patch into this one, as requested by Michael Walle. Both call sites now share a small helper.
- Cut the commit message down.
- Add an Assisted-by: tag, as requested by Michael Walle.
- Drop patch 1/3, queued in spi-nor/next.
- Link to v2:
https://lore.kernel.org/r/[email protected]

 drivers/mtd/spi-nor/core.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

drivers/mtd/spi-nor/core.c +17 -2

index ccf4396cdcd0..c891197ffa5b 100644
@@ -3849,11 +3849,26 @@ static int spi_nor_probe(struct spi_mem *spimem)
 				   data ? data->nr_parts : 0);
 }
 
+/*
+ * Restore between operations, not during one. Removal and shutdown both run
+ * with MTD users still attached: a busy flash silently ignores the commands
+ * spi_nor_restore() sends, and a restore landing inside a read changes the
+ * chip's address width under the transfer.
+ */
+static void spi_nor_restore_locked(struct spi_nor *nor)
+{
+	if (spi_nor_prep_and_lock(nor))
+		return;
+
+	spi_nor_restore(nor);
+	spi_nor_unlock_and_unprep(nor);
+}
+
 static int spi_nor_remove(struct spi_mem *spimem)
 {
 	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
 
-	spi_nor_restore(nor);
+	spi_nor_restore_locked(nor);
 
 	/* Clean up MTD stuff. */
 	return mtd_device_unregister(&nor->mtd);

@@ -3863,7 +3878,7 @@ static void spi_nor_shutdown(struct spi_mem *spimem)
 {
 	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
 
-	spi_nor_restore(nor);
+	spi_nor_restore_locked(nor);
 }
 
 /*

base-commit: 50d05c7c76c96b90462f24debacca971d2e86713

Signature · 1 line

2.34.1

[email protected]

AI review

2026-09-15 11:52 UTC #1 lore raw

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] mtd: spi-nor: skipped flash restore on fatal signal breaks boot - [High] mtd: spi-nor: shutdown…

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mtd: spi-nor: skipped flash restore on fatal signal breaks boot
- [High] mtd: spi-nor: shutdown hang due to ongoing erase operation
--

commit c0578fac7581b63c39bfd99ac3a6f934256685d3
Author: Itai Handler <[email protected]>

mtd: spi-nor: take the flash lock around spi_nor_restore()

This commit changes spi_nor_shutdown() and spi_nor_remove() to acquire nor->lock around spi_nor_restore(). It aims to prevent operations like reads from overlapping with the restore, which could leave the chip in 3-byte addressing mode.

Quoted 8 lines

drivers/mtd/spi-nor/core.c +0 -0

index ccf4396cdcd04..c891197ffa5b3 100644
@@ -3849,11 +3849,26 @@ static int spi_nor_probe(struct spi_mem *spimem)
 				   data ? data->nr_parts : 0);
 }

drivers/mtd/spi-nor/core.c static int spi_nor_probe(struct spi_mem *spimem) · line 3861

Quoted 10 lines

drivers/mtd/spi-nor/core.c +10 -0

+/*
+ * Restore between operations, not during one. Removal and shutdown both run
+ * with MTD users still attached: a busy flash silently ignores the commands
+ * spi_nor_restore() sends, and a restore landing inside a read changes the
+ * chip's address width under the transfer.
+ */
+static void spi_nor_restore_locked(struct spi_nor *nor)
+{
+	if (spi_nor_prep_and_lock(nor))
+		return;

[Severity: High]
Does this code silently skip the flash restore if a fatal signal breaks the lock acquisition?

For example, if a privileged user runs rmmod while an erase or read operation is actively holding the RWW lock, and the rmmod process receives SIGKILL, spi_nor_prep_and_lock() (via wait_event_killable()) will return -ERESTARTSYS:

drivers/mtd/spi-nor/core.c:spi_nor_prep_and_lock() {
    ...
    if (!spi_nor_use_parallel_locking(nor))
        mutex_lock(&nor->lock);
    else
        ret = wait_event_killable(nor->rww.wait,
                                  spi_nor_rww_start_exclusive(nor));

    return ret;
}

Bypassing the hardware teardown path here leaves the flash in 4-byte addressing mode. This might cause the bootloader to fail to read the kernel on the next reboot, soft-bricking the device until a power cycle.

Quoted 5 lines

drivers/mtd/spi-nor/core.c +5 -0

+
+	spi_nor_restore(nor);
+	spi_nor_unlock_and_unprep(nor);
+}
+

[ ... ]

drivers/mtd/spi-nor/core.c static void spi_nor_shutdown(struct spi_mem *spimem) · line 3881

Quoted 6 lines

drivers/mtd/spi-nor/core.c +1 -1

@@ -3863,7 +3878,7 @@ static void spi_nor_shutdown(struct spi_mem *spimem)
 {
 	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
 
-	spi_nor_restore(nor);
+	spi_nor_restore_locked(nor);

[Severity: High]
Could this hang the system indefinitely during shutdown if an erase operation is ongoing?

Erase operations (like spi_nor_erase_multi_sectors) hold nor->lock continuously for their entire duration, looping over spi_nor_wait_till_ready() with cond_resched(). A large chip erase can take several minutes.

Because device_shutdown() iterates devices synchronously without a timeout, this synchronous wait for nor->lock could hang the reboot process until the erase finishes, forcing the user to hard-reset the machine.

Quoted 3 lines

drivers/mtd/spi-nor/core.c +0 -0
 }
 
 /*

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

Michael Walle

maintainer

2026-09-18 09:34 UTC #2 lore raw

Quoted 3 lines · sashiko-bot

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mtd: spi-nor: skipped flash restore on fatal signal breaks boot
- [High] mtd: spi-nor: shutdown hang due to ongoing erase operation

Does Sashiko have some valid points here?

-michael

Quoted 73 lines

--
commit c0578fac7581b63c39bfd99ac3a6f934256685d3
Author: Itai Handler <[email protected]>
mtd: spi-nor: take the flash lock around spi_nor_restore()
This commit changes spi_nor_shutdown() and spi_nor_remove() to acquire nor->lock around spi_nor_restore(). It aims to prevent operations like reads from overlapping with the restore, which could leave the chip in 3-byte addressing mode.
Quoted 8 lines
drivers/mtd/spi-nor/core.c +0 -0
index ccf4396cdcd04..c891197ffa5b3 100644
@@ -3849,11 +3849,26 @@ static int spi_nor_probe(struct spi_mem *spimem)
 				   data ? data->nr_parts : 0);
 }
 
drivers/mtd/spi-nor/core.c static int spi_nor_probe(struct spi_mem *spimem) · line 3861
Quoted 10 lines
drivers/mtd/spi-nor/core.c +10 -0
+/*
+ * Restore between operations, not during one. Removal and shutdown both run
+ * with MTD users still attached: a busy flash silently ignores the commands
+ * spi_nor_restore() sends, and a restore landing inside a read changes the
+ * chip's address width under the transfer.
+ */
+static void spi_nor_restore_locked(struct spi_nor *nor)
+{
+	if (spi_nor_prep_and_lock(nor))
+		return;
[Severity: High]
Does this code silently skip the flash restore if a fatal signal breaks the lock acquisition?
For example, if a privileged user runs rmmod while an erase or read operation is actively holding the RWW lock, and the rmmod process receives SIGKILL, spi_nor_prep_and_lock() (via wait_event_killable()) will return -ERESTARTSYS:
drivers/mtd/spi-nor/core.c:spi_nor_prep_and_lock() {
    ...
    if (!spi_nor_use_parallel_locking(nor))
        mutex_lock(&nor->lock);
    else
        ret = wait_event_killable(nor->rww.wait,
                                  spi_nor_rww_start_exclusive(nor));

    return ret;
}
Bypassing the hardware teardown path here leaves the flash in 4-byte addressing mode. This might cause the bootloader to fail to read the kernel on the next reboot, soft-bricking the device until a power cycle.
Quoted 5 lines
drivers/mtd/spi-nor/core.c +5 -0
+
+	spi_nor_restore(nor);
+	spi_nor_unlock_and_unprep(nor);
+}
+
[ ... ]
drivers/mtd/spi-nor/core.c static void spi_nor_shutdown(struct spi_mem *spimem) · line 3881
Quoted 6 lines
drivers/mtd/spi-nor/core.c +1 -1
@@ -3863,7 +3878,7 @@ static void spi_nor_shutdown(struct spi_mem *spimem)
 {
 	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
 
-	spi_nor_restore(nor);
+	spi_nor_restore_locked(nor);
[Severity: High]
Could this hang the system indefinitely during shutdown if an erase operation is ongoing?

Quoted 7 lines

Erase operations (like spi_nor_erase_multi_sectors) hold nor->lock continuously for their entire duration, looping over spi_nor_wait_till_ready() with cond_resched(). A large chip erase can take several minutes.
Because device_shutdown() iterates devices synchronously without a timeout, this synchronous wait for nor->lock could hang the reboot process until the erase finishes, forcing the user to hard-reset the machine.

Also I'm not sure, what would be the actual expected behavior here. Cancel the erase? Wait until the erase is completed? Pratyush, Miquel, do you have any thoughts on that?

-michael

Quoted 4 lines

Quoted 4 lines
drivers/mtd/spi-nor/core.c +0 -0
 }
 
 /*

Itai Handler

author

2026-09-18 15:29 UTC #3 lore raw

Quoted 7 lines · Michael Walle

Quoted 3 lines · sashiko-bot
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mtd: spi-nor: skipped flash restore on fatal signal breaks boot
- [High] mtd: spi-nor: shutdown hang due to ongoing erase operation
Does Sashiko have some valid points here?

Here is my perspective on the two points raised by Sashiko:

Signal Interruption during Lock Acquisition (Issue 1):
Sashiko's point is valid, though this scenario only applies to RWW chips where spi_nor_prep_and_lock() uses wait_event_killable(). The window for this to occur is quite narrow (e.g., sending SIGKILL to rmmod while an operation is in progress).
If lock acquisition is interrupted, calling spi_nor_restore() without the lock as a fallback risks having the command silently ignored by a busy flash. However, it may still be worth attempting as a best-effort fallback alongside a kernel warning log to notify the user.

Erase Duration during Shutdown (Issue 2):
Waiting for active operations to complete before restoring 3-byte mode is intentional. Interrupting an ongoing erase or write cycle risks corrupting the flash or leaving the chip in an undefined state. Allowing active operations to finish before teardown is standard behavior to guarantee data integrity.

If this rationale makes sense, I can send a v4 that adds a best-effort spi_nor_restore() call and a kernel warning on lock acquisition failure.

Itai