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

[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
--

commit c0578fac7581b63c39bfd99ac3a6f934256685d3
Author: Itai Handler <itai.handler@xxxxxxxxx>

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.

> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index ccf4396cdcd04..c891197ffa5b3 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -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;

[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.

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

[ ... ]

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

>  }
>  
>  /*

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