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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect teardown sequence in bridge detach and attach error paths allows use-after-unregister of the DP AUX channel.
- [Medium] Downstream bridge lookup occurs in `attach` rather than `probe`, causing `-EPROBE_DEFER` log spam and preventing native panel support.
--

--- Patch [10]: [PATCH v10 10/21] drm/bridge: synopsys: dw-dp: Add follow-up bridge support ---
commit 57506c503f9891e787cc8982280fc0136a4fc611
Author: Sebastian Reichel <sebastian.reichel-ZGY8ohtN/[email protected]>

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
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> @@ -1893,8 +1895,28 @@ static int dw_dp_bridge_attach(struct drm_bridge *bridge,
>  		goto err_disable_irq;
>  	}
>  
> +	dp->next_bridge = of_drm_get_bridge_by_endpoint(dev->of_node, 1, 0);
> +	if (IS_ERR(dp->next_bridge)) {
> +		ret = PTR_ERR(dp->next_bridge);
> +		dev_err(dev, "failed to get follow-up bridge: %d\n", ret);

[Severity: Medium]
Should this bridge lookup be moved to the probe function instead of being
done in the attach callback? 

Since this is in the attach path, if the downstream bridge is not yet probed,
this returns -EPROBE_DEFER and logs an unconditional error on every retry
since dev_err() is used instead of dev_err_probe().

Also, does using of_drm_get_bridge_by_endpoint() prevent native panel
support here? Moving this to the probe function would allow using
devm_drm_of_get_bridge(), which handles panels automatically and safely
manages allocations without leaking during component unbind/rebind.

> +		goto err_unregister_dp_aux;
> +	}
> +
> +	ret = drm_bridge_attach(encoder, dp->next_bridge, bridge,
> +				DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> +	if (ret) {
> +		dev_err(dev, "Failed to attach next bridge: %d\n", ret);
> +		goto err_put_next_bridge;
> +	}
> +
>  	return 0;
>  
> +err_put_next_bridge:
> +	drm_bridge_put(dp->next_bridge);
> +
> +err_unregister_dp_aux:
> +	drm_dp_aux_unregister(&dp->aux);
> +
>  err_disable_irq:
>  	disable_irq(dp->irq);
>  	cancel_work_sync(&dp->hpd_work);

[Severity: High]
Can this sequence cause a use-after-unregister of the DP AUX channel?

If we unregister the AUX device before disabling the IRQ and cancelling the
HPD worker, hardware events could still trigger interrupts and queue up 
the work. The worker could run concurrently while the AUX device is being 
unregistered, potentially leading to undefined behavior.

Should the IRQ be disabled and the work cancelled synchronously before
unregistering the AUX device?

> @@ -1905,6 +1927,7 @@ static void dw_dp_bridge_detach(struct drm_bridge *bridge)
>  {
>  	struct dw_dp *dp = bridge_to_dp(bridge);
>  
> +	drm_bridge_put(dp->next_bridge);
>  	drm_dp_aux_unregister(&dp->aux);
>  	disable_irq(dp->irq);
>  	cancel_work_sync(&dp->hpd_work);

[Severity: High]
Similarly, does this detach path need the teardown order reversed?

If drm_dp_aux_unregister() runs before disabling the IRQ and cancelling the
work, the same race condition could occur during standard module unbind.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806-synopsys-dw-dp-improvements-v10-0-d84a26c303d6-ZGY8ohtN/[email protected]?part=10