[PATCH v7 0/4] usb: gadget: reduce usb gadget trace event buffer usage

[PATCH v7 0/4] usb: gadget: reduce usb gadget trace event buffer usage

[Linyu Yuan]

some trace event use an integer 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.

save one integer member which can parse it for multiple bit field
information.

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

in order to make sure bit field have same bit postion in memory for little
and big endian, define the bit fields differently according host cpu
endian type.

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
    https://lore.kernel.org/linux-usb/20230912104455.7737-1-quic_linyyuan@quicinc.com/
v4: add DECLARE_EVENT_CLASS_PRINT_INIT() new trace class and use it
    https://lore.kernel.org/linux-usb/20230914100302.30274-1-quic_linyyuan@quicinc.com/
v5: use cpu_to_le32() at fast assign stage to fix endian issue
    https://lore.kernel.org/linux-usb/20230915051123.26486-1-quic_linyyuan@quicinc.com/
v6: missing three cpu_to_le32() usage in dwc3 trace
    https://lore.kernel.org/linux-usb/20230915052716.28540-1-quic_linyyuan@quicinc.com/
v7: define bit field according cpu endian type, remove wrong cpu_to_le32()
    usage which point by Alan Stern.

Linyu Yuan (4):
  usb: gadget: remove UDC_TRACE_STR_MAX definition
  usb: gadget: add anonymous definition in some struct for trace purpose
  usb: udc: trace: reduce buffer usage of trace event
  usb: dwc3: trace: reduce buffer usage of trace event

 drivers/usb/dwc3/trace.h       |  63 ++++----
 drivers/usb/gadget/udc/trace.h | 114 ++++----------
 include/linux/usb/gadget.h     | 268 +++++++++++++++++++++++++++------
 3 files changed, 285 insertions(+), 160 deletions(-)

-- 
2.17.1

[PATCH v7 1/4] usb: gadget: remove UDC_TRACE_STR_MAX definition

[Linyu Yuan]

It is not used anymore.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
v7: new patch

 include/linux/usb/gadget.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 75bda0783395..5b5a8c5f2c47 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -26,8 +26,6 @@
 #include <linux/workqueue.h>
 #include <linux/usb/ch9.h>
 
