Re: [PATCH 1/5] drm/bridge: Implement generic USB Type-C DP HPD bridge

[sashiko-bot@kernel.org]

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 <chaoyi.chen@rock-chips.com>
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/20260608070805.88-1-kernel@airkyi.com?part=1

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy