Re: [PATCH v7 2/7] drm/bridge: Implement generic USB Type-C DP HPD bridge

[sashiko-bot@kernel.org]

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 <chaoyi.chen@rock-chips.com>

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/20260828084737.565-1-kernel@airkyi.com?part=2