Re: [PATCH v10 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] Using asynchronous pm_runtime_put_autosuspend() during teardown paths leaves hardware in an active power state.
- [High] Calling sleeping functions from within the bridge atomic enable and disable callbacks violates atomic context restrictions.
- [Medium] Calling `phy_init()` while the APB and AUX clocks are disabled may cause hardware faults.
--

--- Patch [14]: [PATCH v10 14/21] drm/bridge: synopsys: dw-dp: Add Runtime PM support ---
commit c4724f477a725eaa23bb5ef5b62b3fe8b8f02e8a
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.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index 847c2aa95e27c..545dddce69e7b 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
[ ... ]
> @@ -1681,6 +1689,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 introduce a sleeping function into an atomic context?

During a non-blocking atomic commit, the DRM core invokes
dw_dp_bridge_atomic_enable(). This callback executes in an atomic context
where sleeping is forbidden. However, pm_runtime_get_active()
synchronously resumes the device, invoking dw_dp_runtime_resume(), which
subsequently calls msleep(110).

This isn't a bug introduced by this patch, but there are pre-existing
sleeping operations in these paths, including a mutex lock
(scoped_guard(mutex, &dp->audio_lock)) and usleep_range() inside
dw_dp_reset(), which may also need attention to avoid kernel warnings.

[ ... ]
> @@ -1940,6 +1972,9 @@ static int dw_dp_bridge_attach(struct drm_bridge *bridge,
>  err_disable_irq:
>  	disable_irq(dp->irq);
>  	cancel_work_sync(&dp->hpd_work);
> +
> +	pm_runtime_put_autosuspend(dp->dev);
> +
>  	return ret;
>  }

[Severity: High]
Will this async put leave the hardware in an active power state?

This async put is used in the attach error cleanup path, which will also be
immediately followed by driver unbind and PM disable. The delayed suspend
is scheduled right before the device is unregistered. 

Because device removal immediately follows, the devres cleanup executes
pm_runtime_disable(), which cancels the pending async suspend work. This
aborts the power-down sequence and leaves the apb_clk and aux_clk enabled.
Should pm_runtime_put_sync() be used here instead?

[ ... ]
> @@ -1951,6 +1986,8 @@ static void dw_dp_bridge_detach(struct drm_bridge *bridge)
>  	drm_dp_aux_unregister(&dp->aux);
>  	disable_irq(dp->irq);
>  	cancel_work_sync(&dp->hpd_work);
> +	if (!dp->usbc_mode)
> +		pm_runtime_put_autosuspend(dp->dev);
>  }

[Severity: High]
Similar to the error path in dw_dp_bridge_attach(), does this need to be a
synchronous put? pm_runtime_put_sync() ensures the idle/suspend completes
before the subsequent device deletion.

[ ... ]
> @@ -2209,31 +2283,57 @@ struct dw_dp *dw_dp_probe(struct platform_device *pdev, const struct dw_dp_plat_
[ ... ]
> +	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: Medium]
Can calling phy_init() while the APB and AUX clocks are disabled cause
hardware faults?

If PM runtime is active (which is the default if autosuspend_delay > 0),
the apb_clk and aux_clk remain disabled until the device is explicitly
resumed. However, phy_init() is still called unconditionally at the end of
probe. If the generic PHY driver accesses internal registers that rely on
the display controller's APB bus clock, the unclocked access might hang the
system.

[ ... ]
> +int dw_dp_runtime_resume(struct dw_dp *dp)
> +{
[ ... ]
> +	/*
> +	 * HPD_HOT_PLUG bit is asserted only after the sink holds HPD
> +	 * high for at least 100ms.
> +	 */
> +	msleep(110);
> +
> +	return 0;
> +}

[Severity: High]
As mentioned earlier, does this msleep(110) introduce a sleep into atomic
context when reached from dw_dp_bridge_atomic_enable() via
pm_runtime_get_active()?

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