Greg Kroah-Hartman wrote:
> On Fri, Jul 17, 2020 at 07:47:03AM +0000, Thinh Nguyen wrote:
>> 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?
> include/linux/usb/ch9.h perhaps?
Sure. I'll place it there.
>>>> 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
