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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] spi: spi-mem: panic write deadlocks system by calling spi_mem_access_start
--

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

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

Add a panic_write flag to the controller capabilities. When set, this
indicates the SPI controller can send commands to a connected NOR or NAND
memory in the context of a kernel panic, without sleeping or relying on IRQs.

> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> index b94680b60af95..89710f9f555ed 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -398,6 +398,31 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>  	u8 *tmpbuf;
>  	int ret;
>  
> +	if (op->panic_write) {
> +		if (!spi_mem_controller_is_capable(ctlr, panic_write))
> +			return -EOPNOTSUPP;
> +
> +		/*
> +		 * Only support panic writes if we'll go through .exec_op().
> +		 * The regular SPI path (spi_sync(), etc.) explicitly mentions
> +		 * that it can only be used from a context that may sleep, which
> +		 * is not the case on a kernel panic.
> +		 * If this changes in the future, this could be reworked, but
> +		 * for now this will have to do.
> +		 */
> +		if (WARN_ON_ONCE(!ctlr->mem_ops || !ctlr->mem_ops->exec_op))
> +			return -EOPNOTSUPP;
> +
> +		if (spi_get_csgpiod(mem->spi, 0))
> +			return -EPERM;
> +
> +		/* On panic IRQs are disabled so we cannot wait on locks */
> +		if (mutex_is_locked(&ctlr->bus_lock_mutex) ||
> +		    mutex_is_locked(&ctlr->io_mutex)) {
> +			return -EPERM;
> +		}
> +	}

[Severity: High]
Does this unconditionally invoke sleepable operations during a panic write?

If the mutexes are currently unlocked when op->panic_write is true,
spi_mem_exec_op() proceeds and calls spi_mem_access_start():

drivers/spi/spi-mem.c:spi_mem_exec_op() {
    ...
    if (ctlr->mem_ops && ctlr->mem_ops->exec_op && !spi_get_csgpiod(mem->spi, 0)) {
        ret = spi_mem_access_start(mem);
    ...
}

spi_mem_access_start() explicitly calls mutex_lock(), which violates atomic
constraints and triggers might_sleep() warnings, and calls
pm_runtime_resume_and_get(), which may sleep.

Additionally, spi_mem_access_start() unconditionally calls spi_flush_queue().
If the SPI message queue is not empty (for example, from an async transfer
queued via spi_async() right before the panic), spi_flush_queue() dequeues
and executes pending regular SPI transfers using the driver standard
callback:

spi_mem_access_start() ->
    spi_flush_queue() ->
        __spi_pump_messages() ->
            .transfer_one_message()

For cadence-xspi (the driver supporting panic writes here), the callback
invokes usleep_range() for polling delays.

Executing usleep_range() or other sleepable wait mechanisms in a panic
context where interrupts and preemption are disabled will result in a
system deadlock, preventing the crash dump or panic logging from
completing.

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

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