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 devre=
s-managed pm_runtime_disable(), leaving the hardware powered on and clocks =
enabled.

Pre-existing issues:
- [High] Missing system sleep PM operations prevents hardware reinitializat=
ion 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 suppor=
t ---
commit 92f08187a6106df5f97a5c7538b516c59d21ea4a
Author: Sebastian Reichel <[email protected]>

drm/rockchip: dw_dp: Add runtime PM support
   =20
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_de=
vice *pdev)
>  	component_del(&pdev->dev, &dw_dp_rockchip_component_ops);
>  }

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

When component_del() is called, the unbind path eventually calls dw_dp_unbi=
nd()
which asynchronously drops the PM reference using pm_runtime_put_autosuspen=
d().

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 st=
ate?

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 =3D 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 =3D dev_get_drvdata(dev);
> +
> +	return dw_dp_runtime_resume(dp->base);
> +}
> +
> +static const struct dev_pm_ops dw_dp_pm_ops =3D {
> +	RUNTIME_PM_OPS(dw_dp_rockchip_runtime_suspend, dw_dp_rockchip_runtime_r=
esume, NULL)
> +};

[Severity: High]
This isn't a bug introduced by this patch, but could the lack of system sle=
ep
callbacks in dw_dp_pm_ops prevent hardware reinitialization on system resum=
e?

For non-USB-C displays, the driver holds a runtime PM reference indefinitely
to maintain active hotplug detection. During system suspend (S3), the DRM c=
ore
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_susp=
end
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?

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260803-synopsys-d=
[email protected]?part=3D16