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

[Miquel Raynal <miquel.raynal@bootlin.com>]

On 14/09/2026 at 11:11:48 +03, Itai Handler <itai.handler@gmail.com> wrote:

> spi_nor_shutdown() calls spi_nor_restore() to put the flash back into
> 3-byte addressing before the system reboots or kexecs. It does so
> without taking nor->lock, which every other path that talks to the chip
> acquires through spi_nor_prep_and_lock().
>
> device_shutdown() does not freeze userspace and does not stop kernel
> threads; it walks the device list calling ->shutdown with all CPUs
> online. Another thread can therefore be in the middle of an operation,
> with the restore running concurrently with it. A write and a read are
> both damaged, in different ways.
>
> A program or erase leaves the flash busy, and a busy flash accepts only
> status register reads and ignores everything else, including the EX4B
> that spi_nor_restore() sends. Neither spi_nor_write_enable() nor
> spi_nor_set_4byte_addr_mode() reads anything back, so the restore
> reports success while the flash is left in 4-byte addressing. The next
> boot stage then addresses it with 3 bytes and reads the wrong data,
> which is the failure commit 59b356ffd0b0 ("mtd: m25p80: restore the
> status of SPI flash when exiting") introduced this restore to prevent.
>
> A read, by contrast, does not ignore the restore - it is corrupted by
> it. spi_nor_read() holds the lock across a loop that issues one
> spi_nor_read_data() per chunk, each using nor->addr_nbytes.
> spi_nor_set_4byte_addr_mode() updates nor->params->addr_nbytes and not
> nor->addr_nbytes, so a restore landing between two chunks switches the
> chip to 3-byte addressing while the driver carries on sending 4 address
> bytes. The rest of the transfer is addressed wrongly and returns wrong
> data, and nothing reports an error. A restore may also soft reset the
> chip in the middle of that same read.
>
> Take nor->lock for the restore, so it runs between operations instead of
> during one: a program or erase has finished waiting on the chip, and a
> read has issued its last chunk. This is a locking fix rather than a
> missing wait - each operation already waits for completion at the site
> that started it.
>
> This narrows the race without closing it. The restore still runs while
> MTD users are attached, so an operation that starts after it has
> completed will address a chip that is now in 3-byte mode while
> nor->addr_nbytes is still 4. Closing that as well would mean having MTD
> stop accepting operations before ->shutdown runs, which is a larger
> change; serialising against the operations already in flight is what
> keeps the restore itself from being issued into a busy chip.

6 paragraphs to tell "Accesses to the flash should be
serialized". Please reduce it.

> Fixes: 59b356ffd0b0 ("mtd: m25p80: restore the status of SPI flash when exiting")
> Cc: stable@vger.kernel.org
> Signed-off-by: Itai Handler <itai.handler@gmail.com>
> ---
>  drivers/mtd/spi-nor/core.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 8bc117b46e02..647bf8dce719 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.
> +	 */

I'm fine with the diff, but please drop this comment.

Thanks,
Miquèl


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