Re: [PATCH 2/5] mtd: spi-nor: Add support for panic writes

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mtd: spi-nor: infinite loop in panic write timeout via jiffies
- [High] mtd: spi-nor: panic write ignores ongoing_pe causing hardware conflict
- [High] mtd: spi-nor: sleep in atomic context during panic write
--

commit 6866cee3049043ab91dbf3b45f823a4010bb97cc
Author: Tanmay Jagdale <tanmay@xxxxxxxxxxx>

mtd: spi-nor: Add support for panic writes

This commit adds support for panic writes to the SPI NOR framework. It
plumbs the oops_panic_write flag through to the SPI controllers,
avoiding mutexes and blocking calls when preemption and IRQs are
disabled.

> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c

[ ... ]

> @@ -729,7 +738,10 @@ static int spi_nor_wait_till_ready_with_timeout(struct spi_nor *nor,
>  		if (ret)
>  			return 0;
>  
> -		cond_resched();
> +		if (nor->mtd.oops_panic_write)
> +			cpu_relax();

[Severity: High]
In spi_nor_wait_till_ready_with_timeout(), the loop condition relies on:

if (time_after_eq(jiffies, deadline))

Since jiffies does not advance in a panic context because local IRQs are
disabled, does this mean the system will hang indefinitely during a panic
if the flash chip takes time to complete or becomes unresponsive?

> +		else
> +			cond_resched();
>  	}

[ ... ]

> @@ -3393,6 +3429,26 @@ static void spi_nor_soft_reset(struct spi_nor *nor)
>  	usleep_range(SPI_NOR_SRST_SLEEP_MIN, SPI_NOR_SRST_SLEEP_MAX);
>  }
>  
> +static int spi_nor_panic_write(struct mtd_info *mtd, loff_t to, size_t len,
> +			       size_t *retlen, const u_char *buf)
> +{
> +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> +
> +	/*
> +	 * At this point preemption and local interrupts are disabled, so we
> +	 * can't get the lock if it's taken.
> +	 */
> +	if (mutex_is_locked(&nor->lock))
> +		return -EPERM;
> +
> +	if (spi_nor_use_parallel_locking(nor) &&
> +	    (nor->rww.ongoing_io || nor->rww.ongoing_rd)) {
> +		return -EPERM;
> +	}

[Severity: High]
Does this omit checking nor->rww.ongoing_pe?

If a panic occurs while a long-running erase or program operation is in
progress, ongoing_pe will be true. If the lock was released during the
parallel hardware operation, the panic write might proceed and send write
commands while the hardware write in progress bit is set.

The hardware will ignore the commands, but the software will loop in
spi_nor_wait_till_ready() until the original operation finishes, incorrectly
assuming the panic dump was successfully written.

> +
> +	return spi_nor_write(mtd, to, len, retlen, buf);

[Severity: High]
Can this sequence lead to a sleep-in-atomic bug?

spi_nor_write() can call down to spi_nor_octal_dtr_write() which does a
sleepable memory allocation when unaligned addresses are passed:

tmp_buf = kmalloc(nor->params->page_size, GFP_KERNEL);

Additionally, the write eventually reaches spi_mem_exec_op() and then
spi_mem_access_start(), which unconditionally takes mutexes:

mutex_lock(&ctlr->bus_lock_mutex);
mutex_lock(&ctlr->io_mutex);

Since mtd->_panic_write operates with local IRQs and preemption disabled,
would these paths cause a recursive panic and completely halt the crash
dump process?

> +}
> +
>  /* mtd suspend handler */
>  static int spi_nor_suspend(struct mtd_info *mtd)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005081141.33688-1-paul.cercueil@xxxxxxxxxxx?part=2