Re: [PATCH v4 07/10] drm/bridge: synopsys: dw-dp: Add Runtime PM support

[sashiko-bot@kernel.org]

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

New issues:
- [High] Inverted boolean condition in `dw_dp_unbind()` causes runtime PM usage count leak and unbalanced puts.
- [High] Missing API return value check for `pm_runtime_get_sync()` causes unsafe hardware access on failure.
- [High] Sleeping function `pm_runtime_get_sync()` called from atomic context.
- [High] Missing runtime PM reference release on error path in `dw_dp_bind()`.
- [High] Asynchronous `pm_runtime_put_autosuspend()` is incorrectly used in the teardown path.
- [High] Clock disable imbalance between `devm_clk_get_enabled()` and explicit `clk_disable_unprepare()` in runtime suspend.
- [Medium] Unconditional sleep in `dw_dp_bridge_detect()` causes severe performance degradation during display polling.

Pre-existing issues:
- [Critical] Use-After-Free due to missing cancellation of `hpd_work` in teardown.
- [High] Preexisting atomic context violation in `dw_dp_bridge_atomic_disable()`.
--

--- Patch [7]: [PATCH v4 07/10] drm/bridge: synopsys: dw-dp: Add Runtime PM support ---
commit e300d93b1e1336324cbbd71c5f0b1b81334c7912
Author: Sebastian Reichel <sebastian.reichel@collabora.com>
Date:   Tue Jul 21 17:52:37 2026 +0000

    drm/bridge: synopsys: dw-dp: Add Runtime PM support
    
    Add runtime PM stubs to the Synopsys DesignWare DisplayPort bridge
    driver. Support is not enabled automatically and must be hooked up
    in the vendor specific glue code.
    
    Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>

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

[ ... ]

