Re: [PATCH 01/11] usb: ch9: Add sublink speed struct

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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@synopsys.com>
> >>>> ---
> >>>>    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