Re: [PATCH v5 03/15] 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 `reinit_completion` in `dw_dp_aux_transfer` allows stale completion signals to falsely succeed future transfers.
- [High] The AUX device is registered and exposed to userspace before the hardware is initialized and the IRQ is enabled.
- [High] Concurrent HPD interrupts can overwrite `dp->hotplug.long_hpd`, causing dropped hotplug events.
--

--- Patch [3]: [PATCH v5 03/15] drm/bridge: synopsys: dw-dp: Add missing mutex cleanups on module removal ---
commit bc7168d1bf0fe1e3fae642d5cb72e8aa27942948
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.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index 21bd08571dcfe..b28040d69fabf 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> @@ -2031,10 +2031,13 @@ 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_WORK(&dp->hpd_work, dw_dp_hpd_work);

[Severity: High]
This is a pre-existing issue, but could concurrent HPD interrupts overwrite
dp->hotplug.long_hpd, causing dropped hotplug events?

In drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_handle_hpd_event(), the flag
is updated based on interrupt status:

	if (value & HPD_IRQ) {
		...
		dp->hotplug.long_hpd = false;
		...
	}

	if (value & HPD_HOT_PLUG) {
		...
		dp->hotplug.long_hpd = true;
		...
	}

Since schedule_work() ensures only a single instance of the work runs,
multiple interrupts firing in rapid succession (e.g., a hot plug followed
immediately by an IRQ/short HPD from the sink requesting link training)
could overwrite the long_hpd flag before the worker can process the first
event.

The worker would only see the final state (e.g., false for short HPD),
permanently dropping the hotplug notification.

>  	init_completion(&dp->complete);

[Severity: High]
This is a pre-existing issue, but is there a missing reinit_completion() in
dw_dp_aux_transfer() that could allow stale completion signals to falsely
succeed future transfers?

In drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_aux_transfer(), the driver
waits for an AUX reply:

	regmap_write(dp->regmap, DW_DP_AUX_CMD, value);

	status = wait_for_completion_timeout(&dp->complete, timeout);
	if (!status) {
		dev_err(dp->dev, "timeout waiting for AUX reply\n");
		return -ETIMEDOUT;
	}

If an AUX transfer times out, the completion's internal done counter is not
consumed. If the hardware later fires a delayed AUX_REPLY_EVENT interrupt,
the IRQ handler will call complete(&dp->complete), incrementing the counter.
Subsequent calls to dw_dp_aux_transfer() would then immediately return from
wait_for_completion_timeout() without waiting for the actual hardware reply.

[Severity: High]
This is another pre-existing issue, but is the AUX device registered and
exposed to userspace before the hardware is initialized and the IRQ is
enabled?

In drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_bind():

	ret = drm_dp_aux_register(&dp->aux);
	if (ret) {
		...
	}
	...
	dw_dp_init_hw(dp);
	
	ret = phy_init(dp->phy);
	...
	enable_irq(dp->irq);

If a userspace process opens the /dev/drm_dp_auxN device and issues a read
during the window after drm_dp_aux_register() but before enable_irq(), the
transfer will time out. This initial timeout would permanently corrupt the
completion state as described above, causing future legitimate EDID reads and
link training to fail.

>  
> +	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);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724-synopsys-dw-dp-improvements-v5-0-9445c2e87441@collabora.com?part=3