Re: [PATCH v3 03/10] drm/bridge: synopsys: dw-dp: Add follow-up bridge support

[sashiko-bot@kernel.org]

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

New issues:
- [High] Memory leak of the follow-up bridge reference count.
- [High] Unconditional probe failure causing DT backward compatibility breakage.

Pre-existing issues:
- [High] Missing completion reinitialization causes race conditions with stale completion states during AUX transfers.
- [High] Premature registration and exposure of the DP AUX channel to userspace before hardware and IRQ initialization.
--

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

drm/bridge: synopsys: dw-dp: Add follow-up bridge support

Add support to use USB-C connectors with the DP altmode helper code on
devicetree based platforms. To get this working there must be a DRM bridge
chain from the DisplayPort controller to the USB-C connector.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index ac7779e42bb24..98cff435dfb87 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> @@ -1970,7 +1970,7 @@ struct dw_dp *dw_dp_bind(struct device *dev, struct drm_encoder *encoder,
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct dw_dp *dp;
> -	struct drm_bridge *bridge;
> +	struct drm_bridge *bridge, *next_bridge;
>  	void __iomem *res;
>  	int ret;
>  
> @@ -2064,6 +2064,20 @@ struct dw_dp *dw_dp_bind(struct device *dev, struct drm_encoder *encoder,
>  		goto unregister_aux;
>  	}
>  
> +	next_bridge = of_drm_get_bridge_by_endpoint(dev->of_node, 1, 0);
> +	if (IS_ERR(next_bridge)) {

[Severity: High]
Can this unconditional check break device tree backward compatibility?

By making the absence of a follow-up bridge a fatal error instead of an
optional configuration, this appears to cause the display driver to
completely fail to probe on existing platforms that do not define a downstream
bridge endpoint in their device tree.

> +		ret = PTR_ERR(next_bridge);
> +		dev_err_probe(dev, ret, "failed to get follow-up bridge.\n");
> +		goto unregister_aux;
> +	}
> +
> +	ret = drm_bridge_attach(encoder, next_bridge, bridge,
> +				DRM_BRIDGE_ATTACH_NO_CONNECTOR);

[Severity: High]
Does this code leak the next_bridge reference?

Calling of_drm_get_bridge_by_endpoint() increments the reference count of the
returned bridge, but there doesn't seem to be a corresponding drm_bridge_put()
on either the success or error paths to release it.

> +	if (ret) {
> +		dev_err_probe(dev, ret, "Failed to attach next bridge\n");
> +		goto unregister_aux;
> +	}
> +
>  	dw_dp_init_hw(dp);
>  

[Severity: High]
This is a pre-existing issue, but does registering the AUX channel early
expose uninitialized hardware to userspace?

Looking earlier in dw_dp_bind():

	ret = drm_dp_aux_register(&dp->aux);
	if (ret) {
		dev_err_probe(dev, ret, "Aux register failed\n");
		return ERR_PTR(ret);
	}

And then later:

	dw_dp_init_hw(dp);

	ret = phy_init(dp->phy);

If a userspace process opens the AUX character device and initiates a
transaction before dw_dp_init_hw() and the IRQ setup are called, the hardware
may enter an invalid state and miss completion interrupts.

>  	ret = phy_init(dp->phy);

[Severity: High]
This is a pre-existing issue, but can the lack of completion reinitialization
cause race conditions?

Looking at dw_dp_aux_transfer() in the same file:

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

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

If a previous AUX transfer timed out but the hardware eventually triggered a
late interrupt, the completion object will remain in a done state. The next
AUX transfer will see this stale state, causing wait_for_completion_timeout()
to return immediately before the new transfer has actually completed.

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