Re: [PATCH v16 2/7] spi: pxa2xx: introduce suspended flag for interrupt synchronization

Hi Mark,

Thanks for the sharp observation and the suggestion!

On Tue, Jul 21, 2026 at 1:18 AM Mark Brown <[email protected]> wrote:
>
> On Tue, Jul 21, 2026 at 12:21:11AM +0800, Shih-Yuan Lee wrote:
> > When a shared interrupt line is used, the interrupt handler ssp_int()
> > can be triggered by other devices sharing the line. The handler must
> > ensure it does not access the SSP controller registers via MMIO when
> > the device is powered down or when its clock is gated; otherwise, it
> > will cause PCIe Completion Timeouts and system hangs.
>
> > Currently, ssp_int() guards MMIO access using:
> >     if (pm_runtime_suspended(drv_data->ssp->dev)) return IRQ_NONE;
>
> ...
>
> > Introduce a custom 'suspended' boolean flag in struct driver_data to
> > track the device's suspended state across all PM transitions. Check
> > both 'drv_data->suspended' and '!drv_data->clk_enabled' in ssp_int()
> > to return IRQ_NONE immediately before any MMIO access is attempted.
>
> >  static void pxa2xx_spi_clk_disable(struct driver_data *drv_data)
> >  {
> >       if (drv_data->clk_enabled) {
> > -             clk_disable_unprepare(drv_data->ssp->clk);
> >               drv_data->clk_enabled = false;
> > +             clk_disable_unprepare(drv_data->ssp->clk);
> >       }
> >  }
>
> It's probably better to avoid extra changes like this, it makes the
> patch bigger and a bit harder to review.
Agreed. I have dropped the unnecessary diff churn from pxa2xx_spi_clk_disable().
>
> > @@ -743,12 +743,12 @@ static irqreturn_t ssp_int(int irq, void *dev_id)
> >       u32 status;
> >
> >       /*
> > -      * The IRQ might be shared with other peripherals so we must first
> > -      * check that are we RPM suspended or not. If we are we assume that
> > -      * the IRQ was not for us (we shouldn't be RPM suspended when the
> > -      * interrupt is enabled).
> > +      * The IRQ might be shared with other peripherals or trigger during
> > +      * power state transitions. First check if device is suspended or if
> > +      * clock is disabled; if so, return IRQ_NONE immediately to avoid
> > +      * unclocked MMIO reads.
> >        */
> > -     if (pm_runtime_suspended(drv_data->ssp->dev))
> > +     if (drv_data->suspended || !drv_data->clk_enabled)
> >               return IRQ_NONE;
> >
> >       /*
>
> The local flags *must* be racy - I'm not seeing any locking which
> protects them, nor anything that stops something else dropping a runtime
> PM reference and powering things down after we checked here.  There's a
> helper in the power management code pm_runtime_get_if_active() which I
> think is what you want here, it'll tell you if the device is runtime
> suspended and if the device is active it'll ensure nothing else drops
> the last reference while we're running.
That makes total sense. Using pm_runtime_get_if_active() is indeed
much cleaner, atomic, and eliminates the potential race condition
where another CPU drops the runtime PM reference while ssp_int() is
running. It also avoids adding an ad-hoc suspended boolean flag to
struct driver_data.

I will update ssp_int() in the next version to use pm_runtime_get_if_active():

    static irqreturn_t ssp_int(int irq, void *dev_id)
    {
        struct driver_data *drv_data = dev_id;
        struct device *dev = &drv_data->pdev->dev;
        u32 sccr1_reg;
        u32 mask = drv_data->mask_sr;
        u32 status;

        /*
         * Ensure the device is active and acquire a PM runtime reference so
         * PM runtime cannot power down the device or gate the clock while
         * ssp_int() is running.
         */
        if (pm_runtime_get_if_active(dev) <= 0)
            return IRQ_NONE;

        ...

        pm_runtime_put_autosuspend(dev);
        return IRQ_HANDLED;
    }

Thanks,
Shih-Yuan