| Message ID | 90961eec3e1ed3c27e41e7169353cc295c551604.1594935978.git.thinhn@synopsys.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: Handle different sublink speeds | expand |
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 >
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. 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; __le16 lsm; } __attribute__((packed)); Let me know how you'd like to handle it. Thanks, Thinh
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. > 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? > __le16 lsm; > } __attribute__((packed)); Do you need to read this from a device? thanks, greg k-h
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? > >> 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
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
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@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? 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
Hi Thinh,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on balbi-usb/testing/next]
[also build test ERROR on usb/usb-testing peter.chen-usb/ci-for-usb-next linus/master v5.8-rc5 next-20200716]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Thinh-Nguyen/usb-Handle-different-sublink-speeds/20200717-060144
base: https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git testing/next
config: x86_64-randconfig-a005-20200717 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
reproduce (this is a W=1 build):
# save the attached .config to linux build tree
make W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from ./usr/include/linux/usb/g_uvc.h:13,
from <command-line>:32:
>> ./usr/include/linux/usb/ch9.h:1184:2: error: unknown type name 'u8'
1184 | u8 id;
| ^~
>> ./usr/include/linux/usb/ch9.h:1185:2: error: unknown type name 'u16'
1185 | u16 mantissa;
| ^~~
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hello! On 17.07.2020 0:58, 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 It's called just 'type' below. > + * @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; > + enum usb_lane_speed_exponent exponent; > + enum usb_sublink_type type; > + enum usb_link_protocol protocol; > +}; > > enum usb_device_state { > /* NOTATTACHED isn't in the USB spec, and this state acts MBR, Sergei
Sergei Shtylyov wrote: > Hello! > > On 17.07.2020 0:58, 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 > > It's called just 'type' below. > Will fix. Thanks, Thinh
Hi Thinh,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on balbi-usb/testing/next]
[also build test ERROR on usb/usb-testing peter.chen-usb/ci-for-usb-next linus/master v5.8-rc5 next-20200717]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Thinh-Nguyen/usb-Handle-different-sublink-speeds/20200717-060144
base: https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git testing/next
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project ed6b578040a85977026c93bf4188f996148f3218)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from <built-in>:1:
>> ./usr/include/linux/usb/ch9.h:1184:2: error: unknown type name 'u8'
u8 id;
^
>> ./usr/include/linux/usb/ch9.h:1185:2: error: unknown type name 'u16'
u16 mantissa;
^
2 errors generated.
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
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; + enum usb_lane_speed_exponent exponent; + enum usb_sublink_type type; + enum usb_link_protocol protocol; +}; enum usb_device_state { /* NOTATTACHED isn't in the USB spec, and this state acts
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(+)