Re: [PATCH v2 3/3] spi: spi-nxp-fspi: propagate clock reconfig failures in nxp_fspi_select_mem()

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

New issues:
- [Critical] Incomplete state rollback on `nxp_fspi_select_mem()` failure leaves cached flags out of sync and the controller permanently stuck in Stop mode.
- [High] Error handling in `nxp_fspi_select_mem()` leaves hardware clocks disabled while PM runtime remains active, leading to deferred synchronous external aborts and unbalanced clock disables.

Pre-existing issues:
- [High] Missing `pm_runtime_put_noidle()` when `pm_runtime_get_sync()` fails, leaking the PM runtime usage counter.
- [High] The return value of `nxp_fspi_default_setup()` is completely ignored, masking initialization and clock setup failures.
- [High] The timeout error from polling the controller status is completely discarded in `nxp_fspi_exec_op()`.
--

--- Patch [3]: [PATCH v2 3/3] spi: spi-nxp-fspi: propagate clock reconfig failures in nxp_fspi_select_mem() ---
commit dbeb737d74251745ba44e92f371ba23d08069151
Author: Haibo Chen <[email protected]>

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

This commit modifies nxp_fspi_select_mem() to return an error code if
clk_set_rate() or clock enabling fails. It also updates nxp_fspi_exec_op()
to check this return value and bail out to prevent accessing registers while
the clock is gated off.

> diff --git a/drivers/spi/spi-nxp-fspi.c b/drivers/spi/spi-nxp-fspi.c
> --- a/drivers/spi/spi-nxp-fspi.c
> +++ b/drivers/spi/spi-nxp-fspi.c
> @@ -922,12 +922,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;

[Severity: Critical]
If clk_set_rate() fails and we return early here, does this leave the
controller permanently in stop mode?

Earlier in nxp_fspi_select_mem(), the controller is placed into stop mode:

    reg = fspi_readl(f, f->iobase + FSPI_MCR0);
    fspi_writel(f, reg | FSPI_MCR0_MDIS, f->iobase + FSPI_MCR0);

Since the early return bypasses the exit from stop mode at the end of the
function, the controller remains stopped. Additionally, f->flags might have
been updated with FSPI_DTR_MODE. If a subsequent SPI operation matches the
fast-path check at the beginning of nxp_fspi_select_mem(), it would bypass
reconfiguration and attempt to execute commands on a stopped controller,
resulting in a hang.

[ ... ]

> @@ -1132,7 +1141,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);

[Severity: High]
This is a pre-existing issue, but does assigning to err here discard the
timeout error from fspi_readl_poll_tout() immediately above?

If fspi_readl_poll_tout() times out, it returns -ETIMEDOUT to err, which is
flagged by WARN_ON(), but the function does not abort. Instead, err is
immediately overwritten by nxp_fspi_select_mem(), causing subsequent
operations to execute on a busy controller.

> +	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);

[Severity: High]
If nxp_fspi_select_mem() fails and leaves the clocks physically disabled, does
calling pm_runtime_put_autosuspend() leave the PM runtime state out of sync?

pm_runtime_put_autosuspend() maintains the RPM_ACTIVE state until the
autosuspend timer expires. If a subsequent SPI operation begins before the
timer fires, pm_runtime_get_sync() will succeed without re-enabling the
clocks. nxp_fspi_exec_op() will then attempt register access while the clock
is gated off, triggering a synchronous external abort.

If no operation occurs, the timer fires and runtime_suspend will disable
the already-gated clocks, potentially causing an enable count underflow.

> +		return err;
> +	}
>  
>  	nxp_fspi_prepare_lut(f, op);

[ ... ]

For pre-existing issues not directly modified by this patch, there are a few
other observations in this file:

[Severity: High]
This is a pre-existing issue, but does nxp_fspi_exec_op() leak the PM
runtime usage counter if pm_runtime_get_sync() fails?

drivers/spi/spi-nxp-fspi.c:nxp_fspi_exec_op() {
    ...
    err = pm_runtime_get_sync(f->dev);
    if (err < 0) {
        dev_err(f->dev, "Failed to enable clock %d\n", __LINE__);
        return err;
    }
    ...
}

If pm_runtime_get_sync() returns a negative error code, the usage counter
remains incremented. Returning immediately without calling
pm_runtime_put_noidle() permanently prevents the device from runtime
suspending again.

[Severity: High]
This is a pre-existing issue, but are initialization and clock setup failures
being masked during probe and resume?

drivers/spi/spi-nxp-fspi.c:nxp_fspi_probe() {
    ...
    nxp_fspi_default_setup(f);
    ...
}

The return value of nxp_fspi_default_setup() is completely ignored in both
nxp_fspi_probe() and nxp_fspi_runtime_resume(). If clk_set_rate() or clock
enabling fails inside setup, the driver proceeds as if the hardware is
fully initialized, which could lead to silent failures.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3