Re: [PATCH v4 2/2] usb: hub: Retry SuperSpeed for a device stuck on its USB 2.0 peer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] usb: hub: unnotified and unlocked port reset in
  hub_reset_peer_child()
- [Medium] usb: hub: wrong peer disconnect on status error in
  hub_reset_peer_child()
- [Medium] usb: hub: concurrent SuperSpeed enumeration before USB 2.0
  peer disconnect
--

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

usb: hub: Retry SuperSpeed for a device stuck on its USB 2.0 peer

When a SuperSpeed root hub starts, this commit queues work to reset any
SuperSpeed-capable non-hub device attached at high speed on a USB 2.0 peer
port from a different controller, disconnecting the USB 2.0 port if the
device appears on the SuperSpeed port or restoring it with
usb_reset_device() otherwise.

> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 199d3f535a891..b56ee90e55f0f 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c

[ ... ]

> @@ -3278,6 +3289,127 @@ static void usb_unlock_port(struct usb_port *port_dev)
>  	__release(&port_dev->status_lock);
>  }
>  
> +static void hub_reset_peer_child(struct usb_hub *hub, int port1,
> +				 struct usb_device *peer_hdev, int peer_port1)
> +{
> +	struct usb_hub *peer_hub;
> +	struct usb_device *udev;
> +
> +	usb_lock_device(peer_hdev);
> +	if (hub->quiescing)
> +		goto out;
> +	peer_hub = usb_hub_to_struct_hub(peer_hdev);
> +	if (!peer_hub || peer_port1 > peer_hdev->maxchild)
> +		goto out;
> +
> +	udev = peer_hub->ports[peer_port1 - 1]->child;
> +	if (!udev || udev->speed != USB_SPEED_HIGH)
> +		goto out;
> +
> +	if (udev->descriptor.bDeviceClass == USB_CLASS_HUB)
> +		goto out;
> +
> +	usb_lock_device(udev);
> +	if (udev->state != USB_STATE_NOTATTACHED &&
> +	    udev->bos && udev->bos->ss_cap &&
> +	    usb_autoresume_device(udev) == 0) {
> +		struct usb_port *port_dev = peer_hub->ports[peer_port1 - 1];
> +		u16 portstatus, portchange;
> +		int i, ret;
> +
> +		dev_info(&udev->dev,
> +			 "SuperSpeed peer port available, resetting\n");
> +		/* A second reset hands the EHCI port to its companion */
> +		usb_lock_port(port_dev);
> +		ret = hub_port_reset(peer_hub, peer_port1, NULL,
> +				     HUB_ROOT_RESET_TIME, false);

[Severity: High]
When a SuperSpeed-capable device stays on the USB 2.0 port (for example,
when attached through a USB 2.0 cable or when SuperSpeed link training
fails), does calling hub_port_reset() directly here leave the physical
device enabled at USB address 0 for a full second without holding
address0_mutex or notifying bound interface drivers?

Unlike usb_reset_device() -> usb_reset_and_verify_device(), this reset does
not invoke driver pre_reset() callbacks, disable USB 2.0 hardware LPM, or
acquire bus_to_hcd(peer_hdev->bus)->address0_mutex, and passing NULL for
udev leaves udev->state as USB_STATE_CONFIGURED. While the loop below
sleeps for 1000 ms polling the SuperSpeed port:

- peer_port1 is enabled with the device responding to USB address 0, which
  can collide with concurrent enumeration on an external hub on the same
  USB 2.0 bus or a reset on another port of peer_hdev.
- Interface drivers on udev that were just woken by
  usb_autoresume_device() can submit URBs to udev->devnum while the
  hardware is unconfigured at address 0, failing with bus errors or timing
  out in error recovery (such as usb_lock_device_for_reset() while
  usb_lock_device(udev) is held for over a second).
- When usb_reset_device(udev) finally runs after the 1 s poll, any
  pre_reset() callback that communicates with the device will talk to
  hardware that has already been reset.

> +		for (i = 0; !ret && i < 20; i++) {
> +			msleep(50);
> +			ret = hub->quiescing ? -ENODEV :
> +				usb_hub_port_status(hub, port1, &portstatus,
> +						    &portchange);
> +			if (!ret && (portstatus & USB_PORT_STAT_CONNECTION))
> +				ret = -ENOTCONN;
> +		}
> +		if (ret)
> +			hub_port_logical_disconnect(peer_hub, peer_port1);

[Severity: Medium]
Can an error while polling the SuperSpeed port cause
hub_reset_peer_child() to disconnect the high-speed device on peer_hub?

If hub->quiescing becomes set (for instance during system suspend or HCD
unbind) or usb_hub_port_status(hub, port1, ...) returns an error while the
device is still attached at high speed on peer_port1, ret is overwritten
with a negative errno. The if (ret) check then calls
hub_port_logical_disconnect(peer_hub, peer_port1) and skips
usb_reset_device(udev), disconnecting the working USB 2.0 device instead
of restoring it. The same happens if the SuperSpeed port already had a
connection before peer_port1 was reset.

> +		/* Else hub_wq resets the port again */
> +		for (i = 0; ret == -ENOTCONN && i < 20; i++) {
> +			if (usb_hub_port_status(peer_hub, peer_port1, &portstatus,
> +						&portchange) ||
> +			    !(portstatus & USB_PORT_STAT_CONNECTION))
> +				break;
> +			msleep(50);
> +		}
> +		usb_unlock_port(port_dev);
> +
> +		if (!ret)
> +			usb_reset_device(udev);
> +		usb_autosuspend_device(udev);
> +	}
> +	usb_unlock_device(udev);
> +out:
> +	usb_unlock_device(peer_hdev);
> +}

