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 <[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 > 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-dc61e6352...@collabora.com?part=3
