Re: [PATCH v12 15/23] drm/rockchip: dw_dp: Add runtime PM support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] drm/rockchip: dw_dp: unguarded phy_power_on() in
  dw_dp_bridge_edid_read()
--

commit 7986dcc1b84b1f1885c1e7812534566eeb9a1c10
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

drm/rockchip: dw_dp: Add runtime PM support

This commit enables runtime PM with a 500ms autosuspend delay and wires up
runtime suspend and resume callbacks for the Rockchip DesignWare DisplayPort
driver.

> diff --git a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
> index 1cc6cfad41042..99d948a7c51f9 100644
> --- a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
[ ... ]
> @@ -229,6 +230,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;
> @@ -251,6 +253,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: Medium]
Does enabling runtime PM with autosuspend allow dw_dp_bridge_edid_read() to
call phy_power_on(dp->phy) while dp->dev and its VO power domain are
suspended?

Unlike dw_dp_bridge_detect() and dw_dp_bridge_atomic_enable(),
dw_dp_bridge_edid_read() powers on the PHY without first acquiring a runtime
PM reference on dp->dev, which is only resumed later inside
drm_edid_read_ddc() -> dw_dp_aux_transfer():

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_bridge_edid_read() {
	...
	ret = phy_power_on(dp->phy);
	if (ret)
		return NULL;

	edid = drm_edid_read_ddc(connector, &dp->aux.ddc);
	...
}

In USB-C mode, dp->dev autosuspends when idle.  If dw_dp_bridge_edid_read() is
reached while dp->dev is suspended (for example, when the connector is forced
on so drm_helper_probe_detect() is skipped, or if autosuspend expires before
the EDID read starts), phy_power_on(dp->phy) on RK3576 and RK3588 calls
rk_udphy_dp_phy_power_on() -> rk_udphy_dp_lane_select(), which writes to
udphy->vogrf while the VO power domain is off and triggers an SError crash.
Even without the fault, resuming dp->dev later in dw_dp_aux_transfer() powers
on the VO domain and resets vo_grf, wiping out the lane mux settings written
by rk_udphy_dp_lane_select().

Should dw_dp_bridge_edid_read() acquire a runtime PM reference on dp->dev
before calling phy_power_on(dp->phy)?

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