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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Asynchronous runtime PM put operations during device removal and error paths are cancelled by pm_runtime_disable(), leaving the device permanently powered on.
- [High] The device will remain powered on during system suspend because it uses runtime PM autosuspend without providing system sleep PM operations.
--

--- Patch [15]: [PATCH v8 15/21] drm/rockchip: dw_dp: Add runtime PM support ---
commit 7c3ff2ff43dcc0fce64eaf4461fe2a1101bcb4ad
Author: Sebastian Reichel <[email protected]>

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 d516b3910b516..dde3881efad84 100644
> --- a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
[ ... ]
> @@ -231,16 +232,21 @@ 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]
Does setting autosuspend_delay here expose a resource leak in the core
driver's removal/error paths?

When autosuspend_delay > 0, the core dw-dp driver opts into the shared PM
runtime framework. During device unbind (dw_dp_unbind()) and devm IRQ
cleanup (dw_dp_irq_free()), the core driver releases its PM references
asynchronously using pm_runtime_put_autosuspend().

Immediately afterward, the removal process triggers the devres cleanup for
devm_pm_runtime_enable(), which calls pm_runtime_disable(). As documented
in the PM subsystem guidelines, pm_runtime_disable() invokes
__pm_runtime_barrier(), which flushes and cancels any pending asynchronous
suspend requests. This leaves the DP controller's clocks (apb_clk and
aux_clk) enabled indefinitely.

Should the core driver use synchronous puts (like pm_runtime_put_sync())
before pm_runtime_disable() is called?

>  	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;
[ ... ]
> @@ -249,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]
Will the device remain powered on during system suspend?

This dev_pm_ops struct defines runtime PM callbacks but omits system sleep
callbacks (like SYSTEM_SLEEP_PM_OPS with pm_runtime_force_suspend).

When the system suspends, the DRM core disables the display, which drops
the runtime PM reference and schedules the 500ms autosuspend timer via
pm_runtime_put_autosuspend().

If the PM core suspends the system before the 500ms timer expires, the
device is left in the RPM_ACTIVE state. Without system sleep callbacks to
force suspension, the DP controller's clocks could remain enabled while
the rest of the system sleeps.

> +
>  static const struct rockchip_dw_dp_plat_data rk3588_dp_plat_data = {
>  	.num_ctrls = 2,
>  	.ctrl_ids = {0xfde50000, 0xfde60000},

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