Re: [PATCH v6 03/16] 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() before AUX transfer causes subsequent transfers to falsely succeed if a timeout previously occurred or stray interrupts arrived.
- [High] Short HPD and Long HPD events share a single boolean state variable, causing critical hot plug events to be lost if interrupts arrive in quick succession.
- [High] Hardware is powered down and interrupts are disabled before the DP AUX interface is unregistered, allowing userspace to crash the system or hang by accessing powered-down hardware.
--

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

Fixes: 86eecc3a9c2e ("drm/bridge: synopsys: Add DW DPTX Controller support library")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>

> 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

[ ... ]

This is a pre-existing issue, but I noticed a potential problem in
dw_dp_aux_transfer():

	value |= FIELD_PREP(AUX_ADDR, msg->address);
	regmap_write(dp->regmap, DW_DP_AUX_CMD, value);

	status = wait_for_completion_timeout(&dp->complete, timeout);
	if (!status) {

[Severity: High]
Does this completion need to be reinitialized before the transfer?
If a previous transfer timed out and the hardware later replies, 
could a subsequent transfer falsely succeed immediately because the
completion counter was left > 0?

[ ... ]

This isn't a bug introduced by this patch, but there is a potential
event loss in the HPD handler.

If a physical hotplug triggers an interrupt:

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

Could a subsequent short HPD interrupt (e.g., from a link status change)
overwrite this state before dw_dp_hpd_work() evaluates it?

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

[Severity: High]
If dw_dp_hpd_work() runs after this short pulse, will it read long_hpd as
false and perform a link retrain instead of notifying userspace of the
hotplug?

[ ... ]

This is also a pre-existing issue, but I noticed a potential hardware hang
in the unbind sequence just above the probe function:

void dw_dp_unbind(struct dw_dp *dp)
{
	disable_irq(dp->irq);
	cancel_work_sync(&dp->hpd_work);
	phy_exit(dp->phy);
	drm_dp_aux_unregister(&dp->aux);

[Severity: High]
Does this order allow userspace to access unpowered hardware?
Since phy_exit() powers down the PHY before the AUX interface is
unregistered, could a concurrent read/write to the /dev/drm_dp_auxN
character device cause a synchronous external abort or hardware hang?

> @@ -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);
>  	init_completion(&dp->complete);
>  
> +	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-v6-0-041d99a19c4e@collabora.com?part=3