[PATCH v4] mtd: spi-nor: take the flash lock in spi_nor_restore()

[PATCH v4] mtd: spi-nor: take the flash lock in spi_nor_restore()

[Itai Handler]

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

Neither outcome of that race is good. While the chip is busy the
address mode exit is typically ignored, so the restore is silently
dropped and the chip is left in 4-byte addressing, which a kexec then
hands to a kernel that cannot read it. The soft reset is typically
accepted even while busy, so it instead aborts an in-flight erase part
way through.

Take the lock in spi_nor_restore(); both callers are teardown paths
that can sleep and neither already holds it. Waiting is deliberate: a
partially erased sector cannot be undone from here, and the wait is
bounded by the one operation already in flight. If the lock cannot be
taken the restore is skipped with a warning.

Requests arriving after the restore are a separate problem: they race
with device removal itself rather than with the restore.

Fixes: 59b356ffd0b0 ("mtd: m25p80: restore the status of SPI flash when exiting")
Cc: stable@vger.kernel.org
Assisted-by: Claude Opus 5
Signed-off-by: Itai Handler <itai.handler@gmail.com>
---
Changes in v4:
- Correct what v3 claimed a busy chip does. v3 said it "ignores
  everything but status reads", which is roughly right for the address
  mode exit but wrong for the soft reset: that one is accepted while
  busy and aborts an in-flight erase. This strengthens the case for
  taking the lock rather than weakening it, but the old wording was
  not accurate and the comment now says both halves.
- Lock inside spi_nor_restore() instead of adding a
  spi_nor_restore_locked() wrapper around it. The _locked suffix
  conventionally means "the caller holds the lock", which is the
  opposite of what that wrapper did, and spi_nor_restore() is static
  with only the two teardown callers.
- Handle the spi_nor_prep_and_lock() error return instead of ignoring
  it, and warn when the restore is skipped.
- Say what happens to an operation that is already in flight, which
  Michael asked about on v3. Short answer: it is waited for, because
  an erase cannot be cancelled safely.
- Rebased on mainline. v3 depended on the spi_nor_rww_start_exclusive()
  mutex leak fix, which has since merged as 44b8a0bf5f96 ("mtd:
  spi-nor: core: Fix mutex leak in spi_nor_rww_start_exclusive()"), so
  this no longer depends on anything unmerged. Note that it is not yet
  in a released tag: v7.3-rc5 does not have it.

Link to v3:
  https://lore.kernel.org/r/20260915113811.2429311-1-itai.handler@gmail.com
Link to v2:
  https://lore.kernel.org/r/20260914081149.1916589-1-itai.handler@gmail.com

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

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 8bc117b..94dbf1b 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -3455,6 +3455,21 @@ static void spi_nor_restore(struct spi_nor *nor)
 {
 	int ret;
 
+	/*
+	 * The MTD device is still registered here, so another thread may
+	 * have an operation in flight. Wait for it rather than cut in:
+	 * while the chip is busy the address mode exit below is typically
+	 * ignored, which silently loses the restore, and the soft reset is
+	 * typically accepted, which aborts an erase part way through.
+	 */
+	ret = spi_nor_prep_and_lock(nor);
+	if (ret) {
+		dev_warn(nor->dev,
+			 "Failed to lock flash, skipping restore, err = %d\n",
+			 ret);
+		return;
+	}
+
 	/* restore the addressing mode */
 	if (nor->addr_nbytes == 4 && !(nor->flags & SNOR_F_4B_OPCODES) &&
 	    nor->flags & SNOR_F_BROKEN_RESET) {
@@ -3470,6 +3485,8 @@ static void spi_nor_restore(struct spi_nor *nor)
 
 	if (nor->flags & SNOR_F_SOFT_RESET)
 		spi_nor_soft_reset(nor);
+
+	spi_nor_unlock_and_unprep(nor);
 }
 
 static const struct flash_info *spi_nor_match_name(struct spi_nor *nor,

base-commit: 6addb4f385570ebc11c4eb499a4f1c149f313e84

Re: [PATCH v4] mtd: spi-nor: take the flash lock in spi_nor_restore()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mtd: spi-nor: hardware restore skipped on fatal signal during teardown
- [High] mtd: spi-nor: shutdown and unbind hang waiting for multi-sector erase
--

commit 470af3f1f5f19ab1426ba1f38a4c4420e0274e2b
Author: Itai Handler <itai.handler@gmail.com>

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

This commit takes the flash lock during teardown to avoid racing with
in-flight operations. It waits for active operations to complete so that
exiting 4-byte address mode and soft resets are not ignored or do not
abort an ongoing erase. If the lock cannot be acquired, the hardware
restore is skipped.

> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 8bc117b46e028..94dbf1be776f8 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3455,6 +3455,21 @@ static void spi_nor_restore(struct spi_nor *nor)
>  {
>  	int ret;
>  
> +	/*
> +	 * The MTD device is still registered here, so another thread may
> +	 * have an operation in flight. Wait for it rather than cut in:
> +	 * while the chip is busy the address mode exit below is typically
> +	 * ignored, which silently loses the restore, and the soft reset is
> +	 * typically accepted, which aborts an erase part way through.
> +	 */
> +	ret = spi_nor_prep_and_lock(nor);

[Severity: High]
Does this introduce a potential deadlock during shutdown or driver unbind?

The commit message notes that the wait is bounded by the one operation
already in flight. However, looking at spi_nor_erase(), the exclusive lock
is acquired before looping over all sectors and is held continuously across
the entire multi-sector operation:

spi_nor_erase()
    spi_nor_prep_and_lock_pe(nor, instr->addr, instr->len);
    ...
    while (len) {
        spi_nor_lock_device(nor);
        spi_nor_erase_sector(nor, addr);
        spi_nor_unlock_device(nor);
        ...
    }

Since spi_nor_prep_and_lock() calls spi_nor_rww_start_exclusive() to wait
for the ongoing erase to finish, it appears this will block the teardown
thread until the entire batch operation completes. For a full-chip erase,
could this stall system shutdown or driver unbind for several minutes?

> +	if (ret) {
> +		dev_warn(nor->dev,
> +			 "Failed to lock flash, skipping restore, err = %d\n",
> +			 ret);
> +		return;
> +	}

[Severity: High]
Can a fatal signal cause the device to be left in an incompatible state?

If a long-running erase is taking place during unbind, and a user sends
SIGKILL to abort the hang, wait_event_killable() inside
spi_nor_prep_and_lock() will return -ERESTARTSYS.

Because this error path skips the restore entirely, the driver core will
continue with device unregistration while the hardware is left stuck in
4-byte address mode. Will this break subsequent driver rebinds or kexec
kernels that expect the default 3-byte mode?

> +
>  	/* restore the addressing mode */
>  	if (nor->addr_nbytes == 4 && !(nor->flags & SNOR_F_4B_OPCODES) &&
>  	    nor->flags & SNOR_F_BROKEN_RESET) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261004104311.2196527-1-itai.handler@gmail.com?part=1