Messages in this thread | | | Date | Thu, 3 Aug 2023 17:39:56 +0200 | Subject | Re: [PATCH] USB: add usbfs ioctl to get specific superspeedplus rates | From | 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@redhat.com> 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
| |