Greg Kroah-Hartman wrote:

On Fri, Jul 17, 2020 at 07:47:03AM +0000, Thinh Nguyen wrote:

quoted

Greg Kroah-Hartman wrote:

quoted

On Fri, Jul 17, 2020 at 07:06:10AM +0000, Thinh Nguyen wrote:

quoted

Greg Kroah-Hartman wrote:

quoted

On Thu, Jul 16, 2020 at 02:58:36PM -0700, Thinh Nguyen wrote:

quoted

USB 3.2 specification supports dual-lane for super-speed-plus. USB
devices may operate at different sublink speeds. To avoid using magic
numbers and capture the sublink speed better, introduce the
usb_sublink_speed structure and various sublink speed attribute enum.

See SSP BOS descriptor in USB 3.2 specification section 9.6.2.5

Signed-off-by: Thinh Nguyen <redacted>
---
    include/uapi/linux/usb/ch9.h | 42 ++++++++++++++++++++++++++++++++++++++++++
    1 file changed, 42 insertions(+)

diff --git a/include/uapi/linux/usb/ch9.h b/include/uapi/linux/usb/ch9.h
index 2b623f36af6b..d4fd403a3664 100644
--- a/include/uapi/linux/usb/ch9.h
+++ b/include/uapi/linux/usb/ch9.h

@@ -1145,6 +1145,48 @@ enum usb_device_speed {
    	USB_SPEED_SUPER_PLUS,			/* usb 3.1 */
    };
    
+/* USB 3.2 sublink speed attributes */
+
+enum usb_lane_speed_exponent {
+	USB_LSE_BPS = 0,
+	USB_LSE_KBPS = 1,
+	USB_LSE_MBPS = 2,
+	USB_LSE_GBPS = 3,
+};
+
+enum usb_sublink_type {
+	USB_ST_SYMMETRIC_RX = 0,
+	USB_ST_ASYMMETRIC_RX = 1,
+	USB_ST_SYMMETRIC_TX = 2,
+	USB_ST_ASYMMETRIC_TX = 3,
+};
+
+enum usb_link_protocol {
+	USB_LP_SS = 0,
+	USB_LP_SSP = 1,
+};
+
+/**
+ * struct usb_sublink_speed - sublink speed attribute
+ * @id: sublink speed attribute ID (SSID)
+ * @mantissa: lane speed mantissa
+ * @exponent: lane speed exponent
+ * @sublink type: sublink type
+ * @protocol: sublink protocol
+ *
+ * Super-speed-plus supports multiple lanes. Use the sublink speed attributes to
+ * describe the sublink speed.
+ *
+ * See USB 3.2 spec section 9.6.2.6 for super-speed-plus capability for more
+ * information.
+ */
+struct usb_sublink_speed {
+	u8				id;
+	u16				mantissa;

You have to use the proper data types for crossing the user/kernel
boundry here.  That would be __u8 and __u16, right?

quoted

+	enum usb_lane_speed_exponent	exponent;
+	enum usb_sublink_type		type;
+	enum usb_link_protocol		protocol;

Are you _sure_ that an enum is the correct size for these fields?  How
can you guarantee this?  We do not use enums in this way for any other
field in this file for a reason...

And did you look at the layout of this structure to verify it actually
matches what is on the wire with USB?  I think you need to add a packed
attribute to guarantee it.

This struct is not intended to be packed to be sent over the bus. It's a
simple struct for host and gadget driver use only. I intended to use
enum to make it more clear not to be used that way. From your question,
it's obviously not clear.

Then why are you putting it in this file?  This file is only for things
that are described in the USB spec that need to cross the user/kernel
boundry.

Ok, it seemed to fit here. I'll place it under /include/linux/usb.h then?

include/linux/usb/ch9.h perhaps?

Sure. I'll place it there.

quoted

quoted

quoted

Otherwise, it may look something like this:
struct usb_sublink_speed {
           __u8    ssid:4;
           __u8    lse:2;
           __u8    st:2;
           __u8    rsvd:6;
           __u8    lp:2;

Are you sure those bit fields will work on big-endian systems?

No. Because of the way the bitfields are placed, it's a path to
unnecessary headache/bugs with boundary and endianness checks. That's
why I decided to go with the other solution.

That's good, but again, this is a uapi file, not a "normal" include file
in the kernel, you have to be careful about this.

Will do. Thanks for the quick responses.

BR,
Thinh