| Message ID | 20230819054655.5495-1-18500469033@163.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] USB: Support 20Gbps speed for ioctl USBDEVFS_GET_SPEED | expand |
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
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
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)
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(-)