Re: [PATCH v9 06/22] drm/bridge: synopsys: dw-dp: Add missing mutex cleanups on module removal

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing Completion Reinitialization causes permanent link training desynchronization on timeouts.
- [High] Concurrent hardware modification between the HPD worker and non-blocking atomic commits leads to hardware state corruption.
- [Medium] Hotplug events can be lost due to overwriting the single boolean tracking variable `dp->hotplug.long_hpd` before the worker processes it.
--

--- Patch [6]: [PATCH v9 06/22] drm/bridge: synopsys: dw-dp: Add missing mutex cleanups on module removal ---
commit d8b2520d774bd92589d70544eca3e846bc879920
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

drm/bridge: synopsys: dw-dp: Add missing mutex cleanups on module removal

The driver is currently missing to fully clean up after itself. Ensure
that the mutex is cleaned up.

Reported-by: Sashiko <sashiko-bot@kernel.org>

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index facd1c4799923..af3fa7886429b 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> @@ -2039,9 +2039,12 @@ struct dw_dp *dw_dp_probe(struct platform_device *pdev, const struct dw_dp_plat_
>  	dp->pixel_mode = plat_data->pixel_mode;
>  	dp->plat_data.max_link_rate = plat_data->max_link_rate;
>  
> -	mutex_init(&dp->irq_lock);
>  	init_completion(&dp->complete);

[Severity: High]
This isn't a bug introduced by this patch, but while looking at the
completion initialization, I noticed a potential regression.

If dw_dp_aux_transfer() times out and returns -ETIMEDOUT, and a delayed
hardware reply or spurious IRQ later completes the completion:

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_aux_transfer() {
	...
	regmap_write(dp->regmap, DW_DP_AUX_CMD, value);

	status = wait_for_completion_timeout(&dp->complete, timeout);
	...
}

Would a subsequent call to dw_dp_aux_transfer() immediately consume the stale
completion since reinit_completion() is not called before the transfer?
Could this lead to garbage DPCD reads and permanent link training failure?

>  
> +	ret = devm_mutex_init(dev, &dp->irq_lock);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
>  	res = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(res))
>  		return ERR_CAST(res);

[Severity: Medium]
This is a pre-existing issue, but could hotplug events be lost if a short
pulse overwrites the state before the worker processes it?

If a monitor is plugged in and asserts a long HPD pulse, and a short HPD
pulse occurs before the worker executes:

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_handle_hpd_event() {
	...
	if (value & HPD_IRQ) {
		dev_dbg(dp->dev, "IRQ from the HPD\n");
		dp->hotplug.long_hpd = false;
		regmap_write(dp->regmap, DW_DP_HPD_STATUS, HPD_IRQ);
	}
	...
}

Does this overwrite the boolean flag, causing the worker to branch incorrectly
and skip the hotplug notification?

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_hpd_work() {
	...
	if (!long_hpd) {
		if (dw_dp_needs_link_retrain(dp)) {
			ret = dw_dp_link_retrain(dp);
			...
}

[Severity: High]
This is a pre-existing issue, but I am wondering if there is a concurrent
hardware modification regression between the HPD worker and non-blocking
atomic commits.

The worker relies on connection_mutex, which DRM does not hold during
asynchronous atomic hardware commits:

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_link_retrain() {
	...
	for (;;) {
		ret = drm_modeset_lock(&dev->mode_config.connection_mutex, &ctx);
		if (ret != -EDEADLK)
			break;

		drm_modeset_backoff(&ctx);
	}
	...
}

Concurrently, dw_dp_bridge_atomic_enable() executes in the commit worker
without connection_mutex:

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_bridge_atomic_enable() {
	...
	ret = dw_dp_link_enable(dp);
	if (ret < 0) {
	...
}

Could both paths attempt to modify PHY and DPCD registers simultaneously,
leading to hardware corruption or link training failures?

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