-#define UDC_TRACE_STR_MAX	512
-
 struct usb_ep;
 
 /**
-- 
2.17.1

[PATCH v7 2/4] usb: gadget: add anonymous definition in some struct for trace purpose

[Linyu Yuan]

Some UDC trace event will save usb udc information, but it use one int
size buffer to save one bit information of usb udc, it waste trace buffer.

Add anonymous union which have u32 members can be used by trace event
during fast assign stage to save more entries with same trace ring buffer
size.

In order to access each bit with BIT() macro, add different definition for
each bit fields according host little/big endian to make sure it has same
eacho bit field have same bit position in memory.

Add some macros or helper for later trace event usage which follow the
udc structs, As when possible future changes to udc related structs,
developers will easy notice them.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
 include/linux/usb/gadget.h | 266 +++++++++++++++++++++++++++++++------
 1 file changed, 226 insertions(+), 40 deletions(-)

diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 5b5a8c5f2c47..e06a3b972268 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -24,6 +24,10 @@
 #include <linux/scatterlist.h>
 #include <linux/types.h>
 #include <linux/workqueue.h>
+#include <linux/stdarg.h>
+#include <linux/bitops.h>
+#include <linux/sprintf.h>
+#include <linux/bitfield.h>
 #include <linux/usb/ch9.h>
 
 struct usb_ep;
@@ -39,6 +43,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).
@@ -103,12 +108,28 @@ 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 {
+#ifdef CONFIG_CPU_LITTLE_ENDIAN
+			u32	stream_id:16;
+			u32	is_last:1;
+			u32	no_interrupt:1;
+			u32	zero:1;
+			u32	short_not_ok:1;
+			u32	dma_mapped:1;
+			u32	pad:11;
+#else
+			u32	pad:11;
+			u32	dma_mapped:1;
+			u32	short_not_ok:1;
+			u32	zero:1;
+			u32	no_interrupt:1;
+			u32	is_last:1;
+			u32	stream_id:16;
+#endif
+		};
+		u32		dw1;
+	};
 
 	void			(*complete)(struct usb_ep *ep,
 					struct usb_request *req);
@@ -121,6 +142,20 @@ struct usb_request {
 	unsigned		actual;
 };
 
+#define		USB_REQ_STREAM_ID	GENMASK(15, 0)
+#define		USB_REQ_IS_LAST		BIT(16)
+#define		USB_REQ_NO_INTERRUPT	BIT(17)
+#define		USB_REQ_ZERO		BIT(18)
+#define		USB_REQ_SHORT_NOT_OK	BIT(19)
+#define		USB_REQ_DMA_MAPPED	BIT(20)
+
+#define USB_REQ_FLAGS	\
+	{USB_REQ_IS_LAST, "last"},\
+	{USB_REQ_NO_INTERRUPT, "no-irq"},\
+	{USB_REQ_ZERO, "zero-len"},\
+	{USB_REQ_SHORT_NOT_OK, "short-not-ok"},\
+	{USB_REQ_DMA_MAPPED, "dma-mapped"}
+
 /*-------------------------------------------------------------------------*/
 
 /* endpoint-specific parts of the api to the usb controller hardware.
@@ -161,12 +196,23 @@ 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;
+#ifdef CONFIG_CPU_LITTLE_ENDIAN
+	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	pad:2;
+#else
+	u8	pad:2;
+	u8	dir_out:1;
+	u8	dir_in:1;
+	u8	type_int:1;
+	u8	type_bulk:1;
+	u8	type_iso:1;
+	u8	type_control:1;
+#endif
 };
 
 #define USB_EP_CAPS_TYPE_CONTROL     0x01
@@ -197,6 +243,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.
@@ -226,19 +275,84 @@ 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 {
+#ifdef CONFIG_CPU_LITTLE_ENDIAN
+			u32	maxpacket:16;
+			u32	maxpacket_limit:16;
+#else
+			u32	maxpacket_limit:16;
+			u32	maxpacket:16;
+#endif
+		};
+		u32	dw1;
+	};
+	union {
+		struct {
+#ifdef CONFIG_CPU_LITTLE_ENDIAN
+			u32	max_streams:16;
+			u32	mult:2;
+			u32	maxburst:5;
+			u32	pad:9;
+#else
+			u32	pad:9;
+			u32	maxburst:5;
+			u32	mult:2;
+			u32	max_streams:16;
+#endif
+		};
+		u32	dw2;
+	};
+	union {
+		struct {
+#ifdef CONFIG_CPU_LITTLE_ENDIAN
+			struct usb_ep_caps	caps;
+			u8			claimed:1;
+			u8			enabled:1;
+			u8			pad1:6;
+			u8			address;
+			u8			reserved;
+#else
+			u8			reserved;
+			u8			address;
+			u8			pad1:6;
+			u8			enabled:1;
+			u8			claimed:1;
+			struct usb_ep_caps	caps;
+#endif
+		};
+		u32	dw3;
+	};
 	const struct usb_endpoint_descriptor	*desc;
 	const struct usb_ss_ep_comp_descriptor	*comp_desc;
 };
 
+#define		USB_EP_MAXPACKET		GENMASK(15, 0)
+#define		USB_EP_MAXPACKET_LIMIT		GENMASK(31, 16)
+#define		USB_EP_MAX_STREAMS		GENMASK(15, 0)
+#define		USB_EP_MULT			GENMASK(17, 16)
+#define		USB_EP_MAXBURST			GENMASK(22, 18)
+#define		USB_EP_DIR_BI			GENMASK(5, 4)
+#define		USB_EP_DIR_IN			BIT(4)
+#define		USB_EP_DIR_OUT			BIT(5)
+#define		USB_EP_CLAIMED			BIT(8)
+#define		USB_EP_ENABLED			BIT(9)
+#define		USB_EP_ADDRESS			GENMASK(23, 16)
+
+#define USB_EP_FLAGS	\
+	{USB_EP_CLAIMED, "claimed"},\
+	{USB_EP_ENABLED, "enabled"}
+
+#define USB_EP_MAX_NAME_LEN	16
+static inline char *usb_gadget_ep_name(char *s, u32 dw)
+{
+	snprintf(s, USB_EP_MAX_NAME_LEN, "ep%d%s", u32_get_bits(dw, USB_EP_ADDRESS),
+		u32_get_bits(dw, USB_EP_DIR_BI) == 3 ? "" :
+		u32_get_bits(dw, USB_EP_DIR_IN) ? "in" : "out");
+
+	return s;
+}
+
 /*-------------------------------------------------------------------------*/
 
 #if IS_ENABLED(CONFIG_USB_GADGET)
