Re: [PATCH v2 1/7] spi: spi-mem: Add support for panic mem writes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] spi: spi-mem: inverted pm_runtime_get_if_active() return check
- [High] spi: spi-mem: fallback path deadlocks during panic_write on -EOPNOTSUPP
- [High] spi: deadlock in panic context from pm_runtime_get_if_active()
--

commit 3d801ffb98468bd4b61d20480140ce8dbbcf4ae2
Author: Tanmay Jagdale <tanmay@marvell.com>

spi: spi-mem: Add support for panic mem writes

Add a 'panic_write' flag to the controller's capabilities struct.
When set, the SPI controller indicates that it will be able to send
commands to a connected NOR or NAND memory in the context of a kernel
panic, which means without sleeping and without relying on IRQs.

> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> index b94680b60af95..3b6fa44e45504 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c

[ ... ]

> @@ -420,7 +445,10 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>  		return -EOPNOTSUPP;
>  
>  	if (ctlr->mem_ops && ctlr->mem_ops->exec_op && !spi_get_csgpiod(mem->spi, 0)) {
> -		ret = spi_mem_access_start(mem);
> +		if (!op->panic_write)
> +			ret = spi_mem_access_start(mem);
> +		else if (ctlr->auto_runtime_pm)
> +			ret = pm_runtime_get_if_active(ctlr->dev.parent);
>  		if (ret)
>  			return ret;

[Severity: High]
Is it safe to call pm_runtime_get_if_active() here during a kernel panic?

pm_runtime_get_if_active() relies on pm_runtime_get_conditional(), which
unconditionally acquires dev->power.lock via spin_lock_irqsave(). In a panic
context with NMIs or IRQs disabled, trying to acquire a spinlock that may be
held by a halted CPU causes an infinite spin. On PREEMPT_RT, spin_lock_irqsave()
maps to a sleeping rt_mutex, which is strictly forbidden in atomic context.

[Severity: High]
Does this logic correctly handle the return value of pm_runtime_get_if_active()?

The API pm_runtime_get_if_active() returns 1 if the device is active (and its
usage count is incremented), 0 if inactive, and a negative errno on failure. By
evaluating if (ret), the intended logic is inverted in two ways:

1. If the device is active (returns 1), the condition evaluates to true, causing
the function to prematurely return 1. The panic write is aborted, and because
execution exits early, the pm_runtime_put_noidle() clean-up is skipped, leaking
the PM usage counter permanently.

2. If the device is suspended (returns 0), the condition evaluates to false.
Execution erroneously proceeds to perform SPI operations on powered-down
hardware.

> @@ -428,7 +456,10 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>  		ret = ctlr->mem_ops->exec_op(mem, op);
>  		trace_spi_mem_stop_op(mem, op);
>  
> -		spi_mem_access_end(mem);
> +		if (!op->panic_write)
> +			spi_mem_access_end(mem);
> +		else if (ctlr->auto_runtime_pm)
> +			pm_runtime_put_noidle(ctlr->dev.parent);

[Severity: High]
Following up on the return value logic above, if pm_runtime_get_if_active()
returned 0 (device suspended), does this incorrectly drop a reference?

Since the function proceeded on 0, this pm_runtime_put_noidle() drops a PM
reference that was never actually acquired, resulting in an unbalanced decrement
and an underflow of the usage counter.

>  		/*
>  		 * Some controllers only optimize specific paths (typically the
>  		 * read path) and expect the core to use the regular SPI
>  		 * interface in other cases.
>  		 */
>  		if (!ret || (ret != -ENOTSUPP && ret != -EOPNOTSUPP)) {
>  			spi_mem_add_op_stats(ctlr->pcpu_statistics, op, ret);
>  			spi_mem_add_op_stats(mem->spi->pcpu_statistics, op, ret);
>  
>  			return ret;
>  		}
>  	}

[Severity: High]
If the controller's exec_op implementation dynamically returns -EOPNOTSUPP
during a panic write, could this fallback path cause a deadlock?

Operations executed during a kernel panic must not sleep or wait on completions.
If this check fails, execution bypasses the return and falls through to the
generic SPI message path:

tmpbuf = kzalloc(tmpbufsize, GFP_KERNEL | GFP_DMA);
[ ... ]
ret = spi_sync(mem->spi, &msg);

This fallback path unconditionally allocates memory with GFP_KERNEL and executes
spi_sync(), which waits on completions with interrupts disabled. Hitting this
fallthrough path during a panic will cause a hard deadlock and prevent the panic
dump from completing.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013141350.189996-1-paul.cercueil@bootlin.com?part=1

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