On Fri, Aug 04, 2023 at 12:16:19PM +0800, Dingyan Li wrote: > > At 2023-08-04 01:56:03, "Alan Stern" <stern@xxxxxxxxxxxxxxxxxxx> wrote: > >On Fri, Aug 04, 2023 at 12:06:15AM +0800, Dingyan Li wrote: > >> So after usb_device_speed is extended with Gen2x1, Gen1x2 and Gen2x2, > >> it feels that enum usb_ssp_rate becomes useless. Is it okay to just delete it? > >> I'm asking this since it is also used in several other source files so the fix may > >> not be as trivial as it looks. > > > >As long as the file is being used by other source files, don't delete > >it. If you want to fix up all those other places and then delete the > >file, that's fine. But of course, it would have to be a separate set of > >patches. > > > >It will also be necessary to audit the places in the kernel that > >currently use usb_device_speed. Some of them may need to be extended to > >handle the new entries properly. (Including, obviously, the parts of > >the code that store the device's speed in the first place.) > > > > >Alan Stern > > 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. > 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
