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

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

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.

thanks,

greg k-h


> +};
>  
>  enum usb_device_state {
>  	/* NOTATTACHED isn't in the USB spec, and this state acts
> -- 
> 2.11.0
>