Re: [PATCH v4 1/3] usb: xhci: Add support for eUSB2v2 1024-byte bulk packet size

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] usb: xhci: eUSB2v2 1KB mode erroneously enabled for Full-Speed devices
- [High] usb: class drivers under-allocate RX buffers due to unmodified wMaxPacketSize
- [Medium] usb: xhci: incorrect URB_ZERO_PACKET evaluation for 1KB bulk OUT endpoints
--

commit 54ae7d8065840029212e7f3efbe68ad6defca543
Author: Pawel Laszczak <pawell@cadence.com>
Subject: usb: xhci: Add support for eUSB2v2 1024-byte bulk packet size

This commit introduces support for 1024-byte maximum packet sizes for Bulk
endpoints in High-Speed mode on eUSB2v2 capable hosts. It introduces an
is_eusb2v2 flag and issues a SET_FEATURE request to switch compatible
peripherals to 1024-byte packet mode.

> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> index 0cd6dd2b6334b..311b8fd3bf44f 100644
> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c
[ ... ]
> @@ -2013,6 +2013,69 @@ int usb_set_wireless_status(struct usb_interface *iface,
>  }
>  EXPORT_SYMBOL_GPL(usb_set_wireless_status);
>  
> +/*
> + * eusb_update_max_packet - enable 1024-byte bulk mode for eUSB2v2 device
> + * @udev: target device
> + * @cp: configuration to be checked and enabled
> + *
> + * Per eUSB2v2 spec section 5.2, an eUSB2v2 peripheral will revert the
> + * maximum packet size to 512 for bulk endpoints after bus reset, disconnect,
> + * or deconfiguration.
> + * This function sends the BULK_MAX_PACKET_UPDATE request to restore the
> + * 1024-byte mode. It is valid only if the configuration has bulk endpoints.
> + */
> +int eusb_update_max_packet(struct usb_device *udev, struct usb_host_config *cp)
> +{
> +	struct usb_host_config *config = cp ? cp : udev->actconfig;
> +	struct usb_hcd *hcd = bus_to_hcd(udev->bus);
> +	struct usb_interface_cache *intfc;
> +	struct usb_host_interface *alt;
> +	struct usb_host_endpoint *ep;
> +	int i, j, a;
> +	int ret;
> +
> +	udev->eusb2v2_mps_active = 0;
> +
> +	if (le16_to_cpu(udev->descriptor.bcdUSB) != 0x0230 ||
> +	    !hcd->self.is_eusb2v2)
> +		return -EOPNOTSUPP;

[Severity: High]
Does this need to check udev->speed == USB_SPEED_HIGH? If an eUSB2v2-compliant
device is connected but enumerates at Full-Speed, eusb_update_max_packet()
could send the feature request and set udev->eusb2v2_mps_active to 1.

If this happens, xhci_usb_endpoint_maxp() will unconditionally return 1024 for
the Full-Speed bulk endpoint, leading to a conflict in xhci_endpoint_init()
where the hardware endpoint context is clamped to 64 bytes:

drivers/usb/host/xhci-mem.c:xhci_endpoint_init() {
		if (udev->speed == USB_SPEED_FULL) {
			max_packet = rounddown_pow_of_two(max_packet);
			max_packet = clamp_val(max_packet, 8, 64);
		}
}

Can this discrepancy between the 64-byte hardware context and the 1024-byte
TRBs generated by software lead to malformed TRBs and transfer failures?

> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index a9e47e178c280..8e630d323c6fb 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
[ ... ]
> @@ -2949,6 +2949,11 @@ int xhci_usb_endpoint_maxp(struct usb_device *udev,
>  {
>  	if (usb_endpoint_is_hs_isoc_double(udev, host_ep))
>  		return le16_to_cpu(host_ep->eusb2_isoc_ep_comp.wMaxPacketSize);
> +
> +	if (udev->eusb2v2_mps_active &&
> +	    usb_endpoint_xfer_bulk(&host_ep->desc))
> +		return 1024;
> +
>  	return usb_endpoint_maxp(&host_ep->desc);
>  }

[Severity: High]
By dynamically overriding the packet size to 1024 bytes here for xHCI, while
leaving ep->desc.wMaxPacketSize as 512 bytes, could this cause higher-level
USB class drivers to under-allocate their transfer buffers?

Class drivers (like CDC NCM) rely entirely on usb_endpoint_maxp(&ep->desc) to
determine the size of incoming packets and pre-allocate their buffers. If they
allocate 512 bytes based on the unmodified descriptor, an incoming 1024-byte
packet could overflow the buffer and abort the transfer.

[Severity: Medium]
Additionally, does leaving the descriptor at 512 bytes cause an incorrect
URB_ZERO_PACKET evaluation for Bulk OUT transfers?

In xhci_urb_enqueue(), the need for a Zero-Length Packet is evaluated as:

    !(urb->transfer_buffer_length % usb_endpoint_maxp(&urb->ep->desc))

Since usb_endpoint_maxp() reads the unadjusted descriptor value of 512, an OUT
transfer length that is a multiple of 512 but not 1024 (e.g., 1536 bytes)
might incorrectly satisfy this condition and append an unnecessary Zero-Length
Packet, violating the bulk protocol.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-eusb2v2-packet-size-v4-0-a505ee17b460@cadence.com?part=1