Re: [PATCH v9 15/22] drm/bridge: synopsys: dw-dp: Add Runtime PM support

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, raci=
ng with pm_runtime_disable() and leaving the device explicitly powered on.
- [High] Sleeping functions are invoked synchronously from an atomic contex=
t during bridge atomic enable.

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

--- Patch [15]: [PATCH v9 15/22] drm/bridge: synopsys: dw-dp: Add Runtime P=
M support ---
commit 2b85362372523d505a6d2f6be68c1219a8790052
Author: Sebastian Reichel <[email protected]>

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/br=
idge/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;
> =20
> +	ret =3D pm_runtime_get_active(dp->dev, RPM_TRANSPARENT);
> +	if (ret) {
> +		dev_err(dp->dev, "runtime PM failure\n");
> +		return;
> +	}
> +	dp->pm_active =3D true;
> +

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

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

If the device is suspended, pm_runtime_get_active() can synchronously execu=
te
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 =3D drm_atomic_get_new_connector_for_encoder(state, bridge->e=
ncoder);
>  	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_acti=
ve
set to true.

Later, the DRM core invokes dw_dp_bridge_atomic_disable() which sees
dp->pm_active =3D=3D 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 asynchron=
ous
suspend. However, pm_runtime_disable() cancels pending asynchronous suspend
requests. Will this leave the device permanently active? Should a synchrono=
us
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);
>  }
> =20
> -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 =3D data;
> +
> +	dw_dp_runtime_suspend(dp);
> +}
> +
> +static void dw_dp_irq_free(void *data)
> +{
> +	struct dw_dp *dp =3D 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 =3D -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?

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260803-synopsys-d=
[email protected]?part=3D15