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

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-bl=
ocking 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 mu=
tex cleanups on module removal ---
commit d8b2520d774bd92589d70544eca3e846bc879920
Author: Sebastian Reichel <[email protected]>

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 <[email protected]>

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/br=
idge/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 =3D plat_data->pixel_mode;
>  	dp->plat_data.max_link_rate =3D plat_data->max_link_rate;
> =20
> -	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 =3D wait_for_completion_timeout(&dp->complete, timeout);
	...
}

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

> =20
> +	ret =3D devm_mutex_init(dev, &dp->irq_lock);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
>  	res =3D 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 =3D false;
		regmap_write(dp->regmap, DW_DP_HPD_STATUS, HPD_IRQ);
	}
	...
}

Does this overwrite the boolean flag, causing the worker to branch incorrec=
tly
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 =3D 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 =3D drm_modeset_lock(&dev->mode_config.connection_mutex, &ctx);
		if (ret !=3D -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 =3D 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?

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