[Severity: Medium]
When the device does move to the SuperSpeed port, can it be enumerated and
bound on hub before the old high-speed udev is disconnected from peer_hub?

hub_reset_peer_child() holds usb_lock_device(peer_hdev) across the reset
and polling loops without locking the SuperSpeed port
hub->ports[port1 - 1], and hub_wq allows concurrent work items across ports
and hubs:

kworker/0 (peer_reset_work)       kworker/1 (hub->events)
hub_reset_peer_child()
  usb_lock_device(peer_hdev)
  hub_port_reset(peer_hub, ...)
    /* device moves to SS port */
    hub_port_wait_reset()
      /* sleeps up to 920 ms */
                                  hub_event() /* on SS hub */
                                    port_event(hub, port1)
                                      hub_port_connect_change()
                                        hub_port_connect()
                                          usb_new_device(ss_udev)
                                          /* binds SS drivers while old
                                           * HS udev is still bound */
  hub_port_logical_disconnect()
    kick_hub_wq(peer_hub)
  usb_unlock_device(peer_hdev)
                                  hub_event() /* on peer_hub */
                                    usb_lock_device(peer_hdev)
                                    usb_disconnect(&port_dev->child)

Because hub_port_wait_reset() only exits early when
USB_PORT_STAT_CONNECTION remains set, a device that drops its USB 2.0
connection during the reset causes hub_port_wait_reset() to poll until
HUB_RESET_TIMEOUT (920 ms) while holding usb_lock_device(peer_hdev). Even
after hub_port_logical_disconnect() kicks hub_wq for peer_hub, peer_hub's
hub_event() cannot acquire usb_lock_device(peer_hdev) to run
usb_disconnect() until hub_reset_peer_child() finishes, so both the old
high-speed usb_device and the new SuperSpeed usb_device for the same
hardware can be bound to drivers at the same time. Locking
hub->ports[port1 - 1] with usb_lock_port() during hub_reset_peer_child()
would prevent port_event() on the SuperSpeed hub from enumerating the new
device until the peer reset work completes.

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