Re: [PATCH v8 14/21] drm/bridge: synopsys: dw-dp: Add Runtime PM support

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Asynchronous pm_runtime_put_autosuspend() during driver teardown and error paths is cancelled by pm_runtime_disable(), leaving the device permanently active and leaking clock resources.
- [High] phy_init() is called while the device is in its initial RPM_SUSPENDED state with APB and AUX clocks disabled, risking a bus hang.
--

--- Patch [14]: [PATCH v8 14/21] drm/bridge: synopsys: dw-dp: Add Runtime PM support ---
commit e3dd87d799174b12a4a2ad492ab236cf0e86bc18
Author: Sebastian Reichel <[email protected]>
Subject: drm/bridge: synopsys: dw-dp: Add Runtime PM support

This commit adds runtime PM stubs to the Synopsys DesignWare DisplayPort
bridge driver. It splits the early parts of dw_dp_probe into dw_dp_alloc
to allow platform drivers to assign it before probing.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index b21c2016dbd29..c24a5436e8fe8 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
[ ... ]
> @@ -2108,76 +2162,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]
Will this asynchronous autosuspend be cancelled by pm_runtime_disable() during
device removal?

When this devm teardown action runs, it releases the PM runtime reference
asynchronously via the PM_RUNTIME_ACQUIRE_AUTOSUSPEND scope guard. Immediately
afterward, the devm_pm_runtime_enable cleanup callback executes
pm_runtime_disable().

As documented in the PM subsystem rules, pm_runtime_disable() cancels pending
autosuspend work, meaning dw_dp_runtime_suspend() will never be called. Would
it be safer to use a synchronous suspend operation like pm_runtime_put_sync()
here to ensure the clocks do not remain permanently enabled?

[ ... ]
> +	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;
> +	}
>  
>  	ret = phy_init(dp->phy);
>  	if (ret) {
>  		dev_err_probe(dev, ret, "phy init failed\n");
> -		return ERR_PTR(ret);
> +		return ret;
>  	}

[Severity: High]
Could this phy_init() call trigger a bus hang if the device is still suspended?

Prior to this patch, the APB and AUX clocks were explicitly enabled before
phy_init() was called. Now, clock enablement is delegated to
dw_dp_runtime_resume().

If autosuspend_delay > 0, devm_pm_runtime_enable() is called but the device is
left in its default RPM_SUSPENDED state. Since phy_init() is called without
first waking the device via pm_runtime_get_sync(), the DP controller's APB
and AUX clocks are still disabled.

If the external PHY shares the APB bus or relies on the controller being
clocked to access its registers, will this unclocked access cause a
synchronous external abort?

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