Re: [PATCH v4 1/2] usb: core: Pair ports of different hubs through their connector

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] usb: core: OF graph port numbering mismatch in
  usb_port_graph_id()
- [Medium] usb: core: raw port number not passed to
  usb_of_get_connect_type()
- [Medium] usb: core: update connect_type and connector_peer after
  device_register()
--

commit a8c7879971bee16d5ebf91273578d78c7dde5445
Author: Arun Iyengar <aiyengar1990@gmail.com>

usb: core: Pair ports of different hubs through their connector

This commit pairs USB 2.0 and SuperSpeed ports across different hubs or
host controllers when they are linked to the same usb-connector node in
the OF graph.

> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
> index b4452b665f591..5b2041d891eca 100644
> --- a/drivers/usb/core/port.c
> +++ b/drivers/usb/core/port.c
> @@ -649,10 +649,49 @@ static int match_location(struct usb_device *peer_hdev, void *p)
>  	return 0;
>  }
>  
> +static u32 usb_port_graph_id(struct usb_device *hdev, int port1)
> +{
> +	if (hdev->parent)
> +		return port1;
> +	return usb_hcd_find_raw_port_number(bus_to_hcd(hdev->bus), port1);
> +}
> +
> +struct usb_connector_peer {
> +	struct usb_port *port_dev;
> +	struct device_node *np;
> +	u32 port;
> +};
> +
> +static int match_connector_peer(struct usb_device *peer_hdev, void *p)
> +{
> +	struct usb_connector_peer *cp = p;
> +	struct usb_hub *peer_hub = usb_hub_to_struct_hub(peer_hdev);
> +	struct usb_port *peer;
> +	int port1;
> +
> +	if (!peer_hub || peer_hdev->dev.of_node != cp->np ||
> +	    hub_is_superspeed(peer_hdev) == cp->port_dev->is_superspeed)
> +		return 0;
> +
> +	for (port1 = 1; port1 <= peer_hdev->maxchild; port1++) {
> +		if (usb_port_graph_id(peer_hdev, port1) == cp->port)
> +			break;
> +	}

[Severity: High]
Does usb_port_graph_id() match the OF graph port numbering used by USB
controller bindings, and does usb_of_get_connector_peer() leave USB 2.0
ports unpeered when the peer controller uses dwc3 graph numbering?

On dwc3 xHCI controllers, xhci_find_raw_port_number() returns 1 for the
High-Speed root hub port and 2 for the SuperSpeed root hub port, whereas
USB controller DT bindings (such as snps,dwc3-common.yaml and
rockchip,dwc3.yaml) define port@0 as High-Speed and port@1 as SuperSpeed.

When find_and_link_peer() runs for a USB 2.0 hub port whose connector
port@1 points to a dwc3 controller's port@1 (as on
rk3576-lckfb-tspi-3m.dts), usb_of_get_connector_peer() checks !*peer_port,
which rejects port@0 when starting from a SuperSpeed port but accepts
*peer_port == 1 when starting from a USB 2.0 port.  find_and_link_peer()
then sets port_dev->connector_peer = 1 and returns early, while
match_connector_peer() compares usb_port_graph_id() (2) against cp->port
(1) and fails to link the ports.  Meanwhile, when the dwc3 SuperSpeed root
hub port runs find_and_link_peer(), usb_port_graph_id() returns 2, which
misses port@1 on the controller node and falls through to default peering
with the wrong port (or is blocked by !peer->connector_peer when the
default peer was already marked).

> @@ -671,7 +711,21 @@ static void find_and_link_peer(struct usb_hub *hub, int port1)
>  		/* we link the peer in match_location() if found */
>  		usb_for_each_dev(port_dev, match_location);
>  		return;
> -	} else if (!hdev->parent) {
> +	}
> +
> +	cp.np = usb_of_get_connector_peer(hdev, usb_port_graph_id(hdev, port1),
> +					  port_dev->is_superspeed, &cp.port);
> +	if (cp.np) {
> +		cp.port_dev = port_dev;
> +		port_dev->connector_peer = 1;

[Severity: Medium]
Does setting port_dev->connector_peer and port_dev->connect_type in
find_and_link_peer() happen too late, after device_register() has already
exposed the port to sysfs?

In usb_hub_create_port_device(), device_register(&port_dev->dev) publishes
the sysfs attributes and emits KOBJ_ADD before find_and_link_peer() runs.
Userspace handlers reacting to KOBJ_ADD can read the stale connect_type,
and the unlocked bitfield write to connector_peer can race with sysfs
writes to sibling bitfields in struct usb_port (early_stop_store() and
usb3_lpm_permit_store()):

CPU0                                  CPU1
usb_hub_create_port_device()
  device_register(&port_dev->dev)
                                      early_stop_store()
                                        port_dev->early_stop = ...
  find_and_link_peer()
    port_dev->connector_peer = 1;
    <-- bitfield RMW clobbers early_stop

> +		/* usb_of_get_connect_type() misses graph ports */
> +		port_dev->connect_type = USB_PORT_CONNECT_TYPE_HOT_PLUG;

[Severity: Medium]
Why does find_and_link_peer() override port_dev->connect_type here instead
of usb_hub_create_port_device() passing usb_port_graph_id(hdev, port1) to
usb_of_get_connect_type()?

usb_of_get_connect_type() already checks OF graph endpoints via
of_graph_get_endpoint_by_regs(), but usb_hub_create_port_device() passes
the HCD-relative port1 index instead of the controller's raw port number:

drivers/usb/core/port.c:usb_hub_create_port_device() {
    ...
	port_dev->connect_type = usb_of_get_connect_type(hdev, port1);
    ...
}

On a split-roothub controller where usb_port_graph_id(hdev, port1) !=
port1, usb_of_get_connect_type() inspects the wrong port number on the
controller DT node.  Whenever usb_of_get_connector_peer() returns NULL
(for example, when both ports of the connector belong to the same
controller node, or for a hardwired child device node), connect_type is
never fixed up and remains wrong (such as USB_PORT_NOT_USED), causing
set_usb_port_removable() to misclassify hotplugged devices as fixed.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261115-usb-port-pairing-v4-0-25d3c9adcc21@gmail.com?part=1