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

[Dingyan Li]

At 2023-07-26 22:39:32, "Hans de Goede" <hdegoede@xxxxxxxxxx> wrote:
>Hi All,
>
>On 7/26/23 05:20, Xiaofan Chen wrote:
>> On Wed, Jul 26, 2023 at 5:38 PM Oliver Neukum <oneukum@xxxxxxxx> wrote:
>>>
>>> On 26.07.23 03:37, Xiaofan Chen wrote:
>>>> On Tue, Jul 25, 2023 at 10:23 PM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>>>
>>> Hi,
>>>
>>>>> So unless there is some actual need from userspace tools like libusb (or
>>>>> anything else?) that requires this new ioctl, let's not add it otherwise
>>>>> we are signing ourselves up to support it for forever.
>>>>
>>>> Interestingly there is PR in libusb now, which uses sysfs for 20Gbps.
>>>
>>> True. Now would you write a patch for libusb?
>>> This looks to be turning into a chicken and egg problem.
>>>
>>>> Maybe this new usbfs IOCTL is indeed good to have if we can not extend
>>>
>>> Looking at the code of libusb you can see that libusb has two modes
>>> of operation. Either it finds sysfs, then it uses it, if not it
>>> goes for the ioctl.
>>>
>>> Now, how well shall it work without sysfs? That is a design decision
>>> and we should not be having this discussion again and again.
>>>
>>> BTW, that is not aimed at anybody personally, we are just trying to
>>> avoid a basic decision and it will come back.
>>>
>>>> the existing IOCTL USBDEVFS_GET_SPEED (but why not?).
>>>
>>> It does not include the lane count.
>>> And sort of fudging this into speed is a bad idea in the long
>>> run because we are likely to have collisions in the future.
>>>
>>> We have a basic issue here. Do we require libusb to use sysfs or not?
>> 
>> Adding Hans de Goede and Tormod Volder (libusb admins) here in the discussions
>> as I am more into the testing and support side of libusb and not a
>> real developer.
>> 
>> libusb does work with or without sysfs and there are multiple commits related
>> to sysfs vs usbfs.
>> 
>> An example commit from Hans in Sept 202 which is related to this discussion.
>> https://github.com/libusb/libusb/commit/f6068e83c4f5e5fba16b23b6a87f1f6d7ab7200a
>> ++++++++++++++++
>> linux: Fix libusb_get_device_speed() not working on wrapped devices
>> 
>> We don't have a sysfs_dir for wrapped devices, so we cannot read the speed
>> from sysfs.
>> 
>> The Linux kernel has supported a new ioctl to get the speed directly from
>> the fd for a while now, use that when we don't have sysfs access.
>> 
>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1871818
>> Reported-by: Gerd Hoffmann <kraxel@xxxxxxxxxx>
>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>> +++++++++++++++++
>> 
>> To Hans and Tormod:
>> Discussion thread for reference:
>> https://lore.kernel.org/linux-usb/da536c80-7398-dae0-a22c-16e521be697a@xxxxxxxx/T/#t
>
>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?

Regards,
Dingyan