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

[Dingyan Li]

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.
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.
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.
5. Okay, now I get it that sysfs is a replacement for usbfs. Even in libusb, sysfs is the
    preferred way, then fall back to usbfs if sysfs doesn't exist. My intention is not to see
    all of the sysfs-provided information in usbfs also. Anyway, if you think this patch is
    really unnecessary, I'm totally fine to withdraw it too.


Regards,
Dingyan