Re: [PATCH v5 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 2 potential issue(s) to consider:
- [High] drm/bridge: analogix_dp: level IRQ storm when pm_runtime_resume_and_get() fails
- [Medium] drm/bridge: analogix_dp: pm_runtime_resume_and_get() in IRQ clears HPD status
--

commit 2a41fb565a5b1a6da39d3e4bf4efd7dfc99b3b81
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/disable
handlers into analogix_dp_bind() and analogix_dp_unbind(). It also converts the
interrupt handler to a pure threaded IRQ handler that acquires a runtime PM
reference before accessing controller registers.

> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index a8a2d5d615aea..e92d86c678947 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -700,24 +700,22 @@ 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.
> +	 */
> +	ret = pm_runtime_resume_and_get(dp->dev);
> +	if (ret)
> +		return IRQ_HANDLED;

[Severity: High]
Can returning IRQ_HANDLED when pm_runtime_resume_and_get() fails cause a
level-triggered IRQ storm or unclocked MMIO access during system suspend and
resume?

On Rockchip and Exynos platforms, the native DP controller interrupt is a
level-high interrupt (IRQ_TYPE_LEVEL_HIGH) requested with IRQF_ONESHOT.
Moving disable_irq() from analogix_dp_bridge_disable() to
analogix_dp_unbind() leaves dp->irq enabled across system suspend and resume,
where rockchip_dp_pm_ops and exynos_dp_pm_ops use pm_runtime_force_suspend()
in dpm_suspend and pm_runtime_force_resume() in dpm_resume (outside the
dpm_suspend_noirq and dpm_resume_noirq window where device IRQs are masked).

If an interrupt fires when dp->dev is runtime-suspended and
dev->power.disable_depth > 0 (or if analogix_dp_resume() fails at runtime),
pm_runtime_resume_and_get() returns a negative error code such as -EACCES.
Returning IRQ_HANDLED here without clearing or masking the hardware interrupt
status registers causes irq_thread_fn() to increment desc->threads_handled
and irq_finalize_oneshot() to unmask the still-asserted level IRQ line. That
immediately re-triggers the interrupt while preventing note_interrupt() in
the spurious IRQ detector from ever disabling the screaming line.

Furthermore, if dev->power.usage_count > 1 during pm_runtime_force_suspend()
(such as when a later commit in this series, "drm/bridge: analogix_dp: Manage
pm runtime and IRQ for native HPD pin detection", holds a runtime PM
reference in analogix_dp_bind(), or when runtime PM is forbidden via sysfs),
pm_runtime_force_suspend() disables the DP clocks via analogix_dp_suspend()
while leaving runtime_status and last_status set to RPM_ACTIVE with
disable_depth > 0. In that state, rpm_resume() returns 1 and
pm_runtime_resume_and_get() returns 0, allowing analogix_dp_irq_thread() to
perform MMIO accesses on an unclocked controller.

>  
>  	irq_type = analogix_dp_get_irq_type(dp);
>  	if (!dp->hpd_gpiod && irq_type)

[Severity: Medium]
If the device has actually runtime suspended before analogix_dp_irq_thread()
runs, won't pm_runtime_resume_and_get() wipe the pending interrupt status
registers before analogix_dp_get_irq_type() can read them?

When dp->dev is runtime suspended, pm_runtime_resume_and_get() invokes the
runtime resume callback (rockchip_dp_resume() or exynos_dp_resume() ->
analogix_dp_resume()):

drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:analogix_dp_resume() {
	...
	if (dp->plat_data->power_on)
		dp->plat_data->power_on(dp->plat_data);

	phy_set_mode(dp->phy, PHY_MODE_DP);
	phy_power_on(dp->phy);

	analogix_dp_init_dp(dp);
	...
}

On Rockchip, dp->plat_data->power_on() calls rockchip_dp_poweron() ->
rockchip_dp_pre_init(), which asserts and deasserts dp->rst and dp->apbrst,
wiping all controller registers.

On all platforms, analogix_dp_init_dp() then resets the controller and clears
all pending interrupts:

drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:analogix_dp_init_dp() {
	analogix_dp_reset(dp);

	analogix_dp_swreset(dp);

	analogix_dp_init_analog_param(dp);
	analogix_dp_init_interrupt(dp);
	...
	analogix_dp_init_hpd(dp);
	analogix_dp_init_aux(dp);
}

Both analogix_dp_init_interrupt() and analogix_dp_init_hpd() explicitly clear
ANALOGIX_DP_COMMON_INT_STA_4 and ANALOGIX_DP_INT_STA. By the time
pm_runtime_resume_and_get() returns to analogix_dp_irq_thread(),
analogix_dp_get_irq_type() reads cleared registers (0), so irq_type is 0 and
the hotplug event is silently dropped. Note that the later commit in this
series ("drm/bridge: analogix_dp: Manage pm runtime and IRQ for native HPD
pin detection") only keeps runtime PM active across bind/unbind on
RK3576_EDP and RK3588_EDP, leaving EXYNOS_DP, RK3288_DP, RK3399_EDP, and
other configurations susceptible to runtime suspend while the IRQ is enabled.

Conversely, when dp->hpd_gpiod is used, analogix_dp_get_irq_type() (as well
as analogix_dp_get_plug_in_status() in commit "drm/bridge: analogix_dp:
Declare DRM_BRIDGE_OP_HPD for hotplug reporting") only reads the GPIO level
via gpiod_get_value(dp->hpd_gpiod) and never touches DP controller MMIO
registers. Does analogix_dp_irq_thread() need to call
pm_runtime_resume_and_get() when dp->hpd_gpiod is present, since doing so
powers on, resets, and reinitializes the DP controller and PHY on every GPIO
HPD interrupt while the display is off?

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