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

[Hans de Goede]

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