| Message ID | 20230721084039.9728-1-18500469033@163.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | USB: add usbfs ioctl to get specific superspeedplus rates | expand |
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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 */
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(+)