@@ -355,6 +469,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.
@@ -430,30 +545,101 @@ 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 {
+#ifdef CONFIG_CPU_LITTLE_ENDIAN
+			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;
+			u32		pad:13;
+#else
+			u32		pad:13;
+			u32		wakeup_armed:1;
+			u32		wakeup_capable:1;
+			u32		lpm_capable:1;
+			u32		connected:1;
+			u32		deactivated:1;
+			u32		is_selfpowered:1;
+			u32		quirk_avoids_skb_reserve:1;
+			u32		quirk_zlp_not_supp:1;
+			u32		quirk_stall_not_supp:1;
+			u32		quirk_altset_not_supp:1;
+			u32		quirk_ep_out_aligned_size:1;
+			u32		host_request_flag:1;
+			u32		hnp_polling_support:1;
+			u32		a_alt_hnp_support:1;
+			u32		a_hnp_support:1;
+			u32		b_hnp_enable:1;
+			u32		is_a_peripheral:1;
+			u32		is_otg:1;
+			u32		sg_supported:1;
+#endif
+		};
+
+		u32			dw1;
+	};
 	int				irq;
 	int				id_number;
 };
 #define work_to_gadget(w)	(container_of((w), struct usb_gadget, work))
 
+#define		USB_GADGET_SG_SUPPORTED			BIT(0)
+#define		USB_GADGET_IS_OTG			BIT(1)
+#define		USB_GADGET_IS_A_PERIPHERAL		BIT(2)
+#define		USB_GADGET_B_HNP_ENABLE			BIT(3)
+#define		USB_GADGET_A_HNP_SUPPORT		BIT(4)
+#define		USB_GADGET_A_ALT_HNP_SUPPORT		BIT(5)
+#define		USB_GADGET_HNP_POLLING_SUPPORT		BIT(6)
+#define		USB_GADGET_HOST_REQUEST_FLAG		BIT(7)
+#define		USB_GADGET_QUIRK_EP_OUT_ALIGNED_SIZE	BIT(8)
+#define		USB_GADGET_QUIRK_ALTSET_NOT_SUPP	BIT(9)
+#define		USB_GADGET_QUIRK_STALL_NOT_SUPP		BIT(10)
+#define		USB_GADGET_QUIRK_ZLP_NOT_SUPP		BIT(11)
+#define		USB_GADGET_QUIRK_AVOIDS_SKB_RESERVE	BIT(12)
+#define		USB_GADGET_IS_SELFPOWERED		BIT(13)
+#define		USB_GADGET_DEACTIVATED			BIT(14)
+#define		USB_GADGET_CONNECTED			BIT(15)
+#define		USB_GADGET_LPM_CAPABLE			BIT(16)
+#define		USB_GADGET_WAKEUP_CAPABLE		BIT(17)
+#define		USB_GADGET_WAKEUP_ARMED			BIT(18)
+
+#define USB_GADGET_FLAGS	\
+	{USB_GADGET_SG_SUPPORTED, "sg"},\
+	{USB_GADGET_IS_OTG, "OTG"},\
+	{USB_GADGET_IS_A_PERIPHERAL, "a_peripheral"},\
+	{USB_GADGET_B_HNP_ENABLE, "b_hnp"},\
+	{USB_GADGET_A_HNP_SUPPORT, "a_hnp"},\
+	{USB_GADGET_A_ALT_HNP_SUPPORT, "a_alt_hnp"},\
+	{USB_GADGET_HNP_POLLING_SUPPORT, "hnp_poll"},\
+	{USB_GADGET_HOST_REQUEST_FLAG, "hostreq"},\
+	{USB_GADGET_QUIRK_EP_OUT_ALIGNED_SIZE, "out_aligned"},\
+	{USB_GADGET_QUIRK_ALTSET_NOT_SUPP, "no_altset"},\
+	{USB_GADGET_QUIRK_STALL_NOT_SUPP, "no_stall"},\
+	{USB_GADGET_QUIRK_ZLP_NOT_SUPP, "no_zlp"},\
+	{USB_GADGET_QUIRK_AVOIDS_SKB_RESERVE, "no_skb_reserve"},\
+	{USB_GADGET_IS_SELFPOWERED, "self-powered"},\
+	{USB_GADGET_DEACTIVATED, "deactivated"},\
+	{USB_GADGET_CONNECTED, "connected"},\
+	{USB_GADGET_LPM_CAPABLE, "lpm-capable"},\
+	{USB_GADGET_WAKEUP_CAPABLE, "wakeup-capable"},\
+	{USB_GADGET_WAKEUP_ARMED, "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

[PATCH v7 3/4] usb: udc: trace: reduce buffer usage of trace event

[Linyu Yuan]

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

take below trace event class for explanation, save two u32 members and
parse it to generate ep name and other information.

old:
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;
	),
	...
);

