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

["Michael Walle" <mwalle@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3548 bytes --]

On Tue Sep 15, 2026 at 1:52 PM CEST, sashiko-bot wrote:
> 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

Does Sashiko have some valid points here? 

-michael

> --
>
> commit c0578fac7581b63c39bfd99ac3a6f934256685d3
> Author: Itai Handler <itai.handler@gmail.com>
>
> 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.

Also I'm not sure, what would be the actual expected behavior here.
Cancel the erase? Wait until the erase is completed? Pratyush,
Miquel, do you have any thoughts on that?

-michael

>
>>  }
>>  
>>  /*


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]