Re: [PATCH 3/4] 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 4 potential
issues to consider:
- [High] usb: hub: hub_reset_peer_child() resets external USB 3.0 hub
  USB 2.0 half
- [High] usb: hub: hub_reset_peer_child() resets suspended peer on
  HUB_RESET_RESUME
- [Medium] usb: hub: peer_reset_work not canceled in hub_quiesce() or
  hub_disconnect()
- [Medium] usb: hub: missing runtime PM reference in
  hub_queue_peer_reset()
--

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

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

This commit queues a work item when a SuperSpeed hub activates to check
connector-paired USB 2.0 peer ports and reset any SuperSpeed-capable
device connected at high speed so it can re-enumerate at SuperSpeed.

> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 199d3f535a891..df88de5bbf54d 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -1076,6 +1076,89 @@ int usb_remove_device(struct usb_device *udev)
>  	return 0;
>  }
>  
> +/*
> + * A SuperSpeed port paired through a connector with a port of another host
> + * controller can start after a SuperSpeed-capable device has connected at
> + * high speed on that peer: with VBUS always on, the device looks for a
> + * SuperSpeed link while the SuperSpeed host is still down, gives up, and
> + * connects at high speed. It only tries SuperSpeed again after VBUS is cycled
> + * or after a USB 2.0 bus reset, so reset it now that the link can train.
> + */
> +static void hub_reset_peer_child(struct usb_device *peer_hdev, int peer_port1)
> +{
> +	struct usb_hub *peer_hub;
> +	struct usb_device *udev;
> +
> +	/* Wait for an enumeration in progress on the peer port to finish */
> +	usb_lock_device(peer_hdev);
> +	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;
> +
> +	usb_lock_device(udev);
> +	if (udev->state != USB_STATE_NOTATTACHED &&
> +	    udev->bos && udev->bos->ss_cap) {
> +		dev_info(&udev->dev,
> +			 "SuperSpeed peer port available, resetting\n");
> +		usb_reset_device(udev);
> +	}
> +	usb_unlock_device(udev);
> +out:
> +	usb_unlock_device(peer_hdev);
> +}

[Severity: High]
Does hub_reset_peer_child() reset the USB 2.0 half of an external USB 3.x
hub and disconnect all of its downstream USB 1.1/2.0 devices?

An external USB 3.x hub contains both a SuperSpeed hub and a separate
high-speed USB 2.0 hub wired in parallel.  The USB 2.0 hub enumerates on
peer_hdev at USB_SPEED_HIGH and advertises udev->bos->ss_cap in its BOS
descriptor, but it stays on the USB 2.0 bus permanently alongside the
SuperSpeed hub rather than migrating on reset.  Because neither
udev->descriptor.bDeviceClass == USB_CLASS_HUB nor port_dev->child is
checked, calling usb_reset_device(udev) runs hub_pre_reset() ->
hub_quiesce(hub, HUB_PRE_RESET), which calls usb_disconnect() on every
child attached to the external USB 2.0 hub.

> +
> +static void hub_peer_reset_work(struct work_struct *ws)
> +{
> +	struct usb_hub *hub = container_of(ws, struct usb_hub, peer_reset_work);
> +	struct usb_device *hdev = hub->hdev;
> +	int port1;
> +
> +	for (port1 = 1; ; port1++) {
> +		struct usb_device *peer_hdev = NULL;
> +		struct usb_port *port_dev;
> +		int peer_port1 = 0;
> +
> +		mutex_lock(&usb_port_peer_mutex);
> +		if (port1 > hdev->maxchild) {
> +			mutex_unlock(&usb_port_peer_mutex);
> +			break;
> +		}
> +		port_dev = hub->ports[port1 - 1];
> +		if (port_dev->connector_peer && port_dev->peer) {
> +			peer_hdev = to_usb_device(port_dev->peer->dev.parent->parent);
> +			peer_port1 = port_dev->peer->portnum;
> +			usb_get_dev(peer_hdev);
> +		}
> +		mutex_unlock(&usb_port_peer_mutex);
> +
> +		if (peer_hdev) {
> +			hub_reset_peer_child(peer_hdev, peer_port1);
> +			usb_put_dev(peer_hdev);
> +		}
> +	}
> +
> +	hub_put(hub);
> +}

