Re: [PATCH v9 16/22] drm/rockchip: dw_dp: Add runtime PM support

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

New issues:
- [Medium] Asynchronous autosuspend on driver removal is cancelled by devres-managed pm_runtime_disable(), leaving the hardware powered on and clocks enabled.

Pre-existing issues:
- [High] Missing system sleep PM operations prevents hardware reinitialization on system resume and leaks power during sleep for non-USB-C displays.
--

--- Patch [16]: [PATCH v9 16/22] drm/rockchip: dw_dp: Add runtime PM support ---
commit 92f08187a6106df5f97a5c7538b516c59d21ea4a
Author: Sebastian Reichel <sebastian.reichel-ZGY8ohtN/[email protected]>

drm/rockchip: dw_dp: Add runtime PM support
    
Add support for runtime PM to the Rockchip RK3576/3588 Synopsys
DesignWare DisplayPort driver.

> --- a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c

[ ... ]

> @@ -249,6 +255,24 @@ static void dw_dp_rockchip_remove(struct platform_device *pdev)
>  	component_del(&pdev->dev, &dw_dp_rockchip_component_ops);
>  }

[Severity: Medium]
Will this leave the hardware powered on and clocks enabled after driver unload?

When component_del() is called, the unbind path eventually calls dw_dp_unbind()
which asynchronously drops the PM reference using pm_runtime_put_autosuspend().

After dw_dp_rockchip_remove() returns, the devres cleanup sequence runs and
calls pm_runtime_disable() (registered via devm_pm_runtime_enable() during
probe). Because pm_runtime_disable() unconditionally cancels pending
autosuspend timers and async works, does this cancel the suspend operation
before it can execute and leave the device permanently in the RPM_ACTIVE state?

Would it be safer to ensure a synchronous suspend happens during the remove
path to prevent this leak?

> +
> +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]
This isn't a bug introduced by this patch, but could the lack of system sleep
callbacks in dw_dp_pm_ops prevent hardware reinitialization on system resume?

For non-USB-C displays, the driver holds a runtime PM reference indefinitely
to maintain active hotplug detection. During system suspend (S3), the DRM core
disables the display pipeline which drops one reference, but the usage count
remains at 1, so the device is not runtime suspended.

Since dw_dp_pm_ops lacks system sleep callbacks (like pm_runtime_force_suspend
and pm_runtime_force_resume), does the PM core fail to synchronously suspend
the device?

If the system forcefully cuts power during S3, the hardware state is lost.
Upon system resume, the runtime PM framework still considers the device
RPM_ACTIVE, so a subsequent pm_runtime_get_sync() during pipeline enable
might not trigger dw_dp_runtime_resume(). Does this leave the hardware
uninitialized and result in a blank display?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803-synopsys-dw-dp-improvements-v9-0-2e6e46135312-ZGY8ohtN/[email protected]?part=16