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 <[email protected]> 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-dc61e6352...@collabora.com?part=5
