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?
> >> 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.
greg k-h
