Greg Kroah-Hartman wrote:
> On Fri, Jul 17, 2020 at 07:06:10AM +0000, Thinh Nguyen wrote:
>> 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.
> 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?
>
>> 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.
Thanks,
Thinh