new:
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(
		__field(u32, edw3)
		__field(unsigned, length)
		__field(unsigned, actual)
		__field(unsigned, num_sgs)
		__field(unsigned, num_mapped_sgs)
		__field(u32, rdw1)
		__field(int, status)
		__field(int, ret)
		__field(struct usb_request *, req)
	),
	TP_fast_assign(
		__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->rdw1 = req->dw1;
		__entry->status = req->status;
		__entry->ret = ret;
		__entry->req = req;
	),
	...
);

consider 32 bit ARCH,
for old definition, one trace entry size is:
4 (ring buffer event header ) + 8 (trace event header ) +
48 (trace class header) + 9 (ep string name) = 69 bytes.

for new definition, one trace entry size is:
4 (ring buffer event header ) + 8 (trace event header ) +
36 (trace class header)  = 48 bytes.

consider there is 1MB trace buffer space,
for old definition, it can save 15196 entries,
for new definition, it can save 21845 entries.

Use new common trace event macro in gadget.h for output stage.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
 drivers/usb/gadget/udc/trace.h | 114 +++++++++------------------------
 1 file changed, 30 insertions(+), 84 deletions(-)

diff --git a/drivers/usb/gadget/udc/trace.h b/drivers/usb/gadget/udc/trace.h
index a5ed26fbc2da..690810b66260 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,12 @@ 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",
+	TP_printk("speed %d/%d state %d %dmA [%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",
+		__print_flags(__entry->gdw1, ":", USB_GADGET_FLAGS),
 		__entry->ret)
 );
 
@@ -145,34 +106,26 @@ 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(u8, address)
-		__field(bool, claimed)
-		__field(bool, enabled)
+		__field(u32, edw3)
+		__field(u32, edw1)
+		__field(u32, edw2)
 		__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->address = ep->address,
-		__entry->claimed = ep->claimed;
-		__entry->enabled = ep->enabled;
+		__entry->edw3 = ep->dw3;
+		__entry->edw1 = ep->dw1;
+		__entry->edw2 = ep->dw2;
 		__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,