> @@ -1467,6 +1468,8 @@ static ssize_t dw_dp_aux_transfer(struct drm_dp_aux *aux,
>  	if (WARN_ON(msg->size > 16))
>  		return -E2BIG;
>  
> +	ACQUIRE(pm_runtime_active_auto, pm)(dp->dev);
> +

[Severity: High]
Does the ACQUIRE macro wrapper silently ignore the return value of the
underlying get_sync operation?

If the device fails to power on or clock enablement fails, the driver might
proceed to access hardware registers anyway. Could we handle the error here 
and abort the transfer instead of proceeding?

>  	switch (msg->request & ~DP_AUX_I2C_MOT) {
>  	case DP_AUX_NATIVE_WRITE:
>  	case DP_AUX_I2C_WRITE:

[ ... ]

> @@ -1657,6 +1660,8 @@ static void dw_dp_bridge_atomic_enable(struct drm_bridge *bridge,
>  	struct drm_connector_state *conn_state;
>  	int ret;
>  
> +	pm_runtime_get_sync(dp->dev);
> +

[Severity: High]
Are we allowed to call a sleeping function from atomic context here?

The dw_dp_bridge_atomic_enable() callback executes in atomic context where
sleeping is forbidden. However, pm_runtime_get_sync() triggers
dw_dp_runtime_resume(), which calls clk_prepare_enable() and sleeps.

Also, similar to the ACQUIRE wrapper above, the return value of
pm_runtime_get_sync() is ignored here. If it fails, will the driver proceed
to access hardware registers while the device is unclocked?

>  	connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
>  	if (!connector) {
>  		dev_err(dp->dev, "failed to get connector\n");

[ ... ]

> @@ -1711,6 +1716,7 @@ static void dw_dp_bridge_atomic_disable(struct drm_bridge *bridge,
>  	dw_dp_link_disable(dp);
>  	bitmap_zero(dp->sdp_reg_bank, SDP_REG_BANK_SIZE);
>  	dw_dp_reset(dp);
> +	pm_runtime_put_autosuspend(dp->dev);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does dw_dp_reset() sleep inside
an atomic context?

The dw_dp_bridge_atomic_disable() callback runs in atomic context, but
dw_dp_reset() unconditionally calls usleep_range() and
regmap_read_poll_timeout(). Should this hardware reset sequence be moved out
of the atomic path or use non-sleeping delays?

>  static bool dw_dp_hpd_detect_link(struct dw_dp *dp, struct drm_connector *connector)
> @@ -1731,6 +1737,14 @@ static enum drm_connector_status dw_dp_bridge_detect(struct drm_bridge *bridge,
>  {
>  	struct dw_dp *dp = bridge_to_dp(bridge);
>  
> +	ACQUIRE(pm_runtime_active_auto, pm)(dp->dev);
> +
> +	/*
> +	 * HPD_HOT_PLUG bit is asserted only after the sink holds HPD high for
> +	 * at least 100ms. Thus we need to wait 100ms after runtime PM resume.
> +	 */
> +	msleep(110);
> +

[Severity: Medium]
Will this sleep degrade display polling performance?

By using the ACQUIRE wrapper, the return value of pm_runtime_get_sync() is
discarded, making it impossible to differentiate a fresh resume (returning 0)
from an already-active device (returning 1).

As a result, this 110ms sleep executes unconditionally on every detect call,
even when the device is already active (such as in native DP mode where it is
kept active permanently).

>  	if (!dw_dp_hpd_detect(dp))
>  		return connector_status_disconnected;
>  

[ ... ]

> @@ -2032,10 +2046,15 @@ int dw_dp_bind(struct dw_dp *dp, struct drm_encoder *encoder)
>  	}
>  
>  	if (dw_dp_is_routed_to_usb_c(encoder)) {
> +		dp->usbc_mode = true;
>  		dev_dbg(dev, "USB-C mode\n");
>  
>  		if (dp->plat_data.hpd_sw_sel)
>  			dp->plat_data.hpd_sw_sel(dp->plat_data.data, 1);
> +	} else {
> +		dp->usbc_mode = false;
> +		/* Keep runtime PM enabled to have working native HPD IRQ */
> +		pm_runtime_get_sync(dp->dev);
>  	}
>  
>  	dw_dp_init_hw(dp);

[Severity: High]
Does the error path for hardware initialization following this block leak the
runtime PM reference?

If dp->usbc_mode is false, pm_runtime_get_sync() increments the usage count.
If phy_init() fails shortly after dw_dp_init_hw(), the function jumps to the
err_put_next_bridge label and returns without balancing this get_sync
operation, causing the PM reference to permanently leak.

> @@ -2065,6 +2084,8 @@ EXPORT_SYMBOL_GPL(dw_dp_bind);
>  
>  void dw_dp_unbind(struct dw_dp *dp)
>  {
> +	if (dp->usbc_mode)
> +		pm_runtime_put_autosuspend(dp->dev);

[Severity: High]
Is this boolean condition inverted?

In dw_dp_bind(), pm_runtime_get_sync() is called when usbc_mode is false.
By checking for true here, we call put on a reference we never acquired
(causing an underflow/warning for USB-C) and leak the reference when native DP
mode is used.

[Severity: High]
Is it safe to use the asynchronous pm_runtime_put_autosuspend() variant in a
teardown path?

When device removal or pm_runtime_disable() follows immediately, the pending
async idle/suspend work can be cancelled by the PM core. This could leave the
hardware active indefinitely. Should a synchronous pm_runtime_put_sync() be
used here instead?

>  	disable_irq(dp->irq);

[Severity: Critical]
This is a pre-existing issue, but is there a use-after-free risk because
hpd_work is not canceled?

The hpd_work struct is allocated via devm_kzalloc. In this teardown path, the
IRQ is disabled, but cancel_work_sync(&dp->hpd_work) is never called. If the
work was scheduled by an interrupt just before disable_irq(), the work queue
thread could execute after the devm subsystem frees the memory.

>  	phy_exit(dp->phy);
>  	drm_bridge_put(dp->next_bridge);

[ ... ]

> @@ -2163,6 +2184,35 @@ 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)
> +{
> +	clk_disable_unprepare(dp->aux_clk);
> +	clk_disable_unprepare(dp->apb_clk);
> +
> +	return 0;
> +}

[Severity: High]
Will manually disabling these clocks here cause an imbalance on device teardown?

If the apb_clk and aux_clk clocks are acquired using devm_clk_get_enabled(),
a cleanup action is registered to call clk_disable_unprepare() upon device
unbind. If the device is runtime suspended when it is unbound, the devm
teardown mechanism will invoke clk_disable_unprepare() again on these
already-disabled clocks, dropping the enable counters below zero.

> +EXPORT_SYMBOL_GPL(dw_dp_runtime_suspend);
> +
> +int dw_dp_runtime_resume(struct dw_dp *dp)
> +{
> +	int ret;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-synopsys-dw-dp-improvements-v4-0-f0f4a4ede712@collabora.com?part=7