Re:Re: Re: Re: [PATCH] USB: add usbfs ioctl to get specific superspeedplus rates

[Dingyan Li]

At 2023-08-04 22:55:49, "Alan Stern" <stern@xxxxxxxxxxxxxxxxxxx> wrote:

>> Another issue is that USB_SPEED_SUPER_PLUS has been widely used in
>> so many files to execute conditional banches. Once we extend and store device's
>> speed with new values in the first place, we might need to check all places where
>> USB_SPEED_SUPER_PLUS is used in case of any regression.
>
>Certainly.  That's part of auditing all the current users of 
>usb_device_speed.

This might not be a good idea and feels kind of drift away from what we originally
want to do. Besides, suppose later new speed values are added, someone still has
to revisit all the files to modify those checks. I think we should try to avoid this situation.

>> I think maybe we can try to remove the dependency on enum usb_device_speed
>> in usbfs and define a separate set of speed values similar to previous design
>> at https://www.spinics.net/lists/linux-usb/msg157709.html
>> By this way, in usbfs we get more freedom to determine how to explain
>> usb_device_speed and usb_ssp_rate, without the risk of breaking anything
>> elsewhere.
>> 
>> For example, define an USBDEVFS_SPEED_SUPER_PLUS to indicate
>> USB_SPEED_SUPER_PLUS with ssp rates GEN_UNKNOWN, GEN_2x1 and
>> GEN_1x2. They all stand for 10Gbps and we don't need to tell one from
>> another, similar to how it works in sysfs. Then define an
>> USBDEVFS_SPEED_SUPER_PLUS_BY2(maybe there is a more proper name)
>> to indicate USB_SPEED_SUPER_PLUS with ssp rate GEN_2x2, which stands
>> for 20Gbps.
>
>You can't really do that.  The values returned by the USBDEVFS_GET_SPEED 
>ioctl are the ones defined in include/uapi/linux/usb/ch9.h.  They are 
>USB_SPEED_UNKNOWN, etc., not USBDEVFS_SPEED_UNKNOWN, etc.  The only way 
>to extend them is by adding new entries to enum usb_device_speed.
>
>For example, you must not do anything that would break a user program 
>which does something like this:
>
>#include <linux/usbdevfs.h>
>#include <linux/usb/ch9.h>
>
>...
>
>	enum usb_device_speed s;
>
>	s = ioctl(fd, USBDEVFS_GET_SPEED);
>	if (s == USB_SPEED_HIGH) ...
>
>Alan Stern

Since those speed definitions are just int values, we could manage to maintain the compatibility
by keeping the same value. But anyway, my latest idea is not to extend enum usb_device_speed.
There are three basic cases:
1) When speed is less than USB_SPEED_SUPER_PLUS, just return dev->speed;
2) When speed is USB_SPEED_SUPER_PLUS but ssp_rate is less than USB_SSP_GEN_2x2,
return dev->speed;
3) When speed is USB_SPEED_SUPER_PLUS and ssp_rate is USB_SSP_GEN_2x2, a new
speed for usbdevfs should be #defined and let's call it USBDEVFS_SPEED_SUPER_PLUS_BY2.
This value won't be overlapped with any value in enum usb_device_speed, for example 7.
In this case, return USBDEVFS_SPEED_SUPER_PLUS_BY2.

The code could be like:

        case USBDEVFS_GET_SPEED:
                ret = ps->dev->speed;
+               if (ret == USB_SPEED_SUPER_PLUS &&
+                               ps->dev->ssp_rate == USB_SSP_GEN_2x2)
+                       ret = USBDEVFS_SPEED_SUPER_PLUS_BY2;
                break;

By this way, no need to add a new ioctl. No need to touch so many files. And when new
speeds are added later, just #define new values and extend the checks in above code.
Compatibility is also maintained. Before this change, USBDEVFS_GET_SPEED could
return 0-6. After this change, we can still return 0-6 for most of the cases, and 7 for
GEN_2x2 devices.

Regards,
Dingyan