Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Critical] Unbounded memory leak and devres accumulation due to missing BUS_NOTIFY_DEL_DEVICE handler. - [High] Missing initialization for pre-existing Type-C devices. - [High] Double bridge registration conflicts with explicit controller driver bridges. --
commit 8b46d64e255268bcd7c9f7656649f0dfbcb9e277 Author: Chaoyi Chen <[email protected]> Subject: drm/bridge: Implement generic USB Type-C DP HPD bridge This patch implements a generic DP HPD bridge that monitors Type-C bus events. When a Type-C port device containing the DP SVID is registered, it automatically creates an HPD bridge for it without requiring the USB-C controller driver to implement the feature. > 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..c09579ff7ab91 > --- /dev/null > +++ b/drivers/gpu/drm/bridge/aux-hpd-typec-dp-bridge.c > @@ -0,0 +1,54 @@ > +// SPDX-License-Identifier: GPL-2.0+ [ ... ] > +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; [Severity: Critical] Since this only handles BUS_NOTIFY_ADD_DEVICE, will this leak memory when a cable is unplugged? Type-C altmode devices are frequently registered and unregistered during normal operation. Because there is no BUS_NOTIFY_DEL_DEVICE handler to clean up these bridges, and the registration uses a devm allocation attached to the parent controller, won't every plug event leak a new bridge allocation and IDA entry? > + > + /* > + * 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 unconditionally allocate a new bridge even if the controller driver already allocated one? Several Type-C controller drivers already explicitly allocate a DP HPD bridge for their device nodes during probe. If this notifier registers a second bridge for the exact same device node, won't this create duplicate bridges? This could break of_drm_find_bridge(), causing it to return the bridge created by this generic notifier while the controller driver continues sending events to its explicitly created bridge, leading to missed HPD events. > + > + return NOTIFY_OK; > +} > + > +static struct notifier_block drm_typec_event_nb = { > + .notifier_call = drm_typec_bus_event, > +}; > + > +static void drm_aux_hpd_typec_dp_bridge_module_exit(void) > +{ > + bus_unregister_notifier(&typec_bus, &drm_typec_event_nb); > +} [Severity: Critical] Does this module exit function leave dangling devres actions? Unregistering the notifier prevents new events, but without a cleanup routine here or a device removal handler, all previously accumulated devm actions from plug events will remain attached to the controller devices indefinitely. > + > +static int __init drm_aux_hpd_typec_dp_bridge_module_init(void) > +{ > + bus_register_notifier(&typec_bus, &drm_typec_event_nb); > + > + return 0; > +} [Severity: High] If this module is loaded after the Type-C subsystem has already probed, will it miss all existing devices? bus_register_notifier() only invokes the callback for events that occur after the notifier is registered. Do we need to manually iterate over existing devices on typec_bus to ensure bridges are created for pre-existing DP altmodes? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
