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

On Tue, Jul 28, 2026 at 10:28:31AM +0000, [email protected] wrote:
> 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.

Is it true?

Frank
>
> [ ... ]
>
> > @@ -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