Re: [PATCH v2 01/10] drm/bridge: analogix_dp: Manage pm runtime and IRQ for native HPD pin detection

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] The DP controller IRQ for native HPD mode is left enabled during system suspend, risking a kernel crash on hotplug events.
- [High] Component bind failure leaves HPD IRQ enabled and PM reference leaked, causing a Use-After-Free.

Pre-existing issues:
- [High] The return value of `pm_runtime_get_sync()` is ignored in `analogix_dp_set_bridge()`, potentially leading to hardware access on a suspended device.
- [High] Sleeping in atomic context via `usleep_range()` in bridge atomic enable callback.
- [High] Memory leak and list corruption due to `devm_drm_bridge_add` in component bind.
--

--- Patch [1]: [PATCH v2 01/10] drm/bridge: analogix_dp: Manage pm runtime and IRQ for native HPD pin detection ---
commit c884a0bafddd9bc289fdebe294b1a5fb1f401308
Author: Damon Ding <[email protected]>

drm/bridge: analogix_dp: Manage pm runtime and IRQ for native HPD pin detection

This commit modifies the Analogix DP driver to keep the native HPD pin
IRQ active while the display is disabled on Rockchip platforms. It splits
the IRQ management paths so that native HPD pin mode holds a runtime PM
power reference and enables the IRQ during bind/unbind, rather than during
bridge enable/disable.

> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 30c0c0b41d5aa..9f8544d3c8870 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c

[ ... ]

> @@ -1018,7 +1024,8 @@ static int analogix_dp_set_bridge(struct analogix_dp_device *dp)
>  		goto out_dp_init;
>  	}
>  
> -	enable_irq(dp->irq);
> +	if (!analogix_dp_require_pm_for_hpd_irq(dp))
> +		enable_irq(dp->irq);

[Severity: High]
This is a pre-existing issue, but does this code ignore the return value of
pm_runtime_get_sync()?

If pm_runtime_get_sync(dp->dev) fails earlier in analogix_dp_set_bridge()
and returns a negative error code, the device remains suspended. The driver
subsequently calls analogix_dp_init_analog_func(), which accesses hardware
registers on the unpowered device.

>  	return 0;
>  
>  out_dp_init:

[ ... ]

> @@ -1160,7 +1167,8 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
>  	if (dp->dpms_mode != DRM_MODE_DPMS_ON)
>  		return;
>  
> -	disable_irq(dp->irq);
> +	if (!analogix_dp_require_pm_for_hpd_irq(dp))
> +		disable_irq(dp->irq);

[Severity: High]
Does skipping disable_irq() here leave the DP controller IRQ enabled during
system suspend?

During dpm_suspend, pm_runtime_force_suspend() runs and calls
analogix_dp_suspend(), which disables the DP clock. Device IRQs are not
disabled by the IRQ core until dpm_suspend_noirq().

If a hotplug event occurs in this window, the active IRQ triggers the handler:

analogix_dp_hardirq() {
    irq_type = analogix_dp_get_irq_type(dp);
    ...
}

This reads MMIO registers while the DP clock is disabled, which can crash
the system.

>  
>  	analogix_dp_set_analog_power_down(dp, POWER_ALL, 1);

[ ... ]

> @@ -1508,6 +1516,14 @@ int analogix_dp_bind(struct analogix_dp_device *dp, struct drm_device *drm_dev)
>  		goto err_unregister_aux;
>  	}
>  

[Severity: High]
This is a pre-existing issue, but does using devm_drm_bridge_add() earlier
in this component bind callback cause a memory leak and list corruption
on failure?

If the component bind fails and is retried, the devm resources are tied to
the physical platform device and are not freed between attempts. The retry
will call devm_drm_bridge_add() on the same dp->bridge object again,
causing list_add() on a node that is already part of the global bridge list.

> +	if (analogix_dp_require_pm_for_hpd_irq(dp)) {
> +		ret = pm_runtime_resume_and_get(dp->dev);
> +		if (ret)
> +			goto err_unregister_aux;
> +
> +		enable_irq(dp->irq);

[Severity: High]
Does this code leak the PM reference and leave the IRQ enabled if a subsequent
setup step fails?

If the platform caller rockchip_dp_bind() fails at
drm_bridge_connector_init() after analogix_dp_bind() succeeds, its error
path is:

rockchip_dp_bind() {
    ...
err_cleanup_encoder:
    dp->encoder.encoder.funcs->destroy(&dp->encoder.encoder);
    return ret;
}

Because analogix_dp_unbind() is not called in this error path, the PM
reference is permanently leaked and the IRQ remains enabled. When the IRQ
fires later, analogix_dp_irq_thread() dereferences the stale dp->drm_dev
pointer.

> +	}
> +
>  	return 0;
>  
>  err_unregister_aux:

[ ... ]

[Severity: High]
This is a pre-existing issue, but does analogix_dp_bridge_atomic_enable()
sleep in atomic context?

analogix_dp_bridge_atomic_enable() is called during the atomic commit sequence
from drm_atomic_helper_commit_tail(). The driver explicitly calls
usleep_range(10, 11) in a loop:

analogix_dp_bridge_atomic_enable() {
    ...
    while (timeout_loop < MAX_PLL_LOCK_LOOP) {
        if (analogix_dp_set_bridge(dp) == 0) {
            ...
        }
        ...
        usleep_range(10, 11);
    }
}

Calling sleeping functions in this atomic context violates DRM subsystem rules.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1