Greg Kroah-Hartman wrote:
> On Thu, Jul 16, 2020 at 02:58:36PM -0700, Thinh Nguyen wrote:
>> 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 <thinhn@xxxxxxxxxxxx>
>> ---
>> 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?
>
>> + 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.
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;
__le16 lsm;
} __attribute__((packed));
Let me know how you'd like to handle it.
Thanks,
Thinh
