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

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

[Dingyan Li]

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@163.com>
---
 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;
 	case USBDEVFS_FORBID_SUSPEND:
 		ret = proc_forbid_suspend(ps);
 		break;
diff --git a/include/uapi/linux/usbdevice_fs.h b/include/uapi/linux/usbdevice_fs.h
index 74a84e02422a..f5522e910329 100644
--- a/include/uapi/linux/usbdevice_fs.h
+++ b/include/uapi/linux/usbdevice_fs.h
@@ -227,5 +227,6 @@ struct usbdevfs_streams {
 #define USBDEVFS_FORBID_SUSPEND    _IO('U', 33)
 #define USBDEVFS_ALLOW_SUSPEND     _IO('U', 34)
 #define USBDEVFS_WAIT_FOR_RESUME   _IO('U', 35)
+#define USBDEVFS_GET_SSP_RATE      _IO('U', 36)
 
 #endif /* _UAPI_LINUX_USBDEVICE_FS_H */
-- 
2.25.1

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

[Greg KH]

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@163.com>
> ---
>  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

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

[Greg KH]

On Fri, Jul 21, 2023 at 08:09:37PM +0800, Dingyan Li wrote:

<snip?

For some reason your email client responded in html form, which is
rejected by the mailing lists.  Please fix up your email client and
resend it and I will be glad to respond.

thanks,

greg k-h

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

[Dingyan Li]

At 2023-07-21 19:04:29, "Greg KH" <gregkh@linuxfoundation.org> 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@163.com>
>> ---
>>  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

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

[Greg KH]

On Fri, Jul 21, 2023 at 08:35:37PM +0800, Dingyan Li wrote:
> 
> At 2023-07-21 19:04:29, "Greg KH" <gregkh@linuxfoundation.org> 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@163.com>
> >> ---
> >>  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

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@linuxfoundation.org> wrote:
>On Fri, Jul 21, 2023 at 08:35:37PM +0800, Dingyan Li wrote:
>> 
>> At 2023-07-21 19:04:29, "Greg KH" <gregkh@linuxfoundation.org> 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@163.com>
>> >> ---
>> >>  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

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

[Alan Stern]

On Fri, Jul 21, 2023 at 11:43:29PM +0800, Dingyan Li wrote:
> 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.

For what it's worth, many of the other ioctls in usbfs are documented 
(very incompletely) in Documentation/driver-api/usb/usb.rst.  That's 
probably the best place to add any documentation for new APIs.

Alan Stern

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

[Oliver Neukum]

On 21.07.23 16:51, Greg KH wrote:

>> 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.

Documentation would be good.
Where should it go, though? These enums are part of the uapi
hierarchy. Now, documentation for uapi would be good, but we
should not mix it with documentation for ioctl
That is if an ioctl uses an enum out of uapi it needs to be
explicitly mentioned by name, but documenting the semantics
of the enum _there_ would be wrong.

> 
>> 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.

I am sorry, but that looks pretty much like a question of API design to me.
To what extent is libusb supposed to be functional without sysfs? There is
no technical answer to this. It is a question of design goals.

If we follow the precedent of c01b244ad848a
("USB: add usbfs ioctl to retrieve the connection speed")
then we should apply an updated version of Dingyan Li's patch, preferably
coupled with a patch for libusb or we go and deprecate some ioctls.

	Regards
		Oliver

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

[Greg KH]

On Mon, Jul 24, 2023 at 11:47:49AM +0200, Oliver Neukum wrote:
> On 21.07.23 16:51, Greg KH wrote:
> 
> > > 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.
> 
> Documentation would be good.
> Where should it go, though? These enums are part of the uapi
> hierarchy. Now, documentation for uapi would be good, but we
> should not mix it with documentation for ioctl
> That is if an ioctl uses an enum out of uapi it needs to be
> explicitly mentioned by name, but documenting the semantics
> of the enum _there_ would be wrong.
> 
> > 
> > > 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.
> 
> I am sorry, but that looks pretty much like a question of API design to me.
> To what extent is libusb supposed to be functional without sysfs? There is
> no technical answer to this. It is a question of design goals.
> 
> If we follow the precedent of c01b244ad848a
> ("USB: add usbfs ioctl to retrieve the connection speed")
> then we should apply an updated version of Dingyan Li's patch, preferably
> coupled with a patch for libusb or we go and deprecate some ioctls.

We can never "deprecate" ioctls, sorry.

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.

thanks,

greg k-h

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

[Dingyan Li]

At 2023-07-25 21:24:32, "Greg KH" <gregkh@linuxfoundation.org> wrote:
>On Mon, Jul 24, 2023 at 11:47:49AM +0200, Oliver Neukum wrote:
>> On 21.07.23 16:51, Greg KH wrote:
>> 
>> > > 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.
>> 
>> Documentation would be good.
>> Where should it go, though? These enums are part of the uapi
>> hierarchy. Now, documentation for uapi would be good, but we
>> should not mix it with documentation for ioctl
>> That is if an ioctl uses an enum out of uapi it needs to be
>> explicitly mentioned by name, but documenting the semantics
>> of the enum _there_ would be wrong.
>> 
>> > 
>> > > 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.
>> 
>> I am sorry, but that looks pretty much like a question of API design to me.
>> To what extent is libusb supposed to be functional without sysfs? There is
>> no technical answer to this. It is a question of design goals.
>> 
>> If we follow the precedent of c01b244ad848a
>> ("USB: add usbfs ioctl to retrieve the connection speed")
>> then we should apply an updated version of Dingyan Li's patch, preferably
>> coupled with a patch for libusb or we go and deprecate some ioctls.
>
>We can never "deprecate" ioctls, sorry.
>
>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.
>
>thanks,
>

>greg k-h

If we can't "deprecate" ioctls, can we change the returned contents of existing ones?

I found even in usbfs, we got two different ioctls which can be used to get device speed,
including USBDEVFS_GET_SPEED and USBDEVFS_CONNINFO_EX. Maybe we can reduce
some redundancy there.

And by saying actual needs, you mean it's not enough to just add this new ioctl to libusb and
imagine this part of libusb will be used by anyone in the future. There must be some real-world
requests for now which make libusb have to use this new ioctl, right?


Regards,
Dingyan

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

[Oliver Neukum]

On 25.07.23 15:54, Dingyan Li wrote:

> If we can't "deprecate" ioctls, can we change the returned contents of existing ones?

No. Absolutely not. That is totally unacceptable. It would be much
worse than just removing the support.

	Regards
		Oliver

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

[Dingyan Li]

At 2023-07-25 22:08:44, "Oliver Neukum" <oneukum@suse.com> wrote:
>On 25.07.23 15:54, Dingyan Li wrote:
>
>> If we can't "deprecate" ioctls, can we change the returned contents of existing ones?
>
>No. Absolutely not. That is totally unacceptable. It would be much
>worse than just removing the support.
>
>	Regards
>		Oliver

Got it, I guess this is for backward-compatibility.

I also have another thought. USBDEVFS_CONNINFO_EX is kind of special and
used to retrieve contents of variable length. If you check proc_conninfo_ex(),
I think maybe we can append a new member to "struct usbdevfs_conninfo_ex"
without breaking backward-compatibility.

By this way, we can avoid adding a new ioctl. Or even more aggressively,
drop USBDEVFS_GET_SPEED and force everyone to use USBDEVFS_CONNINFO_EX
since it can also return device speed.


Regards,
Dingyan

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

[Greg KH]

On Tue, Jul 25, 2023 at 10:40:10PM +0800, Dingyan Li wrote:
> 
> At 2023-07-25 22:08:44, "Oliver Neukum" <oneukum@suse.com> wrote:
> >On 25.07.23 15:54, Dingyan Li wrote:
> >
> >> If we can't "deprecate" ioctls, can we change the returned contents of existing ones?
> >
> >No. Absolutely not. That is totally unacceptable. It would be much
> >worse than just removing the support.
> >
> >	Regards
> >		Oliver
> 
> Got it, I guess this is for backward-compatibility.
> 
> I also have another thought. USBDEVFS_CONNINFO_EX is kind of special and
> used to retrieve contents of variable length. If you check proc_conninfo_ex(),
> I think maybe we can append a new member to "struct usbdevfs_conninfo_ex"
> without breaking backward-compatibility.

How exactly would that work?  Remember, new kernels still need to work
with old userspace code.

> By this way, we can avoid adding a new ioctl. Or even more aggressively,
> drop USBDEVFS_GET_SPEED and force everyone to use USBDEVFS_CONNINFO_EX
> since it can also return device speed.

We can not "force" anyone to change, that's not how the kernel works,
sorry.

greg k-h

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

[Dingyan Li]

At 2023-07-25 23:12:01, "Greg KH" <gregkh@linuxfoundation.org> wrote:
>On Tue, Jul 25, 2023 at 10:40:10PM +0800, Dingyan Li wrote:
>> 
>> At 2023-07-25 22:08:44, "Oliver Neukum" <oneukum@suse.com> wrote:
>> >On 25.07.23 15:54, Dingyan Li wrote:
>> >
>> >> If we can't "deprecate" ioctls, can we change the returned contents of existing ones?
>> >
>> >No. Absolutely not. That is totally unacceptable. It would be much
>> >worse than just removing the support.
>> >
>> >	Regards
>> >		Oliver
>> 
>> Got it, I guess this is for backward-compatibility.
>> 
>> I also have another thought. USBDEVFS_CONNINFO_EX is kind of special and
>> used to retrieve contents of variable length. If you check proc_conninfo_ex(),
>> I think maybe we can append a new member to "struct usbdevfs_conninfo_ex"
>> without breaking backward-compatibility.
>
>How exactly would that work?  Remember, new kernels still need to work
>with old userspace code.
>
>greg k-h

In proc_conninfo_ex(), the number of returned bytes is determined by
the smaller number between sizeof(struct usbdevfs_conninfo_ex) and a
user specified size. So if we only append new members to the end of
struct usbdevfs_conninfo_ex, it won't impact the bytes in the beginning.

For example, current sizeof(struct usbdevfs_conninfo_ex) is 24 bytes.
Let's assume there is already some code using ioctl USBDEVFS_CONNINFO_EX
and requesting a full size, which is 24 bytes. Now we append a new __u32
member called ssp_rate in the end of struct usbdevfs_conninfo_ex. For the old
code, the meaning of the beginning 24 bytes is still the same. But for new code,
we can now request a full size of 28 bytes.


Regards,
Dingyan 

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

[Oliver Neukum]

On 25.07.23 18:11, Dingyan Li wrote:
  
> In proc_conninfo_ex(), the number of returned bytes is determined by
> the smaller number between sizeof(struct usbdevfs_conninfo_ex) and a
> user specified size. So if we only append new members to the end of
> struct usbdevfs_conninfo_ex, it won't impact the bytes in the beginning.

You have just caused memory corruption in user space by overwriting what
was right behind the buffer of the agreed upon size. Or, not much better,
caused a segmentation fault.

	Regards
		Oliver

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

[Dingyan Li]

At 2023-07-26 16:33:22, "Oliver Neukum" <oneukum@suse.com> wrote:
>On 25.07.23 18:11, Dingyan Li wrote:
>  
>> In proc_conninfo_ex(), the number of returned bytes is determined by
>> the smaller number between sizeof(struct usbdevfs_conninfo_ex) and a
>> user specified size. So if we only append new members to the end of
>> struct usbdevfs_conninfo_ex, it won't impact the bytes in the beginning.
>
>You have just caused memory corruption in user space by overwriting what
>was right behind the buffer of the agreed upon size. Or, not much better,
>caused a segmentation fault.
>
>	Regards
>		Oliver

How come?

The actual returned bytes must be smaller than or equal to user specified size.
You can check https://elixir.bootlin.com/linux/v6.5-rc3/source/drivers/usb/core/devio.c#L1493

Regards,
Dingyan

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

[Oliver Neukum]

On 26.07.23 11:36, Dingyan Li wrote:
> At 2023-07-26 16:33:22, "Oliver Neukum" <oneukum@suse.com> wrote:
>> On 25.07.23 18:11, Dingyan Li wrote:
>>   
>>> In proc_conninfo_ex(), the number of returned bytes is determined by
>>> the smaller number between sizeof(struct usbdevfs_conninfo_ex) and a
>>> user specified size. So if we only append new members to the end of
>>> struct usbdevfs_conninfo_ex, it won't impact the bytes in the beginning.
>>
>> You have just caused memory corruption in user space by overwriting what
>> was right behind the buffer of the agreed upon size. Or, not much better,
>> caused a segmentation fault.
>>
>> 	Regards
>> 		Oliver
> 
> How come?

Sorry, I misread the check at the start.

> The actual returned bytes must be smaller than or equal to user specified size.
> You can check https://elixir.bootlin.com/linux/v6.5-rc3/source/drivers/usb/core/devio.c#L1493

Yes, we can add. But where is the point?
User space has to be changed to use new sizes.

The problem is not your patch. Add documentation to it and it is fine.
We have a basic issue here. Do we require libusb to use sysfs or not?

	Regards
		Oliver

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

[Dingyan Li]

At 2023-07-26 17:49:26, "Oliver Neukum" <oneukum@suse.com> wrote:
>
>
>On 26.07.23 11:36, Dingyan Li wrote:
>> At 2023-07-26 16:33:22, "Oliver Neukum" <oneukum@suse.com> wrote:
>>> On 25.07.23 18:11, Dingyan Li wrote:
>>>   
>>>> In proc_conninfo_ex(), the number of returned bytes is determined by
>>>> the smaller number between sizeof(struct usbdevfs_conninfo_ex) and a
>>>> user specified size. So if we only append new members to the end of
>>>> struct usbdevfs_conninfo_ex, it won't impact the bytes in the beginning.
>>>
>>> You have just caused memory corruption in user space by overwriting what
>>> was right behind the buffer of the agreed upon size. Or, not much better,
>>> caused a segmentation fault.
>>>
>>> 	Regards
>>> 		Oliver
>> 
>> How come?
>
>Sorry, I misread the check at the start.
>
>> The actual returned bytes must be smaller than or equal to user specified size.
>> You can check https://elixir.bootlin.com/linux/v6.5-rc3/source/drivers/usb/core/devio.c#L1493
>
>Yes, we can add. But where is the point?
>User space has to be changed to use new sizes.
>

Not necessarily, by this way at least old user space code has a chance to stay
put since it can still get basically the same bytes like before, just not include
the newly appended fields. Of course, if anyone want to access the new fields,
they have to use the new size.

>The problem is not your patch. Add documentation to it and it is fine.
>We have a basic issue here. Do we require libusb to use sysfs or not?
>

Yeah, agree with this.

Regards,
Dingyan

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

[Xiaofan Chen]

On Tue, Jul 25, 2023 at 10:23 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jul 24, 2023 at 11:47:49AM +0200, Oliver Neukum wrote:
> > On 21.07.23 16:51, Greg KH wrote:
>> ...
> > > > 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.
> >
> > I am sorry, but that looks pretty much like a question of API design to me.
> > To what extent is libusb supposed to be functional without sysfs? There is
> > no technical answer to this. It is a question of design goals.
> >
> > If we follow the precedent of c01b244ad848a
> > ("USB: add usbfs ioctl to retrieve the connection speed")
> > then we should apply an updated version of Dingyan Li's patch, preferably
> > coupled with a patch for libusb or we go and deprecate some ioctls.
>
> We can never "deprecate" ioctls, sorry.
>
> 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.
Maybe this new usbfs IOCTL is indeed good to have if we can not extend
the existing IOCTL USBDEVFS_GET_SPEED (but why not?).
https://github.com/libusb/libusb/pull/1298


-- 
Xiaofan

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

[Oliver Neukum]

On 26.07.23 03:37, Xiaofan Chen wrote:
> On Tue, Jul 25, 2023 at 10:23 PM Greg KH <gregkh@linuxfoundation.org> 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.

	Regards
		Oliver

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

[Xiaofan Chen]

On Wed, Jul 26, 2023 at 5:38 PM Oliver Neukum <oneukum@suse.com> wrote:
>
> On 26.07.23 03:37, Xiaofan Chen wrote:
> > On Tue, Jul 25, 2023 at 10:23 PM Greg KH <gregkh@linuxfoundation.org> 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@redhat.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
+++++++++++++++++

To Hans and Tormod:
Discussion thread for reference:
https://lore.kernel.org/linux-usb/da536c80-7398-dae0-a22c-16e521be697a@suse.com/T/#t


-- 
Xiaofan

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

[Hans de Goede]

Hi All,

On 7/26/23 05:20, Xiaofan Chen wrote:
> On Wed, Jul 26, 2023 at 5:38 PM Oliver Neukum <oneukum@suse.com> wrote:
>>
>> On 26.07.23 03:37, Xiaofan Chen wrote:
>>> On Tue, Jul 25, 2023 at 10:23 PM Greg KH <gregkh@linuxfoundation.org> 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@redhat.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> +++++++++++++++++
> 
> To Hans and Tormod:
> Discussion thread for reference:
> https://lore.kernel.org/linux-usb/da536c80-7398-dae0-a22c-16e521be697a@suse.com/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

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@redhat.com> wrote:
>Hi All,
>
>On 7/26/23 05:20, Xiaofan Chen wrote:
>> On Wed, Jul 26, 2023 at 5:38 PM Oliver Neukum <oneukum@suse.com> wrote:
>>>
>>> On 26.07.23 03:37, Xiaofan Chen wrote:
>>>> On Tue, Jul 25, 2023 at 10:23 PM Greg KH <gregkh@linuxfoundation.org> 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@redhat.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> +++++++++++++++++
>> 
>> To Hans and Tormod:
>> Discussion thread for reference:
>> https://lore.kernel.org/linux-usb/da536c80-7398-dae0-a22c-16e521be697a@suse.com/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

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

[Alan Stern]

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.

(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?

Alan Stern

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

[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

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

[Dingyan Li]

At 2023-08-03 23:39:56, "Hans de Goede" <hdegoede@redhat.com> wrote:
>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
>

So after usb_device_speed is extended with Gen2x1, Gen1x2 and Gen2x2,
it feels that enum usb_ssp_rate becomes useless. Is it okay to just delete it?
I'm asking this since it is also used in several other source files so the fix may
not be as trivial as it looks.

Regards,
Dingyan

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

[Alan Stern]

On Fri, Aug 04, 2023 at 12:06:15AM +0800, Dingyan Li wrote:
> So after usb_device_speed is extended with Gen2x1, Gen1x2 and Gen2x2,
> it feels that enum usb_ssp_rate becomes useless. Is it okay to just delete it?
> I'm asking this since it is also used in several other source files so the fix may
> not be as trivial as it looks.

As long as the file is being used by other source files, don't delete 
it.  If you want to fix up all those other places and then delete the 
file, that's fine.  But of course, it would have to be a separate set of 
patches.

It will also be necessary to audit the places in the kernel that 
currently use usb_device_speed.  Some of them may need to be extended to 
handle the new entries properly.  (Including, obviously, the parts of 
the code that store the device's speed in the first place.)

Alan Stern

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

[Dingyan Li]

At 2023-08-04 01:56:03, "Alan Stern" <stern@rowland.harvard.edu> wrote:
>On Fri, Aug 04, 2023 at 12:06:15AM +0800, Dingyan Li wrote:
>> So after usb_device_speed is extended with Gen2x1, Gen1x2 and Gen2x2,
>> it feels that enum usb_ssp_rate becomes useless. Is it okay to just delete it?
>> I'm asking this since it is also used in several other source files so the fix may
>> not be as trivial as it looks.
>
>As long as the file is being used by other source files, don't delete 
>it.  If you want to fix up all those other places and then delete the 
>file, that's fine.  But of course, it would have to be a separate set of 
>patches.
>
>It will also be necessary to audit the places in the kernel that 
>currently use usb_device_speed.  Some of them may need to be extended to 
>handle the new entries properly.  (Including, obviously, the parts of 
>the code that store the device's speed in the first place.)
>

>Alan Stern

Another issue is that USB_SPEED_SUPER_PLUS has been widely used in
so many files to execute conditional banches. Once we extend and store device's
speed with new values in the first place, we might need to check all places where
USB_SPEED_SUPER_PLUS is used in case of any regression.

I think maybe we can try to remove the dependency on enum usb_device_speed
in usbfs and define a separate set of speed values similar to previous design
at https://www.spinics.net/lists/linux-usb/msg157709.html
By this way, in usbfs we get more freedom to determine how to explain
usb_device_speed and usb_ssp_rate, without the risk of breaking anything
elsewhere.

For example, define an USBDEVFS_SPEED_SUPER_PLUS to indicate
USB_SPEED_SUPER_PLUS with ssp rates GEN_UNKNOWN, GEN_2x1 and
GEN_1x2. They all stand for 10Gbps and we don't need to tell one from
another, similar to how it works in sysfs. Then define an
USBDEVFS_SPEED_SUPER_PLUS_BY2(maybe there is a more proper name)
to indicate USB_SPEED_SUPER_PLUS with ssp rate GEN_2x2, which stands
for 20Gbps.

Regards,
Dingyan

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

[Alan Stern]

On Fri, Aug 04, 2023 at 12:16:19PM +0800, Dingyan Li wrote:
> 
> At 2023-08-04 01:56:03, "Alan Stern" <stern@rowland.harvard.edu> wrote:
> >On Fri, Aug 04, 2023 at 12:06:15AM +0800, Dingyan Li wrote:
> >> So after usb_device_speed is extended with Gen2x1, Gen1x2 and Gen2x2,
> >> it feels that enum usb_ssp_rate becomes useless. Is it okay to just delete it?
> >> I'm asking this since it is also used in several other source files so the fix may
> >> not be as trivial as it looks.
> >
> >As long as the file is being used by other source files, don't delete 
> >it.  If you want to fix up all those other places and then delete the 
> >file, that's fine.  But of course, it would have to be a separate set of 
> >patches.
> >
> >It will also be necessary to audit the places in the kernel that 
> >currently use usb_device_speed.  Some of them may need to be extended to 
> >handle the new entries properly.  (Including, obviously, the parts of 
> >the code that store the device's speed in the first place.)
> >
> 
> >Alan Stern
> 
> Another issue is that USB_SPEED_SUPER_PLUS has been widely used in
> so many files to execute conditional banches. Once we extend and store device's
> speed with new values in the first place, we might need to check all places where
> USB_SPEED_SUPER_PLUS is used in case of any regression.

Certainly.  That's part of auditing all the current users of 
usb_device_speed.

> I think maybe we can try to remove the dependency on enum usb_device_speed
> in usbfs and define a separate set of speed values similar to previous design
> at https://www.spinics.net/lists/linux-usb/msg157709.html
> By this way, in usbfs we get more freedom to determine how to explain
> usb_device_speed and usb_ssp_rate, without the risk of breaking anything
> elsewhere.
> 
> For example, define an USBDEVFS_SPEED_SUPER_PLUS to indicate
> USB_SPEED_SUPER_PLUS with ssp rates GEN_UNKNOWN, GEN_2x1 and
> GEN_1x2. They all stand for 10Gbps and we don't need to tell one from
> another, similar to how it works in sysfs. Then define an
> USBDEVFS_SPEED_SUPER_PLUS_BY2(maybe there is a more proper name)
> to indicate USB_SPEED_SUPER_PLUS with ssp rate GEN_2x2, which stands
> for 20Gbps.

You can't really do that.  The values returned by the USBDEVFS_GET_SPEED 
ioctl are the ones defined in include/uapi/linux/usb/ch9.h.  They are 
USB_SPEED_UNKNOWN, etc., not USBDEVFS_SPEED_UNKNOWN, etc.  The only way 
to extend them is by adding new entries to enum usb_device_speed.

For example, you must not do anything that would break a user program 
which does something like this:

#include <linux/usbdevfs.h>
#include <linux/usb/ch9.h>

...

	enum usb_device_speed s;

	s = ioctl(fd, USBDEVFS_GET_SPEED);
	if (s == USB_SPEED_HIGH) ...

Alan Stern

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

[Dingyan Li]

At 2023-08-04 22:55:49, "Alan Stern" <stern@rowland.harvard.edu> wrote:

>> Another issue is that USB_SPEED_SUPER_PLUS has been widely used in
>> so many files to execute conditional banches. Once we extend and store device's
>> speed with new values in the first place, we might need to check all places where
>> USB_SPEED_SUPER_PLUS is used in case of any regression.
>
>Certainly.  That's part of auditing all the current users of 
>usb_device_speed.

This might not be a good idea and feels kind of drift away from what we originally
want to do. Besides, suppose later new speed values are added, someone still has
to revisit all the files to modify those checks. I think we should try to avoid this situation.

>> I think maybe we can try to remove the dependency on enum usb_device_speed
>> in usbfs and define a separate set of speed values similar to previous design
>> at https://www.spinics.net/lists/linux-usb/msg157709.html
>> By this way, in usbfs we get more freedom to determine how to explain
>> usb_device_speed and usb_ssp_rate, without the risk of breaking anything
>> elsewhere.
>> 
>> For example, define an USBDEVFS_SPEED_SUPER_PLUS to indicate
>> USB_SPEED_SUPER_PLUS with ssp rates GEN_UNKNOWN, GEN_2x1 and
>> GEN_1x2. They all stand for 10Gbps and we don't need to tell one from
>> another, similar to how it works in sysfs. Then define an
>> USBDEVFS_SPEED_SUPER_PLUS_BY2(maybe there is a more proper name)
>> to indicate USB_SPEED_SUPER_PLUS with ssp rate GEN_2x2, which stands
>> for 20Gbps.
>
>You can't really do that.  The values returned by the USBDEVFS_GET_SPEED 
>ioctl are the ones defined in include/uapi/linux/usb/ch9.h.  They are 
>USB_SPEED_UNKNOWN, etc., not USBDEVFS_SPEED_UNKNOWN, etc.  The only way 
>to extend them is by adding new entries to enum usb_device_speed.
>
>For example, you must not do anything that would break a user program 
>which does something like this:
>
>#include <linux/usbdevfs.h>
>#include <linux/usb/ch9.h>
>
>...
>
>	enum usb_device_speed s;
>
>	s = ioctl(fd, USBDEVFS_GET_SPEED);
>	if (s == USB_SPEED_HIGH) ...
>
>Alan Stern

Since those speed definitions are just int values, we could manage to maintain the compatibility
by keeping the same value. But anyway, my latest idea is not to extend enum usb_device_speed.
There are three basic cases:
1) When speed is less than USB_SPEED_SUPER_PLUS, just return dev->speed;
2) When speed is USB_SPEED_SUPER_PLUS but ssp_rate is less than USB_SSP_GEN_2x2,
return dev->speed;
3) When speed is USB_SPEED_SUPER_PLUS and ssp_rate is USB_SSP_GEN_2x2, a new
speed for usbdevfs should be #defined and let's call it USBDEVFS_SPEED_SUPER_PLUS_BY2.
This value won't be overlapped with any value in enum usb_device_speed, for example 7.
In this case, return USBDEVFS_SPEED_SUPER_PLUS_BY2.

The code could be like:

        case USBDEVFS_GET_SPEED:
                ret = ps->dev->speed;
+               if (ret == USB_SPEED_SUPER_PLUS &&
+                               ps->dev->ssp_rate == USB_SSP_GEN_2x2)
+                       ret = USBDEVFS_SPEED_SUPER_PLUS_BY2;
                break;

By this way, no need to add a new ioctl. No need to touch so many files. And when new
speeds are added later, just #define new values and extend the checks in above code.
Compatibility is also maintained. Before this change, USBDEVFS_GET_SPEED could
return 0-6. After this change, we can still return 0-6 for most of the cases, and 7 for
GEN_2x2 devices.

Regards,
Dingyan

[PATCH v2] USB: Support 20Gbps speed for ioctl USBDEVFS_GET_SPEED

[Dingyan Li]

Currently ioctl USBDEVFS_GET_SPEED can only return
USB_SPEED_SUPER_PLUS at most. However, there are also
ssp rates to indicate different connection speeds, which
we can not tell further via USBDEVFS_GET_SPEED.

To fix it, this patch still uses USB_SPEED_SUPER_PLUS
to indicate USB_SSP_GEN_UNKNOWN, USB_SSP_GEN_2x1, and
USB_SSP_GEN_1x2. But need to #define a new value for
USB_SSP_GEN_2x2. Besides, move the definition of enum
usb_ssp_rate from include/linux/usb/ch9.h to
include/uapi/linux/usb/ch9.h, which is a better place
to hold it.

Signed-off-by: Dingyan Li <18500469033@163.com>
---
 drivers/usb/core/devio.c          | 3 +++
 include/linux/usb/ch9.h           | 9 ---------
 include/uapi/linux/usb/ch9.h      | 8 ++++++++
 include/uapi/linux/usbdevice_fs.h | 8 ++++++--
 4 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 1a16a8bdea60..ad13f58cbd06 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -2782,6 +2782,9 @@ static long usbdev_do_ioctl(struct file *file, unsigned int cmd,
 		break;
 	case USBDEVFS_GET_SPEED:
 		ret = ps->dev->speed;
+		if (ret == USB_SPEED_SUPER_PLUS &&
+				ps->dev->ssp_rate == USB_SSP_GEN_2x2)
+			ret = USBDEVFS_SPEED_SUPER_PLUS_BY2;
 		break;
 	case USBDEVFS_FORBID_SUSPEND:
 		ret = proc_forbid_suspend(ps);
diff --git a/include/linux/usb/ch9.h b/include/linux/usb/ch9.h
index c93b410b314a..b5a0bc89de5c 100644
--- a/include/linux/usb/ch9.h
+++ b/include/linux/usb/ch9.h
@@ -32,15 +32,6 @@
 
 #include <uapi/linux/usb/ch9.h>
 
-/* USB 3.2 SuperSpeed Plus phy signaling rate generation and lane count */
-
-enum usb_ssp_rate {
-	USB_SSP_GEN_UNKNOWN = 0,
-	USB_SSP_GEN_2x1,
-	USB_SSP_GEN_1x2,
-	USB_SSP_GEN_2x2,
-};
-
 struct device;
 
 extern const char *usb_ep_type_string(int ep_type);
diff --git a/include/uapi/linux/usb/ch9.h b/include/uapi/linux/usb/ch9.h
index 8a147abfc680..62591eb4d30a 100644
--- a/include/uapi/linux/usb/ch9.h
+++ b/include/uapi/linux/usb/ch9.h
@@ -1185,6 +1185,14 @@ enum usb_device_speed {
 	USB_SPEED_SUPER_PLUS,			/* usb 3.1 */
 };
 
+/* USB 3.2 SuperSpeed Plus phy signaling rate generation and lane count */
+
+enum usb_ssp_rate {
+	USB_SSP_GEN_UNKNOWN = 0,
+	USB_SSP_GEN_2x1,
+	USB_SSP_GEN_1x2,
+	USB_SSP_GEN_2x2,
+};
 
 enum usb_device_state {
 	/* NOTATTACHED isn't in the USB spec, and this state acts
diff --git a/include/uapi/linux/usbdevice_fs.h b/include/uapi/linux/usbdevice_fs.h
index 74a84e02422a..46ba793f4938 100644
--- a/include/uapi/linux/usbdevice_fs.h
+++ b/include/uapi/linux/usbdevice_fs.h
@@ -180,9 +180,13 @@ struct usbdevfs_streams {
 };
 
 /*
- * USB_SPEED_* values returned by USBDEVFS_GET_SPEED are defined in
- * linux/usb/ch9.h
+ * For USBDEVFS_GET_SPEED:
+ *
+ * When speed is USB_SPEED_SUPER_PLUS and ssp_rate is USB_SSP_GEN_2x2
+ * or higher, use below definitions to indicate actual connection speed.
+ * Otherwise, just return USB_SPEED_* values defined in linux/usb/ch9.h.
  */
+#define USBDEVFS_SPEED_SUPER_PLUS_BY2	7	/* USB_SSP_GEN_2x2, 20Gbps */
 
 #define USBDEVFS_CONTROL           _IOWR('U', 0, struct usbdevfs_ctrltransfer)
 #define USBDEVFS_CONTROL32           _IOWR('U', 0, struct usbdevfs_ctrltransfer32)
-- 
2.25.1

Re: [PATCH v2] USB: Support 20Gbps speed for ioctl USBDEVFS_GET_SPEED

[Alan Stern]

On Sat, Aug 19, 2023 at 01:46:55PM +0800, Dingyan Li wrote:
> Currently ioctl USBDEVFS_GET_SPEED can only return
> USB_SPEED_SUPER_PLUS at most. However, there are also
> ssp rates to indicate different connection speeds, which
> we can not tell further via USBDEVFS_GET_SPEED.
> 
> To fix it, this patch still uses USB_SPEED_SUPER_PLUS
> to indicate USB_SSP_GEN_UNKNOWN, USB_SSP_GEN_2x1, and
> USB_SSP_GEN_1x2. But need to #define a new value for
> USB_SSP_GEN_2x2. Besides, move the definition of enum
> usb_ssp_rate from include/linux/usb/ch9.h to
> include/uapi/linux/usb/ch9.h, which is a better place
> to hold it.
> 
> Signed-off-by: Dingyan Li <18500469033@163.com>

I'm not going to ACK this.  It's clumsy -- having two separate 
enumerations for USB device speeds just looks ridiculous.

We should fix the whole situation once and for all, recognizing that any 
code which depends on the speed needs to be upward compatible because 
new speeds and bus technologies may be added at any time.

> --- a/include/uapi/linux/usb/ch9.h
> +++ b/include/uapi/linux/usb/ch9.h
> @@ -1185,6 +1185,14 @@ enum usb_device_speed {
>  	USB_SPEED_SUPER_PLUS,			/* usb 3.1 */
>  };
>  
> +/* USB 3.2 SuperSpeed Plus phy signaling rate generation and lane count */
> +
> +enum usb_ssp_rate {
> +	USB_SSP_GEN_UNKNOWN = 0,
> +	USB_SSP_GEN_2x1,
> +	USB_SSP_GEN_1x2,
> +	USB_SSP_GEN_2x2,
> +};

This would make more sense if you kept very clear the distinction 
between the overall speed and the physical communication mechanism.  In 
other words, 10000 bps is 10000 bps, no matter whether the underlying 
technology uses one lane carrying 10000 bits per second or two lanes 
each carrying 5000 bits per second.

I'm not sure if anything in the kernel or userspace really cares about 
the number of lanes, as opposed to the total speed.  If it turns out 
that nothing does, the usb_ssp_rate enumeration could be removed.  
Besides, it should named something else, like usb_ssp_gen or 
usb_sp_generation, since it isn't just a rate designation.  (Whereas as 
enum usb_device_speed _is_ just a rate designation.)

Regardless of what happens to usb_ssp_rate, usb_device_speed should be 
enlarged to encompass all possible existing speeds.  That would 
immediately fix the ioctl problem.  Doing this in an upward-compatible 
way might end up being a little awkward but it ought to be possible.

Alan Stern

Re:Re: [PATCH v2] USB: Support 20Gbps speed for ioctl USBDEVFS_GET_SPEED

[Dingyan Li]

At 2023-08-20 03:03:05, "Alan Stern" <stern@rowland.harvard.edu> wrote:
>
>This would make more sense if you kept very clear the distinction 
>between the overall speed and the physical communication mechanism.  In 
>other words, 10000 bps is 10000 bps, no matter whether the underlying 
>technology uses one lane carrying 10000 bits per second or two lanes 
>each carrying 5000 bits per second.
>
>I'm not sure if anything in the kernel or userspace really cares about 
>the number of lanes, as opposed to the total speed.  If it turns out 
>that nothing does, the usb_ssp_rate enumeration could be removed.  
>Besides, it should named something else, like usb_ssp_gen or 
>usb_sp_generation, since it isn't just a rate designation.  (Whereas as 
>enum usb_device_speed _is_ just a rate designation.)

It seems that dwc3 code has a slightly different behaviors between
GEN_1x2 and GEN_2x1, so it's better to keep it. But I agree with you.
In enum usb_device_speed, we only care about overall speed instead of
physical links. And we could rename usb_ssp_rate to a more proper name.

>Regardless of what happens to usb_ssp_rate, usb_device_speed should be 
>enlarged to encompass all possible existing speeds.  That would 
>immediately fix the ioctl problem.  Doing this in an upward-compatible 
>way might end up being a little awkward but it ought to be possible.

Thanks for the detailed explanation, which makes things more clear.
I'll take your suggestions and try again.

Regards,
Dingyan

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

[Alan Stern]

On Sat, Aug 19, 2023 at 12:32:43PM +0800, Dingyan Li wrote:
> 
> At 2023-08-04 22:55:49, "Alan Stern" <stern@rowland.harvard.edu> wrote:
> 
> >> Another issue is that USB_SPEED_SUPER_PLUS has been widely used in
> >> so many files to execute conditional banches. Once we extend and store device's
> >> speed with new values in the first place, we might need to check all places where
> >> USB_SPEED_SUPER_PLUS is used in case of any regression.
> >
> >Certainly.  That's part of auditing all the current users of 
> >usb_device_speed.
> 
> This might not be a good idea and feels kind of drift away from what we originally
> want to do. Besides, suppose later new speed values are added, someone still has
> to revisit all the files to modify those checks. I think we should try to avoid this situation.

That's bad reasoning.  If you're worried that existing code will stop 
working right when a new speed designation is added then you _have_ to 
audit the code sooner or later.

> >> I think maybe we can try to remove the dependency on enum usb_device_speed
> >> in usbfs and define a separate set of speed values similar to previous design
> >> at https://www.spinics.net/lists/linux-usb/msg157709.html
> >> By this way, in usbfs we get more freedom to determine how to explain
> >> usb_device_speed and usb_ssp_rate, without the risk of breaking anything
> >> elsewhere.
> >> 
> >> For example, define an USBDEVFS_SPEED_SUPER_PLUS to indicate
> >> USB_SPEED_SUPER_PLUS with ssp rates GEN_UNKNOWN, GEN_2x1 and
> >> GEN_1x2. They all stand for 10Gbps and we don't need to tell one from
> >> another, similar to how it works in sysfs. Then define an
> >> USBDEVFS_SPEED_SUPER_PLUS_BY2(maybe there is a more proper name)
> >> to indicate USB_SPEED_SUPER_PLUS with ssp rate GEN_2x2, which stands
> >> for 20Gbps.
> >
> >You can't really do that.  The values returned by the USBDEVFS_GET_SPEED 
> >ioctl are the ones defined in include/uapi/linux/usb/ch9.h.  They are 
> >USB_SPEED_UNKNOWN, etc., not USBDEVFS_SPEED_UNKNOWN, etc.  The only way 
> >to extend them is by adding new entries to enum usb_device_speed.
> >
> >For example, you must not do anything that would break a user program 
> >which does something like this:
> >
> >#include <linux/usbdevfs.h>
> >#include <linux/usb/ch9.h>
> >
> >...
> >
> >	enum usb_device_speed s;
> >
> >	s = ioctl(fd, USBDEVFS_GET_SPEED);
> >	if (s == USB_SPEED_HIGH) ...
> >
> >Alan Stern
> 
> Since those speed definitions are just int values, we could manage to maintain the compatibility
> by keeping the same value.

No.  The values would be the same, but someone who tried to compile an 
old program under a new kernel would get an error because USB_SPEED_HIGH 
would be undefined.  The update would be binary-compatible but it 
wouldn't be source-compatible.

>  But anyway, my latest idea is not to extend enum usb_device_speed.
> There are three basic cases:
> 1) When speed is less than USB_SPEED_SUPER_PLUS, just return dev->speed;
> 2) When speed is USB_SPEED_SUPER_PLUS but ssp_rate is less than USB_SSP_GEN_2x2,
> return dev->speed;
> 3) When speed is USB_SPEED_SUPER_PLUS and ssp_rate is USB_SSP_GEN_2x2, a new
> speed for usbdevfs should be #defined and let's call it USBDEVFS_SPEED_SUPER_PLUS_BY2.
> This value won't be overlapped with any value in enum usb_device_speed, for example 7.
> In this case, return USBDEVFS_SPEED_SUPER_PLUS_BY2.
> 
> The code could be like:
> 
>         case USBDEVFS_GET_SPEED:
>                 ret = ps->dev->speed;
> +               if (ret == USB_SPEED_SUPER_PLUS &&
> +                               ps->dev->ssp_rate == USB_SSP_GEN_2x2)
> +                       ret = USBDEVFS_SPEED_SUPER_PLUS_BY2;
>                 break;
> 
> By this way, no need to add a new ioctl. No need to touch so many files.

If you do it my way (add new entries to enum usb_device_speed) then you 
wouldn't need a new ioctl.  And if one way requires touching a bunch of 
files, so does the other way.

>  And when new
> speeds are added later, just #define new values and extend the checks in above code.
> Compatibility is also maintained. Before this change, USBDEVFS_GET_SPEED could
> return 0-6. After this change, we can still return 0-6 for most of the cases, and 7 for
> GEN_2x2 devices.

The same would be true if you take my advice.

Alan Stern