-		__entry->address, __entry->claimed ? "claimed:" : "released:",
-		__entry->enabled ? "enabled" : "disabled", ret)
+	TP_printk("%s: mps %d/%d streams %d mult %d burst %d addr %02x %s --> %d",
+		usb_gadget_ep_name(__get_buf(USB_EP_MAX_NAME_LEN), __entry->edw3),
+		u32_get_bits(__entry->edw1, USB_EP_MAXPACKET),
+		u32_get_bits(__entry->edw1, USB_EP_MAXPACKET_LIMIT),
+		u32_get_bits(__entry->edw2, USB_EP_MAX_STREAMS),
+		u32_get_bits(__entry->edw2, USB_EP_MULT),
+		u32_get_bits(__entry->edw2, USB_EP_MAXBURST),
+		u32_get_bits(__entry->edw3, USB_EP_ADDRESS),
+		__print_flags(__entry->edw3, ":", USB_EP_FLAGS), ret)
 );
 
 DEFINE_EVENT(udc_log_ep, usb_ep_set_maxpacket_limit,
@@ -219,41 +172,34 @@ 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",
-		__entry->status, __entry->ret
-	)
+	TP_printk("%s: req %p length %d/%d sgs %d/%d stream %d %s status %d --> %d",
+		usb_gadget_ep_name(__get_buf(USB_EP_MAX_NAME_LEN), __entry->edw3),
+		__entry->req, __entry->actual, __entry->length,
+		__entry->num_mapped_sgs, __entry->num_sgs,
+		u32_get_bits(__entry->rdw1, USB_REQ_STREAM_ID),
+		__print_flags(__entry->rdw1, ":", USB_REQ_FLAGS),
+		__entry->status, __entry->ret)
 );
 
 DEFINE_EVENT(udc_log_req, usb_ep_alloc_request,
-- 
2.17.1

[PATCH v7 4/4] usb: dwc3: trace: reduce buffer usage of trace event

[Linyu Yuan]

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

take below trace event class for explanation, save two u32 members and
parse it to generate ep name and other information.

old:
DECLARE_EVENT_CLASS(dwc3_log_request,
	TP_PROTO(struct dwc3_request *req),
	TP_ARGS(req),
	TP_STRUCT__entry(
		__string(name, req->dep->name)
		__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)
	),
	TP_fast_assign(
		__assign_str(name, req->dep->name);
		__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;
	),
	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",
		__entry->status
	)
);

new:
DECLARE_EVENT_CLASS(dwc3_log_request,
	TP_PROTO(struct dwc3_request *req),
	TP_ARGS(req),
	TP_STRUCT__entry(
		__field(u32, edw3)
		__field(struct dwc3_request *, req)
		__field(unsigned int, actual)
		__field(unsigned int, length)
		__field(int, status)
		__field(u32, rdw1)
	),
	TP_fast_assign(
		__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->rdw1 = req->request.dw1;
	),
	TP_printk("%s: req %p length %u/%u %s ==> %d",
		usb_gadget_ep_name(__get_buf(USB_EP_MAX_NAME_LEN), __entry->edw3),
		__entry->req, __entry->actual, __entry->length,
		__print_flags(__entry->rdw1, ":", USB_REQ_FLAGS),
		__entry->status
	)
);

consider 32 bit ARCH,
for old definition, one trace entry size is:
4 (ring buffer event header ) + 8 (trace event header ) +
32 (trace class header) + 9 (ep string name) = 53 bytes.

for new definition, one trace entry size is:
4 (ring buffer event header ) + 8 (trace event header ) +
24 (trace class header)  = 36 bytes.

consider there is 1MB trace buffer space,
for old definition, it can save 19784 entries,
for new definition, it can save 29127 entries.

Use new common trace event macro in gadget.h for output stage.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
 drivers/usb/dwc3/trace.h | 63 ++++++++++++++++++----------------------
 1 file changed, 29 insertions(+), 34 deletions(-)

