Re: [PATCH v3 05/10] drm/bridge: synopsys: dw-dp: Support software triggered OOB HPD

[sashiko-bot@kernel.org]

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

New issues:
- [High] Reference leak of the downstream bridge in `dw_dp_bind()` on both success and error paths.
- [High] The driver incorrectly treats the absence of a downstream bridge as a fatal error, breaking backward compatibility.

Pre-existing issues:
- [High] Missing `cancel_work_sync()` for `dp->hpd_work` during unbind leads to a Use-After-Free.
--

commit 121b9fc711d52bcdbd8b50a6dc50a5308864d167
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

drm/bridge: synopsys: dw-dp: Support software triggered OOB HPD

Add support for USB-C DP AltMode out-of-band hotplug handling. The
handling itself is implemented in the platform specific driver as the
registers to force HPD state are not part of the Designware DisplayPort
IP itself. Instead the platform integration might provide the necessary
functionality to mux the HPD signal.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index 98cff435dfb87..7fa38145e35ce 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c

[ ... ]

> @@ -2078,6 +2107,13 @@ struct dw_dp *dw_dp_bind(struct device *dev, struct drm_encoder *encoder,
>  		goto unregister_aux;
>  	}

[Severity: High]
Does dw_dp_bind() leak the reference to the downstream bridge?

of_drm_get_bridge_by_endpoint() returns a bridge pointer with an incremented 
reference count, which is stored in next_bridge:

	next_bridge = of_drm_get_bridge_by_endpoint(dev->of_node, 1, 0);
	if (IS_ERR(next_bridge)) {
		ret = PTR_ERR(next_bridge);
        ...

The reference is not dropped after drm_bridge_attach() succeeds, nor is it 
dropped on the error path before jumping to unregister_aux. Could this leak 
the reference on both success and error paths, preventing the downstream 
bridge from cleanly unloading?


[Severity: High]
Should the absence of a downstream bridge be treated as a fatal error?

If the device tree defines the DP controller as the end of the display chain
(i.e., no endpoint 1 is defined), of_drm_get_bridge_by_endpoint() returns
-ENODEV. The code unconditionally aborts initialization here instead of 
gracefully ignoring it:

	next_bridge = of_drm_get_bridge_by_endpoint(dev->of_node, 1, 0);
	if (IS_ERR(next_bridge)) {
		ret = PTR_ERR(next_bridge);
		dev_err_probe(dev, ret, "failed to get follow-up bridge.\n");
		goto unregister_aux;
	}

Could this break backward compatibility and cause the driver to fail to probe 
on boards that do not define a downstream bridge?

>  
> +	if (dw_dp_is_routed_to_usb_c(encoder)) {
> +		dev_dbg(dev, "USB-C mode\n");
> +
> +		if (dp->plat_data.hpd_sw_sel)
> +			dp->plat_data.hpd_sw_sel(dp->plat_data.data, 1);
> +	}
> +
>  	dw_dp_init_hw(dp);

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but does dw_dp_unbind() leave 
pending work scheduled?

During bind, the driver initializes dp->hpd_work and schedules it in the 
interrupt handler dw_dp_handle_hpd_event(). However, dw_dp_unbind() does not 
cancel it:

void dw_dp_unbind(struct dw_dp *dp)
{
	drm_dp_aux_unregister(&dp->aux);
}

Because the dp context is allocated with devm_drm_bridge_alloc(), it is 
automatically freed shortly after unbind returns. If the scheduled work 
executes after dp is freed, could it cause a use-after-free in 
dw_dp_hpd_work()?

Should cancel_work_sync(&dp->hpd_work) be added to dw_dp_unbind()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-synopsys-dw-dp-improvements-v3-0-dc61e6352508@collabora.com?part=5