Re: [PATCH v7 12/18] drm/bridge: synopsys: dw-dp: Add Runtime PM support

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Incorrect teardown order in dw_dp_unbind() allows AUX transfers to cause a kernel panic or hang.
- [High] Calling a sleeping function (msleep) from a runtime PM resume callback that is reachable from an atomic commit path.

Pre-existing issues:
- [High] Unbalanced phy_power_off() in dw_dp_bridge_atomic_disable() corrupts the PHY power count when atomic_enable() fails early.
--

--- Patch [12]: [PATCH v7 12/18] drm/bridge: synopsys: dw-dp: Add Runtime PM support ---
commit 6fa63f17bf7f03d90ce981c3f9cd86d3e8761c32
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

drm/bridge: synopsys: dw-dp: Add Runtime PM support

This commit adds runtime PM support to the Synopsys DesignWare DisplayPort
bridge driver. The support is not enabled automatically and must be hooked
up in the vendor specific glue code.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index d4f75b7f85d0a..8b650f904575d 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c

[ ... ]

> @@ -1664,6 +1672,13 @@ static void dw_dp_bridge_atomic_enable(struct drm_bridge *bridge,
>  	struct drm_connector_state *conn_state;
>  	int ret;
>  
> +	ret = pm_runtime_get_active(dp->dev, RPM_TRANSPARENT);
> +	if (ret) {
> +		dev_err(dp->dev, "runtime PM failure\n");
> +		return;
> +	}
> +	dp->pm_active = true;
> +
>  	connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
>  	if (!connector) {
>  		dev_err(dp->dev, "failed to get connector\n");
>  		return;
>  	}

[Severity: High]
This is a pre-existing issue, but does this early return cause an unbalanced
phy_power_off() later?

If dw_dp_bridge_atomic_enable() returns early here, it skips a successful
phy_power_on(). However, the subsequent dw_dp_bridge_atomic_disable()
unconditionally calls dw_dp_link_disable(), which executes phy_power_off().

Would calling phy_power_off() without a preceding successful phy_power_on()
decrease the PHY power count below 0 and permanently prevent the PHY from
being powered on again?

[ ... ]

> @@ -2090,9 +2146,22 @@ EXPORT_SYMBOL_GPL(dw_dp_bind);
>  
>  void dw_dp_unbind(struct dw_dp *dp)
>  {
> +	if (dp->usbc_mode)
> +		pm_runtime_get_sync(dp->dev);
> +
> +	dp->drm_bound = false;
>  	disable_irq(dp->irq);
> +
>  	cancel_work_sync(&dp->hpd_work);
>  	phy_exit(dp->phy);
> +
> +	/* Last PM runtime suspend must be done with drm_bound = false */
> +	pm_runtime_put_sync(dp->dev);
> +
> +	/* suspend once if runtime PM is disabled */
> +	if (!pm_runtime_enabled(dp->dev))
> +		dw_dp_runtime_suspend(dp);
> +
>  	drm_bridge_put(dp->next_bridge);
>  	drm_dp_aux_unregister(&dp->aux);
>  }

[Severity: Critical]
Does this teardown order in dw_dp_unbind() introduce a race condition with
concurrent AUX transfers?

Because drm_dp_aux_unregister() is called at the very end, the AUX interface
remains active while the underlying hardware is shut down.

If a concurrent dw_dp_aux_transfer() occurs from userspace while runtime PM
is disabled, dw_dp_unbind() will manually call dw_dp_runtime_suspend()
disabling the APB clock. The AUX transfer will succeed its PM guard and
its subsequent regmap_write() will access unclocked hardware, triggering a
synchronous bus abort.

[ ... ]

> @@ -2202,6 +2271,47 @@ struct dw_dp *dw_dp_probe(struct platform_device *pdev, const struct dw_dp_plat_
>  }
>  EXPORT_SYMBOL_GPL(dw_dp_probe);
>  
> +int dw_dp_runtime_suspend(struct dw_dp *dp)
> +{
> +	if (dp->drm_bound)
> +		disable_irq(dp->irq);
> +
> +	clk_disable_unprepare(dp->aux_clk);
> +	clk_disable_unprepare(dp->apb_clk);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(dw_dp_runtime_suspend);
> +
> +int dw_dp_runtime_resume(struct dw_dp *dp)
> +{
> +	int ret;
> +
> +	ret = clk_prepare_enable(dp->apb_clk);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_prepare_enable(dp->aux_clk);
> +	if (ret) {
> +		clk_disable_unprepare(dp->apb_clk);
> +		return ret;
> +	}
> +
> +	dw_dp_init_hw(dp);
> +
> +	/*
> +	 * HPD_HOT_PLUG bit is asserted only after the sink holds HPD
> +	 * high for at least 100ms.
> +	 */
> +	msleep(110);
> +
> +	if (dp->drm_bound)
> +		enable_irq(dp->irq);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(dw_dp_runtime_resume);

[Severity: High]
Does this introduce a sleeping delay in an atomic commit path?

The drm_atomic_helper_commit_tail() path is an atomic context in DRM display
drivers. During a commit, it calls dw_dp_bridge_atomic_enable(), which now
makes a synchronous pm_runtime_get_active() call.

That will invoke this dw_dp_runtime_resume() callback which executes
msleep(110). Calling a sleeping function from an atomic commit path violates
DRM subsystem requirements and can cause system instability and warnings.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-synopsys-dw-dp-improvements-v7-0-b7640fa8cf48@collabora.com?part=12