Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] The module registers a bus notifier to catch device additions but fails to iterate over already-registered devices, ignoring any Type-C ports present before the module was loaded. - [High] The module attaches devm-managed bridges to a foreign USB-C controller device but fails to clean them up on module unload or device removal, leading to resource leaks and an execute-after-free kernel panic. - [Medium] The return value of `bus_register_notifier()` is ignored. --
--- Patch [1]: [PATCH v3 1/5] drm/bridge: Implement generic USB Type-C DP HPD bridge --- commit 5c3c19cbb4491c572fdd386c797dfba9412af2b3 Author: Chaoyi Chen <[email protected]> drm/bridge: Implement generic USB Type-C DP HPD bridge This patch implements a generic DP HPD bridge based on aux-hpd-bridge.c. It monitors Type-C bus events and automatically creates an HPD bridge when a Type-C port device containing the DP svid is registered. > 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 > --- /dev/null > +++ b/drivers/gpu/drm/bridge/aux-hpd-typec-dp-bridge.c > @@ -0,0 +1,54 @@ [ ... ] > +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] Will this leak devm resources on the controller device when a port is removed and re-added, since BUS_NOTIFY_DEL_DEVICE is ignored? Also, because the devm cleanup callbacks are tied to the controller device rather than this module, could this cause an execute-after-free panic? If this module and aux-hpd-bridge are unloaded, the devres callbacks on the controller will point to unmapped memory when the controller is later unbound. [ ... ] > +static int __init drm_aux_hpd_typec_dp_bridge_module_init(void) > +{ > + bus_register_notifier(&typec_bus, &drm_typec_event_nb); [Severity: Medium] Is it safe to ignore the return value of bus_register_notifier() here? If the notifier registration fails, the module initialization will succeed but remain non-functional. [Severity: High] Does this code miss existing Type-C DP ports that were probed before this module was loaded? bus_register_notifier() only triggers notifications for devices added after registration. Since there is no bus_for_each_dev() iteration over existing devices on the typec_bus after registering the notifier, pre-existing ports might never get an HPD bridge created. > + > + return 0; > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
