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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The driver implements runtime PM support but lacks system sleep callbacks, leaving the hardware fully powered on during system suspend.
- [Medium] Using asynchronous autosuspend before devres teardown on driver removal causes pending suspends to be cancelled, permanently leaking hardware clocks.
--

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

drm/rockchip: dw_dp: Add runtime PM support

Add support for runtime PM 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;
>  	plat_data->max_link_rate = plat_data_const->max_link_rate;
>  	plat_data->pixel_mode = plat_data_const->pixel_mode;
>  	plat_data->hpd_sw_sel = dw_dp_rockchip_hpd_sw_sel;
> @@ -253,6 +255,24 @@ static void dw_dp_rockchip_remove(struct platform_device *pdev)
>  	component_del(&pdev->dev, &dw_dp_rockchip_component_ops);
>  }

[Severity: Medium]
When the driver is removed, component_del() unbinds the DRM pipeline, queuing
an async autosuspend with the 500ms delay set in dw_dp_rockchip_probe().

Because dw_dp_rockchip_remove() returns immediately, devres teardown will call
pm_runtime_disable() which cancels the pending autosuspend.

Does this bypass dw_dp_runtime_suspend() and permanently leak the apb_clk
and aux_clk clocks? Should there be a synchronous suspend here?

> +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: Medium]
The dev_pm_ops structure uses RUNTIME_PM_OPS but entirely omits
SYSTEM_SLEEP_PM_OPS.

During system suspend, pm_runtime_disable() cancels pending autosuspend timers.

Without system sleep callbacks (like pm_runtime_force_suspend), will the
hardware remain in RPM_ACTIVE with clocks running throughout system sleep?

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