diff --git a/drivers/usb/dwc3/trace.h b/drivers/usb/dwc3/trace.h
index d2997d17cfbe..1dbd56e98463 100644
--- a/drivers/usb/dwc3/trace.h
+++ b/drivers/usb/dwc3/trace.h
@@ -102,30 +102,25 @@ 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",
+	TP_printk("%s: req %p length %u/%u %s ==> %d",
+		usb_gadget_ep_name(__get_buf(USB_EP_MAX_NAME_LEN), __entry->edw3),
+		__entry->req, __entry->actual, __entry->length,
+		__print_flags(__entry->rdw1, ":", USB_REQ_FLAGS),
 		__entry->status
 	)
 );
@@ -185,7 +180,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 +188,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 +196,8 @@ 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_gadget_ep_name(__get_buf(USB_EP_MAX_NAME_LEN), __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,8 @@ 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_gadget_ep_name(__get_buf(USB_EP_MAX_NAME_LEN), __entry->edw3),
+		__entry->trb, __entry->enqueue,
 		__entry->dequeue, __entry->bph, __entry->bpl,
 		({char *s;
 		int pcm = ((__entry->size >> 24) & 3) + 1;
@@ -272,7 +269,7 @@ DECLARE_EVENT_CLASS(dwc3_log_trb,
 		__entry->ctrl & DWC3_TRB_CTRL_CSP ? 'S' : 's',
 		__entry->ctrl & DWC3_TRB_CTRL_ISP_IMI ? 'S' : 's',
 		__entry->ctrl & DWC3_TRB_CTRL_IOC ? 'C' : 'c',
-		  dwc3_trb_type_string(DWC3_TRBCTL_TYPE(__entry->ctrl))
+		dwc3_trb_type_string(DWC3_TRBCTL_TYPE(__entry->ctrl))
 	)
 );
 
@@ -290,32 +287,30 @@ 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,
-		__entry->trb_dequeue,
+		usb_gadget_ep_name(__get_buf(USB_EP_MAX_NAME_LEN), __entry->edw3),
+		u32_get_bits(__entry->edw1, USB_EP_MAXPACKET),
+		u32_get_bits(__entry->edw1, USB_EP_MAXPACKET_LIMIT),
+		u32_get_bits(__entry->edw2, USB_EP_MAX_STREAMS),
+		u32_get_bits(__entry->edw2, USB_EP_MAXBURST),
+		__entry->trb_enqueue, __entry->trb_dequeue,
 		__entry->flags & DWC3_EP_ENABLED ? 'E' : 'e',
 		__entry->flags & DWC3_EP_STALL ? 'S' : 's',
 		__entry->flags & DWC3_EP_WEDGE ? 'W' : 'w',
-- 
2.17.1

Re: [PATCH v7 2/4] usb: gadget: add anonymous definition in some struct for trace purpose

[Greg Kroah-Hartman]

On Mon, Sep 18, 2023 at 07:25:32PM +0800, Linyu Yuan wrote:
> Some UDC trace event will save usb udc information, but it use one int
> size buffer to save one bit information of usb udc, it waste trace buffer.
> 
> Add anonymous union which have u32 members can be used by trace event
> during fast assign stage to save more entries with same trace ring buffer
> size.
> 
> In order to access each bit with BIT() macro, add different definition for
> each bit fields according host little/big endian to make sure it has same
> eacho bit field have same bit position in memory.

typo?

> Add some macros or helper for later trace event usage which follow the
> udc structs, As when possible future changes to udc related structs,
> developers will easy notice them.

This isn't going to work at all, there's nothing to keep the two in
sync.

As you are using bitmasks now, wonderful, just use those only and ignore
the bitfield definitions, that's not going to work mixing the two at
all.

thanks,

greg k-h

Re: [PATCH v7 2/4] usb: gadget: add anonymous definition in some struct for trace purpose

[Alan Stern]

On Mon, Sep 18, 2023 at 02:06:34PM +0200, Greg Kroah-Hartman wrote:
> On Mon, Sep 18, 2023 at 07:25:32PM +0800, Linyu Yuan wrote:
> > Some UDC trace event will save usb udc information, but it use one int
> > size buffer to save one bit information of usb udc, it waste trace buffer.
> > 
> > Add anonymous union which have u32 members can be used by trace event
> > during fast assign stage to save more entries with same trace ring buffer
> > size.
> > 
> > In order to access each bit with BIT() macro, add different definition for
> > each bit fields according host little/big endian to make sure it has same
> > eacho bit field have same bit position in memory.
> 
> typo?
> 
> > Add some macros or helper for later trace event usage which follow the
> > udc structs, As when possible future changes to udc related structs,
> > developers will easy notice them.
> 
> This isn't going to work at all, there's nothing to keep the two in
> sync.
> 
> As you are using bitmasks now, wonderful, just use those only and ignore
> the bitfield definitions, that's not going to work mixing the two at
> all.

Greg is right.  If you really want to access these fields using bitmask 
operations, you should do it by changing all of the fields into 
bitmasks; mixing bitmasks and bitfields is not a good idea.

For example, in order to do this you will have to change the definition 
of struct usb_gadget.  Replace

	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;

with

	unsigned int			dw1;

#define USB_GADGET_SG_SUPPORTED		BIT(0)
#define USB_GADGET_IS_OTG		BIT(1)
...
#define USB_GADGET_WAKEUP_ARMED		BIT(18)

Then change all the drivers that use these fields.  For example, change

	g->connected = 1;

to

	g->dw1 |= USB_GADGET_CONNECTED;

and change

	if (g->is_selfpowered)

to

	if (g->dw1 & USB_GADGET_IS_SELFPOWERED)

Or if you prefer, invent some inline routines or macros that will handle 
this for you.

Make these changes in every gadget and UDC driver.  Then it will be 
safe to say

	__entry->dw1 = g->dw1;

in the fast assign stage.  But it's not safe to use bitfields in the 
gadget and UDC drivers while using bitmasks in the tracing code.

Alan Stern

Re: [PATCH v7 2/4] usb: gadget: add anonymous definition in some struct for trace purpose

[Linyu Yuan]

On 9/18/2023 10:14 PM, Alan Stern wrote:
> On Mon, Sep 18, 2023 at 02:06:34PM +0200, Greg Kroah-Hartman wrote:
>> On Mon, Sep 18, 2023 at 07:25:32PM +0800, Linyu Yuan wrote:
>>> Some UDC trace event will save usb udc information, but it use one int
>>> size buffer to save one bit information of usb udc, it waste trace buffer.
>>>
>>> Add anonymous union which have u32 members can be used by trace event
>>> during fast assign stage to save more entries with same trace ring buffer
>>> size.
>>>
>>> In order to access each bit with BIT() macro, add different definition for
>>> each bit fields according host little/big endian to make sure it has same
>>> eacho bit field have same bit position in memory.
>> typo?
>>
>>> Add some macros or helper for later trace event usage which follow the
>>> udc structs, As when possible future changes to udc related structs,
>>> developers will easy notice them.
>> This isn't going to work at all, there's nothing to keep the two in
>> sync.
>>
>> As you are using bitmasks now, wonderful, just use those only and ignore
>> the bitfield definitions, that's not going to work mixing the two at
>> all.
> Greg is right.  If you really want to access these fields using bitmask
> operations, you should do it by changing all of the fields into
> bitmasks; mixing bitmasks and bitfields is not a good idea.
>
> For example, in order to do this you will have to change the definition
> of struct usb_gadget.  Replace
>
> 	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;
>
> with
>
> 	unsigned int			dw1;
>
> #define USB_GADGET_SG_SUPPORTED		BIT(0)
> #define USB_GADGET_IS_OTG		BIT(1)
> ...
> #define USB_GADGET_WAKEUP_ARMED		BIT(18)
>
> Then change all the drivers that use these fields.  For example, change
>
> 	g->connected = 1;
>
> to
>
> 	g->dw1 |= USB_GADGET_CONNECTED;
>
> and change
>
> 	if (g->is_selfpowered)
>
> to
>
> 	if (g->dw1 & USB_GADGET_IS_SELFPOWERED)
>
> Or if you prefer, invent some inline routines or macros that will handle
> this for you.


it is a lot of changes. i only expect  common and minimal 
change(macros/helpers) in gadget.h.


if you have better idea, please help share a patch.


>
> Make these changes in every gadget and UDC driver.  Then it will be
> safe to say
>
> 	__entry->dw1 = g->dw1;
>
> in the fast assign stage.  But it's not safe to use bitfields in the
> gadget and UDC drivers while using bitmasks in the tracing code.


thanks everyone for reviewing for a long time, it is end from my side of 
this topic.


>
> Alan Stern

Re: [PATCH v7 2/4] usb: gadget: add anonymous definition in some struct for trace purpose

[Greg Kroah-Hartman]

On Tue, Sep 19, 2023 at 08:01:43AM +0800, Linyu Yuan wrote:
> 
> On 9/18/2023 10:14 PM, Alan Stern wrote:
> > On Mon, Sep 18, 2023 at 02:06:34PM +0200, Greg Kroah-Hartman wrote:
> > > On Mon, Sep 18, 2023 at 07:25:32PM +0800, Linyu Yuan wrote:
> > > > Some UDC trace event will save usb udc information, but it use one int
> > > > size buffer to save one bit information of usb udc, it waste trace buffer.
> > > > 
> > > > Add anonymous union which have u32 members can be used by trace event
> > > > during fast assign stage to save more entries with same trace ring buffer
> > > > size.
> > > > 
> > > > In order to access each bit with BIT() macro, add different definition for
> > > > each bit fields according host little/big endian to make sure it has same
> > > > eacho bit field have same bit position in memory.
> > > typo?
> > > 
> > > > Add some macros or helper for later trace event usage which follow the
> > > > udc structs, As when possible future changes to udc related structs,
> > > > developers will easy notice them.
> > > This isn't going to work at all, there's nothing to keep the two in
> > > sync.
> > > 
> > > As you are using bitmasks now, wonderful, just use those only and ignore
> > > the bitfield definitions, that's not going to work mixing the two at
> > > all.
> > Greg is right.  If you really want to access these fields using bitmask
> > operations, you should do it by changing all of the fields into
> > bitmasks; mixing bitmasks and bitfields is not a good idea.
> > 
> > For example, in order to do this you will have to change the definition
> > of struct usb_gadget.  Replace
> > 
> > 	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;
> > 
> > with
> > 
> > 	unsigned int			dw1;
> > 
> > #define USB_GADGET_SG_SUPPORTED		BIT(0)
> > #define USB_GADGET_IS_OTG		BIT(1)
> > ...
> > #define USB_GADGET_WAKEUP_ARMED		BIT(18)
> > 
> > Then change all the drivers that use these fields.  For example, change
> > 
> > 	g->connected = 1;
> > 
> > to
> > 
> > 	g->dw1 |= USB_GADGET_CONNECTED;
> > 
> > and change
> > 
> > 	if (g->is_selfpowered)
> > 
> > to
> > 
> > 	if (g->dw1 & USB_GADGET_IS_SELFPOWERED)
> > 
> > Or if you prefer, invent some inline routines or macros that will handle
> > this for you.
> 
> 
> it is a lot of changes. i only expect  common and minimal
> change(macros/helpers) in gadget.h.

Why?

> if you have better idea, please help share a patch.

Alan is right here, please just take the time to do this properly, after
documenting exactly how much time/space is going to be saved here, I
don't seem to see that anywhere in your patch sets so I still do not
understand why this patch set was created in the first place.

thanks,

greg k-h