[PATCH v3 00/10] usb: gadget: reduce usb gadget trace event buffer usage

[PATCH v3 00/10] usb: gadget: reduce usb gadget trace event buffer usage

[Linyu Yuan]

some trace event use an interger to to save a bit field info of gadget,
also some trace save endpoint name in string forat, it all can be
chagned to other way at trace event store phase.

bit field can be replace with a union interger member which include
multiple bit fields.

ep name stringe can be replace to a interger which contaion number
and dir info.

v1: https://lore.kernel.org/linux-usb/20230911042843.2711-1-quic_linyyuan@quicinc.com/
v2: fix two compile issues that COMPILE_TEST not covered
    https://lore.kernel.org/linux-usb/20230911112446.1791-1-quic_linyyuan@quicinc.com/
v3: fix reviewer comments, allow bit fields work on both little and big endian

Linyu Yuan (10):
  usb: gadget: add anonymous definition in struct usb_gadget
  usb: gadget: add anonymous definition in struct usb_request
  usb: gadget: add anonymous definition in struct usb_ep
  usb: udc: trace: reduce buffer usage of trace event
  usb: cdns3: cdnsp: reduce buffer usage of trace event
  usb: cdns3: trace: reduce buffer usage of trace event
  usb: dwc3: trace: reduce buffer usage of trace event
  usb: cdns2: trace: reduce buffer usage of trace event
  usb: mtu3: trace: reduce buffer usage of trace event
  usb: musb: trace: reduce buffer usage of trace event

 drivers/usb/cdns3/cdns3-trace.h            |  93 ++++----
 drivers/usb/cdns3/cdnsp-trace.h            |  45 ++--
 drivers/usb/dwc3/trace.h                   |  54 ++---
 drivers/usb/gadget/udc/cdns2/cdns2-trace.h |  77 +++----
 drivers/usb/gadget/udc/trace.h             | 106 +++------
 drivers/usb/mtu3/mtu3_trace.h              |  42 ++--
 drivers/usb/musb/musb_trace.h              |  14 +-
 include/linux/usb/gadget.h                 | 245 +++++++++++++++++----
 8 files changed, 383 insertions(+), 293 deletions(-)

-- 
2.17.1

[PATCH v3 01/10] usb: gadget: add anonymous definition in struct usb_gadget

[Linyu Yuan]

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(-)

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); }
-- 
2.17.1

Re: [PATCH v3 01/10] usb: gadget: add anonymous definition in struct usb_gadget

[Greg Kroah-Hartman]

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

Re: [PATCH v3 01/10] usb: gadget: add anonymous definition in struct usb_gadget

[Linyu Yuan]

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

Re: [PATCH v3 01/10] usb: gadget: add anonymous definition in struct usb_gadget

