Re: [PATCH v6 14/18] drm/bridge: analogix_dp: Move IRQ enable/disable to bind/unbind

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] drm/bridge: analogix_dp: IRQ storm and unclocked MMIO on PM
  suspend
--

commit a4a512968af1f8f658769af2378618f972e953d9
Author: Damon Ding <damon.ding@rock-chips.com>

drm/bridge: analogix_dp: Move IRQ enable/disable to bind/unbind

This commit moves enable_irq() and disable_irq() from the bridge enable and
disable callbacks to analogix_dp_bind() and analogix_dp_unbind(), and
converts the interrupt handler to a threaded handler that acquires a
runtime PM reference.

> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index b14a8aa12394e..7f6346d45a01c 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -700,24 +700,25 @@ static int analogix_dp_enable_scramble(struct analogix_dp_device *dp,
>  	return ret < 0 ? ret : 0;
>  }
>  
> -static irqreturn_t analogix_dp_hardirq(int irq, void *arg)
> -{
> -	struct analogix_dp_device *dp = arg;
> -	irqreturn_t ret = IRQ_NONE;
> -	u32 irq_type;
> -
> -	irq_type = analogix_dp_get_irq_type(dp);
> -	if (irq_type)
> -		ret = IRQ_WAKE_THREAD;
> -
> -	return ret;
> -}
> -
>  static irqreturn_t analogix_dp_irq_thread(int irq, void *arg)
>  {
>  	struct analogix_dp_device *dp = arg;
>  	u32 irq_type;
>  	bool hpd_detected;
> +	int ret;
> +
> +	/*
> +	 * The device may have been runtime suspended since the interrupt was
> +	 * raised. Hold a runtime PM reference before touching the registers,
> +	 * as the controller clocks are only guaranteed to be enabled while
> +	 * the device is runtime PM active. The GPIO HPD mode needs no such
> +	 * reference, since it only reads the GPIO level.
> +	 */
> +	if (!dp->hpd_gpiod) {
> +		ret = pm_runtime_resume_and_get(dp->dev);
> +		if (ret)
> +			return IRQ_HANDLED;
> +	}
>  
>  	irq_type = analogix_dp_get_irq_type(dp);
>  	if (!dp->hpd_gpiod && irq_type)

[Severity: High]
Can analogix_dp_irq_thread() race with pm_runtime_force_suspend() during
system sleep, or trigger an interrupt storm when pm_runtime_resume_and_get()
fails?

Both rockchip_dp_pm_ops and exynos_dp_pm_ops use DEFINE_RUNTIME_DEV_PM_OPS(),
which wires pm_runtime_force_suspend() and pm_runtime_force_resume() as the
system sleep callbacks. Previously, analogix_dp_bridge_disable() called
disable_irq(dp->irq) during drm_mode_config_helper_suspend() before
pm_runtime_force_suspend() ran. Keeping dp->irq enabled across bind/unbind
leaves the interrupt active during dpm_suspend() and dpm_resume():

CPU0                                    CPU1
analogix_dp_irq_thread()
  pm_runtime_resume_and_get(dp->dev)
                                        pm_runtime_force_suspend(dp->dev)
                                          pm_runtime_disable(dp->dev)
                                          ...
                                          analogix_dp_suspend(dp)
                                            clk_disable_unprepare(dp->clock)
  analogix_dp_get_irq_type(dp)
    readl(dp->reg_base + ...)  <-- unclocked MMIO access

pm_runtime_disable() waits for in-flight runtime PM transitions, not active
usage_count holders, so analogix_dp_suspend() gates the controller clocks
while analogix_dp_irq_thread() accesses registers, causing an external abort
on ARM64. In addition, when pm_runtime_need_not_resume() is false during
pm_runtime_force_suspend() (whenever usage_count > 1), the clocks are turned
off without setting the runtime status to RPM_SUSPENDED; if an interrupt
fires before suspend_device_irqs() or after resume_device_irqs(),
pm_runtime_resume_and_get() sees RPM_ACTIVE and returns 0 while the clocks
are still gated.

Separately, when dp->dev is runtime-suspended and pm_runtime_resume_and_get()
fails (for example, returning -EACCES while runtime PM is disabled by
pm_runtime_force_suspend()), returning IRQ_HANDLED without clearing the
level-triggered interrupt status causes irq_finalize_oneshot() to unmask the
still-asserted IRQ line, re-triggering analogix_dp_irq_thread() in a tight
loop while preventing the spurious IRQ detector from disabling the line.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113082305.541652-1-damon.ding@rock-chips.com?part=14