Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Resource leak and duplicate bridge registration on module reload due to lifecycle mismatch. - [High] Duplicate registration conflict with USB-C controller drivers that natively implement HPD bridges. - [Medium] Race condition between bus notifier registration and device iteration leading to duplicate bridge registration. --
--- Patch [1]: [PATCH v5 1/6] drm/bridge: Implement generic USB Type-C DP HPD bridge --- commit 9877135b6e3e85a72f21170c14a083ad3e0308af Author: Chaoyi Chen <[email protected]> drm/bridge: Implement generic USB Type-C DP HPD bridge This patch implements a generic USB Type-C DP HPD bridge that monitors the Type-C bus for devices containing the DP SVID. It automatically creates an HPD bridge for these devices without requiring the USB-C controller driver to implement it manually. > diff --git a/drivers/gpu/drm/bridge/aux-hpd-typec-dp-bridge.c > b/drivers/gpu/drm/bridge/aux-hpd-typec-dp-bridge.c > new file mode 100644 > index 0000000000000..43af3ea20f201 > --- /dev/null > +++ b/drivers/gpu/drm/bridge/aux-hpd-typec-dp-bridge.c > @@ -0,0 +1,64 @@ [ ... ] > +static int drm_typec_bus_event(struct notifier_block *nb, > + unsigned long action, void *data) > +{ > + struct device *dev = (struct device *)data; > + struct typec_altmode *alt = to_typec_altmode(dev); > + > + if (action != BUS_NOTIFY_ADD_DEVICE) > + return NOTIFY_OK; > + > + /* > + * alt->dev.parent->parent : USB-C controller device > + * alt->dev.parent : USB-C connector device > + */ > + if (is_typec_port_altmode(&alt->dev) && alt->svid == USB_TYPEC_DP_SID) > + drm_dp_hpd_bridge_register(alt->dev.parent->parent, > + to_of_node(alt->dev.parent->fwnode)); [Severity: High] Does this unconditional registration create duplicate bridges for controller drivers (like fusb302 or qcom_pmic_typec) that already manually allocate their own HPD bridge for the same device node? If the DRM subsystem binds to the generic bridge instead of the native one, could HPD events sent by the controller via drm_aux_hpd_bridge_notify() to its private bridge be lost? [ ... ] > +static void drm_aux_hpd_typec_dp_bridge_module_exit(void) > +{ > + bus_unregister_notifier(&typec_bus, &drm_typec_event_nb); > +} [Severity: High] Does this leak resources and create duplicate bridges if the module is unloaded and reloaded? Since drm_dp_hpd_bridge_register() uses devm semantics tied to the USB-C controller device (alt->dev.parent->parent) rather than this observer module, the bridges will outlive the module. When the module is reloaded, it will iterate the bus and unconditionally allocate a second set of duplicate bridges. > + > +static int __init drm_aux_hpd_typec_dp_bridge_module_init(void) > +{ > + bus_register_notifier(&typec_bus, &drm_typec_event_nb); > + /* > + * Before module initialization, some devices may have already been > added. > + * Register the HPD bridge for these devices. > + */ > + bus_for_each_dev(&typec_bus, NULL, NULL, check_device_already_added); [Severity: Medium] Is there a race condition here between registering the bus notifier and iterating over the existing devices? If a Type-C altmode device is added concurrently during module initialization, could it trigger the notifier callback and also be processed by the bus iteration loop? Since drm_dp_hpd_bridge_register() lacks deduplication logic, this TOCTOU race might result in duplicate bridge registrations. > + return 0; > +} > + > +module_init(drm_aux_hpd_typec_dp_bridge_module_init); > +module_exit(drm_aux_hpd_typec_dp_bridge_module_exit); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
