| Message ID | 20230918112534.2108-3-quic_linyyuan@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: gadget: reduce usb gadget trace event buffer usage | expand |
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
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
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
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
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); }
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(-)