Re: [PATCH v11 14/21] 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:
- [High] Devm action unmasks the IRQ during driver unload due to skipped runtime suspend, creating a Use-After-Free window.
- [Medium] Asynchronous pm_runtime_put_autosuspend() in teardown paths races with pm_runtime_disable(), leaving the device powered on.
- [Medium] The return value of dw_dp_runtime_resume() is ignored in dw_dp_probe() when runtime PM is disabled.
--

--- Patch [14]: [PATCH v11 14/21] drm/bridge: synopsys: dw-dp: Add Runtime PM support ---
commit af390b7b3091a44b56036dc11fc278ff1a6d55a3
Author: Sebastian Reichel <sebastian.reichel@collabora.com>
Subject: drm/bridge: synopsys: dw-dp: Add Runtime PM support

This commit adds runtime power management stubs to the Synopsys DesignWare
DisplayPort bridge driver. It splits the probe function to allow platform
drivers to assign it early so that runtime events can be properly routed.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index 3af5b1f643c10..31f944e143ea5 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> @@ -1931,6 +1963,9 @@ static int dw_dp_bridge_attach(struct drm_bridge *bridge,
>  
>  	drm_dp_aux_unregister(&dp->aux);
>  
> +err_runtime_pm_put:
> +	pm_runtime_put_autosuspend(dp->dev);
> +
>  	return ret;
>  }
>  
> @@ -1941,6 +1976,9 @@ static void dw_dp_bridge_detach(struct drm_bridge *bridge)
>  	disable_irq(dp->irq);
>  	cancel_work_sync(&dp->hpd_work);
>  	drm_dp_aux_unregister(&dp->aux);
> +
> +	if (!dp->usbc_mode)
> +		pm_runtime_put_autosuspend(dp->dev);
>  }

[Severity: Medium]
Will this asynchronous pm_runtime_put_autosuspend() race with
pm_runtime_disable() during device removal or probe failure?

In dw_dp_bridge_detach() and the error path of dw_dp_bridge_attach(), the
driver drops its runtime reference using an asynchronous put. However,
during component teardown or probe failure, this is quickly followed by the
devm cleanup phase, which calls pm_runtime_disable() (registered via
devm_pm_runtime_enable() in probe).

According to power management guidelines, pm_runtime_disable() cancels any
pending asynchronous suspend timers. Because the put was asynchronous and the
timer is cancelled before it expires, the device remains in the RPM_ACTIVE
state and is never physically suspended, causing a power leak.

Could a synchronous put (like pm_runtime_put_sync()) be used here instead?

[ ... ]

> @@ -2205,41 +2280,67 @@ struct dw_dp *dw_dp_probe(struct platform_device *pdev, const struct dw_dp_plat_
> +	/*
> +	 * Disable IRQ a second time; this ensures the interrupt is only
> +	 * enabled when the bridge is attached AND runtime PM is enabled.
> +	 * Also register a devm action to restore the correct balance during
> +	 * device removal.
> +	 */
> +	disable_irq(dp->irq);
> +
> +	ret = devm_add_action_or_reset(dev, dw_dp_enable_irq, dp);
> +	if (ret)
> +		return ret;

[Severity: High]
Does this devm action risk a use-after-free by unmasking the interrupt during
driver unload?

The driver uses the interrupt disable depth as a refcount: IRQF_NO_AUTOEN
(depth=1) plus the explicit disable_irq() in probe (depth=2). It expects two
enable_irq() calls (attach and runtime_resume) to reach depth 0.

If the device fails to runtime suspend during teardown (e.g., due to the
pm_runtime_put_autosuspend() cancellation race described above),
dw_dp_runtime_suspend() is skipped, and its corresponding disable_irq() is
missed. When dw_dp_bridge_detach() calls disable_irq(), the depth only
returns to 1. 

Later, the devm cleanup action dw_dp_enable_irq() decrements the depth.
Since the depth was 1, it reaches 0 and physically unmasks the interrupt.

If an interrupt fires in the small window between dw_dp_enable_irq() and
free_irq(), dw_dp_irq() queues dp->hpd_work. Since cancel_work_sync() was
already called in dw_dp_bridge_detach(), this requeues the work on a dp
structure that is about to be freed by devres, resulting in a use-after-free
when the workqueue runs.

> +	if (dp->plat_data.autosuspend_delay > 0) {
> +		pm_runtime_use_autosuspend(dev);
> +		pm_runtime_set_autosuspend_delay(dev, dp->plat_data.autosuspend_delay);
> +		ret = devm_pm_runtime_enable(dev);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (!pm_runtime_enabled(dev)) {
> +		dw_dp_runtime_resume(dp);
> +
> +		ret = devm_add_action_or_reset(dev, dw_dp_manual_suspend, dp);
> +		if (ret)
> +			return ret;
> +	}

[Severity: Medium]
Should the return value of dw_dp_runtime_resume() be checked here?

When pm_runtime_enabled(dev) is false, dw_dp_runtime_resume() is called
manually to power up the hardware, but its return value is ignored. If it
fails (for example, if clk_prepare_enable() fails), it aborts early and
leaves resources like aux_clk and the interrupt disabled. 

However, the probe continues and registers the dw_dp_manual_suspend devres
action. During driver teardown, dw_dp_manual_suspend() unconditionally calls
clk_disable_unprepare(dp->aux_clk) and disable_irq(dp->irq). Calling these
on resources that were never enabled triggers subsystem warnings.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806-synopsys-dw-dp-improvements-v11-0-0d508505f383@collabora.com?part=14