Re: [PATCH] USB: core: Add eUSB2 descriptor and parsing in USB core

[Mathias Nyman <mathias.nyman@linux.intel.com>]

On 20.2.2025 18.35, Greg KH wrote:
> On Thu, Feb 20, 2025 at 04:13:39PM +0200, Mathias Nyman wrote:
>> From: Kannappan R <r.kannappan@intel.com>
>> --->> @@ -64,9 +65,10 @@ struct ep_device;
>>    * descriptor within an active interface in a given USB configuration.
>>    */
>>   struct usb_host_endpoint {
>> -	struct usb_endpoint_descriptor		desc;
>> -	struct usb_ss_ep_comp_descriptor	ss_ep_comp;
>> -	struct usb_ssp_isoc_ep_comp_descriptor	ssp_isoc_ep_comp;
>> +	struct usb_endpoint_descriptor			desc;
>> +	struct usb_ss_ep_comp_descriptor		ss_ep_comp;
>> +	struct usb_ssp_isoc_ep_comp_descriptor		ssp_isoc_ep_comp;
>> +	struct usb_eusb2_isoc_ep_comp_descriptor	eusb2_isoc_ep_comp;
> 
> No real need to indent any of these, but oh well :)

It looked odd when adding one new variable off by a space compared to all the
other neatly tab-aligned variables. So I shifted them all right.

>> +/* USB_DT_EUSB2_ISOC_ENDPOINT_COMP: eUSB2 Isoch Endpoint Companion descriptor */
>> +struct usb_eusb2_isoc_ep_comp_descriptor {
>> +	__u8	bLength;
>> +	__u8	bDescriptorType;
>> +	__le16	wMaxPacketSize;
>> +	__le32	dwBytesPerInterval;
>> +} __attribute__ ((packed));
>> +
>> +#define USB_DT_EUSB2_ISOC_EP_COMP_SIZE	8
> 
> Can't we use a sizeof() for this as well?  I guess we don't do it for
> other structures, so maybe not.
> 
> Anyway, this looks fine, if you want to just send an update for the
> 0x0220 later on if you think it's needed, please do.

Thanks for looking at this.

We did consider defining 0x0220, but checked that usb core uses magic numbers
for bcdUSB in other places:

hcd.c:  if (le16_to_cpu(usb_dev->descriptor.bcdUSB) >= 0x0201) {
hub.c:                  (le16_to_cpu(udev->descriptor.bcdUSB) < 0x0300)) {
hub.c:  if (le16_to_cpu(udev->descriptor.bcdUSB) >= 0x0201) {
hub.c:          if (le16_to_cpu(udev->descriptor.bcdUSB) >= 0x0200
hub.h:          le16_to_cpu(hdev->descriptor.bcdUSB) >= 0x0310 &&

Makes sense to add a separate patch later on that define all these.

Thanks
Mathias