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

[Dingyan Li]

At 2023-07-21 22:51:32, "Greg KH" <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>On Fri, Jul 21, 2023 at 08:35:37PM +0800, Dingyan Li wrote:
>> 
>> At 2023-07-21 19:04:29, "Greg KH" <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>> >On Fri, Jul 21, 2023 at 04:40:39PM +0800, Dingyan Li wrote:
>> >> The usbfs interface does not provide any way to get specific
>> >> superspeedplus rate, like Gen2x1, Gen1x2 or Gen2x2. Current
>> >> API include an USBDEVFS_GET_SPEED ioctl, but it can only return
>> >> general superspeedplus speed instead of any specific rates.
>> >> Therefore we can't tell whether it's a Gen2x2(20Gbps) device.
>> >> 
>> >> This patch introduce a new ioctl USBDEVFS_GET_SSP_RATE to fix
>> >> it. Similar information is already available via sysfs, it's
>> >> good to add it for usbfs too.
>> >> 
>> >> Signed-off-by: Dingyan Li <18500469033@xxxxxxx>
>> >> ---
>> >>  drivers/usb/core/devio.c          | 3 +++
>> >>  include/uapi/linux/usbdevice_fs.h | 1 +
>> >>  2 files changed, 4 insertions(+)
>> >> 
>> >> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
>> >> index 1a16a8bdea60..2f57eb163360 100644
>> >> --- a/drivers/usb/core/devio.c
>> >> +++ b/drivers/usb/core/devio.c
>> >> @@ -2783,6 +2783,9 @@ static long usbdev_do_ioctl(struct file *file, unsigned int cmd,
>> >>  	case USBDEVFS_GET_SPEED:
>> >>  		ret = ps->dev->speed;
>> >>  		break;
>> >> +	case USBDEVFS_GET_SSP_RATE:
>> >> +		ret = ps->dev->ssp_rate;
>> >> +		break;
>> >
>> >Shouldn't this new ioctl be documented somewhere?  What are the valid
>> >values it can return?  What if it in't a superspeed device?  Who is
>> >going to use this?
>> >
>> >And we have traditionally only been adding new information like this to
>> >sysfs, which was not around when usbfs was created.  Why not just use
>> >that instead?  Are you wanting to see all of the sysfs-provided
>> >information in usbfs also?
>> >
>> >thanks,
>> >
>> 
>> >greg k-h
>> 
>> 1. By saying "be documented somewhere", do you mean there is extra
>>     documentation work which needs to be done? Sorry that I missed this
>>     part since it's the first time for me to work on a kernel patch.
>
>It needs to be documented somewhere, otherwise no one knows how to use
>it.
>
>> 2. If no error, returned values are "enum usb_ssp_rate" defined in include/linux/usb/ch9.h
>> 3. ssp rate is only valid for superspeedplus. For other speeds, it should be
>>     USB_SSP_GEN_UNKNOWN.
>
>Ok, that should be documented.
>
>> 4. I found in libusb, there are two ways to get speed value for a device.
>>     One way is via sysfs, which has supported 20Gbps now. Another way is
>>     to use ioctl USBDEVFS_GET_SPEED. This is when I found this ioctl can only
>>     return USB_SPEED_SUPER_PLUS at most, it cannot determine current ssp rate
>>     further, no matter Gen1x2(10Gbps), Gen2x1(10Gbps) or Gen2x2(20Gbps). So I
>>     thought maybe it's good to provide a similar way like ioctl USBDEVFS_GET_SPEED
>>     in order to get ssp rates.
>
>If libusb doesn't need this ioctl, who would use it?  We only add apis
>that are actually going to be used.
>
>So if libusb doesn't use it, we need a real-world user for us to be able
>to add this.
>
>thanks,
>

>greg k-h

Okay, got it. The motivation should come from real-world needs.

Just like I mentioned above, currently in libusb ioctl USBDEVFS_GET_SPEED
is still used, especially where sysfs is not supported. My original idea
was exactly trying to add this new ioctl into libusb. So in order to get 20Gbps
speed, we need extra information. The basic workflow is like below:

// This is pretty much how libusb does it, get 10Gbps at most
ret = ioctl(USBDEVFS_GET_SPEED)
if (ret == USB_SPEED_SUPER_PLUS) then
    speed = 10Gbps
    // With this new ioctl, we can get 20Gbps now
    ret = ioctl(USBDEVFS_GET_SSP_RATE)
    if (ret == USB_SSP_GEN_2x2)
        speed = 20Gbps

libusb can be a good place to document the usage of this new ioctl if similar patch
can be accepted into it. And I can't think of other real-world users now. Of course,
like you've explained, it seems quite unnecessary when sysfs is supported.

Regards,
Dingyan