[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

Re: [PATCH v3 01/10] usb: gadget: add anonymous definition in struct usb_gadget

[Linyu Yuan]

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

Re: [PATCH v3 01/10] usb: gadget: add anonymous definition in struct usb_gadget

[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

Re: [PATCH v3 01/10] usb: gadget: add anonymous definition in struct usb_gadget

[Linyu Yuan]

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

Re: [PATCH v3 01/10] usb: gadget: add anonymous definition in struct usb_gadget

[Linyu Yuan]

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

Re: [PATCH v3 01/10] usb: gadget: add anonymous definition in struct usb_gadget

[Alan Stern]

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

Re: [PATCH v3 01/10] usb: gadget: add anonymous definition in struct usb_gadget

[Linyu Yuan]

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

[PATCH v3 02/10] usb: gadget: add anonymous definition in struct usb_request

[Linyu Yuan]

Some UDC trace event will save usb request information, but it will use
one int size buffer to save one bit information of usb request, so more
than one int buffer to save several bit fields which is not good.

First add an anonymous union which have one u32 member dw1 which can be
used by trace event during fast assign stage to reduce trace buffer
usage, add related macro to extract bit fields from dw1 for later trace
event output state usage.

Also move exist stread_id and other bit fields into one anonymous struct
which inside anonymous union, change them from unsigned to u32 type,
it will make sure union member have same memory size as dw1.

In order to allow trace event output stage access the bit from saved u32
'dw1', add following macros,
define USB_REQ_BITFIELD(n, name) \
	({\
	union {\
		struct {\
			u32	stream_id:16;\
			u32	is_last:1;\
			u32	no_interrupt:1;\
			u32	zero:1;\
			u32	short_not_ok:1;\
			u32	dma_mapped:1;\
		} __packed;\
		u32		dw1;\
	} __aligned(4) __r_u_##name;\
	u32 __r_##name; \
	BUILD_BUG_ON(sizeof(__r_u_##name) != 4);\
	__r_u_##name.dw1 = (n); __r_##name = __r_u_##name.name;\
	__r_##name; })

define USB_REQ_STREAM_ID(n) USB_REQ_BITFIELD((n), stream_id)
...
change to use this kind of macros 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 | 43 ++++++++++++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 6 deletions(-)

diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index f02e1bd20924..a1717c633942 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -41,6 +41,7 @@ struct usb_ep;
  * @num_sgs: number of SG entries
  * @num_mapped_sgs: number of SG entries mapped to DMA (internal)
  * @length: Length of that data
+ * @dw1: trace event purpose
  * @stream_id: The stream id, when USB3.0 bulk streams are being used
  * @is_last: Indicates if this is the last request of a stream_id before
  *	switching to a different stream (required for DWC3 controllers).
@@ -105,12 +106,17 @@ struct usb_request {
 	unsigned		num_sgs;
 	unsigned		num_mapped_sgs;
 
-	unsigned		stream_id:16;
-	unsigned		is_last:1;
-	unsigned		no_interrupt:1;
-	unsigned		zero:1;
-	unsigned		short_not_ok:1;
-	unsigned		dma_mapped:1;
+	union {
+		struct {
+			u32	stream_id:16;
+			u32	is_last:1;
+			u32	no_interrupt:1;
+			u32	zero:1;
+			u32	short_not_ok:1;
+			u32	dma_mapped:1;
+		} __packed;
+		u32		dw1;
+	} __aligned(4);
 
 	void			(*complete)(struct usb_ep *ep,
 					struct usb_request *req);
@@ -123,6 +129,31 @@ struct usb_request {
 	unsigned		actual;
 };
 
+#define USB_REQ_BITFIELD(n, name) \
+	({\
+	union {\
+		struct {\
+			u32	stream_id:16;\
+			u32	is_last:1;\
+			u32	no_interrupt:1;\
+			u32	zero:1;\
+			u32	short_not_ok:1;\
+			u32	dma_mapped:1;\
+		} __packed;\
+		u32		dw1;\
+	} __aligned(4) __r_u_##name;\
+	u32 __r_##name; \
+	BUILD_BUG_ON(sizeof(__r_u_##name) != 4);\
+	__r_u_##name.dw1 = (n); __r_##name = __r_u_##name.name;\
+	__r_##name; })
+
+#define USB_REQ_STREAM_ID(n) USB_REQ_BITFIELD((n), stream_id)
+#define USB_REQ_IS_LAST(n) USB_REQ_BITFIELD((n), stream_id)
+#define USB_REQ_NO_INTERRUPT(n) USB_REQ_BITFIELD((n), stream_id)
+#define USB_REQ_ZERO(n) USB_REQ_BITFIELD((n), stream_id)
+#define USB_REQ_SHORT_NOT_OK(n) USB_REQ_BITFIELD((n), stream_id)
+#define USB_REQ_DMA_MAPPED(n) USB_REQ_BITFIELD((n), stream_id)
+
 /*-------------------------------------------------------------------------*/
 
 /* endpoint-specific parts of the api to the usb controller hardware.
-- 
2.17.1

[PATCH v3 03/10] usb: gadget: add anonymous definition in struct usb_ep

[Linyu Yuan]

Some UDC trace event will save usb endpoint information, but it will use
one int size buffer to save one bit information of usb endpoint, so more
than one int buffer to save several bit fields which is not good.

Add some anonymous union have three u32 members which can be used by trace
event during fast assign stage to reduce trace buffer usage, and add
related macro to extract bit fields from u32 members for later trace event
output state usage.

Also move exist maxpacket and other bit field into anonymous struct which
inside anonymous union and Change them from unsigned to u32 type.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
v2: no change
v3: change method to extract bit field

 include/linux/usb/gadget.h | 106 +++++++++++++++++++++++++++++++------
 1 file changed, 90 insertions(+), 16 deletions(-)

diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index a1717c633942..1b63843f5c31 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -194,13 +194,14 @@ struct usb_ep_ops {
  * @dir_out:Endpoint supports OUT direction.
  */
 struct usb_ep_caps {
-	unsigned type_control:1;
-	unsigned type_iso:1;
-	unsigned type_bulk:1;
-	unsigned type_int:1;
-	unsigned dir_in:1;
-	unsigned dir_out:1;
-};
+	u8	type_control:1;
+	u8	type_iso:1;
+	u8	type_bulk:1;
+	u8	type_int:1;
+	u8	dir_in:1;
+	u8	dir_out:1;
+	u8	reserve:2;
+} __packed;
 
 #define USB_EP_CAPS_TYPE_CONTROL     0x01
 #define USB_EP_CAPS_TYPE_ISO         0x02
@@ -230,6 +231,9 @@ struct usb_ep_caps {
  * @caps:The structure describing types and directions supported by endpoint.
  * @enabled: The current endpoint enabled/disabled state.
  * @claimed: True if this endpoint is claimed by a function.
+ * @dw1: trace event purpose
+ * @dw2: trace event purpose
+ * @dw3: trace event purpose
  * @maxpacket:The maximum packet size used on this endpoint.  The initial
  *	value can sometimes be reduced (hardware allowing), according to
  *	the endpoint descriptor used to configure the endpoint.
@@ -259,19 +263,89 @@ struct usb_ep {
 	const char		*name;
 	const struct usb_ep_ops	*ops;
 	struct list_head	ep_list;
-	struct usb_ep_caps	caps;
-	bool			claimed;
-	bool			enabled;
-	unsigned		maxpacket:16;
-	unsigned		maxpacket_limit:16;
-	unsigned		max_streams:16;
-	unsigned		mult:2;
-	unsigned		maxburst:5;
-	u8			address;
+	union {
+		struct {
+			u32	maxpacket:16;
+			u32	maxpacket_limit:16;
+		} __packed;
+		u32	dw1;
+	} __aligned(4);
+	union {
+		struct {
+			u32	max_streams:16;
+			u32	mult:2;
+			u32	maxburst:5;
+		} __packed;
+		u32	dw2;
+	} __aligned(4);
+	union {
+		struct {
+			struct usb_ep_caps	caps;
+			u8			claimed:1;
+			u8			enabled:1;
+			u8			address;
+		} __packed;
+		u32	dw3;
+	} __aligned(4);
 	const struct usb_endpoint_descriptor	*desc;
 	const struct usb_ss_ep_comp_descriptor	*comp_desc;
 };
 
+#define USB_EP_DW1_BITFIELD(n, name) \
+	({\
+	union {\
+		struct {\
+			u32	maxpacket:16;\
+			u32	maxpacket_limit:16;\
+		} __packed;\
+		u32		dw1;\
+	} __aligned(4) __e_u_##name;\
+	u32 __e_##name;\
+	BUILD_BUG_ON(sizeof(__e_u_##name) != 4);\
+	__e_u_##name.dw1 = (n); __e_##name = __e_u_##name.name;\
+	__e_##name; })
+
+#define USB_EP_MAXPACKET(n) USB_EP_DW1_BITFIELD((n), maxpacket)
+#define USB_EP_MAXPACKET_LIMIT(n) USB_EP_DW1_BITFIELD((n), maxpacket_limit)
+
+#define USB_EP_DW2_BITFIELD(n, name) \
+	({\
+	union {\
+		struct {\
+			u32	max_streams:16;\
+			u32	mult:2;\
+			u32	maxburst:5;\
+		} __packed;\
+		u32		dw2;\
+	} __aligned(4) __e_u_##name;\
+	u32 __e_##name; \
+	BUILD_BUG_ON(sizeof(__e_u_##name) != 4);\
+	__e_u_##name.dw2 = (n); __e_##name = __e_u_##name.name;\
+	__e_##name; })
+
+#define USB_EP_MAX_STREAMS(n) USB_EP_DW2_BITFIELD((n), max_streams)
+#define USB_EP_MULT(n) USB_EP_DW2_BITFIELD((n), mult)
+#define USB_EP_MAXBURST(n) USB_EP_DW2_BITFIELD((n), maxburst)
+
+
+#define USB_EP_NAME(n) \
+	({char __s[9]; /* max 8: ep127out */ \
+	union {\
+		struct {\
+			struct usb_ep_caps	caps;\
+			u8			claimed:1;\
+			u8			enabled:1;\
+			u8			address;\
+		} __packed;\
+		u32		dw3;\
+	} __aligned(4) __e_u_##name;\
+	BUILD_BUG_ON(sizeof(__e_u_##name) != 4);\
+	__e_u_##name.dw3 = (n);\
+	snprintf(__s, 9, "ep%d%s", __e_u_##name.address, \
+		(__e_u_##name.caps.dir_in && __e_u_##name.caps.dir_out) ? "" : \
+			__e_u_##name.caps.dir_in ? "in" : "out");\
+	__s; })
+
 /*-------------------------------------------------------------------------*/
 
 #if IS_ENABLED(CONFIG_USB_GADGET)
-- 
2.17.1

[PATCH v3 04/10] usb: udc: trace: reduce buffer usage of trace event

[Linyu Yuan]

Save u32 members into trace event ring buffer and parse it for possible
bit fields.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
v2: no change
v3: no change

 drivers/usb/gadget/udc/trace.h | 106 +++++++++++----------------------
 1 file changed, 35 insertions(+), 71 deletions(-)

diff --git a/drivers/usb/gadget/udc/trace.h b/drivers/usb/gadget/udc/trace.h
index a5ed26fbc2da..10cc6437432d 100644
--- a/drivers/usb/gadget/udc/trace.h
+++ b/drivers/usb/gadget/udc/trace.h
@@ -25,20 +25,7 @@ DECLARE_EVENT_CLASS(udc_log_gadget,
 		__field(enum usb_device_speed, max_speed)
 		__field(enum usb_device_state, state)
 		__field(unsigned, mA)
-		__field(unsigned, sg_supported)
-		__field(unsigned, is_otg)
-		__field(unsigned, is_a_peripheral)
-		__field(unsigned, b_hnp_enable)
-		__field(unsigned, a_hnp_support)
-		__field(unsigned, hnp_polling_support)
-		__field(unsigned, host_request_flag)
-		__field(unsigned, quirk_ep_out_aligned_size)
-		__field(unsigned, quirk_altset_not_supp)
-		__field(unsigned, quirk_stall_not_supp)
-		__field(unsigned, quirk_zlp_not_supp)
-		__field(unsigned, is_selfpowered)
-		__field(unsigned, deactivated)
-		__field(unsigned, connected)
+		__field(u32, gdw1)
 		__field(int, ret)
 	),
 	TP_fast_assign(
@@ -46,38 +33,25 @@ DECLARE_EVENT_CLASS(udc_log_gadget,
 		__entry->max_speed = g->max_speed;
 		__entry->state = g->state;
 		__entry->mA = g->mA;
-		__entry->sg_supported = g->sg_supported;
-		__entry->is_otg = g->is_otg;
-		__entry->is_a_peripheral = g->is_a_peripheral;
-		__entry->b_hnp_enable = g->b_hnp_enable;
-		__entry->a_hnp_support = g->a_hnp_support;
-		__entry->hnp_polling_support = g->hnp_polling_support;
-		__entry->host_request_flag = g->host_request_flag;
-		__entry->quirk_ep_out_aligned_size = g->quirk_ep_out_aligned_size;
-		__entry->quirk_altset_not_supp = g->quirk_altset_not_supp;
-		__entry->quirk_stall_not_supp = g->quirk_stall_not_supp;
-		__entry->quirk_zlp_not_supp = g->quirk_zlp_not_supp;
-		__entry->is_selfpowered = g->is_selfpowered;
-		__entry->deactivated = g->deactivated;
-		__entry->connected = g->connected;
+		__entry->gdw1 = g->dw1;
 		__entry->ret = ret;
 	),
 	TP_printk("speed %d/%d state %d %dmA [%s%s%s%s%s%s%s%s%s%s%s%s%s%s] --> %d",
 		__entry->speed, __entry->max_speed, __entry->state, __entry->mA,
-		__entry->sg_supported ? "sg:" : "",
-		__entry->is_otg ? "OTG:" : "",
-		__entry->is_a_peripheral ? "a_peripheral:" : "",
-		__entry->b_hnp_enable ? "b_hnp:" : "",
-		__entry->a_hnp_support ? "a_hnp:" : "",
-		__entry->hnp_polling_support ? "hnp_poll:" : "",
-		__entry->host_request_flag ? "hostreq:" : "",
-		__entry->quirk_ep_out_aligned_size ? "out_aligned:" : "",
-		__entry->quirk_altset_not_supp ? "no_altset:" : "",
-		__entry->quirk_stall_not_supp ? "no_stall:" : "",
-		__entry->quirk_zlp_not_supp ? "no_zlp" : "",
-		__entry->is_selfpowered ? "self-powered:" : "bus-powered:",
-		__entry->deactivated ? "deactivated:" : "activated:",
-		__entry->connected ? "connected" : "disconnected",
+		USB_GADGET_SG_SUPPORTED(__entry->gdw1) ? "sg:" : "",
+		USB_GADGET_IS_OTG(__entry->gdw1) ? "OTG:" : "",
+		USB_GADGET_IS_A_PERIPHERAL(__entry->gdw1) ? "a_peripheral:" : "",
+		USB_GADGET_B_HNP_ENABLE(__entry->gdw1) ? "b_hnp:" : "",
+		USB_GADGET_A_HNP_SUPPORT(__entry->gdw1) ? "a_hnp:" : "",
+		USB_GADGET_HNP_POLLING_SUPPORT(__entry->gdw1) ? "hnp_poll:" : "",
+		USB_GADGET_HOST_REQUEST_FLAG(__entry->gdw1) ? "hostreq:" : "",
+		USB_GADGET_QUIRK_EP_OUT_ALIGNED_SIZE(__entry->gdw1) ? "out_aligned:" : "",
+		USB_GADGET_QUIRK_ALTSET_NOT_SUPP(__entry->gdw1) ? "no_altset:" : "",
+		USB_GADGET_QUIRK_STALL_NOT_SUPP(__entry->gdw1) ? "no_stall:" : "",
+		USB_GADGET_QUIRK_ZLP_NOT_SUPP(__entry->gdw1) ? "no_zlp" : "",
+		USB_GADGET_IS_SELFPOWERED(__entry->gdw1) ? "self-powered:" : "bus-powered:",
+		USB_GADGET_DEACTIVATED(__entry->gdw1) ? "deactivated:" : "activated:",
+		USB_GADGET_CONNECTED(__entry->gdw1) ? "connected" : "disconnected",
 		__entry->ret)
 );
 
@@ -145,32 +119,28 @@ DECLARE_EVENT_CLASS(udc_log_ep,
 	TP_PROTO(struct usb_ep *ep, int ret),
 	TP_ARGS(ep, ret),
 	TP_STRUCT__entry(
-		__string(name, ep->name)
-		__field(unsigned, maxpacket)
-		__field(unsigned, maxpacket_limit)
-		__field(unsigned, max_streams)
-		__field(unsigned, mult)
-		__field(unsigned, maxburst)
+		__field(u32, edw3)
+		__field(u32, edw1)
+		__field(u32, edw2)
 		__field(u8, address)
 		__field(bool, claimed)
 		__field(bool, enabled)
 		__field(int, ret)
 	),
 	TP_fast_assign(
-		__assign_str(name, ep->name);
-		__entry->maxpacket = ep->maxpacket;
-		__entry->maxpacket_limit = ep->maxpacket_limit;
-		__entry->max_streams = ep->max_streams;
-		__entry->mult = ep->mult;
-		__entry->maxburst = ep->maxburst;
+		__entry->edw3 = ep->dw3;
+		__entry->edw1 = ep->dw1;
+		__entry->edw2 = ep->dw2;
 		__entry->address = ep->address,
 		__entry->claimed = ep->claimed;
 		__entry->enabled = ep->enabled;
 		__entry->ret = ret;
 	),
 	TP_printk("%s: mps %d/%d streams %d mult %d burst %d addr %02x %s%s --> %d",
-		__get_str(name), __entry->maxpacket, __entry->maxpacket_limit,
-		__entry->max_streams, __entry->mult, __entry->maxburst,
+		USB_EP_NAME(__entry->edw3), USB_EP_MAXPACKET(__entry->edw1),
+		USB_EP_MAXPACKET_LIMIT(__entry->edw1),
+		USB_EP_MAX_STREAMS(__entry->edw2), USB_EP_MULT(__entry->edw2),
+		USB_EP_MAXBURST(__entry->edw2),
 		__entry->address, __entry->claimed ? "claimed:" : "released:",
 		__entry->enabled ? "enabled" : "disabled", ret)
 );
@@ -219,39 +189,33 @@ 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(u32, edw3)
 		__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(u32, rdw1)
 		__field(int, status)
 		__field(int, ret)
 		__field(struct usb_request *, req)
 	),
 	TP_fast_assign(
-		__assign_str(name, ep->name);
+		__entry->edw3 = ep->dw3;
 		__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->rdw1 = req->dw1;
 		__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",
+		USB_EP_NAME(__entry->edw3), __entry->req, __entry->actual, __entry->length,
+		__entry->num_mapped_sgs, __entry->num_sgs, USB_REQ_STREAM_ID(__entry->rdw1),
+		USB_REQ_ZERO(__entry->rdw1) ? "Z" : "z",
+		USB_REQ_SHORT_NOT_OK(__entry->rdw1) ? "S" : "s",
+		USB_REQ_NO_INTERRUPT(__entry->rdw1) ? "i" : "I",
 		__entry->status, __entry->ret
 	)
 );
-- 
2.17.1

[PATCH v3 05/10] usb: cdns3: cdnsp: reduce buffer usage of trace event

[Linyu Yuan]

Save u32 members into trace event ring buffer and parse it for possible
bit information.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
v2: no change
v3: no change

 drivers/usb/cdns3/cdnsp-trace.h | 45 +++++++++++++++------------------
 1 file changed, 20 insertions(+), 25 deletions(-)

diff --git a/drivers/usb/cdns3/cdnsp-trace.h b/drivers/usb/cdns3/cdnsp-trace.h
index 4b51011eb00b..b1ceb6755e6d 100644
--- a/drivers/usb/cdns3/cdnsp-trace.h
+++ b/drivers/usb/cdns3/cdnsp-trace.h
@@ -38,7 +38,7 @@ DECLARE_EVENT_CLASS(cdnsp_log_ep,
 	TP_PROTO(struct cdnsp_ep *pep, u32 stream_id),
 	TP_ARGS(pep, stream_id),
 	TP_STRUCT__entry(
-		__string(name, pep->name)
+		__field(u32, edw3)
 		__field(unsigned int, state)
 		__field(u32, stream_id)
 		__field(u8, enabled)
@@ -48,7 +48,7 @@ DECLARE_EVENT_CLASS(cdnsp_log_ep,
 		__field(u8, drbls_count)
 	),
 	TP_fast_assign(
-		__assign_str(name, pep->name);
+		__entry->edw3 = pep->endpoint.dw3;
 		__entry->state = pep->ep_state;
 		__entry->stream_id = stream_id;
 		__entry->enabled = pep->ep_state & EP_HAS_STREAMS;
@@ -59,7 +59,7 @@ DECLARE_EVENT_CLASS(cdnsp_log_ep,
 	),
 	TP_printk("%s: SID: %08x, ep state: %x, stream: enabled: %d num %d "
 		  "tds %d, first prime: %d drbls %d",
-		  __get_str(name), __entry->stream_id, __entry->state,
+		  USB_EP_NAME(__entry->edw3), __entry->stream_id, __entry->state,
 		  __entry->enabled, __entry->num_streams, __entry->td_count,
 		  __entry->first_prime_det, __entry->drbls_count)
 );
@@ -296,21 +296,21 @@ DECLARE_EVENT_CLASS(cdnsp_log_bounce,
 		 dma_addr_t dma, unsigned int unalign),
 	TP_ARGS(preq, new_buf_len, offset, dma, unalign),
 	TP_STRUCT__entry(
-		__string(name, preq->pep->name)
+		__field(u32, edw3)
 		__field(u32, new_buf_len)
 		__field(u32, offset)
 		__field(dma_addr_t, dma)
 		__field(unsigned int, unalign)
 	),
 	TP_fast_assign(
-		__assign_str(name, preq->pep->name);
+		__entry->edw3 = preq->pep->endpoint.dw3;
 		__entry->new_buf_len = new_buf_len;
 		__entry->offset = offset;
 		__entry->dma = dma;
 		__entry->unalign = unalign;
 	),
 	TP_printk("%s buf len %d, offset %d, dma %pad, unalign %d",
-		  __get_str(name), __entry->new_buf_len,
+		  USB_EP_NAME(__entry->edw3), __entry->new_buf_len,
 		  __entry->offset, &__entry->dma, __entry->unalign
 	)
 );
@@ -452,7 +452,7 @@ DECLARE_EVENT_CLASS(cdnsp_log_request,
 	TP_PROTO(struct cdnsp_request *req),
 	TP_ARGS(req),
 	TP_STRUCT__entry(
-		__string(name, req->pep->name)
+		__field(u32, edw3)
 		__field(struct usb_request *, request)
 		__field(struct cdnsp_request *, preq)
 		__field(void *, buf)
@@ -460,17 +460,14 @@ DECLARE_EVENT_CLASS(cdnsp_log_request,
 		__field(unsigned int, length)
 		__field(int, status)
 		__field(dma_addr_t, dma)
-		__field(unsigned int, stream_id)
-		__field(unsigned int, zero)
-		__field(unsigned int, short_not_ok)
-		__field(unsigned int, no_interrupt)
+		__field(u32, rdw1)
 		__field(struct scatterlist*, sg)
 		__field(unsigned int, num_sgs)
 		__field(unsigned int, num_mapped_sgs)
 
 	),
 	TP_fast_assign(
-		__assign_str(name, req->pep->name);
+		__entry->edw3 = req->pep->endpoint.dw3;
 		__entry->request = &req->request;
 		__entry->preq = req;
 		__entry->buf = req->request.buf;
@@ -478,10 +475,7 @@ DECLARE_EVENT_CLASS(cdnsp_log_request,
 		__entry->length = req->request.length;
 		__entry->status = req->request.status;
 		__entry->dma = req->request.dma;
-		__entry->stream_id = req->request.stream_id;
-		__entry->zero = req->request.zero;
-		__entry->short_not_ok = req->request.short_not_ok;
-		__entry->no_interrupt = req->request.no_interrupt;
+		__entry->rdw1 = req->request.dw1;
 		__entry->sg = req->request.sg;
 		__entry->num_sgs = req->request.num_sgs;
 		__entry->num_mapped_sgs = req->request.num_mapped_sgs;
@@ -489,12 +483,12 @@ DECLARE_EVENT_CLASS(cdnsp_log_request,
 	TP_printk("%s; req U:%p/P:%p, req buf %p, length %u/%u, status %d, "
 		  "buf dma (%pad), SID %u, %s%s%s, sg %p, num_sg %d,"
 		  " num_m_sg %d",
-		  __get_str(name), __entry->request, __entry->preq,
+		  USB_EP_NAME(__entry->edw3), __entry->request, __entry->preq,
 		  __entry->buf, __entry->actual, __entry->length,
 		  __entry->status, &__entry->dma,
-		  __entry->stream_id, __entry->zero ? "Z" : "z",
-		  __entry->short_not_ok ? "S" : "s",
-		  __entry->no_interrupt ? "I" : "i",
+		  USB_REQ_STREAM_ID(__entry->rdw1), USB_REQ_ZERO(__entry->rdw1) ? "Z" : "z",
+		  USB_REQ_SHORT_NOT_OK(__entry->rdw1) ? "S" : "s",
+		  USB_REQ_NO_INTERRUPT(__entry->rdw1) ? "I" : "i",
 		  __entry->sg, __entry->num_sgs, __entry->num_mapped_sgs
 		)
 );
@@ -666,7 +660,7 @@ DECLARE_EVENT_CLASS(cdnsp_log_td_info,
 	TP_PROTO(struct cdnsp_request *preq),
 	TP_ARGS(preq),
 	TP_STRUCT__entry(
-		__string(name, preq->pep->name)
+		__field(u32, edw3)
 		__field(struct usb_request *, request)
 		__field(struct cdnsp_request *, preq)
 		__field(union cdnsp_trb *, first_trb)
@@ -674,7 +668,7 @@ DECLARE_EVENT_CLASS(cdnsp_log_td_info,
 		__field(dma_addr_t, trb_dma)
 	),
 	TP_fast_assign(
-		__assign_str(name, preq->pep->name);
+		__entry->edw3 = preq->pep->endpoint.dw3;
 		__entry->request = &preq->request;
 		__entry->preq = preq;
 		__entry->first_trb = preq->td.first_trb;
@@ -683,7 +677,7 @@ DECLARE_EVENT_CLASS(cdnsp_log_td_info,
 							 preq->td.first_trb)
 	),
 	TP_printk("%s req/preq:  %p/%p, first trb %p[vir]/%pad(dma), last trb %p",
-		  __get_str(name), __entry->request, __entry->preq,
+		  USB_EP_NAME(__entry->edw3), __entry->request, __entry->preq,
 		  __entry->first_trb, &__entry->trb_dma,
 		  __entry->last_trb
 		)
@@ -800,16 +794,17 @@ TRACE_EVENT(cdnsp_stream_number,
 	TP_PROTO(struct cdnsp_ep *pep, int num_stream_ctxs, int num_streams),
 	TP_ARGS(pep, num_stream_ctxs, num_streams),
 	TP_STRUCT__entry(
-		__string(name, pep->name)
+		__field(u32, edw3)
 		__field(int, num_stream_ctxs)
 		__field(int, num_streams)
 	),
 	TP_fast_assign(
+		__entry->edw3 = pep->endpoint.dw3;
 		__entry->num_stream_ctxs = num_stream_ctxs;
 		__entry->num_streams = num_streams;
 	),
 	TP_printk("%s Need %u stream ctx entries for %u stream IDs.",
-		  __get_str(name), __entry->num_stream_ctxs,
+		  USB_EP_NAME(__entry->edw3), __entry->num_stream_ctxs,
 		  __entry->num_streams)
 );
 
-- 
2.17.1

[PATCH v3 06/10] usb: cdns3: trace: reduce buffer usage of trace event

[Linyu Yuan]

Save u32 members into trace event ring buffer and parse it for possible
bit information.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
v2: no change
v3: no change

 drivers/usb/cdns3/cdns3-trace.h | 93 +++++++++++++++------------------
 1 file changed, 42 insertions(+), 51 deletions(-)

diff --git a/drivers/usb/cdns3/cdns3-trace.h b/drivers/usb/cdns3/cdns3-trace.h
index 40db89e3333c..391547546530 100644
--- a/drivers/usb/cdns3/cdns3-trace.h
+++ b/drivers/usb/cdns3/cdns3-trace.h
@@ -28,45 +28,45 @@ TRACE_EVENT(cdns3_halt,
 	TP_PROTO(struct cdns3_endpoint *ep_priv, u8 halt, u8 flush),
 	TP_ARGS(ep_priv, halt, flush),
 	TP_STRUCT__entry(
-		__string(name, ep_priv->name)
+		__field(u32, edw3)
 		__field(u8, halt)
 		__field(u8, flush)
 	),
 	TP_fast_assign(
-		__assign_str(name, ep_priv->name);
+		__entry->edw3 = ep_priv->endpoint.dw3;
 		__entry->halt = halt;
 		__entry->flush = flush;
 	),
 	TP_printk("Halt %s for %s: %s", __entry->flush ? " and flush" : "",
-		  __get_str(name), __entry->halt ? "set" : "cleared")
+		  USB_EP_NAME(__entry->edw3), __entry->halt ? "set" : "cleared")
 );
 
 TRACE_EVENT(cdns3_wa1,
 	TP_PROTO(struct cdns3_endpoint *ep_priv, char *msg),
 	TP_ARGS(ep_priv, msg),
 	TP_STRUCT__entry(
-		__string(ep_name, ep_priv->name)
+		__field(u32, edw3)
 		__string(msg, msg)
 	),
 	TP_fast_assign(
-		__assign_str(ep_name, ep_priv->name);
+		__entry->edw3 = ep_priv->endpoint.dw3;
 		__assign_str(msg, msg);
 	),
-	TP_printk("WA1: %s %s", __get_str(ep_name), __get_str(msg))
+	TP_printk("WA1: %s %s", USB_EP_NAME(__entry->edw3), __get_str(msg))
 );
 
 TRACE_EVENT(cdns3_wa2,
 	TP_PROTO(struct cdns3_endpoint *ep_priv, char *msg),
 	TP_ARGS(ep_priv, msg),
 	TP_STRUCT__entry(
-		__string(ep_name, ep_priv->name)
+		__field(u32, edw3)
 		__string(msg, msg)
 	),
 	TP_fast_assign(
-		__assign_str(ep_name, ep_priv->name);
+		__entry->edw3 = ep_priv->endpoint.dw3;
 		__assign_str(msg, msg);
 	),
-	TP_printk("WA2: %s %s", __get_str(ep_name), __get_str(msg))
+	TP_printk("WA2: %s %s", USB_EP_NAME(__entry->edw3), __get_str(msg))
 );
 
 DECLARE_EVENT_CLASS(cdns3_log_doorbell,
@@ -118,14 +118,14 @@ DECLARE_EVENT_CLASS(cdns3_log_epx_irq,
 	TP_PROTO(struct cdns3_device *priv_dev, struct cdns3_endpoint *priv_ep),
 	TP_ARGS(priv_dev, priv_ep),
 	TP_STRUCT__entry(
-		__string(ep_name, priv_ep->name)
+		__field(u32, edw3)
 		__field(u32, ep_sts)
 		__field(u32, ep_traddr)
 		__field(u32, ep_last_sid)
 		__field(u32, use_streams)
 	),
 	TP_fast_assign(
-		__assign_str(ep_name, priv_ep->name);
+		__entry->edw3 = priv_ep->endpoint.dw3;
 		__entry->ep_sts = readl(&priv_dev->regs->ep_sts);
 		__entry->ep_traddr = readl(&priv_dev->regs->ep_traddr);
 		__entry->ep_last_sid = priv_ep->last_stream_id;
@@ -133,7 +133,7 @@ DECLARE_EVENT_CLASS(cdns3_log_epx_irq,
 	),
 	TP_printk("%s, ep_traddr: %08x ep_last_sid: %08x use_streams: %d",
 		  cdns3_decode_epx_irq(__get_buf(CDNS3_MSG_MAX),
-				       __get_str(ep_name),
+				       USB_EP_NAME(__entry->edw3),
 				       __entry->ep_sts),
 		  __entry->ep_traddr,
 		  __entry->ep_last_sid,
@@ -199,47 +199,42 @@ DECLARE_EVENT_CLASS(cdns3_log_request,
 	TP_PROTO(struct cdns3_request *req),
 	TP_ARGS(req),
 	TP_STRUCT__entry(
-		__string(name, req->priv_ep->name)
+		__field(u32, edw3)
 		__field(struct cdns3_request *, req)
 		__field(void *, buf)
 		__field(unsigned int, actual)
 		__field(unsigned int, length)
 		__field(int, status)
-		__field(int, zero)
-		__field(int, short_not_ok)
-		__field(int, no_interrupt)
+		__field(u32, rdw1)
 		__field(int, start_trb)
 		__field(int, end_trb)
 		__field(int, flags)
 		__field(unsigned int, stream_id)
 	),
 	TP_fast_assign(
-		__assign_str(name, req->priv_ep->name);
+		__entry->edw3 = req->priv_ep->endpoint.dw3;
 		__entry->req = req;
 		__entry->buf = req->request.buf;
 		__entry->actual = req->request.actual;
 		__entry->length = req->request.length;
 		__entry->status = req->request.status;
-		__entry->zero = req->request.zero;
-		__entry->short_not_ok = req->request.short_not_ok;
-		__entry->no_interrupt = req->request.no_interrupt;
+		__entry->rdw1 = req->request.dw1;
 		__entry->start_trb = req->start_trb;
 		__entry->end_trb = req->end_trb;
 		__entry->flags = req->flags;
-		__entry->stream_id = req->request.stream_id;
 	),
 	TP_printk("%s: req: %p, req buff %p, length: %u/%u %s%s%s, status: %d,"
 		  " trb: [start:%d, end:%d], flags:%x SID: %u",
-		__get_str(name), __entry->req, __entry->buf, __entry->actual,
+		USB_EP_NAME(__entry->edw3), __entry->req, __entry->buf, __entry->actual,
 		__entry->length,
-		__entry->zero ? "Z" : "z",
-		__entry->short_not_ok ? "S" : "s",
-		__entry->no_interrupt ? "I" : "i",
+		USB_REQ_ZERO(__entry->rdw1) ? "Z" : "z",
+		USB_REQ_SHORT_NOT_OK(__entry->rdw1) ? "S" : "s",
+		USB_REQ_NO_INTERRUPT(__entry->rdw1) ? "I" : "i",
 		__entry->status,
 		__entry->start_trb,
 		__entry->end_trb,
 		__entry->flags,
-		__entry->stream_id
+		USB_REQ_STREAM_ID(__entry->rdw1)
 	)
 );
 
@@ -287,21 +282,21 @@ DECLARE_EVENT_CLASS(cdns3_stream_split_transfer_len,
 	TP_PROTO(struct cdns3_request *req),
 	TP_ARGS(req),
 	TP_STRUCT__entry(
-		__string(name, req->priv_ep->name)
+		__field(u32, edw3)
 		__field(struct cdns3_request *, req)
 		__field(unsigned int, length)
 		__field(unsigned int, actual)
 		__field(unsigned int, stream_id)
 	),
 	TP_fast_assign(
-		__assign_str(name, req->priv_ep->name);
+		__entry->edw3 = req->priv_ep->endpoint.dw3;
 		__entry->req = req;
 		__entry->actual = req->request.length;
 		__entry->length = req->request.actual;
 		__entry->stream_id = req->request.stream_id;
 	),
 	TP_printk("%s: req: %p,request length: %u actual length: %u  SID: %u",
-		  __get_str(name), __entry->req, __entry->length,
+		  USB_EP_NAME(__entry->edw3), __entry->req, __entry->length,
 		  __entry->actual, __entry->stream_id)
 );
 
@@ -320,7 +315,7 @@ DECLARE_EVENT_CLASS(cdns3_log_aligned_request,
 	TP_PROTO(struct cdns3_request *priv_req),
 	TP_ARGS(priv_req),
 	TP_STRUCT__entry(
-		__string(name, priv_req->priv_ep->name)
+		__field(u32, edw3)
 		__field(struct usb_request *, req)
 		__field(void *, buf)
 		__field(dma_addr_t, dma)
@@ -329,7 +324,7 @@ DECLARE_EVENT_CLASS(cdns3_log_aligned_request,
 		__field(u32, aligned_buf_size)
 	),
 	TP_fast_assign(
-		__assign_str(name, priv_req->priv_ep->name);
+		__entry->edw3 = priv_req->priv_ep->endpoint.dw3;
 		__entry->req = &priv_req->request;
 		__entry->buf = priv_req->request.buf;
 		__entry->dma = priv_req->request.dma;
@@ -338,7 +333,7 @@ DECLARE_EVENT_CLASS(cdns3_log_aligned_request,
 		__entry->aligned_buf_size = priv_req->aligned_buf->size;
 	),
 	TP_printk("%s: req: %p, req buf %p, dma %pad a_buf %p a_dma %pad, size %d",
-		__get_str(name), __entry->req, __entry->buf, &__entry->dma,
+		USB_EP_NAME(__entry->edw3), __entry->req, __entry->buf, &__entry->dma,
 		__entry->aligned_buf, &__entry->aligned_dma,
 		__entry->aligned_buf_size
 	)
@@ -358,19 +353,19 @@ DECLARE_EVENT_CLASS(cdns3_log_map_request,
 	TP_PROTO(struct cdns3_request *priv_req),
 	TP_ARGS(priv_req),
 	TP_STRUCT__entry(
-		__string(name, priv_req->priv_ep->name)
+		__field(u32, edw3)
 		__field(struct usb_request *, req)
 		__field(void *, buf)
 		__field(dma_addr_t, dma)
 	),
 	TP_fast_assign(
-		__assign_str(name, priv_req->priv_ep->name);
+		__entry->edw3 = priv_req->priv_ep->endpoint.dw3;
 		__entry->req = &priv_req->request;
 		__entry->buf = priv_req->request.buf;
 		__entry->dma = priv_req->request.dma;
 	),
 	TP_printk("%s: req: %p, req buf %p, dma %p",
-		  __get_str(name), __entry->req, __entry->buf, &__entry->dma
+		  USB_EP_NAME(__entry->edw3), __entry->req, __entry->buf, &__entry->dma
 	)
 );
 DEFINE_EVENT(cdns3_log_map_request, cdns3_map_request,
@@ -386,7 +381,7 @@ DECLARE_EVENT_CLASS(cdns3_log_trb,
 	TP_PROTO(struct cdns3_endpoint *priv_ep, struct cdns3_trb *trb),
 	TP_ARGS(priv_ep, trb),
 	TP_STRUCT__entry(
-		__string(name, priv_ep->name)
+		__field(u32, edw3)
 		__field(struct cdns3_trb *, trb)
 		__field(u32, buffer)
 		__field(u32, length)
@@ -395,7 +390,7 @@ DECLARE_EVENT_CLASS(cdns3_log_trb,
 		__field(unsigned int, last_stream_id)
 	),
 	TP_fast_assign(
-		__assign_str(name, priv_ep->name);
+		__entry->edw3 = priv_ep->endpoint.dw3;
 		__entry->trb = trb;
 		__entry->buffer = le32_to_cpu(trb->buffer);
 		__entry->length = le32_to_cpu(trb->length);
@@ -404,7 +399,7 @@ DECLARE_EVENT_CLASS(cdns3_log_trb,
 		__entry->last_stream_id = priv_ep->last_stream_id;
 	),
 	TP_printk("%s: trb %p, dma buf: 0x%08x, size: %ld, burst: %d ctrl: 0x%08x (%s%s%s%s%s%s%s) SID:%lu LAST_SID:%u",
-		__get_str(name), __entry->trb, __entry->buffer,
+		USB_EP_NAME(__entry->edw3), __entry->trb, __entry->buffer,
 		TRB_LEN(__entry->length),
 		(u8)TRB_BURST_LEN_GET(__entry->length),
 		__entry->control,
@@ -455,24 +450,20 @@ DECLARE_EVENT_CLASS(cdns3_log_ep,
 	TP_PROTO(struct cdns3_endpoint *priv_ep),
 	TP_ARGS(priv_ep),
 	TP_STRUCT__entry(
-		__string(name, priv_ep->name)
-		__field(unsigned int, maxpacket)
-		__field(unsigned int, maxpacket_limit)
-		__field(unsigned int, max_streams)
+		__field(u32, edw3)
+		__field(u32, edw1)
+		__field(u32, edw2)
 		__field(unsigned int, use_streams)
-		__field(unsigned int, maxburst)
 		__field(unsigned int, flags)
 		__field(unsigned int, dir)
 		__field(u8, enqueue)
 		__field(u8, dequeue)
 	),
 	TP_fast_assign(
-		__assign_str(name, priv_ep->name);
-		__entry->maxpacket = priv_ep->endpoint.maxpacket;
-		__entry->maxpacket_limit = priv_ep->endpoint.maxpacket_limit;
-		__entry->max_streams = priv_ep->endpoint.max_streams;
+		__entry->edw3 = priv_ep->endpoint.dw3;
+		__entry->edw2 = priv_ep->endpoint.dw2;
+		__entry->edw1 = priv_ep->endpoint.dw1;
 		__entry->use_streams = priv_ep->use_streams;
-		__entry->maxburst = priv_ep->endpoint.maxburst;
 		__entry->flags = priv_ep->flags;
 		__entry->dir = priv_ep->dir;
 		__entry->enqueue = priv_ep->enqueue;
@@ -480,10 +471,10 @@ DECLARE_EVENT_CLASS(cdns3_log_ep,
 	),
 	TP_printk("%s: mps: %d/%d. streams: %d, stream enable: %d, burst: %d, "
 		  "enq idx: %d, deq idx: %d, flags %s%s%s%s%s%s%s%s, dir: %s",
-		__get_str(name), __entry->maxpacket,
-		__entry->maxpacket_limit, __entry->max_streams,
+		USB_EP_NAME(__entry->edw3), USB_EP_MAXPACKET(__entry->edw1),
+		USB_EP_MAXPACKET_LIMIT(__entry->edw1), USB_EP_MAX_STREAMS(__entry->edw2),
 		__entry->use_streams,
-		__entry->maxburst, __entry->enqueue,
+		USB_EP_MAXBURST(__entry->edw2), __entry->enqueue,
 		__entry->dequeue,
 		__entry->flags & EP_ENABLED ? "EN | " : "",
 		__entry->flags & EP_STALLED ? "STALLED | " : "",
-- 
2.17.1

[PATCH v3 07/10] usb: dwc3: trace: reduce buffer usage of trace event

[Linyu Yuan]

Save u32 members into trace event ring buffer and parse it for possible
bit information.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
v2: no change
v3: no change

 drivers/usb/dwc3/trace.h | 54 +++++++++++++++++-----------------------
 1 file changed, 23 insertions(+), 31 deletions(-)

diff --git a/drivers/usb/dwc3/trace.h b/drivers/usb/dwc3/trace.h
index d2997d17cfbe..e1885b83fff8 100644
--- a/drivers/usb/dwc3/trace.h
+++ b/drivers/usb/dwc3/trace.h
@@ -102,30 +102,26 @@ DECLARE_EVENT_CLASS(dwc3_log_request,
 	TP_PROTO(struct dwc3_request *req),
 	TP_ARGS(req),
 	TP_STRUCT__entry(
-		__string(name, req->dep->name)
+		__field(u32, edw3)
 		__field(struct dwc3_request *, req)
 		__field(unsigned int, actual)
 		__field(unsigned int, length)
 		__field(int, status)
-		__field(int, zero)
-		__field(int, short_not_ok)
-		__field(int, no_interrupt)
+		__field(u32, rdw1)
 	),
 	TP_fast_assign(
-		__assign_str(name, req->dep->name);
+		__entry->edw3 = req->dep->endpoint.dw3;
 		__entry->req = req;
 		__entry->actual = req->request.actual;
 		__entry->length = req->request.length;
 		__entry->status = req->request.status;
-		__entry->zero = req->request.zero;
-		__entry->short_not_ok = req->request.short_not_ok;
-		__entry->no_interrupt = req->request.no_interrupt;
+		__entry->rdw1 = req->request.dw1;
 	),
 	TP_printk("%s: req %p length %u/%u %s%s%s ==> %d",
-		__get_str(name), __entry->req, __entry->actual, __entry->length,
-		__entry->zero ? "Z" : "z",
-		__entry->short_not_ok ? "S" : "s",
-		__entry->no_interrupt ? "i" : "I",
+		USB_EP_NAME(__entry->edw3), __entry->req, __entry->actual, __entry->length,
+		USB_REQ_ZERO(__entry->rdw1) ? "Z" : "z",
+		USB_REQ_SHORT_NOT_OK(__entry->rdw1) ? "S" : "s",
+		USB_REQ_NO_INTERRUPT(__entry->rdw1) ? "i" : "I",
 		__entry->status
 	)
 );
@@ -185,7 +181,7 @@ DECLARE_EVENT_CLASS(dwc3_log_gadget_ep_cmd,
 		struct dwc3_gadget_ep_cmd_params *params, int cmd_status),
 	TP_ARGS(dep, cmd, params, cmd_status),
 	TP_STRUCT__entry(
-		__string(name, dep->name)
+		__field(u32, edw3)
 		__field(unsigned int, cmd)
 		__field(u32, param0)
 		__field(u32, param1)
@@ -193,7 +189,7 @@ DECLARE_EVENT_CLASS(dwc3_log_gadget_ep_cmd,
 		__field(int, cmd_status)
 	),
 	TP_fast_assign(
-		__assign_str(name, dep->name);
+		__entry->edw3 = dep->endpoint.dw3;
 		__entry->cmd = cmd;
 		__entry->param0 = params->param0;
 		__entry->param1 = params->param1;
@@ -201,7 +197,7 @@ DECLARE_EVENT_CLASS(dwc3_log_gadget_ep_cmd,
 		__entry->cmd_status = cmd_status;
 	),
 	TP_printk("%s: cmd '%s' [%x] params %08x %08x %08x --> status: %s",
-		__get_str(name), dwc3_gadget_ep_cmd_string(__entry->cmd),
+		USB_EP_NAME(__entry->edw3), dwc3_gadget_ep_cmd_string(__entry->cmd),
 		__entry->cmd, __entry->param0,
 		__entry->param1, __entry->param2,
 		dwc3_ep_cmd_status_string(__entry->cmd_status)
@@ -218,7 +214,7 @@ DECLARE_EVENT_CLASS(dwc3_log_trb,
 	TP_PROTO(struct dwc3_ep *dep, struct dwc3_trb *trb),
 	TP_ARGS(dep, trb),
 	TP_STRUCT__entry(
-		__string(name, dep->name)
+		__field(u32, edw3)
 		__field(struct dwc3_trb *, trb)
 		__field(u32, bpl)
 		__field(u32, bph)
@@ -229,7 +225,7 @@ DECLARE_EVENT_CLASS(dwc3_log_trb,
 		__field(u32, dequeue)
 	),
 	TP_fast_assign(
-		__assign_str(name, dep->name);
+		__entry->edw3 = dep->endpoint.dw3;
 		__entry->trb = trb;
 		__entry->bpl = trb->bpl;
 		__entry->bph = trb->bph;
@@ -240,7 +236,7 @@ DECLARE_EVENT_CLASS(dwc3_log_trb,
 		__entry->dequeue = dep->trb_dequeue;
 	),
 	TP_printk("%s: trb %p (E%d:D%d) buf %08x%08x size %s%d ctrl %08x sofn %08x (%c%c%c%c:%c%c:%s)",
-		__get_str(name), __entry->trb, __entry->enqueue,
+		USB_EP_NAME(__entry->edw3), __entry->trb, __entry->enqueue,
 		__entry->dequeue, __entry->bph, __entry->bpl,
 		({char *s;
 		int pcm = ((__entry->size >> 24) & 3) + 1;
@@ -290,31 +286,27 @@ DECLARE_EVENT_CLASS(dwc3_log_ep,
 	TP_PROTO(struct dwc3_ep *dep),
 	TP_ARGS(dep),
 	TP_STRUCT__entry(
-		__string(name, dep->name)
-		__field(unsigned int, maxpacket)
-		__field(unsigned int, maxpacket_limit)
-		__field(unsigned int, max_streams)
-		__field(unsigned int, maxburst)
+		__field(u32, edw3)
+		__field(u32, edw1)
+		__field(u32, edw2)
 		__field(unsigned int, flags)
 		__field(unsigned int, direction)
 		__field(u8, trb_enqueue)
 		__field(u8, trb_dequeue)
 	),
 	TP_fast_assign(
-		__assign_str(name, dep->name);
-		__entry->maxpacket = dep->endpoint.maxpacket;
-		__entry->maxpacket_limit = dep->endpoint.maxpacket_limit;
-		__entry->max_streams = dep->endpoint.max_streams;
-		__entry->maxburst = dep->endpoint.maxburst;
+		__entry->edw3 = dep->endpoint.dw3;
+		__entry->edw1 = dep->endpoint.dw1;
+		__entry->edw2 = dep->endpoint.dw2;
 		__entry->flags = dep->flags;
 		__entry->direction = dep->direction;
 		__entry->trb_enqueue = dep->trb_enqueue;
 		__entry->trb_dequeue = dep->trb_dequeue;
 	),
 	TP_printk("%s: mps %d/%d streams %d burst %d ring %d/%d flags %c:%c%c%c%c:%c",
-		__get_str(name), __entry->maxpacket,
-		__entry->maxpacket_limit, __entry->max_streams,
-		__entry->maxburst, __entry->trb_enqueue,
+		USB_EP_NAME(__entry->edw3), USB_EP_MAXPACKET(__entry->edw1),
+		USB_EP_MAXPACKET_LIMIT(__entry->edw1), USB_EP_MAX_STREAMS(__entry->edw2),
+		USB_EP_MAXBURST(__entry->edw2), __entry->trb_enqueue,
 		__entry->trb_dequeue,
 		__entry->flags & DWC3_EP_ENABLED ? 'E' : 'e',
 		__entry->flags & DWC3_EP_STALL ? 'S' : 's',
-- 
2.17.1

[PATCH v3 08/10] usb: cdns2: trace: reduce buffer usage of trace event

[Linyu Yuan]

Save u32 members into trace event ring buffer and parse it for possible
bit information.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
v2: no change
v3: no change

 drivers/usb/gadget/udc/cdns2/cdns2-trace.h | 77 ++++++++++------------
 1 file changed, 35 insertions(+), 42 deletions(-)

diff --git a/drivers/usb/gadget/udc/cdns2/cdns2-trace.h b/drivers/usb/gadget/udc/cdns2/cdns2-trace.h
index 61f241634ea5..23d15597dbaa 100644
--- a/drivers/usb/gadget/udc/cdns2/cdns2-trace.h
+++ b/drivers/usb/gadget/udc/cdns2/cdns2-trace.h
@@ -98,46 +98,45 @@ TRACE_EVENT(cdns2_ep_halt,
 	TP_PROTO(struct cdns2_endpoint *ep_priv, u8 halt, u8 flush),
 	TP_ARGS(ep_priv, halt, flush),
 	TP_STRUCT__entry(
-		__string(name, ep_priv->name)
+		__field(u32, edw3)
 		__field(u8, halt)
 		__field(u8, flush)
 	),
 	TP_fast_assign(
-		__assign_str(name, ep_priv->name);
+		__entry->edw3 = ep_priv->endpoint.dw3;
 		__entry->halt = halt;
 		__entry->flush = flush;
 	),
 	TP_printk("Halt %s for %s: %s", __entry->flush ? " and flush" : "",
-		  __get_str(name), __entry->halt ? "set" : "cleared")
+		  USB_EP_NAME(__entry->edw3), __entry->halt ? "set" : "cleared")
 );
 
 TRACE_EVENT(cdns2_wa1,
 	TP_PROTO(struct cdns2_endpoint *ep_priv, char *msg),
 	TP_ARGS(ep_priv, msg),
 	TP_STRUCT__entry(
-		__string(ep_name, ep_priv->name)
+		__field(u32, edw3)
 		__string(msg, msg)
 	),
 	TP_fast_assign(
-		__assign_str(ep_name, ep_priv->name);
+		__entry->edw3 = ep_priv->endpoint.dw3;
 		__assign_str(msg, msg);
 	),
-	TP_printk("WA1: %s %s", __get_str(ep_name), __get_str(msg))
+	TP_printk("WA1: %s %s", USB_EP_NAME(__entry->edw3), __get_str(msg))
 );
 
 DECLARE_EVENT_CLASS(cdns2_log_doorbell,
 	TP_PROTO(struct cdns2_endpoint *pep, u32 ep_trbaddr),
 	TP_ARGS(pep, ep_trbaddr),
 	TP_STRUCT__entry(
-		__string(name, pep->num ? pep->name :
-				(pep->dir ? "ep0in" : "ep0out"))
+		__field(u32, edw3)
 		__field(u32, ep_trbaddr)
 	),
 	TP_fast_assign(
-		__assign_str(name, pep->name);
+		__entry->edw3 = pep->endpoint.dw3;
 		__entry->ep_trbaddr = ep_trbaddr;
 	),
-	TP_printk("%s, ep_trbaddr %08x", __get_str(name),
+	TP_printk("%s, ep_trbaddr %08x", USB_EP_NAME(__entry->edw3),
 		  __entry->ep_trbaddr)
 );
 
@@ -190,20 +189,20 @@ DECLARE_EVENT_CLASS(cdns2_log_epx_irq,
 	TP_PROTO(struct cdns2_device *pdev, struct cdns2_endpoint *pep),
 	TP_ARGS(pdev, pep),
 	TP_STRUCT__entry(
-		__string(ep_name, pep->name)
+		__field(u32, edw3)
 		__field(u32, ep_sts)
 		__field(u32, ep_ists)
 		__field(u32, ep_traddr)
 	),
 	TP_fast_assign(
-		__assign_str(ep_name, pep->name);
+		__entry->edw3 = pep->endpoint.dw3;
 		__entry->ep_sts = readl(&pdev->adma_regs->ep_sts);
 		__entry->ep_ists = readl(&pdev->adma_regs->ep_ists);
 		__entry->ep_traddr = readl(&pdev->adma_regs->ep_traddr);
 	),
 	TP_printk("%s, ep_traddr: %08x",
 		  cdns2_decode_epx_irq(__get_buf(CDNS2_MSG_MAX), CDNS2_MSG_MAX,
-				       __get_str(ep_name),
+				       USB_EP_NAME(__entry->edw3),
 				       __entry->ep_ists, __entry->ep_sts),
 		  __entry->ep_traddr)
 );
@@ -270,7 +269,7 @@ DECLARE_EVENT_CLASS(cdns2_log_request,
 	TP_PROTO(struct cdns2_request *preq),
 	TP_ARGS(preq),
 	TP_STRUCT__entry(
-		__string(name, preq->pep->name)
+		__field(u32, edw3)
 		__field(struct usb_request *, request)
 		__field(struct cdns2_request *, preq)
 		__field(void *, buf)
@@ -278,9 +277,7 @@ DECLARE_EVENT_CLASS(cdns2_log_request,
 		__field(unsigned int, length)
 		__field(int, status)
 		__field(dma_addr_t, dma)
-		__field(int, zero)
-		__field(int, short_not_ok)
-		__field(int, no_interrupt)
+		__field(u32, rdw1)
 		__field(struct scatterlist*, sg)
 		__field(unsigned int, num_sgs)
 		__field(unsigned int, num_mapped_sgs)
@@ -288,7 +285,7 @@ DECLARE_EVENT_CLASS(cdns2_log_request,
 		__field(int, end_trb)
 	),
 	TP_fast_assign(
-		__assign_str(name, preq->pep->name);
+		__entry->edw3 = preq->pep->endpoint.dw3;
 		__entry->request = &preq->request;
 		__entry->preq = preq;
 		__entry->buf = preq->request.buf;
@@ -296,9 +293,7 @@ DECLARE_EVENT_CLASS(cdns2_log_request,
 		__entry->length = preq->request.length;
 		__entry->status = preq->request.status;
 		__entry->dma = preq->request.dma;
-		__entry->zero = preq->request.zero;
-		__entry->short_not_ok = preq->request.short_not_ok;
-		__entry->no_interrupt = preq->request.no_interrupt;
+		__entry->rdw1 = preq->request.dw1;
 		__entry->sg = preq->request.sg;
 		__entry->num_sgs = preq->request.num_sgs;
 		__entry->num_mapped_sgs = preq->request.num_mapped_sgs;
@@ -308,12 +303,12 @@ DECLARE_EVENT_CLASS(cdns2_log_request,
 	TP_printk("%s: req: %p, preq: %p, req buf: %p, length: %u/%u, status: %d,"
 		  "buf dma: (%pad), %s%s%s, sg: %p, num_sgs: %d, num_m_sgs: %d,"
 		  "trb: [start: %d, end: %d]",
-		  __get_str(name), __entry->request, __entry->preq,
+		  USB_EP_NAME(__entry->edw3), __entry->request, __entry->preq,
 		  __entry->buf, __entry->actual, __entry->length,
 		  __entry->status, &__entry->dma,
-		  __entry->zero ? "Z" : "z",
-		  __entry->short_not_ok ? "S" : "s",
-		  __entry->no_interrupt ? "I" : "i",
+		  USB_REQ_ZERO(__entry->rdw1) ? "Z" : "z",
+		  USB_REQ_SHORT_NOT_OK(__entry->rdw1) ? "S" : "s",
+		  USB_REQ_NO_INTERRUPT(__entry->rdw1) ? "I" : "i",
 		  __entry->sg, __entry->num_sgs, __entry->num_mapped_sgs,
 		  __entry->start_trb,
 		  __entry->end_trb
@@ -374,19 +369,19 @@ DECLARE_EVENT_CLASS(cdns2_log_map_request,
 	TP_PROTO(struct cdns2_request *priv_req),
 	TP_ARGS(priv_req),
 	TP_STRUCT__entry(
-		__string(name, priv_req->pep->name)
+		__field(u32, edw3)
 		__field(struct usb_request *, req)
 		__field(void *, buf)
 		__field(dma_addr_t, dma)
 	),
 	TP_fast_assign(
-		__assign_str(name, priv_req->pep->name);
+		__entry->edw3 = priv_req->pep->endpoint.dw3;
 		__entry->req = &priv_req->request;
 		__entry->buf = priv_req->request.buf;
 		__entry->dma = priv_req->request.dma;
 	),
 	TP_printk("%s: req: %p, req buf %p, dma %p",
-		  __get_str(name), __entry->req, __entry->buf, &__entry->dma
+		  USB_EP_NAME(__entry->edw3), __entry->req, __entry->buf, &__entry->dma
 	)
 );
 
@@ -403,7 +398,7 @@ DECLARE_EVENT_CLASS(cdns2_log_trb,
 	TP_PROTO(struct cdns2_endpoint *pep, struct cdns2_trb *trb),
 	TP_ARGS(pep, trb),
 	TP_STRUCT__entry(
-		__string(name, pep->name)
+		__field(u32, edw3)
 		__field(struct cdns2_trb *, trb)
 		__field(u32, buffer)
 		__field(u32, length)
@@ -411,7 +406,7 @@ DECLARE_EVENT_CLASS(cdns2_log_trb,
 		__field(u32, type)
 	),
 	TP_fast_assign(
-		__assign_str(name, pep->name);
+		__entry->edw3 = pep->endpoint.dw3;
 		__entry->trb = trb;
 		__entry->buffer = le32_to_cpu(trb->buffer);
 		__entry->length = le32_to_cpu(trb->length);
@@ -419,7 +414,7 @@ DECLARE_EVENT_CLASS(cdns2_log_trb,
 		__entry->type = usb_endpoint_type(pep->endpoint.desc);
 	),
 	TP_printk("%s: trb V: %p, dma buf: P: 0x%08x, %s",
-		 __get_str(name), __entry->trb, __entry->buffer,
+		 USB_EP_NAME(__entry->edw3), __entry->trb, __entry->buffer,
 		 cdns2_decode_trb(__get_buf(CDNS2_MSG_MAX), CDNS2_MSG_MAX,
 				  __entry->control, __entry->length,
 				  __entry->buffer))
@@ -467,18 +462,16 @@ DECLARE_EVENT_CLASS(cdns2_log_ep,
 	TP_PROTO(struct cdns2_endpoint *pep),
 	TP_ARGS(pep),
 	TP_STRUCT__entry(
-		__string(name, pep->name)
-		__field(unsigned int, maxpacket)
-		__field(unsigned int, maxpacket_limit)
+		__field(u32, edw3)
+		__field(u32, edw1)
 		__field(unsigned int, flags)
 		__field(unsigned int, dir)
 		__field(u8, enqueue)
 		__field(u8, dequeue)
 	),
 	TP_fast_assign(
-		__assign_str(name, pep->name);
-		__entry->maxpacket = pep->endpoint.maxpacket;
-		__entry->maxpacket_limit = pep->endpoint.maxpacket_limit;
+		__entry->edw3 = pep->endpoint.dw3;
+		__entry->edw1 = pep->endpoint.dw1;
 		__entry->flags = pep->ep_state;
 		__entry->dir = pep->dir;
 		__entry->enqueue = pep->ring.enqueue;
@@ -486,8 +479,8 @@ DECLARE_EVENT_CLASS(cdns2_log_ep,
 	),
 	TP_printk("%s: mps: %d/%d, enq idx: %d, deq idx: %d, "
 		  "flags: %s%s%s%s, dir: %s",
-		__get_str(name), __entry->maxpacket,
-		__entry->maxpacket_limit, __entry->enqueue,
+		USB_EP_NAME(__entry->edw3), USB_EP_MAXPACKET(__entry->edw1),
+		USB_EP_MAXPACKET_LIMIT(__entry->edw1), __entry->enqueue,
 		__entry->dequeue,
 		__entry->flags & EP_ENABLED ? "EN | " : "",
 		__entry->flags & EP_STALLED ? "STALLED | " : "",
@@ -559,7 +552,7 @@ DECLARE_EVENT_CLASS(cdns2_log_epx_reg_config,
 	TP_PROTO(struct cdns2_device *pdev, struct cdns2_endpoint *pep),
 	TP_ARGS(pdev, pep),
 	TP_STRUCT__entry(
-		__string(ep_name, pep->name)
+		__field(u32, edw3)
 		__field(u8, burst_size)
 		__field(__le16, maxpack_reg)
 		__field(__u8, con_reg)
@@ -568,7 +561,7 @@ DECLARE_EVENT_CLASS(cdns2_log_epx_reg_config,
 		__field(u32, ep_cfg_reg)
 	),
 	TP_fast_assign(
-		__assign_str(ep_name, pep->name);
+		__entry->edw3 = pep->endpoint.dw3;
 		__entry->burst_size = pep->trb_burst_size;
 		__entry->maxpack_reg = pep->dir ? readw(&pdev->epx_regs->txmaxpack[pep->num - 1]) :
 						  readw(&pdev->epx_regs->rxmaxpack[pep->num - 1]);
@@ -581,7 +574,7 @@ DECLARE_EVENT_CLASS(cdns2_log_epx_reg_config,
 
 	TP_printk("%s, maxpack: %d, con: %02x, dma_ep_sel: %08x, dma_ep_sts_en: %08x"
 		  " dma_ep_cfg %08x",
-		  __get_str(ep_name), __entry->maxpack_reg, __entry->con_reg,
+		 USB_EP_NAME(__entry->edw3), __entry->maxpack_reg, __entry->con_reg,
 		  __entry->ep_sel_reg, __entry->ep_sts_en_reg,
 		  __entry->ep_cfg_reg
 	)
-- 
2.17.1

[PATCH v3 09/10] usb: mtu3: trace: reduce buffer usage of trace event

[Linyu Yuan]

Save u32 members into trace event ring buffer and parse it for possible
bit information.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
v2: no change
v3: no change

 drivers/usb/mtu3/mtu3_trace.h | 42 ++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 23 deletions(-)

diff --git a/drivers/usb/mtu3/mtu3_trace.h b/drivers/usb/mtu3/mtu3_trace.h
index 03d2a9bac27e..bfd97958c8a6 100644
--- a/drivers/usb/mtu3/mtu3_trace.h
+++ b/drivers/usb/mtu3/mtu3_trace.h
@@ -117,30 +117,28 @@ DECLARE_EVENT_CLASS(mtu3_log_request,
 	TP_PROTO(struct mtu3_request *mreq),
 	TP_ARGS(mreq),
 	TP_STRUCT__entry(
-		__string(name, mreq->mep->name)
+		__field(u32, edw3)
 		__field(struct mtu3_request *, mreq)
 		__field(struct qmu_gpd *, gpd)
 		__field(unsigned int, actual)
 		__field(unsigned int, length)
 		__field(int, status)
-		__field(int, zero)
-		__field(int, no_interrupt)
+		__field(u32, rdw1)
 	),
 	TP_fast_assign(
-		__assign_str(name, mreq->mep->name);
+		__entry->edw3 = mreq->mep->ep.dw3;
 		__entry->mreq = mreq;
 		__entry->gpd = mreq->gpd;
 		__entry->actual = mreq->request.actual;
 		__entry->length = mreq->request.length;
 		__entry->status = mreq->request.status;
-		__entry->zero = mreq->request.zero;
-		__entry->no_interrupt = mreq->request.no_interrupt;
+		__entry->rdw1 = mreq->request.dw1;
 	),
 	TP_printk("%s: req %p gpd %p len %u/%u %s%s --> %d",
-		__get_str(name), __entry->mreq, __entry->gpd,
+		USB_EP_NAME(__entry->edw3), __entry->mreq, __entry->gpd,
 		__entry->actual, __entry->length,
-		__entry->zero ? "Z" : "z",
-		__entry->no_interrupt ? "i" : "I",
+		USB_REQ_ZERO(__entry->rdw1) ? "Z" : "z",
+		USB_REQ_NO_INTERRUPT(__entry->rdw1) ? "i" : "I",
 		__entry->status
 	)
 );
@@ -174,7 +172,7 @@ DECLARE_EVENT_CLASS(mtu3_log_gpd,
 	TP_PROTO(struct mtu3_ep *mep, struct qmu_gpd *gpd),
 	TP_ARGS(mep, gpd),
 	TP_STRUCT__entry(
-		__string(name, mep->name)
+		__field(u32, edw3)
 		__field(struct qmu_gpd *, gpd)
 		__field(u32, dw0)
 		__field(u32, dw1)
@@ -182,7 +180,7 @@ DECLARE_EVENT_CLASS(mtu3_log_gpd,
 		__field(u32, dw3)
 	),
 	TP_fast_assign(
-		__assign_str(name, mep->name);
+		__entry->edw3 = mep->ep.dw3;
 		__entry->gpd = gpd;
 		__entry->dw0 = le32_to_cpu(gpd->dw0_info);
 		__entry->dw1 = le32_to_cpu(gpd->next_gpd);
@@ -190,7 +188,7 @@ DECLARE_EVENT_CLASS(mtu3_log_gpd,
 		__entry->dw3 = le32_to_cpu(gpd->dw3_info);
 	),
 	TP_printk("%s: gpd %p - %08x %08x %08x %08x",
-		__get_str(name), __entry->gpd,
+		USB_EP_NAME(__entry->edw3), __entry->gpd,
 		__entry->dw0, __entry->dw1,
 		__entry->dw2, __entry->dw3
 	)
@@ -215,31 +213,29 @@ DECLARE_EVENT_CLASS(mtu3_log_ep,
 	TP_PROTO(struct mtu3_ep *mep),
 	TP_ARGS(mep),
 	TP_STRUCT__entry(
-		__string(name, mep->name)
+		__field(u32, edw3)
 		__field(unsigned int, type)
 		__field(unsigned int, slot)
-		__field(unsigned int, maxp)
-		__field(unsigned int, mult)
-		__field(unsigned int, maxburst)
+		__field(u32, edw1)
+		__field(u32, edw2)
 		__field(unsigned int, flags)
 		__field(unsigned int, direction)
 		__field(struct mtu3_gpd_ring *, gpd_ring)
 	),
 	TP_fast_assign(
-		__assign_str(name, mep->name);
+		__entry->edw3 = mep->ep.dw3;
 		__entry->type = mep->type;
 		__entry->slot = mep->slot;
-		__entry->maxp = mep->ep.maxpacket;
-		__entry->mult = mep->ep.mult;
-		__entry->maxburst = mep->ep.maxburst;
+		__entry->edw1 = mep->ep.dw1;
+		__entry->edw2 = mep->ep.dw2;
 		__entry->flags = mep->flags;
 		__entry->direction = mep->is_in;
 		__entry->gpd_ring = &mep->gpd_ring;
 	),
 	TP_printk("%s: type %s maxp %d slot %d mult %d burst %d ring %p/%pad flags %c:%c%c%c:%c",
-		__get_str(name), usb_ep_type_string(__entry->type),
-		__entry->maxp, __entry->slot,
-		__entry->mult, __entry->maxburst,
+		USB_EP_NAME(__entry->edw3), usb_ep_type_string(__entry->type),
+		USB_EP_MAXPACKET(__entry->edw1), __entry->slot,
+		USB_EP_MULT(__entry->edw2), USB_EP_MAXBURST(__entry->edw2),
 		__entry->gpd_ring, &__entry->gpd_ring->dma,
 		__entry->flags & MTU3_EP_ENABLED ? 'E' : 'e',
 		__entry->flags & MTU3_EP_STALL ? 'S' : 's',
-- 
2.17.1

[PATCH v3 10/10] usb: musb: trace: reduce buffer usage of trace event

[Linyu Yuan]

Save u32 member into trace event ring buffer and parse it for possible
bit information.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
v2: no change
v3: no change

 drivers/usb/musb/musb_trace.h | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/musb/musb_trace.h b/drivers/usb/musb/musb_trace.h
index f246b14394c4..6986a89767a2 100644
--- a/drivers/usb/musb/musb_trace.h
+++ b/drivers/usb/musb/musb_trace.h
@@ -243,9 +243,7 @@ DECLARE_EVENT_CLASS(musb_req,
 		__field(int, status)
 		__field(unsigned int, buf_len)
 		__field(unsigned int, actual_len)
-		__field(unsigned int, zero)
-		__field(unsigned int, short_not_ok)
-		__field(unsigned int, no_interrupt)
+		__field(u32, rdw1)
 	),
 	TP_fast_assign(
 		__entry->req = &req->request;
@@ -254,16 +252,14 @@ DECLARE_EVENT_CLASS(musb_req,
 		__entry->status = req->request.status;
 		__entry->buf_len = req->request.length;
 		__entry->actual_len = req->request.actual;
-		__entry->zero = req->request.zero;
-		__entry->short_not_ok = req->request.short_not_ok;
-		__entry->no_interrupt = req->request.no_interrupt;
+		__entry->rdw1 = req->request.dw1;
 	),
 	TP_printk("%p, ep%d %s, %s%s%s, len %d/%d, status %d",
 			__entry->req, __entry->epnum,
 			__entry->is_tx ? "tx/IN" : "rx/OUT",
-			__entry->zero ? "Z" : "z",
-			__entry->short_not_ok ? "S" : "s",
-			__entry->no_interrupt ? "I" : "i",
+			USB_REQ_ZERO(__entry->rdw1) ? "Z" : "z",
+			USB_REQ_SHORT_NOT_OK(__entry->rdw1) ? "S" : "s",
+			USB_REQ_NO_INTERRUPT(__entry->rdw1) ? "I" : "i",
 			__entry->actual_len, __entry->buf_len,
 			__entry->status
 	)
-- 
2.17.1

Re: [PATCH v3 10/10] usb: musb: trace: reduce buffer usage of trace event

[Greg Kroah-Hartman]

On Tue, Sep 12, 2023 at 06:44:55PM +0800, Linyu Yuan wrote:
> Save u32 member into trace event ring buffer and parse it for possible
> bit information.
> 
> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> ---
> v2: no change
> v3: no change

I asked you to change previous versions of this patch (and other patches
in this series.)  Why you ignored my review comments is odd...