Re: [PATCH v2 2/3] mtd: spi-nor: take the flash lock in spi_nor_shutdown()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Returning early from `spi_nor_shutdown()` when `spi_nor_prep_and_lock()` is interrupted by a signal skips critical hardware restoration, potentially bricking the device on next boot.

Pre-existing issues:
- [High] Missing `spi_nor_unprep()` on the error path of `spi_nor_prep_and_lock()` and its variants causes controller resource leaks and potential deadlocks.
--

commit 45363f008a63391ea1e85be5cb9b77e5c2d59277
Author: Itai Handler <itai.handler@gmail.com>

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

This commit acquires the flash lock in spi_nor_shutdown() to serialize
device restoration with concurrent operations. This prevents the restore
command from being ignored by a busy flash chip or corrupting an ongoing
read operation during system shutdown.

> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 8bc117b46e028..647bf8dce7193 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3862,8 +3862,21 @@ static int spi_nor_remove(struct spi_mem *spimem)
>  static void spi_nor_shutdown(struct spi_mem *spimem)
>  {
>  	struct spi_nor *nor = spi_mem_get_drvdata(spimem);
> +	int ret;
> +
> +	/*
> +	 * Wait for an operation started by another thread to finish.
> +	 * device_shutdown() runs with MTD users still active: a busy flash
> +	 * ignores the commands spi_nor_restore() issues, leaving it in
> +	 * 4-byte address mode, and a restore landing mid-read changes the
> +	 * chip's address width under the transfer.
> +	 */
> +	ret = spi_nor_prep_and_lock(nor);
> +	if (ret)
> +		return;

[Severity: High]
If wait_event_killable() in spi_nor_prep_and_lock() is interrupted by a fatal
signal, it returns -ERESTARTSYS. Will returning early here skip the critical
hardware restoration in spi_nor_restore()?

If the flash chip is left in 4-byte addressing mode, could the bootloader
read garbage data on the next boot, effectively soft-bricking the device
until a hard power cycle?

[Severity: High]
This isn't a bug introduced by this patch, but does spi_nor_prep_and_lock()
leak controller resources on its error path?

Looking at spi_nor_prep_and_lock() in drivers/mtd/spi-nor/core.c:

int spi_nor_prep_and_lock(struct spi_nor *nor)
{
	int ret;

	ret = spi_nor_prep(nor);
	if (ret)
		return ret;

	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;
}

If wait_event_killable() is interrupted and returns an error, the function
returns without calling spi_nor_unprep(nor). Could this permanently leak the
hardware locks and clocks enabled in spi_nor_prep(), deadlocking the
controller for all subsequent SPI operations?

The same behavior appears to be present in the spi_nor_prep_and_lock_pe() and
spi_nor_prep_and_lock_rd() variants.

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

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

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