| Message ID | 20230912104455.7737-2-quic_linyyuan@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: gadget: reduce usb gadget trace event buffer usage | expand |
On Tue, Sep 12, 2023 at 06:44:46PM +0800, Linyu Yuan wrote: > Some UDC trace event will save usb gadget information, but it use one int > size buffer to save one bit information of usb gadget, so 19 int buffers > needed to save 19 bit fields which is not good. > > Add one anonymous union which have one u32 member 'dw1' to struct > 'usb_gadget', it inlclude all 19 bits and can be used by trace event > during fast assign stage to save more entries with same trace ring buffer > size. > > Also move all original 19 bit fields into one anonymous struct which > inside struct 'usb_gadget'. > > In order to allow trace event output stage access the bit from saved u32 > 'dw1', add following macro, > define USB_GADGET_BITFIELD(n, name) \ > ({\ > union {\ > struct {\ > u32 sg_supported:1;\ > u32 is_otg:1;\ > u32 is_a_peripheral:1;\ > u32 b_hnp_enable:1;\ > u32 a_hnp_support:1;\ > u32 a_alt_hnp_support:1;\ > u32 hnp_polling_support:1;\ > u32 host_request_flag:1;\ > u32 quirk_ep_out_aligned_size:1;\ > u32 quirk_altset_not_supp:1;\ > u32 quirk_stall_not_supp:1;\ > u32 quirk_zlp_not_supp:1;\ > u32 quirk_avoids_skb_reserve:1;\ > u32 is_selfpowered:1;\ > u32 deactivated:1;\ > u32 connected:1;\ > u32 lpm_capable:1;\ > u32 wakeup_capable:1;\ > u32 wakeup_armed:1;\ > } __packed;\ > u32 dw1;\ > } __aligned(4) __g_u_##name;\ > u32 __g_##name; \ > BUILD_BUG_ON(sizeof(__g_u_##name) != 4);\ > __g_u_##name.dw1 = (n); __g_##name = __g_u_##name.name;\ > __g_##name; }) > > define USB_GADGET_SG_SUPPORTED(n) USB_GADGET_BITFIELD((n), sg_supported) > ... > change to use this kind of macro for all related trace files later. I'm sorry, but that's horrible, and is NOT how you deal with bitfields in an endian-neutral way at all. There are much simpler, and easier, ways to do this properly. But I'm still missing the huge _WHY_ any of this is needed. You are not showing any real advantage at all that I have noticed. You need to step back and see if any of this is even anything that needs to change, and if you feel it does need to change, you need to be able to properly justify _why_ it needs to change. good luck! greg k-h
On Tue, Sep 12, 2023 at 06:44:46PM +0800, Linyu Yuan wrote: > Some UDC trace event will save usb gadget information, but it use one int > size buffer to save one bit information of usb gadget, so 19 int buffers > needed to save 19 bit fields which is not good. > > Add one anonymous union which have one u32 member 'dw1' to struct > 'usb_gadget', it inlclude all 19 bits and can be used by trace event > during fast assign stage to save more entries with same trace ring buffer > size. > > Also move all original 19 bit fields into one anonymous struct which > inside struct 'usb_gadget'. > > In order to allow trace event output stage access the bit from saved u32 > 'dw1', add following macro, > define USB_GADGET_BITFIELD(n, name) \ > ({\ > union {\ > struct {\ > u32 sg_supported:1;\ > u32 is_otg:1;\ > u32 is_a_peripheral:1;\ > u32 b_hnp_enable:1;\ > u32 a_hnp_support:1;\ > u32 a_alt_hnp_support:1;\ > u32 hnp_polling_support:1;\ > u32 host_request_flag:1;\ > u32 quirk_ep_out_aligned_size:1;\ > u32 quirk_altset_not_supp:1;\ > u32 quirk_stall_not_supp:1;\ > u32 quirk_zlp_not_supp:1;\ > u32 quirk_avoids_skb_reserve:1;\ > u32 is_selfpowered:1;\ > u32 deactivated:1;\ > u32 connected:1;\ > u32 lpm_capable:1;\ > u32 wakeup_capable:1;\ > u32 wakeup_armed:1;\ > } __packed;\ > u32 dw1;\ > } __aligned(4) __g_u_##name;\ > u32 __g_##name; \ > BUILD_BUG_ON(sizeof(__g_u_##name) != 4);\ > __g_u_##name.dw1 = (n); __g_##name = __g_u_##name.name;\ > __g_##name; }) > > define USB_GADGET_SG_SUPPORTED(n) USB_GADGET_BITFIELD((n), sg_supported) > ... > change to use this kind of macro for all related trace files later. This macro usage is a real mess. Can't you find a better way to do it? For instance, in the code that parses the trace buffer, define a temporary usb_gadget structure and copy the dw1 field from the trace buffer to the temporary structure. Then you can access the fields in that structure directly by their original names, with no macros. Alan Stern
On 9/12/2023 7:09 PM, Greg Kroah-Hartman wrote: > On Tue, Sep 12, 2023 at 06:44:46PM +0800, Linyu Yuan wrote: >> Some UDC trace event will save usb gadget information, but it use one int >> size buffer to save one bit information of usb gadget, so 19 int buffers >> needed to save 19 bit fields which is not good. >> >> Add one anonymous union which have one u32 member 'dw1' to struct >> 'usb_gadget', it inlclude all 19 bits and can be used by trace event >> during fast assign stage to save more entries with same trace ring buffer >> size. >> >> Also move all original 19 bit fields into one anonymous struct which >> inside struct 'usb_gadget'. >> >> In order to allow trace event output stage access the bit from saved u32 >> 'dw1', add following macro, >> define USB_GADGET_BITFIELD(n, name) \ >> ({\ >> union {\ >> struct {\ >> u32 sg_supported:1;\ >> u32 is_otg:1;\ >> u32 is_a_peripheral:1;\ >> u32 b_hnp_enable:1;\ >> u32 a_hnp_support:1;\ >> u32 a_alt_hnp_support:1;\ >> u32 hnp_polling_support:1;\ >> u32 host_request_flag:1;\ >> u32 quirk_ep_out_aligned_size:1;\ >> u32 quirk_altset_not_supp:1;\ >> u32 quirk_stall_not_supp:1;\ >> u32 quirk_zlp_not_supp:1;\ >> u32 quirk_avoids_skb_reserve:1;\ >> u32 is_selfpowered:1;\ >> u32 deactivated:1;\ >> u32 connected:1;\ >> u32 lpm_capable:1;\ >> u32 wakeup_capable:1;\ >> u32 wakeup_armed:1;\ >> } __packed;\ >> u32 dw1;\ >> } __aligned(4) __g_u_##name;\ >> u32 __g_##name; \ >> BUILD_BUG_ON(sizeof(__g_u_##name) != 4);\ >> __g_u_##name.dw1 = (n); __g_##name = __g_u_##name.name;\ >> __g_##name; }) >> >> define USB_GADGET_SG_SUPPORTED(n) USB_GADGET_BITFIELD((n), sg_supported) >> ... >> change to use this kind of macro for all related trace files later. > I'm sorry, but that's horrible, and is NOT how you deal with bitfields > in an endian-neutral way at all. > > There are much simpler, and easier, ways to do this properly. do you mean define two sets of ordering bit field according BIG and LITTLE ENDIAN like below ? #if defined(__LITTLE_ENDIAN_BITFIELD) struct { u32 sg_supported:1; u32 is_otg:1; .. u32 :13; } __packed; #else struct { u32 :13; .. u32 is_otg:1; u32 sg_supported:1; } __packed; #endif but Alan Stern have one comment, do it mean the bit position number is not expect and we can't use it ? @Alan Stern , BIT(0), BIT(1) is not the member we expect ? " As far as I know, the C compiler does not specify that bit fields in packed structures will be assigned starting from the low-order bit position " > > But I'm still missing the huge _WHY_ any of this is needed. You are not > showing any real advantage at all that I have noticed. > > You need to step back and see if any of this is even anything that needs > to change, and if you feel it does need to change, you need to be able > to properly justify _why_ it needs to change. indeed this is a small optimization. I think i explain the benefit in previous version, when user not increase system trace event buffer space, or in lower system trace event buffer space, it allow more trace event entries to be saved. in normal condition, the usb request is most frequent things after enumeration with useful operation, so take below trace event class for explanation, DECLARE_EVENT_CLASS(udc_log_req, TP_PROTO(struct usb_ep *ep, struct usb_request *req, int ret), TP_ARGS(ep, req, ret), TP_STRUCT__entry( __string(name, ep->name) __field(unsigned, length) __field(unsigned, actual) __field(unsigned, num_sgs) __field(unsigned, num_mapped_sgs) __field(unsigned, stream_id) __field(unsigned, no_interrupt) __field(unsigned, zero) __field(unsigned, short_not_ok) __field(int, status) __field(int, ret) __field(struct usb_request *, req) ), TP_fast_assign( __assign_str(name, ep->name); __entry->length = req->length; __entry->actual = req->actual; __entry->num_sgs = req->num_sgs; __entry->num_mapped_sgs = req->num_mapped_sgs; __entry->stream_id = req->stream_id; __entry->no_interrupt = req->no_interrupt; __entry->zero = req->zero; __entry->short_not_ok = req->short_not_ok; __entry->status = req->status; __entry->ret = ret; __entry->req = req; ), TP_printk("%s: req %p length %d/%d sgs %d/%d stream %d %s%s%s status %d --> %d", __get_str(name),__entry->req, __entry->actual, __entry->length, __entry->num_mapped_sgs, __entry->num_sgs, __entry->stream_id, __entry->zero ? "Z" : "z", __entry->short_not_ok ? "S" : "s", __entry->no_interrupt ? "i" : "I", __entry->status, __entry->ret ) ); consider 32 bit ARCH, without change, one trace entry size is: 4 (ring buffer event header ) + 8 (trace event header ) + 48 (trace class header) + 9 (ep string name) = 69 bytes. with change, 4 (ring buffer event header ) + 8 (trace event header ) + 36 (trace class header) = 48 bytes. consider there is 1MB trace buffer space, without change, it can save 15196 entries, with change, it can save 21845 entries. > > good luck! > > greg k-h
On 9/12/2023 11:32 PM, Alan Stern wrote: > On Tue, Sep 12, 2023 at 06:44:46PM +0800, Linyu Yuan wrote: >> Some UDC trace event will save usb gadget information, but it use one int >> size buffer to save one bit information of usb gadget, so 19 int buffers >> needed to save 19 bit fields which is not good. >> >> Add one anonymous union which have one u32 member 'dw1' to struct >> 'usb_gadget', it inlclude all 19 bits and can be used by trace event >> during fast assign stage to save more entries with same trace ring buffer >> size. >> >> Also move all original 19 bit fields into one anonymous struct which >> inside struct 'usb_gadget'. >> >> In order to allow trace event output stage access the bit from saved u32 >> 'dw1', add following macro, >> define USB_GADGET_BITFIELD(n, name) \ >> ({\ >> union {\ >> struct {\ >> u32 sg_supported:1;\ >> u32 is_otg:1;\ >> u32 is_a_peripheral:1;\ >> u32 b_hnp_enable:1;\ >> u32 a_hnp_support:1;\ >> u32 a_alt_hnp_support:1;\ >> u32 hnp_polling_support:1;\ >> u32 host_request_flag:1;\ >> u32 quirk_ep_out_aligned_size:1;\ >> u32 quirk_altset_not_supp:1;\ >> u32 quirk_stall_not_supp:1;\ >> u32 quirk_zlp_not_supp:1;\ >> u32 quirk_avoids_skb_reserve:1;\ >> u32 is_selfpowered:1;\ >> u32 deactivated:1;\ >> u32 connected:1;\ >> u32 lpm_capable:1;\ >> u32 wakeup_capable:1;\ >> u32 wakeup_armed:1;\ >> } __packed;\ >> u32 dw1;\ >> } __aligned(4) __g_u_##name;\ >> u32 __g_##name; \ >> BUILD_BUG_ON(sizeof(__g_u_##name) != 4);\ >> __g_u_##name.dw1 = (n); __g_##name = __g_u_##name.name;\ >> __g_##name; }) >> >> define USB_GADGET_SG_SUPPORTED(n) USB_GADGET_BITFIELD((n), sg_supported) >> ... >> change to use this kind of macro for all related trace files later. > This macro usage is a real mess. Can't you find a better way to do it? > > For instance, in the code that parses the trace buffer, define a > temporary usb_gadget structure and copy the dw1 field from the trace > buffer to the temporary structure. Then you can access the fields in > that structure directly by their original names, with no macros. do it same idea just move it outside of gadget.h ? move other place can't be used for all possible trace files. > > Alan Stern
On Wed, Sep 13, 2023 at 11:46:12AM +0800, Linyu Yuan wrote: > but Alan Stern have one comment, do it mean the bit position number is not > expect and we can't use it ? > > @Alan Stern , BIT(0), BIT(1) is not the member we expect ? They might not be. If you can avoid making this assumption, you should. > > This macro usage is a real mess. Can't you find a better way to do it? > > > > For instance, in the code that parses the trace buffer, define a > > temporary usb_gadget structure and copy the dw1 field from the trace > > buffer to the temporary structure. Then you can access the fields in > > that structure directly by their original names, with no macros. > > do it same idea just move it outside of gadget.h ? Keep the anonymous union in gadget.h, but get rid of the macros. Alan Stern
On 9/14/2023 12:02 AM, Alan Stern wrote: > On Wed, Sep 13, 2023 at 11:46:12AM +0800, Linyu Yuan wrote: >> but Alan Stern have one comment, do it mean the bit position number is not >> expect and we can't use it ? >> >> @Alan Stern , BIT(0), BIT(1) is not the member we expect ? > They might not be. If you can avoid making this assumption, you should. i don't know if it is true or not, seem some driver expect there is no hole for this kind of bit field definition. > >>> This macro usage is a real mess. Can't you find a better way to do it? >>> >>> For instance, in the code that parses the trace buffer, define a >>> temporary usb_gadget structure and copy the dw1 field from the trace >>> buffer to the temporary structure. Then you can access the fields in >>> that structure directly by their original names, with no macros. >> do it same idea just move it outside of gadget.h ? > Keep the anonymous union in gadget.h, but get rid of the macros. do you expect below ? --- a/include/linux/usb/gadget.h +++ b/include/linux/usb/gadget.h @@ -357,6 +357,7 @@ struct usb_gadget_ops { * @in_epnum: last used in ep number * @mA: last set mA value * @otg_caps: OTG capabilities of this gadget. + * @dw1: trace event purpose * @sg_supported: true if we can handle scatter-gather * @is_otg: True if the USB device port uses a Mini-AB jack, so that the * gadget driver must provide a USB OTG descriptor. @@ -432,30 +433,88 @@ struct usb_gadget { unsigned mA; struct usb_otg_caps *otg_caps; - unsigned sg_supported:1; - unsigned is_otg:1; - unsigned is_a_peripheral:1; - unsigned b_hnp_enable:1; - unsigned a_hnp_support:1; - unsigned a_alt_hnp_support:1; - unsigned hnp_polling_support:1; - unsigned host_request_flag:1; - unsigned quirk_ep_out_aligned_size:1; - unsigned quirk_altset_not_supp:1; - unsigned quirk_stall_not_supp:1; - unsigned quirk_zlp_not_supp:1; - unsigned quirk_avoids_skb_reserve:1; - unsigned is_selfpowered:1; - unsigned deactivated:1; - unsigned connected:1; - unsigned lpm_capable:1; - unsigned wakeup_capable:1; - unsigned wakeup_armed:1; + union { + struct { + unsigned sg_supported:1; + unsigned is_otg:1; + unsigned is_a_peripheral:1; + unsigned b_hnp_enable:1; + unsigned a_hnp_support:1; + unsigned a_alt_hnp_support:1; + unsigned hnp_polling_support:1; + unsigned host_request_flag:1; + unsigned quirk_ep_out_aligned_size:1; + unsigned quirk_altset_not_supp:1; + unsigned quirk_stall_not_supp:1; + unsigned quirk_zlp_not_supp:1; + unsigned quirk_avoids_skb_reserve:1; + unsigned is_selfpowered:1; + unsigned deactivated:1; + unsigned connected:1; + unsigned lpm_capable:1; + unsigned wakeup_capable:1; + unsigned wakeup_armed:1; + } __packed; + + u32 dw1; + } __aligned(4); int irq; int id_number; }; #define work_to_gadget(w) (container_of((w), struct usb_gadget, work)) +#define USB_GADGET_BITFIELD(field) \ +static inline u32 usb_gadget_bit_##field(u32 dw1) \ +{ \ + union { \ + struct { \ + u32 sg_supported:1; \ + u32 is_otg:1; \ + u32 is_a_peripheral:1; \ + u32 b_hnp_enable:1; \ + u32 a_hnp_support:1; \ + u32 a_alt_hnp_support:1; \ + u32 hnp_polling_support:1; \ + u32 host_request_flag:1; \ + u32 quirk_ep_out_aligned_size:1; \ + u32 quirk_altset_not_supp:1; \ + u32 quirk_stall_not_supp:1; \ + u32 quirk_zlp_not_supp:1; \ + u32 quirk_avoids_skb_reserve:1; \ + u32 is_selfpowered:1; \ + u32 deactivated:1; \ + u32 connected:1; \ + u32 lpm_capable:1; \ + u32 wakeup_capable:1; \ + u32 wakeup_armed:1; \ + } __packed; \ + u32 dw1; \ + } __aligned(4) u; \ + BUILD_BUG_ON(sizeof(u) != 4); \ + u.dw1 = dw1; \ + return u.field; \ +} + +USB_GADGET_BITFIELD(sg_supported) +USB_GADGET_BITFIELD(is_otg) +USB_GADGET_BITFIELD(is_a_peripheral) +USB_GADGET_BITFIELD(b_hnp_enable) +USB_GADGET_BITFIELD(a_hnp_support) +USB_GADGET_BITFIELD(a_alt_hnp_support) +USB_GADGET_BITFIELD(hnp_polling_support) +USB_GADGET_BITFIELD(host_request_flag) +USB_GADGET_BITFIELD(quirk_ep_out_aligned_size) +USB_GADGET_BITFIELD(quirk_altset_not_supp) +USB_GADGET_BITFIELD(quirk_stall_not_supp) +USB_GADGET_BITFIELD(quirk_zlp_not_supp) +USB_GADGET_BITFIELD(quirk_avoids_skb_reserve) +USB_GADGET_BITFIELD(is_selfpowered) +USB_GADGET_BITFIELD(deactivated) +USB_GADGET_BITFIELD(connected) +USB_GADGET_BITFIELD(lpm_capable) +USB_GADGET_BITFIELD(wakeup_capable) +USB_GADGET_BITFIELD(wakeup_armed) + > > Alan Stern
On Thu, Sep 14, 2023 at 09:08:04AM +0800, Linyu Yuan wrote: > > On 9/14/2023 12:02 AM, Alan Stern wrote: > > On Wed, Sep 13, 2023 at 11:46:12AM +0800, Linyu Yuan wrote: > > > but Alan Stern have one comment, do it mean the bit position number is not > > > expect and we can't use it ? > > > > > > @Alan Stern , BIT(0), BIT(1) is not the member we expect ? > > They might not be. If you can avoid making this assumption, you should. > > > i don't know if it is true or not, seem some driver expect there is no hole > for this kind of bit field definition. I didn't say there would be a hole; I said that BIT(0) might not be the member you expect. For example, sg_supported might be BIT(31) instead of BIT(0). > > > > This macro usage is a real mess. Can't you find a better way to do it? > > > > > > > > For instance, in the code that parses the trace buffer, define a > > > > temporary usb_gadget structure and copy the dw1 field from the trace > > > > buffer to the temporary structure. Then you can access the fields in > > > > that structure directly by their original names, with no macros. > > > do it same idea just move it outside of gadget.h ? > > Keep the anonymous union in gadget.h, but get rid of the macros. > > > do you expect below ? > > > --- a/include/linux/usb/gadget.h > +++ b/include/linux/usb/gadget.h > @@ -357,6 +357,7 @@ struct usb_gadget_ops { > * @in_epnum: last used in ep number > * @mA: last set mA value > * @otg_caps: OTG capabilities of this gadget. > + * @dw1: trace event purpose > * @sg_supported: true if we can handle scatter-gather > * @is_otg: True if the USB device port uses a Mini-AB jack, so that the > * gadget driver must provide a USB OTG descriptor. > @@ -432,30 +433,88 @@ struct usb_gadget { > unsigned mA; > struct usb_otg_caps *otg_caps; > > - unsigned sg_supported:1; > - unsigned is_otg:1; > - unsigned is_a_peripheral:1; > - unsigned b_hnp_enable:1; > - unsigned a_hnp_support:1; > - unsigned a_alt_hnp_support:1; > - unsigned hnp_polling_support:1; > - unsigned host_request_flag:1; > - unsigned quirk_ep_out_aligned_size:1; > - unsigned quirk_altset_not_supp:1; > - unsigned quirk_stall_not_supp:1; > - unsigned quirk_zlp_not_supp:1; > - unsigned quirk_avoids_skb_reserve:1; > - unsigned is_selfpowered:1; > - unsigned deactivated:1; > - unsigned connected:1; > - unsigned lpm_capable:1; > - unsigned wakeup_capable:1; > - unsigned wakeup_armed:1; > + union { > + struct { > + unsigned sg_supported:1; > + unsigned is_otg:1; > + unsigned is_a_peripheral:1; > + unsigned b_hnp_enable:1; > + unsigned a_hnp_support:1; > + unsigned a_alt_hnp_support:1; > + unsigned hnp_polling_support:1; > + unsigned host_request_flag:1; > + unsigned quirk_ep_out_aligned_size:1; > + unsigned quirk_altset_not_supp:1; > + unsigned quirk_stall_not_supp:1; > + unsigned quirk_zlp_not_supp:1; > + unsigned quirk_avoids_skb_reserve:1; > + unsigned is_selfpowered:1; > + unsigned deactivated:1; > + unsigned connected:1; > + unsigned lpm_capable:1; > + unsigned wakeup_capable:1; > + unsigned wakeup_armed:1; > + } __packed; > + > + u32 dw1; > + } __aligned(4); > int irq; > int id_number; > }; > #define work_to_gadget(w) (container_of((w), struct usb_gadget, work)) Stop here. The above is what I expect. Don't include any of the material below. (BTW, you don't need the __aligned(4) thing, do you? Since the union includes a 32-bit integer field, it will naturally be aligned on a 4-byte boundary.) > +#define USB_GADGET_BITFIELD(field) \ > +static inline u32 usb_gadget_bit_##field(u32 dw1) \ > +{ \ > + union { \ > + struct { \ > + u32 sg_supported:1; \ > + u32 is_otg:1; \ > + u32 is_a_peripheral:1; \ > + u32 b_hnp_enable:1; \ > + u32 a_hnp_support:1; \ > + u32 a_alt_hnp_support:1; \ > + u32 hnp_polling_support:1; \ > + u32 host_request_flag:1; \ > + u32 quirk_ep_out_aligned_size:1; \ > + u32 quirk_altset_not_supp:1; \ > + u32 quirk_stall_not_supp:1; \ > + u32 quirk_zlp_not_supp:1; \ > + u32 quirk_avoids_skb_reserve:1; \ > + u32 is_selfpowered:1; \ > + u32 deactivated:1; \ > + u32 connected:1; \ > + u32 lpm_capable:1; \ > + u32 wakeup_capable:1; \ > + u32 wakeup_armed:1; \ > + } __packed; \ > + u32 dw1; \ > + } __aligned(4) u; \ > + BUILD_BUG_ON(sizeof(u) != 4); \ > + u.dw1 = dw1; \ > + return u.field; \ > +} > + > +USB_GADGET_BITFIELD(sg_supported) > +USB_GADGET_BITFIELD(is_otg) > +USB_GADGET_BITFIELD(is_a_peripheral) > +USB_GADGET_BITFIELD(b_hnp_enable) > +USB_GADGET_BITFIELD(a_hnp_support) > +USB_GADGET_BITFIELD(a_alt_hnp_support) > +USB_GADGET_BITFIELD(hnp_polling_support) > +USB_GADGET_BITFIELD(host_request_flag) > +USB_GADGET_BITFIELD(quirk_ep_out_aligned_size) > +USB_GADGET_BITFIELD(quirk_altset_not_supp) > +USB_GADGET_BITFIELD(quirk_stall_not_supp) > +USB_GADGET_BITFIELD(quirk_zlp_not_supp) > +USB_GADGET_BITFIELD(quirk_avoids_skb_reserve) > +USB_GADGET_BITFIELD(is_selfpowered) > +USB_GADGET_BITFIELD(deactivated) > +USB_GADGET_BITFIELD(connected) > +USB_GADGET_BITFIELD(lpm_capable) > +USB_GADGET_BITFIELD(wakeup_capable) > +USB_GADGET_BITFIELD(wakeup_armed) So ignore all of that. Now in your patch 4/10, do something that will have this effect: + struct usb_gadget g; + + g.dw1 = __entry->dw1; + TP_printk(.... - __entry->sg_supported ? "sg:" : "", + g.sg_supported ? "sg:" : "", ... You probably can't do it exactly this way, because this won't work with the tracing macros, but maybe something that is equivalent will work. For example, you could try: +#define USB_GADGET_BITFIELD(field) \ + ({struct usb_gadget g; \ + g.dw1 = __entry->dw1; \ + g.field;}) TP_printk(.... - __entry->sg_supported ? "sg:" : "", + USB_GADGET_BITFIELD(sg_supported) ? "sg:" : "", Do you get the idea now? Alan Stern
On 9/14/2023 10:16 AM, Alan Stern wrote: > On Thu, Sep 14, 2023 at 09:08:04AM +0800, Linyu Yuan wrote: >> On 9/14/2023 12:02 AM, Alan Stern wrote: >>> On Wed, Sep 13, 2023 at 11:46:12AM +0800, Linyu Yuan wrote: >>>> but Alan Stern have one comment, do it mean the bit position number is not >>>> expect and we can't use it ? >>>> >>>> @Alan Stern , BIT(0), BIT(1) is not the member we expect ? >>> They might not be. If you can avoid making this assumption, you should. >> >> i don't know if it is true or not, seem some driver expect there is no hole >> for this kind of bit field definition. > I didn't say there would be a hole; I said that BIT(0) might not be the > member you expect. For example, sg_supported might be BIT(31) instead > of BIT(0). got it, it is same concern of Greg. > >>>>> This macro usage is a real mess. Can't you find a better way to do it? >>>>> >>>>> For instance, in the code that parses the trace buffer, define a >>>>> temporary usb_gadget structure and copy the dw1 field from the trace >>>>> buffer to the temporary structure. Then you can access the fields in >>>>> that structure directly by their original names, with no macros. >>>> do it same idea just move it outside of gadget.h ? >>> Keep the anonymous union in gadget.h, but get rid of the macros. >> >> do you expect below ? >> >> >> --- a/include/linux/usb/gadget.h >> +++ b/include/linux/usb/gadget.h >> @@ -357,6 +357,7 @@ struct usb_gadget_ops { >> * @in_epnum: last used in ep number >> * @mA: last set mA value >> * @otg_caps: OTG capabilities of this gadget. >> + * @dw1: trace event purpose >> * @sg_supported: true if we can handle scatter-gather >> * @is_otg: True if the USB device port uses a Mini-AB jack, so that the >> * gadget driver must provide a USB OTG descriptor. >> @@ -432,30 +433,88 @@ struct usb_gadget { >> unsigned mA; >> struct usb_otg_caps *otg_caps; >> >> - unsigned sg_supported:1; >> - unsigned is_otg:1; >> - unsigned is_a_peripheral:1; >> - unsigned b_hnp_enable:1; >> - unsigned a_hnp_support:1; >> - unsigned a_alt_hnp_support:1; >> - unsigned hnp_polling_support:1; >> - unsigned host_request_flag:1; >> - unsigned quirk_ep_out_aligned_size:1; >> - unsigned quirk_altset_not_supp:1; >> - unsigned quirk_stall_not_supp:1; >> - unsigned quirk_zlp_not_supp:1; >> - unsigned quirk_avoids_skb_reserve:1; >> - unsigned is_selfpowered:1; >> - unsigned deactivated:1; >> - unsigned connected:1; >> - unsigned lpm_capable:1; >> - unsigned wakeup_capable:1; >> - unsigned wakeup_armed:1; >> + union { >> + struct { >> + unsigned sg_supported:1; >> + unsigned is_otg:1; >> + unsigned is_a_peripheral:1; >> + unsigned b_hnp_enable:1; >> + unsigned a_hnp_support:1; >> + unsigned a_alt_hnp_support:1; >> + unsigned hnp_polling_support:1; >> + unsigned host_request_flag:1; >> + unsigned quirk_ep_out_aligned_size:1; >> + unsigned quirk_altset_not_supp:1; >> + unsigned quirk_stall_not_supp:1; >> + unsigned quirk_zlp_not_supp:1; >> + unsigned quirk_avoids_skb_reserve:1; >> + unsigned is_selfpowered:1; >> + unsigned deactivated:1; >> + unsigned connected:1; >> + unsigned lpm_capable:1; >> + unsigned wakeup_capable:1; >> + unsigned wakeup_armed:1; >> + } __packed; >> + >> + u32 dw1; >> + } __aligned(4); >> int irq; >> int id_number; >> }; >> #define work_to_gadget(w) (container_of((w), struct usb_gadget, work)) > Stop here. The above is what I expect. Don't include any of the > material below. > > (BTW, you don't need the __aligned(4) thing, do you? Since the union > includes a 32-bit integer field, it will naturally be aligned on a > 4-byte boundary.) sure, will remove it. > >> +#define USB_GADGET_BITFIELD(field) \ >> +static inline u32 usb_gadget_bit_##field(u32 dw1) \ >> +{ \ >> + union { \ >> + struct { \ >> + u32 sg_supported:1; \ >> + u32 is_otg:1; \ >> + u32 is_a_peripheral:1; \ >> + u32 b_hnp_enable:1; \ >> + u32 a_hnp_support:1; \ >> + u32 a_alt_hnp_support:1; \ >> + u32 hnp_polling_support:1; \ >> + u32 host_request_flag:1; \ >> + u32 quirk_ep_out_aligned_size:1; \ >> + u32 quirk_altset_not_supp:1; \ >> + u32 quirk_stall_not_supp:1; \ >> + u32 quirk_zlp_not_supp:1; \ >> + u32 quirk_avoids_skb_reserve:1; \ >> + u32 is_selfpowered:1; \ >> + u32 deactivated:1; \ >> + u32 connected:1; \ >> + u32 lpm_capable:1; \ >> + u32 wakeup_capable:1; \ >> + u32 wakeup_armed:1; \ >> + } __packed; \ >> + u32 dw1; \ >> + } __aligned(4) u; \ >> + BUILD_BUG_ON(sizeof(u) != 4); \ >> + u.dw1 = dw1; \ >> + return u.field; \ >> +} >> + >> +USB_GADGET_BITFIELD(sg_supported) >> +USB_GADGET_BITFIELD(is_otg) >> +USB_GADGET_BITFIELD(is_a_peripheral) >> +USB_GADGET_BITFIELD(b_hnp_enable) >> +USB_GADGET_BITFIELD(a_hnp_support) >> +USB_GADGET_BITFIELD(a_alt_hnp_support) >> +USB_GADGET_BITFIELD(hnp_polling_support) >> +USB_GADGET_BITFIELD(host_request_flag) >> +USB_GADGET_BITFIELD(quirk_ep_out_aligned_size) >> +USB_GADGET_BITFIELD(quirk_altset_not_supp) >> +USB_GADGET_BITFIELD(quirk_stall_not_supp) >> +USB_GADGET_BITFIELD(quirk_zlp_not_supp) >> +USB_GADGET_BITFIELD(quirk_avoids_skb_reserve) >> +USB_GADGET_BITFIELD(is_selfpowered) >> +USB_GADGET_BITFIELD(deactivated) >> +USB_GADGET_BITFIELD(connected) >> +USB_GADGET_BITFIELD(lpm_capable) >> +USB_GADGET_BITFIELD(wakeup_capable) >> +USB_GADGET_BITFIELD(wakeup_armed) > So ignore all of that. > > Now in your patch 4/10, do something that will have this effect: > > + struct usb_gadget g; > + > + g.dw1 = __entry->dw1; > + > TP_printk(.... > - __entry->sg_supported ? "sg:" : "", > + g.sg_supported ? "sg:" : "", > ... > > You probably can't do it exactly this way, because this won't work with > the tracing macros, but maybe something that is equivalent will work. > > For example, you could try: > > +#define USB_GADGET_BITFIELD(field) \ > + ({struct usb_gadget g; \ > + g.dw1 = __entry->dw1; \ > + g.field;}) > > TP_printk(.... > - __entry->sg_supported ? "sg:" : "", > + USB_GADGET_BITFIELD(sg_supported) ? "sg:" : "", > > Do you get the idea now? this is good idea, it is more simple. but still one question, why can't put in gadget.h, or not it need in every trace file, right ? > > Alan Stern
On 9/14/2023 10:25 AM, Linyu Yuan wrote: > > On 9/14/2023 10:16 AM, Alan Stern wrote: >> On Thu, Sep 14, 2023 at 09:08:04AM +0800, Linyu Yuan wrote: >>> On 9/14/2023 12:02 AM, Alan Stern wrote: >>>> On Wed, Sep 13, 2023 at 11:46:12AM +0800, Linyu Yuan wrote: >>>>> but Alan Stern have one comment, do it mean the bit position >>>>> number is not >>>>> expect and we can't use it ? >>>>> >>>>> @Alan Stern , BIT(0), BIT(1) is not the member we expect ? >>>> They might not be. If you can avoid making this assumption, you >>>> should. >>> >>> i don't know if it is true or not, seem some driver expect there is >>> no hole >>> for this kind of bit field definition. >> I didn't say there would be a hole; I said that BIT(0) might not be the >> member you expect. For example, sg_supported might be BIT(31) instead >> of BIT(0). > > > got it, it is same concern of Greg. > > >> >>>>>> This macro usage is a real mess. Can't you find a better way to >>>>>> do it? >>>>>> >>>>>> For instance, in the code that parses the trace buffer, define a >>>>>> temporary usb_gadget structure and copy the dw1 field from the trace >>>>>> buffer to the temporary structure. Then you can access the >>>>>> fields in >>>>>> that structure directly by their original names, with no macros. >>>>> do it same idea just move it outside of gadget.h ? >>>> Keep the anonymous union in gadget.h, but get rid of the macros. >>> >>> do you expect below ? >>> >>> >>> --- a/include/linux/usb/gadget.h >>> +++ b/include/linux/usb/gadget.h >>> @@ -357,6 +357,7 @@ struct usb_gadget_ops { >>> * @in_epnum: last used in ep number >>> * @mA: last set mA value >>> * @otg_caps: OTG capabilities of this gadget. >>> + * @dw1: trace event purpose >>> * @sg_supported: true if we can handle scatter-gather >>> * @is_otg: True if the USB device port uses a Mini-AB jack, so >>> that the >>> * gadget driver must provide a USB OTG descriptor. >>> @@ -432,30 +433,88 @@ struct usb_gadget { >>> unsigned mA; >>> struct usb_otg_caps *otg_caps; >>> >>> - unsigned sg_supported:1; >>> - unsigned is_otg:1; >>> - unsigned is_a_peripheral:1; >>> - unsigned b_hnp_enable:1; >>> - unsigned a_hnp_support:1; >>> - unsigned a_alt_hnp_support:1; >>> - unsigned hnp_polling_support:1; >>> - unsigned host_request_flag:1; >>> - unsigned quirk_ep_out_aligned_size:1; >>> - unsigned quirk_altset_not_supp:1; >>> - unsigned quirk_stall_not_supp:1; >>> - unsigned quirk_zlp_not_supp:1; >>> - unsigned quirk_avoids_skb_reserve:1; >>> - unsigned is_selfpowered:1; >>> - unsigned deactivated:1; >>> - unsigned connected:1; >>> - unsigned lpm_capable:1; >>> - unsigned wakeup_capable:1; >>> - unsigned wakeup_armed:1; >>> + union { >>> + struct { >>> + unsigned sg_supported:1; >>> + unsigned is_otg:1; >>> + unsigned is_a_peripheral:1; >>> + unsigned b_hnp_enable:1; >>> + unsigned a_hnp_support:1; >>> + unsigned a_alt_hnp_support:1; >>> + unsigned hnp_polling_support:1; >>> + unsigned host_request_flag:1; >>> + unsigned quirk_ep_out_aligned_size:1; >>> + unsigned quirk_altset_not_supp:1; >>> + unsigned quirk_stall_not_supp:1; >>> + unsigned quirk_zlp_not_supp:1; >>> + unsigned quirk_avoids_skb_reserve:1; >>> + unsigned is_selfpowered:1; >>> + unsigned deactivated:1; >>> + unsigned connected:1; >>> + unsigned lpm_capable:1; >>> + unsigned wakeup_capable:1; >>> + unsigned wakeup_armed:1; >>> + } __packed; >>> + >>> + u32 dw1; >>> + } __aligned(4); >>> int irq; >>> int id_number; >>> }; >>> #define work_to_gadget(w) (container_of((w), struct >>> usb_gadget, work)) >> Stop here. The above is what I expect. Don't include any of the >> material below. >> >> (BTW, you don't need the __aligned(4) thing, do you? Since the union >> includes a 32-bit integer field, it will naturally be aligned on a >> 4-byte boundary.) > > > sure, will remove it. > > >> >>> +#define USB_GADGET_BITFIELD(field) \ >>> +static inline u32 usb_gadget_bit_##field(u32 dw1) \ >>> +{ \ >>> + union { \ >>> + struct { \ >>> + u32 sg_supported:1; \ >>> + u32 is_otg:1; \ >>> + u32 is_a_peripheral:1; \ >>> + u32 b_hnp_enable:1; \ >>> + u32 a_hnp_support:1; \ >>> + u32 a_alt_hnp_support:1; \ >>> + u32 hnp_polling_support:1; \ >>> + u32 host_request_flag:1; \ >>> + u32 quirk_ep_out_aligned_size:1; \ >>> + u32 quirk_altset_not_supp:1; \ >>> + u32 quirk_stall_not_supp:1; \ >>> + u32 quirk_zlp_not_supp:1; \ >>> + u32 quirk_avoids_skb_reserve:1; \ >>> + u32 is_selfpowered:1; \ >>> + u32 deactivated:1; \ >>> + u32 connected:1; \ >>> + u32 lpm_capable:1; \ >>> + u32 wakeup_capable:1; \ >>> + u32 wakeup_armed:1; \ >>> + } __packed; \ >>> + u32 dw1; \ >>> + } __aligned(4) u; \ >>> + BUILD_BUG_ON(sizeof(u) != 4); \ >>> + u.dw1 = dw1; \ >>> + return u.field; \ >>> +} >>> + >>> +USB_GADGET_BITFIELD(sg_supported) >>> +USB_GADGET_BITFIELD(is_otg) >>> +USB_GADGET_BITFIELD(is_a_peripheral) >>> +USB_GADGET_BITFIELD(b_hnp_enable) >>> +USB_GADGET_BITFIELD(a_hnp_support) >>> +USB_GADGET_BITFIELD(a_alt_hnp_support) >>> +USB_GADGET_BITFIELD(hnp_polling_support) >>> +USB_GADGET_BITFIELD(host_request_flag) >>> +USB_GADGET_BITFIELD(quirk_ep_out_aligned_size) >>> +USB_GADGET_BITFIELD(quirk_altset_not_supp) >>> +USB_GADGET_BITFIELD(quirk_stall_not_supp) >>> +USB_GADGET_BITFIELD(quirk_zlp_not_supp) >>> +USB_GADGET_BITFIELD(quirk_avoids_skb_reserve) >>> +USB_GADGET_BITFIELD(is_selfpowered) >>> +USB_GADGET_BITFIELD(deactivated) >>> +USB_GADGET_BITFIELD(connected) >>> +USB_GADGET_BITFIELD(lpm_capable) >>> +USB_GADGET_BITFIELD(wakeup_capable) >>> +USB_GADGET_BITFIELD(wakeup_armed) >> So ignore all of that. >> >> Now in your patch 4/10, do something that will have this effect: >> >> + struct usb_gadget g; >> + >> + g.dw1 = __entry->dw1; >> + >> TP_printk(.... >> - __entry->sg_supported ? "sg:" : "", >> + g.sg_supported ? "sg:" : "", >> ... >> >> You probably can't do it exactly this way, because this won't work with >> the tracing macros, but maybe something that is equivalent will work. >> >> For example, you could try: >> >> +#define USB_GADGET_BITFIELD(field) \ >> + ({struct usb_gadget g; \ >> + g.dw1 = __entry->dw1; \ >> + g.field;}) >> >> TP_printk(.... >> - __entry->sg_supported ? "sg:" : "", >> + USB_GADGET_BITFIELD(sg_supported) ? "sg:" : "", >> >> Do you get the idea now? > > > this is good idea, it is more simple. > > but still one question, why can't put in gadget.h, or not it need in > every trace file, right ? i will add new trace event macro to cover this special case. will send next version. > > >> >> Alan Stern
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h index 75bda0783395..f02e1bd20924 100644 --- a/include/linux/usb/gadget.h +++ b/include/linux/usb/gadget.h @@ -357,6 +357,7 @@ struct usb_gadget_ops { * @in_epnum: last used in ep number * @mA: last set mA value * @otg_caps: OTG capabilities of this gadget. + * @dw1: trace event purpose * @sg_supported: true if we can handle scatter-gather * @is_otg: True if the USB device port uses a Mini-AB jack, so that the * gadget driver must provide a USB OTG descriptor. @@ -432,30 +433,87 @@ struct usb_gadget { unsigned mA; struct usb_otg_caps *otg_caps; - unsigned sg_supported:1; - unsigned is_otg:1; - unsigned is_a_peripheral:1; - unsigned b_hnp_enable:1; - unsigned a_hnp_support:1; - unsigned a_alt_hnp_support:1; - unsigned hnp_polling_support:1; - unsigned host_request_flag:1; - unsigned quirk_ep_out_aligned_size:1; - unsigned quirk_altset_not_supp:1; - unsigned quirk_stall_not_supp:1; - unsigned quirk_zlp_not_supp:1; - unsigned quirk_avoids_skb_reserve:1; - unsigned is_selfpowered:1; - unsigned deactivated:1; - unsigned connected:1; - unsigned lpm_capable:1; - unsigned wakeup_capable:1; - unsigned wakeup_armed:1; + union { + struct { + unsigned sg_supported:1; + unsigned is_otg:1; + unsigned is_a_peripheral:1; + unsigned b_hnp_enable:1; + unsigned a_hnp_support:1; + unsigned a_alt_hnp_support:1; + unsigned hnp_polling_support:1; + unsigned host_request_flag:1; + unsigned quirk_ep_out_aligned_size:1; + unsigned quirk_altset_not_supp:1; + unsigned quirk_stall_not_supp:1; + unsigned quirk_zlp_not_supp:1; + unsigned quirk_avoids_skb_reserve:1; + unsigned is_selfpowered:1; + unsigned deactivated:1; + unsigned connected:1; + unsigned lpm_capable:1; + unsigned wakeup_capable:1; + unsigned wakeup_armed:1; + } __packed; + + u32 dw1; + } __aligned(4); int irq; int id_number; }; #define work_to_gadget(w) (container_of((w), struct usb_gadget, work)) +#define USB_GADGET_BITFIELD(n, name) \ + ({\ + union {\ + struct {\ + u32 sg_supported:1;\ + u32 is_otg:1;\ + u32 is_a_peripheral:1;\ + u32 b_hnp_enable:1;\ + u32 a_hnp_support:1;\ + u32 a_alt_hnp_support:1;\ + u32 hnp_polling_support:1;\ + u32 host_request_flag:1;\ + u32 quirk_ep_out_aligned_size:1;\ + u32 quirk_altset_not_supp:1;\ + u32 quirk_stall_not_supp:1;\ + u32 quirk_zlp_not_supp:1;\ + u32 quirk_avoids_skb_reserve:1;\ + u32 is_selfpowered:1;\ + u32 deactivated:1;\ + u32 connected:1;\ + u32 lpm_capable:1;\ + u32 wakeup_capable:1;\ + u32 wakeup_armed:1;\ + } __packed;\ + u32 dw1;\ + } __aligned(4) __g_u_##name;\ + u32 __g_##name; \ + BUILD_BUG_ON(sizeof(__g_u_##name) != 4);\ + __g_u_##name.dw1 = (n); __g_##name = __g_u_##name.name;\ + __g_##name; }) + +#define USB_GADGET_SG_SUPPORTED(n) USB_GADGET_BITFIELD((n), sg_supported) +#define USB_GADGET_IS_OTG(n) USB_GADGET_BITFIELD((n), is_otg) +#define USB_GADGET_IS_A_PERIPHERAL(n) USB_GADGET_BITFIELD((n), is_a_peripheral) +#define USB_GADGET_B_HNP_ENABLE(n) USB_GADGET_BITFIELD((n), b_hnp_enable) +#define USB_GADGET_A_HNP_SUPPORT(n) USB_GADGET_BITFIELD((n), a_hnp_support) +#define USB_GADGET_A_ALT_HNP_SUPPORT(n) USB_GADGET_BITFIELD((n), a_alt_hnp_support) +#define USB_GADGET_HNP_POLLING_SUPPORT(n) USB_GADGET_BITFIELD((n), hnp_polling_support) +#define USB_GADGET_HOST_REQUEST_FLAG(n) USB_GADGET_BITFIELD((n), host_request_flag) +#define USB_GADGET_QUIRK_EP_OUT_ALIGNED_SIZE(n) USB_GADGET_BITFIELD((n), quirk_ep_out_aligned_size) +#define USB_GADGET_QUIRK_ALTSET_NOT_SUPP(n) USB_GADGET_BITFIELD((n), quirk_altset_not_supp) +#define USB_GADGET_QUIRK_STALL_NOT_SUPP(n) USB_GADGET_BITFIELD((n), quirk_stall_not_supp) +#define USB_GADGET_QUIRK_ZLP_NOT_SUPP(n) USB_GADGET_BITFIELD((n), quirk_zlp_not_supp) +#define USB_GADGET_QUIRK_AVOIDS_SKB_RESERVE(n) USB_GADGET_BITFIELD((n), quirk_avoids_skb_reserve) +#define USB_GADGET_IS_SELFPOWERED(n) USB_GADGET_BITFIELD((n), is_selfpowered) +#define USB_GADGET_DEACTIVATED(n) USB_GADGET_BITFIELD((n), deactivated) +#define USB_GADGET_CONNECTED(n) USB_GADGET_BITFIELD((n), connected) +#define USB_GADGET_LPM_CAPABLE(n) USB_GADGET_BITFIELD((n), lpm_capable) +#define USB_GADGET_WAKEUP_CAPABLE(n) USB_GADGET_BITFIELD((n), wakeup_capable) +#define USB_GADGET_WAKEUP_ARMED(n) USB_GADGET_BITFIELD((n), wakeup_armed) + /* Interface to the device model */ static inline void set_gadget_data(struct usb_gadget *gadget, void *data) { dev_set_drvdata(&gadget->dev, data); }
Some UDC trace event will save usb gadget information, but it use one int size buffer to save one bit information of usb gadget, so 19 int buffers needed to save 19 bit fields which is not good. Add one anonymous union which have one u32 member 'dw1' to struct 'usb_gadget', it inlclude all 19 bits and can be used by trace event during fast assign stage to save more entries with same trace ring buffer size. Also move all original 19 bit fields into one anonymous struct which inside struct 'usb_gadget'. In order to allow trace event output stage access the bit from saved u32 'dw1', add following macro, define USB_GADGET_BITFIELD(n, name) \ ({\ union {\ struct {\ u32 sg_supported:1;\ u32 is_otg:1;\ u32 is_a_peripheral:1;\ u32 b_hnp_enable:1;\ u32 a_hnp_support:1;\ u32 a_alt_hnp_support:1;\ u32 hnp_polling_support:1;\ u32 host_request_flag:1;\ u32 quirk_ep_out_aligned_size:1;\ u32 quirk_altset_not_supp:1;\ u32 quirk_stall_not_supp:1;\ u32 quirk_zlp_not_supp:1;\ u32 quirk_avoids_skb_reserve:1;\ u32 is_selfpowered:1;\ u32 deactivated:1;\ u32 connected:1;\ u32 lpm_capable:1;\ u32 wakeup_capable:1;\ u32 wakeup_armed:1;\ } __packed;\ u32 dw1;\ } __aligned(4) __g_u_##name;\ u32 __g_##name; \ BUILD_BUG_ON(sizeof(__g_u_##name) != 4);\ __g_u_##name.dw1 = (n); __g_##name = __g_u_##name.name;\ __g_##name; }) define USB_GADGET_SG_SUPPORTED(n) USB_GADGET_BITFIELD((n), sg_supported) ... change to use this kind of macro for all related trace files later. Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> --- v2: no change v3: change method to extract bit field include/linux/usb/gadget.h | 96 ++++++++++++++++++++++++++++++-------- 1 file changed, 77 insertions(+), 19 deletions(-)