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

Mark Brown <[email protected]> Mon, 20 Jul 2026 18:18:34 +0100

Newsgroups	org.kernel.vger.linux-spi,org.infradead.lists.linux-arm-kernel,org.kernel.vger.linux-kernel
Message-ID	<[email protected]>

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.

> @@ -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.

signature.asc (application/pgp-signature, 488 B)