Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] Attaching devm actions to an unowned device without strict dependencies causes a use-after-free panic on module unload. - [Medium] Missing NULL check for the OF device node causes broken bridge registration on non-Device Tree systems for multi-port controllers. - [Medium] Concurrent execution of bus iteration and notifier callback causes duplicate bridge registration. - [Low] The Kconfig dependency allows the module to be built when CONFIG_TYPEC is disabled, causing a link error. --
commit 9eab2892d2a7baba27c2d68547a8aad1b13a8541 Author: Chaoyi Chen <[email protected]> drm/bridge: Implement generic USB Type-C DP HPD bridge This commit implements a generic DisplayPort HPD bridge based on USB Type-C events. It registers a bus notifier on the typec_bus to automatically create HPD bridges for Type-C ports with the DP alternate mode. This removes the need for individual USB-C controller drivers to manually register the bridge. > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > index 4a57d49b4c6d3..9739b2a197586 100644 > --- a/drivers/gpu/drm/bridge/Kconfig > +++ b/drivers/gpu/drm/bridge/Kconfig > @@ -30,6 +30,16 @@ config DRM_AUX_HPD_BRIDGE [ ... ] > +if DRM_AUX_HPD_BRIDGE > +config DRM_AUX_HPD_TYPEC_BRIDGE > + tristate > + depends on TYPEC || !TYPEC [Severity: Low] Will this dependency allow the module to be built when CONFIG_TYPEC is disabled? If CONFIG_TYPEC=n, the condition "!TYPEC" evaluates to true. However, the source file unconditionally uses symbols from the Type-C subsystem, which would result in unresolved external symbols during linking. > + default TYPEC > + help > + Simple bridge that terminates the bridge chain and provides HPD > + support. It build bridge on each USB-C connector device node. > +endif [ ... ] > 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..682bc192e0934 > --- /dev/null > +++ b/drivers/gpu/drm/bridge/aux-hpd-typec-dp-bridge.c [ ... ] > +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); > + struct device_node *np; > + > + 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) { > + np = to_of_node(alt->dev.parent->fwnode); > + if (!drm_dev_has_dp_hpd_bridge(alt->dev.parent->parent, np)) [Severity: Medium] What happens on non-Device Tree systems (like ACPI) where to_of_node() returns NULL? It looks like np will be NULL, and if there are multiple ports on the same controller, they could all alias to the same NULL platform data. Does this mean only the first port gets a bridge and subsequent ports are skipped? > + drm_dp_hpd_bridge_register(alt->dev.parent->parent, np); [Severity: High] Does attaching a devm-managed resource to the unowned USB-C controller device (alt->dev.parent->parent) risk a use-after-free on module unload? Since the observer module doesn't clean these up on exit, the devm actions could outlive the module. This might crash the system when the controller is eventually unbound and devres executes the unmapped cleanup action. > + } > + > + return NOTIFY_OK; > +} [ ... ] > +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] Can a device be processed twice here? By registering the bus notifier before iterating over existing devices, is there a race condition where a newly added device triggers the notifier concurrently while bus_for_each_dev() is evaluating it? The check-and-register sequence in drm_typec_bus_event() doesn't appear to use locking, which might lead to duplicate bridge registrations. > + return 0; > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
