Re: [PATCH] usb: core: hub: Re-guess ep0 maxpacket size on reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: core: hub: missing usb_ep0_reinit() in hub_port_init() after ep0 guess
--

commit 8488ba17cb81d9272c99646f158ad82af5c82843
Author: Pengfei Lv <lvpengfei20@huawei.com>

usb: core: hub: Re-guess ep0 maxpacket size on reset

This commit removes the if (initial) guard around the ep0 maxpacket size
initialization in hub_port_init() so that udev->ep0.desc.wMaxPacketSize is
re-guessed from udev->speed during port resets as well as initial
enumeration.

> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 199d3f535a891..51f66b24b3538 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -4969,31 +4969,29 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1,
>  	}
>  	oldspeed = udev->speed;
>  
> -	if (initial) {
> -		/* USB 2.0 section 5.5.3 talks about ep0 maxpacket ...
> -		 * it's fixed size except for full speed devices.
> +	/* USB 2.0 section 5.5.3 talks about ep0 maxpacket ...
> +	 * it's fixed size except for full speed devices.
> +	 */
> +	switch (udev->speed) {
> +	case USB_SPEED_SUPER_PLUS:
> +	case USB_SPEED_SUPER:
> +		udev->ep0.desc.wMaxPacketSize = cpu_to_le16(512);
> +		break;
> +	case USB_SPEED_HIGH:		/* fixed at 64 */
> +		udev->ep0.desc.wMaxPacketSize = cpu_to_le16(64);
> +		break;
> +	case USB_SPEED_FULL:		/* 8, 16, 32, or 64 */
> +		/* to determine the ep0 maxpacket size, try to read
> +		 * the device descriptor to get bMaxPacketSize0 and
> +		 * then correct our initial guess.
>  		 */
> -		switch (udev->speed) {
> -		case USB_SPEED_SUPER_PLUS:
> -		case USB_SPEED_SUPER:
> -			udev->ep0.desc.wMaxPacketSize = cpu_to_le16(512);
> -			break;
> -		case USB_SPEED_HIGH:		/* fixed at 64 */
> -			udev->ep0.desc.wMaxPacketSize = cpu_to_le16(64);
> -			break;
> -		case USB_SPEED_FULL:		/* 8, 16, 32, or 64 */
> -			/* to determine the ep0 maxpacket size, try to read
> -			 * the device descriptor to get bMaxPacketSize0 and
> -			 * then correct our initial guess.
> -			 */
> -			udev->ep0.desc.wMaxPacketSize = cpu_to_le16(64);
> -			break;
> -		case USB_SPEED_LOW:		/* fixed at 8 */
> -			udev->ep0.desc.wMaxPacketSize = cpu_to_le16(8);
> -			break;
> -		default:
> -			goto fail;
> -		}
> +		udev->ep0.desc.wMaxPacketSize = cpu_to_le16(64);
> +		break;

[Severity: High]
Does hub_port_init() also need to call usb_ep0_reinit(udev) after resetting
udev->ep0.desc.wMaxPacketSize to 64 when !initial?

The commit message notes that re-establishing the 64-byte guess makes
xhci_check_maxpacket() issue an Evaluate Context command before the
GET_DESCRIPTOR data stage. However, commit e34900f46cd6 ("xhci: Reconfigure
endpoint 0 max packet size only during endpoint reset") removed that check
from xhci_urb_enqueue() and replaced it with xhci_check_ep0_maxpacket(),
which is only invoked from xhci_endpoint_reset() via usb_ep0_reinit():

drivers/usb/host/xhci.c:xhci_endpoint_reset() {
	if (usb_endpoint_xfer_control(&host_ep->desc) && ep_index == 0) {
		...
		xhci_check_ep0_maxpacket(xhci, vdev);

		/* Nothing else should be done here for ep0 during ep reset */
		return;
	}
}

During a port reset in usb_reset_and_verify_device(), usb_ep0_reinit(udev)
is called before hub_port_init():

drivers/usb/core/hub.c:usb_reset_and_verify_device() {
	for (i = 0; i < SET_CONFIG_TRIES; ++i) {

		/* ep0 maxpacket size may change; let the HCD know about it.
		 * Other endpoints will be handled by re-enumeration. */
		usb_ep0_reinit(udev);
		ret = hub_port_init(parent_hub, udev, port1, i, &descriptor);
}

On the first attempt (i == 0), udev->ep0.desc.wMaxPacketSize still holds the
pre-reset value (e.g. 8) when usb_ep0_reinit(udev) runs, matching the xHC
output EP0 context, so xhci_check_ep0_maxpacket() does nothing.

Then hub_port_init() resets the port (leaving the slot in SLOT_STATE_DEFAULT
with max_packet = 8) and sets udev->ep0.desc.wMaxPacketSize = cpu_to_le16(64)
in software without calling usb_ep0_reinit(udev). When do_new_scheme is true,
hub_enable_device() -> xhci_enable_device() -> xhci_setup_device() sees the
slot is already in SLOT_STATE_DEFAULT and returns early without updating the
EP0 context:

drivers/usb/host/xhci.c:xhci_setup_device() {
	if (setup == SETUP_CONTEXT_ONLY) {
		if (GET_SLOT_STATE(le32_to_cpu(slot_ctx->dev_state)) ==
		    SLOT_STATE_DEFAULT) {
			xhci_dbg(xhci, "Slot already in default state\n");
			goto out;
		}
	}
}

As a result, when get_bMaxPacketSize0() submits the 64-byte GET_DESCRIPTOR
URB on i == 0, the xHC hardware EP0 context still has max_packet = 8, so the
first hub_port_init() call still times out across all GET_DESCRIPTOR_TRIES *
GET_MAXPACKET0_TRIES attempts before usb_reset_and_verify_device() retries
with i == 1.

In addition, for standard full-speed devices with bMaxPacketSize0 = 8 whose
18-byte descriptor read succeeds on i == 0 in do_new_scheme,
hub_set_address() calls usb_ep0_reinit(udev) while
udev->ep0.desc.wMaxPacketSize is still 64, issuing an Evaluate Context
command to change max_packet from 8 to 64 right before hub_port_init() sets
wMaxPacketSize back to 8 and calls usb_ep0_reinit(udev) a second time.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104124402.1269650-1-lvpengfei20@huawei.com?part=1