Re: [PATCH AUTOSEL 6.18] spi: spi-nxp-fspi: propagate clock reconfig failures in nxp_fspi_select_mem()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Hardware chip-select configuration is desynchronized from software tracking state on clock reconfiguration failure.
- [High] PM runtime state remains active while hardware clocks are left disabled on clock enable failure, causing a subsequent synchronous external abort.

Pre-existing issues:
- [High] Teardown asymmetry leaves the IRQ handler active after hardware clocks are disabled, risking an external abort.
- [High] Unchecked pm_runtime_get_sync() in nxp_fspi_cleanup() can lead to a synchronous external abort.
- [High] The interrupt handler accesses hardware registers without verifying the PM active state, violating subsystem guidelines and risking an external abort.
--

commit 90f6f57222227859ff0d0df74b2ed0073979680a
Author: Haibo Chen <haibo.chen@nxp.com>

spi: spi-nxp-fspi: propagate clock reconfig failures in nxp_fspi_select_mem()

This commit modifies nxp_fspi_select_mem() to return an integer error code
instead of void. It ensures that if clock reconfiguration fails, the failure
is propagated to nxp_fspi_exec_op(), which then bails out to avoid triggering
a synchronous external abort when accessing gated hardware registers.

Link: https://patch.msgid.link/20260728-fspi-clock-v2-3-dbe786a4a6eb@nxp.com

> diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
> index 50a7e4916a600..f672619c16afb 100644
> --- a/drivers/spi/spi-nxp-fspi.c
> +++ b/drivers/spi/spi-nxp-fspi.c
[ ... ]
> @@ -804,7 +804,7 @@ static void nxp_fspi_select_mem(struct nxp_fspi *f, struct spi_device *spi,
>  	if ((f->selected == spi_get_chipselect(spi, 0)) &&
>  	    (!!(f->flags & FSPI_DTR_MODE) == op_is_dtr) &&
>  	    (f->pre_op_rate == op->max_freq))
> -		return;
> +		return 0;

[Severity: High]
Can hardware chip-select configuration become desynchronized from the software
tracking state if a clock reconfiguration failure occurs?

If clk_set_rate() fails during a chip select change in nxp_fspi_select_mem()
in drivers/spi/spi-nxp-fspi.c:

nxp_fspi_select_mem()
    fspi_writel(f, size_kb, f->iobase + FSPI_FLSHA1CR0 +
                4 * spi_get_chipselect(spi, 0));

Hardware is reprogrammed to the new chip select, and all other chip selects are
cleared to 0 size.

    ret = clk_set_rate(f->clk, rate);
    if (ret)
        return ret;

The function then returns an error, bypassing the state update:

    f->selected = spi_get_chipselect(spi, 0);

On the next operation for the old chip select, wouldn't this early return check
incorrectly succeed (since f->selected still matches the old chip select),
bypassing hardware reconfiguration while the hardware is actually configured for
the new chip select?

[ ... ]
> @@ -836,12 +836,19 @@ static void nxp_fspi_select_mem(struct nxp_fspi *f, struct spi_device *spi,
>  	nxp_fspi_clk_disable_unprep(f);
>  
>  	ret = clk_set_rate(f->clk, rate);
> -	if (ret)
> -		return;
> +	if (ret) {
> +		/*
> +		 * clk_set_rate() failed with the clocks already disabled.
> +		 * Re-enable them so the enable count matches what the caller's
> +		 * pm_runtime_put() (runtime_suspend) will drop.
> +		 */
> +		nxp_fspi_clk_prep_enable(f);
> +		return ret;
> +	}
>  
>  	ret = nxp_fspi_clk_prep_enable(f);
>  	if (ret)
> -		return;
> +		return ret;
[ ... ]
> @@ -1042,7 +1051,16 @@ static int nxp_fspi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>  				   FSPI_STS0_ARB_IDLE, 1, POLL_TOUT, true);
>  	WARN_ON(err);
>  
> -	nxp_fspi_select_mem(f, mem->spi, op);
> +	err = nxp_fspi_select_mem(f, mem->spi, op);
> +	if (err) {
> +		/*
> +		 * On failure the FlexSPI clock may be left disabled, so avoid
> +		 * any further register access (which would trigger a synchronous
> +		 * external abort) and bail out.
> +		 */
> +		pm_runtime_put_autosuspend(f->dev);
> +		return err;
> +	}

[Severity: High]
If nxp_fspi_clk_prep_enable() fails, the function returns an error while the
hardware clocks are still physically disabled. 

When this happens, does nxp_fspi_exec_op() in drivers/spi/spi-nxp-fspi.c
correctly handle the PM runtime state?

nxp_fspi_exec_op()
    err = nxp_fspi_select_mem(f, mem->spi, op);
    if (err) {
        pm_runtime_put_autosuspend(f->dev);
        return err;
    }

Since autosuspend is used on the error path, the device remains in the
RPM_ACTIVE state for the duration of the timeout, even though hardware clocks
are off. If another operation runs immediately:

nxp_fspi_exec_op()
    pm_runtime_get_sync(f->dev); /* succeeds instantly without enabling clocks */
    ...
    fspi_readl_poll_tout(f, f->iobase + FSPI_STS0, ...);

Could this sequence trigger a synchronous external abort when accessing the
memory-mapped registers during the subsequent operation?


While reviewing this patch, I also noticed a few pre-existing issues:

> drivers/spi/spi-nxp-fspi.c:nxp_fspi_probe() {
[ ... ]
> 	ret = devm_request_irq(dev, irq,
> 			nxp_fspi_irq_handler, 0, pdev->name, f);
> 	if (ret)
> 		return dev_err_probe(dev, ret, "Failed to request irq\n");
[ ... ]
> 	ret = devm_add_action_or_reset(dev, nxp_fspi_cleanup, f);
> 	if (ret)
> 		return ret;

[Severity: High]
This is a pre-existing issue, but does this teardown asymmetry risk a
synchronous external abort?

During driver unbind in drivers/spi/spi-nxp-fspi.c, devres will execute LIFO,
meaning nxp_fspi_cleanup() will be called before devm_free_irq() completes:

nxp_fspi_cleanup()
    fspi_writel(f, FSPI_MCR0_MDIS, f->iobase + FSPI_MCR0);
    pm_runtime_disable(f->dev);
    nxp_fspi_clk_disable_unprep(f);

Since the IRQ handler is still active while clocks are disabled, what happens if
a spurious or delayed interrupt fires and nxp_fspi_irq_handler() accesses the
hardware registers?


> drivers/spi/spi-nxp-fspi.c:nxp_fspi_cleanup() {
> 	/* enable clock first since there is register access */
> 	pm_runtime_get_sync(f->dev);
> 
> 	/* disable the hardware */
> 	fspi_writel(f, FSPI_MCR0_MDIS, f->iobase + FSPI_MCR0);

[Severity: High]
This is a pre-existing issue, but is it safe to proceed without checking the
return value of pm_runtime_get_sync()? 

If pm_runtime_get_sync() fails to resume the device in nxp_fspi_cleanup() (for
example, due to a clock enable failure), wouldn't the subsequent write to
FSPI_MCR0 occur with gated clocks and trigger a synchronous external abort?


> drivers/spi/spi-nxp-fspi.c:nxp_fspi_irq_handler() {
> 	struct nxp_fspi *f = dev_id;
> 	u32 reg;
> 
> 	/* clear interrupt */
> 	reg = fspi_readl(f, f->iobase + FSPI_INTR);

[Severity: High]
This is a pre-existing issue, but does the interrupt handler violate PM
subsystem rules by accessing hardware registers without verifying the PM active
state?

If the device is in runtime suspend (with clocks gated) and a spurious
interrupt fires on a shared IRQ line, nxp_fspi_irq_handler() blindly reads
FSPI_INTR in drivers/spi/spi-nxp-fspi.c.

Without calling pm_runtime_get_if_active() first, could this invalid hardware
access crash the system with a synchronous external abort?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831133314.4125787-661-sashal@kernel.org?part=1