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
