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@xxxxxxxxx>

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@xxxxxxxxx?part=1