[Severity: Medium]
Should hub_quiesce() cancel or flush hub->peer_reset_work, and should
hub_peer_reset_work() stop when hub->quiescing or hub->disconnected is
set?

Unlike hub->post_resume_work, hub->leds and hub->tt.clear_work,
hub->peer_reset_work is never canceled or flushed in hub_quiesce() or
hub_disconnect().  If the SuperSpeed hub is suspended, reset or
disconnected while hub->peer_reset_work is pending or running, the work
continues and resets the USB 2.0 peer device while the SuperSpeed hub is
no longer active to accept the connection, and usb_hub_cleanup() can
return from usb_deregister(&hub_driver) while hub->peer_reset_work is
still running on system_power_efficient_wq.

> +
> +static void hub_queue_peer_reset(struct usb_hub *hub)
> +{
> +	int port1;
> +
> +	for (port1 = 1; port1 <= hub->hdev->maxchild; port1++) {
> +		if (hub->ports[port1 - 1]->connector_peer) {
> +			hub_get(hub);
> +			if (!queue_work(system_power_efficient_wq,
> +					&hub->peer_reset_work))
> +				hub_put(hub);
> +			return;
> +		}
> +	}
> +}

[Severity: Medium]
Can the SuperSpeed hub autosuspend before hub_peer_reset_work() finishes
resetting the peer device?

hub_probe() sets a 0 ms autosuspend delay and enables
pm_suspend_ignore_children() on the hub interface.  When the device is
stuck on the USB 2.0 peer, the SuperSpeed hub has no attached children,
and hub_activate() drops its HUB_INIT2/HUB_INIT3 runtime PM reference
immediately after calling hub_queue_peer_reset().  Because
hub_queue_peer_reset() does not take an interface runtime PM reference
(unlike kick_hub_wq() and the HUB_RESUME path), the SuperSpeed hub and
bus can runtime-suspend while hub_peer_reset_work() is waiting on
usb_lock_device(peer_hdev) or resetting udev, preventing the SuperSpeed
link from training.

[ ... ]

> @@ -1346,6 +1429,10 @@ static void hub_activate(struct usb_hub *hub, enum hub_activation_type type)
>  
>  	/* Scan all ports that need attention */
>  	kick_hub_wq(hub);
> +
> +	/* Let devices stuck at high speed on a connector peer move over */
> +	if (hub_is_superspeed(hdev) && type != HUB_RESUME)
> +		hub_queue_peer_reset(hub);
>   abort:

[Severity: High]
During system resume (type == HUB_RESET_RESUME), can hub_peer_reset_work()
run before the peer child device has resumed or before hub_wq has thawed?

system_power_efficient_wq is not freezable, so when hub_reset_resume() ->
hub_activate() queues hub->peer_reset_work during dpm_resume(), the work
can run while udev on the USB 2.0 peer port still has udev->state ==
USB_STATE_SUSPENDED and udev->can_submit == 0.  hub_reset_peer_child()
only checks udev->state != USB_STATE_NOTATTACHED and calls
usb_reset_device(udev), which runs drv->pre_reset() or
usb_forced_unbind_intf() on the suspended interfaces before
usb_reset_and_verify_device() fails with -EINVAL without resetting the
port or rebinding the drivers:

drivers/usb/core/hub.c:usb_reset_and_verify_device() {
	if (udev->state == USB_STATE_NOTATTACHED ||
			udev->state == USB_STATE_SUSPENDED) {
		dev_dbg(&udev->dev, "device reset not allowed in state %d\n",
				udev->state);
		return -EINVAL;
	}

Conversely, if the device fell back to the USB 2.0 peer during system
suspend/resume, hub_wq (which is WQ_FREEZABLE) has not thawed yet when
hub_peer_reset_work() runs, so peer_hub->ports[peer_port1 - 1]->child is
still NULL and the reset is missed.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-usb-port-pairing-v1-0-9307e29d2ed2@gmail.com?part=3