Re: [PATCH v10 15/21] drm/rockchip: dw_dp: Add runtime PM support

[sashiko-bot@kernel.org]

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

New issues:
- [High] Omission of system sleep PM callbacks breaks hardware reinitialization after system suspend (S3).
- [High] Activating runtime PM skips the manual initialization of controller clocks, causing `phy_init()` to be called with disabled clocks.

Pre-existing issues:
- [High] Asynchronous runtime PM put on driver unbind leads to resource leaks because the suspend is cancelled by pm_runtime_disable().
--

--- Patch [15]: [PATCH v10 15/21] drm/rockchip: dw_dp: Add runtime PM support ---
commit a8b68fca7b517a740f570a28e454719c2577d17d
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

drm/rockchip: dw_dp: Add runtime PM support

This patch adds runtime PM support to the Rockchip RK3576/3588 Synopsys
DesignWare DisplayPort driver.

> diff --git a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
> index ffcfb887d0d2a..770ab042a1879 100644
> --- a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
[ ... ]
> @@ -231,6 +232,7 @@ static int dw_dp_rockchip_probe(struct platform_device *pdev)
>  	if (IS_ERR(dp->vo_grf))
>  		return PTR_ERR(dp->vo_grf);
>  
> +	plat_data->autosuspend_delay = 500;

[Severity: High]
This is a pre-existing issue, but activating runtime PM for this driver by
setting this autosuspend delay exposes a potential resource leak during
driver unbind.

During component unbind, devm_drm_bridge_add() cleanup invokes
dw_dp_bridge_detach():

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_bridge_detach() {
    ...
    if (!dp->usbc_mode)
        pm_runtime_put_autosuspend(dp->dev);
    ...
}

Shortly after, the devm_pm_runtime_enable() cleanup invokes
pm_runtime_disable(), which cancels any pending asynchronous suspend requests.

Since this is an async put, will the hardware clocks (apb_clk, aux_clk)
remain enabled indefinitely after driver unbind?

[Severity: High]
Does activating runtime PM here skip the clock enablement needed for PHY
initialization?

In dw_dp_probe(), the fallback manual resume is skipped when runtime PM is
enabled:

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_probe() {
    ...
    if (!pm_runtime_enabled(dev)) {
        dw_dp_runtime_resume(dp);
        ...
    }
    
    ret = phy_init(dp->phy);
    ...
}

Since we set autosuspend_delay here, pm_runtime_enabled() evaluates to true
and we skip dw_dp_runtime_resume(). However, there is no corresponding
pm_runtime_get_sync() before phy_init() is called.

Will this cause phy_init() to execute with the controller's APB clock turned
off, potentially resulting in a bus hang?

>  	plat_data->max_link_rate = plat_data_const->max_link_rate;
>  	plat_data->pixel_mode = plat_data_const->pixel_mode;
[ ... ]
> @@ -253,6 +255,24 @@ static void dw_dp_rockchip_remove(struct platform_device *pdev)
>  	component_del(&pdev->dev, &dw_dp_rockchip_component_ops);
>  }
>  
> +static int dw_dp_rockchip_runtime_suspend(struct device *dev)
> +{
> +	struct rockchip_dw_dp *dp = dev_get_drvdata(dev);
> +
> +	return dw_dp_runtime_suspend(dp->base);
> +}
> +
> +static int dw_dp_rockchip_runtime_resume(struct device *dev)
> +{
> +	struct rockchip_dw_dp *dp = dev_get_drvdata(dev);
> +
> +	return dw_dp_runtime_resume(dp->base);
> +}
> +
> +static const struct dev_pm_ops dw_dp_pm_ops = {
> +	RUNTIME_PM_OPS(dw_dp_rockchip_runtime_suspend, dw_dp_rockchip_runtime_resume, NULL)
> +};

[Severity: High]
Does this struct need to map system sleep callbacks as well?

Because we only define RUNTIME_PM_OPS here, the PM core takes no action for
this device when the system enters S3 suspend. The device remains tracked as
active in software.

Upon system wake, since there is no system resume callback, the software
state remains active.

When a subsequent pm_runtime_get() is called during display enable, will it
skip executing dw_dp_rockchip_runtime_resume() because the software status is
already active? If so, this could leave the hardware uninitialized after a
system suspend.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806-synopsys-dw-dp-improvements-v10-0-d84a26c303d6@collabora.com?part=15