Re: [PATCH v3 05/13] usb: hub: Associate port@ fwnode with USB port device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The firmware node reference obtained from `fwnode_graph_get_port_by_id()` is leaked because it is never explicitly released.
--

commit 25f9337168757350400d5fa5ab05e87193277473
Author: Chen-Yu Tsai <wenst@chromium.org>

usb: hub: Associate port@ fwnode with USB port device

This commit assigns a firmware node to a USB port device if the hub's
firmware node is not an ACPI node. This association allows later M.2 slot
power sequencing providers to match against the requesting port.

> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
> index b1364f0c384ce..1088776ef750b 100644
> --- a/drivers/usb/core/port.c
> +++ b/drivers/usb/core/port.c

[ ... ]

> @@ -780,6 +781,13 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1)
>  	port_dev->dev.driver = &usb_port_driver;
>  	dev_set_name(&port_dev->dev, "%s-port%d", dev_name(&hub->hdev->dev),
>  			port1);
> +	/*
> +	 * ACPI FW nodes are associated later when device_register() happens.
> +	 * Skip assigning one here to avoid potential conflicts.
> +	 */
> +	if (!is_acpi_node(dev_fwnode(&hdev->dev)))
> +		device_set_node(&port_dev->dev,
> +				fwnode_graph_get_port_by_id(dev_fwnode(&hdev->dev), port1));

[Severity: Medium]
Does this code leak the fwnode_handle reference?

fwnode_graph_get_port_by_id() increments the reference count of the returned
firmware node, and the caller is expected to release it.

Since device_set_node() merely assigns dev->fwnode without taking ownership
of the reference, the driver core doesn't automatically drop it when the
device is destroyed.

Looking at usb_port_device_release() in drivers/usb/core/port.c, it
only frees the port structures:

static void usb_port_device_release(struct device *dev)
{
	struct usb_port *port_dev = to_usb_port(dev);

	kfree(port_dev->req);
	kfree(port_dev);
}

Without a corresponding fwnode_handle_put(), will this result in a reference
leak whenever a USB hub is disconnected, or if device_register() fails on the
error path?

>  	mutex_init(&port_dev->status_lock);
>  	retval = device_register(&port_dev->dev);
>  	if (retval) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703110317.1283411-1-wenst@chromium.org?part=5