Re: [PATCH v5 11/15] 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:
- [High] Incorrect teardown sequence in dw_dp_rockchip_unbind() causes invalid hardware access.
- [Medium] Missing PM runtime cleanup when dw_dp_bind() fails.
--

--- Patch [11]: [PATCH v5 11/15] drm/rockchip: dw_dp: Add runtime PM support ---
commit b12de6118be77a7ac8babe1d82f46a8db5857b90
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 0ecbad24dd4e8..b1a69d8821a2c 100644
> --- a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c

[ ... ]

> @@ -165,18 +178,32 @@ static int dw_dp_rockchip_bind(struct device *dev, struct device *master, void *
>  		return ret;
>  	drm_encoder_helper_add(encoder, &dw_dp_encoder_helper_funcs);
>  
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_set_autosuspend_delay(dev, 500);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +
>  	ret = dw_dp_bind(dp->base, encoder);
>  	if (ret)
>  		return ret;

[Severity: Medium]
If dw_dp_bind() returns an error, does this early return bypass the PM runtime
cleanup sequence?

This leaves the device's PM state enabled and unbalanced. Should this jump to
the error path to properly disable PM runtime on failure?

>  
>  	connector = drm_bridge_connector_init(drm_dev, encoder);
>  	if (IS_ERR(connector)) {
> -		dw_dp_unbind(dp->base);
> -		return dev_err_probe(dev, PTR_ERR(connector),
> -				     "Failed to init bridge connector\n");
> +		ret = PTR_ERR(connector);
> +		dev_err_probe(dev, ret, "Failed to init bridge connector\n");
> +		goto error;
>  	}
>  
> +	pm_request_idle(dev);
> +
>  	return 0;
> +
> +error:
> +	pm_runtime_dont_use_autosuspend(dev);
> +	pm_runtime_resume(dev);
> +	pm_runtime_disable(dev);
> +	dw_dp_unbind(dp->base);
> +	return ret;
>  }
>  
>  static void dw_dp_rockchip_unbind(struct device *dev, struct device *master,
> @@ -185,6 +212,9 @@ static void dw_dp_rockchip_unbind(struct device *dev, struct device *master,
>  	struct rockchip_dw_dp *dp = dev_get_drvdata(dev);
>  
>  	dw_dp_unbind(dp->base);
> +	pm_runtime_dont_use_autosuspend(dev);
> +	pm_runtime_resume(dev);
> +	pm_runtime_disable(dev);
>  }

[Severity: High]
Does this sequence cause invalid hardware access if the device is suspended?

dw_dp_unbind() is called before disabling PM runtime. dw_dp_unbind() calls
phy_exit(), shutting down the PHY. If the device was suspended prior to unbind,
pm_runtime_resume() forces a wake-up, which calls dw_dp_runtime_resume() then
dw_dp_init_hw().

This would perform hardware register writes on a device whose PHY has already
been exited. Should PM runtime be disabled before calling dw_dp_unbind()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724-synopsys-dw-dp-improvements-v5-0-9445c2e87441@collabora.com?part=11