Re: [PATCH v2] 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:
- [Medium] usb: core: hub: usb_ep0_reinit() on unaddressed xHCI slot in hub_port_init()
--

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

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

This commit removes the initial guard around the ep0 maxpacket size guess in
hub_port_init() and calls usb_ep0_reinit() on reset so that full-speed devices
with bMaxPacketSize0 < 64 have their ep0 maxpacket reset to 64 in both USB
core and the xHCI ep0 context before re-enumeration issues a 64-byte
GET_DESCRIPTOR request.

> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 199d3f535a891..a6c260d390945 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -4969,33 +4969,34 @@ 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;
> +	case USB_SPEED_LOW:		/* fixed at 8 */
> +		udev->ep0.desc.wMaxPacketSize = cpu_to_le16(8);
> +		break;
> +	default:
> +		goto fail;
>  	}
>  
> +	if (!initial)
> +		usb_ep0_reinit(udev);

[Severity: Medium]
Can calling usb_ep0_reinit(udev) here before hub_enable_device() or
hub_set_address() trigger an invalid Evaluate Context command when the xHCI
slot has been newly allocated during reset?

Earlier in drivers/usb/core/hub.c:hub_port_init(), hub_port_reset() invokes
hcd->driver->reset_device(), which resolves to
drivers/usb/host/xhci.c:xhci_discover_or_reset_device(). When the xHCI slot
was freed and reallocated via xhci_alloc_dev() (for example on Etron hosts
with XHCI_ETRON_HOST, or after host controller power loss / reset on resume),
the newly allocated virt_dev has a zeroed out_ctx in SLOT_STATE_DISABLED
because Address Device (xhci_setup_device()) has not run yet:

drivers/usb/host/xhci.c:xhci_discover_or_reset_device() {
	...
	slot_ctx = xhci_get_slot_ctx(xhci, virt_dev->out_ctx);
	if (GET_SLOT_STATE(le32_to_cpu(slot_ctx->dev_state)) ==
						SLOT_STATE_DISABLED)
		return 0;

	if (xhci->quirks & XHCI_ETRON_HOST) {
		ret = xhci_disable_and_free_slot(xhci, udev->slot_id);
		if (!ret) {
			ret = xhci_alloc_dev(hcd, udev);
	...
}

When usb_ep0_reinit(udev) is then called for a full-speed device via
usb_enable_endpoint() -> usb_hcd_reset_endpoint() -> xhci_endpoint_reset() ->
xhci_check_ep0_maxpacket(), neither xhci_endpoint_reset() nor
xhci_check_ep0_maxpacket() checks whether vdev->out_ctx is still in
SLOT_STATE_DISABLED:

drivers/usb/host/xhci.c:xhci_check_ep0_maxpacket() {
	...
	ep_ctx = xhci_get_ep_ctx(xhci, vdev->out_ctx, 0);
	hw_max_packet_size = MAX_PACKET_DECODED(le32_to_cpu(ep_ctx->ep_info2));
	max_packet_size = usb_endpoint_maxp(&vdev->udev->ep0.desc);

	if (hw_max_packet_size == max_packet_size)
		return 0;
	...
}

Because vdev->out_ctx is still zeroed, hw_max_packet_size is 0 != 64, so
xhci_check_ep0_maxpacket() copies the zeroed out_ctx ep0 into in_ctx ep0 and
issues an Evaluate Context command on an unaddressed slot in the Enabled
state, which the xHC rejects with COMP_CONTEXT_STATE_ERROR (logging a warning
in xhci_evaluate_context_result()).

Furthermore, if a previous attempt in xhci_setup_device() failed with
COMP_USB_TRANSACTION_ERROR, it reallocates the slot via xhci_alloc_dev() and
calls xhci_setup_addressable_virt_dev(xhci, udev), which populates
vdev->in_ctx (including setting slot_ctx->dev_info and initializing in_ctx
ep0) while vdev->out_ctx remains zeroed. If hub_port_init() retries and calls
usb_ep0_reinit(udev) before the next xhci_setup_device(),
xhci_check_ep0_maxpacket() overwrites vdev->in_ctx ep0 from the zeroed
vdev->out_ctx while leaving slot_ctx->dev_info non-zero:

drivers/usb/host/xhci.c:xhci_setup_device() {
	...
	if (!slot_ctx->dev_info)
		xhci_setup_addressable_virt_dev(xhci, udev);
	/* Otherwise, update the control endpoint ring enqueue pointer. */
	else
		xhci_copy_ep0_dequeue_into_input_ctx(xhci, udev);
	...
}

Wouldn't xhci_setup_device() then skip xhci_setup_addressable_virt_dev() and
submit Address Device with a zeroed ep0 context (EP_TYPE == 0)?

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