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

[Dingyan Li]

At 2023-08-03 23:39:56, "Hans de Goede" <hdegoede@xxxxxxxxxx> wrote:
>Hi,
>
>On 8/3/23 17:10, Alan Stern wrote:
>> On Thu, Aug 03, 2023 at 02:13:33PM +0800, Dingyan Li wrote:
>>>
>>> At 2023-07-26 22:39:32, "Hans de Goede" <hdegoede@xxxxxxxxxx> wrote:
>> 
>>>> Right, so the reason why IOCTL USBDEVFS_GET_SPEED was added is so 
>>>> that a confined qemu process which gets just a fd for a /dev/bus/usb/ 
>>>> device passed by a more privileged process can still get the speed 
>>>> despite it not having sysfs access. This is necessary for correct 
>>>> pass-through of USB devices.
>>>>
>>>> Since USBDEVFS_GET_SPEED now no longer tells the full story I believe 
>>>> that the proposed USBDEVFS_GET_SSP_RATE ioctl makes sense.
>>>>
>>>> The current patch however misses moving the enum usb_ssp_rate 
>>>> declaration from include/linux/usb/ch9.h to 
>>>> include/uapi/linux/usb/ch9.h so that needs to be fixed in a version 
>>>> 2. Assuming that with the above explanation of why this is necessary 
>>>> Greg and Alan are ok with adding the ioctl.
>>>>
>>>> Regards,
>>>>
>>>> Hans
>>>>
>>>
>>> Hi Greg and Alan,
>>>
>>> Could you please share your opinions about Hans' justification?
>> 
>> Instead of adding a new ioctl or modifying an existing one, how about 
>> increasing the set of constants in enum usb_device_speed?  Then the 
>> existing ioctls could return the newly defined values when appropriate, 
>> with no other changes necessary.
>
>Right, I was thinking along the same lines but I was not entirely
>sure this would work because I looked at the wrong bits of
>include/uapi/linux/usb/ch9.h while writing my first email on this.
>
>Looking again I see we already have a straight forward
>enum usb_device_speed for this which can easily be extended.
>
>> (This doesn't mean just moving the definition of usb_ssp_rate from one 
>> header file to the other.  The usb_device_speed enumeration should be 
>> extended with the new members.  Perhaps omitting USB_SSP_GEN_UNKNOWN 
>> since we already have USB_SPEED_SUPER_PLUS, or setting the first equal 
>> to the second.)
>> 
>> I don't think there was ever a requirement in the API that the set of 
>> values in usb_device_speed could never increase (and in fact it has 
>> increased in the past).  Such a requirement wouldn't make any sense, 
>> given how the USB-IF keeps defining newer and faster USB bus 
>> implementations.
>> 
>> Hans, would that play well with libusb?
>
>It should, this is how libusb uses the USBDEVFS_GET_SPEED ioctl:
>
>static enum libusb_speed usbfs_get_speed(struct libusb_context *ctx, int fd)
>{
>	int r;
>
>	r = ioctl(fd, IOCTL_USBFS_GET_SPEED, NULL);
>	switch (r) {
>	case USBFS_SPEED_UNKNOWN:	return LIBUSB_SPEED_UNKNOWN;
>	case USBFS_SPEED_LOW:		return LIBUSB_SPEED_LOW;
>	case USBFS_SPEED_FULL:		return LIBUSB_SPEED_FULL;
>	case USBFS_SPEED_HIGH:		return LIBUSB_SPEED_HIGH;
>	case USBFS_SPEED_WIRELESS:	return LIBUSB_SPEED_HIGH;
>	case USBFS_SPEED_SUPER:		return LIBUSB_SPEED_SUPER;
>	case USBFS_SPEED_SUPER_PLUS:	return LIBUSB_SPEED_SUPER_PLUS;
>	default:
>		usbi_warn(ctx, "Error getting device speed: %d", r);
>	}
>
>	return LIBUSB_SPEED_UNKNOWN;
>}
>
>I think that GEN_2x1 should probably be mapped to
>USBFS_SPEED_SUPER_PLUS so as to not break this most common case
>and to keep apps reporting either Super Speed Plus or 10Gbps
>(more common) for this.
>
>GEN_1x2 + GEN_2x2 can then be mapped to new values, which will
>cause libusb to log a warning + return LIBUSB_SPEED_UNKNOWN
>until libusb is updated which seems harmless enough.
>
>Regards,
>
>Hans
>

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.

Regards,
Dingyan