Re: [PATCH v9 15/22] 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:
- [High] Asynchronous runtime PM puts are used during device teardown, racing with pm_runtime_disable() and leaving the device explicitly powered on.
- [High] Sleeping functions are invoked synchronously from an atomic context during bridge atomic enable.

Pre-existing issues:
- [High] Unbalanced resource handling (double-free of phy power and underflow) during failed atomic enable.
--

--- Patch [15]: [PATCH v9 15/22] drm/bridge: synopsys: dw-dp: Add Runtime PM support ---
commit 2b85362372523d505a6d2f6be68c1219a8790052
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

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 platform specific glue code.

The early bits of the dw_dp_probe function are split into a new
function called dw_dp_alloc, so that the platform driver can assign
it before running dw_dp_probe. This is necessary because the runtime
PM resume/suspend events land at the platform driver and must be
forwarded to the helper once runtime PM is enabled in the middle
of the probe function.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index 7b8a9855ceae0..501b28abeca24 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c

[ ... ]

> @@ -1675,6 +1683,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;
> +

[Severity: High]
Does this synchronous runtime PM resume call sleep while in an atomic context?

The DRM core invokes dw_dp_bridge_atomic_enable() during atomic commits, which
runs in an atomic context for non-blocking commits. 

If the device is suspended, pm_runtime_get_active() can synchronously execute
dw_dp_runtime_resume(), which calls clk_prepare_enable() and msleep():

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_runtime_resume() {
    ...
    msleep(110);
    ...
}

Could this lead to kernel warnings for scheduling while atomic, or potential
deadlocks?

>  	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 isn't a bug introduced by this patch, but does this early return cause an
unbalanced resource cleanup if the link enable fails?

If the connector is NULL, the function returns early without clearing
dp->pm_active. Likewise, if dw_dp_link_enable() fails later in the function, it
cleans up by calling phy_power_off() and returns early, leaving dp->pm_active
set to true.

Later, the DRM core invokes dw_dp_bridge_atomic_disable() which sees
dp->pm_active == true and proceeds to call dw_dp_link_disable(), which
unconditionally calls phy_power_off() and drm_dp_link_power_down().

Could this result in a double-free of the phy power or an underflow of the
power count?

[ ... ]

> @@ -2110,6 +2161,9 @@ EXPORT_SYMBOL_GPL(dw_dp_bind);
>  void dw_dp_unbind(struct dw_dp *dp)
>  {
>  	drm_bridge_put(dp->next_bridge);
> +
> +	if (!dp->usbc_mode)
> +		pm_runtime_put_autosuspend(dp->dev);
>  }
>  EXPORT_SYMBOL_GPL(dw_dp_unbind);

[Severity: High]
Can this asynchronous put race with device teardown?

During component teardown, pm_runtime_put_autosuspend() queues an asynchronous
suspend. However, pm_runtime_disable() cancels pending asynchronous suspend
requests. Will this leave the device permanently active? Should a synchronous
put like pm_runtime_put_sync() be used here instead?

[ ... ]

> @@ -2120,76 +2174,125 @@ static void dw_dp_phy_exit(void *data)
>  	phy_exit(dp->phy);
>  }
>  
> -struct dw_dp *dw_dp_probe(struct platform_device *pdev, const struct dw_dp_plat_data *plat_data)
> +static void dw_dp_manual_suspend(void *data)
> +{
> +	struct dw_dp *dp = data;
> +
> +	dw_dp_runtime_suspend(dp);
> +}
> +
> +static void dw_dp_irq_free(void *data)
> +{
> +	struct dw_dp *dp = data;
> +
> +	/* ignore error as the interrupt needs to be free'd in any case */
> +	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(dp->dev, pm);
> +
> +	free_irq(dp->irq, dp);
> +	dp->irq = -1;
> +}

[Severity: High]
Similarly, does this asynchronous put in dw_dp_irq_free() race with
pm_runtime_disable() during devm cleanup?

When devm triggers cleanup on driver unbind or probe failure, it invokes
dw_dp_irq_free() which uses PM_RUNTIME_ACQUIRE_AUTOSUSPEND to release the
reference asynchronously. The next step in devm cleanup is usually
devm_pm_runtime_enable() unwinding, which calls pm_runtime_disable().

Could this cancel the pending suspend request and result in a power and
clock resource leak?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803-synopsys-dw-dp-improvements-v9-0-2e6e46135312@collabora.com?part=15