| Message ID | 20230914100302.30274-3-quic_linyyuan@quicinc.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | usb: gadget: reduce usb gadget trace event buffer usage | expand |
You didn't include the version number in the Subject: line. Undoubtedly Greg's automatic error checker will warn you about this. Unless the version number is clearly marked for each patch, it's difficult for his programs to tell which email message contains the most recent version. On Thu, Sep 14, 2023 at 06:02:56PM +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 is wast trace > buffer. > > Add anonymous union which have one u32 member can be used by trace event > during fast assign stage to save more entries with same trace ring buffer > size. > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> > --- And you didn't include the version change information here, below the "---" line. Apart from that, this is a _lot_ better than before! I don't know if Greg will think this change is worth merging, but at least now it's possible to read the code and understand what's going on. Alan Stern
On 9/14/2023 10:54 PM, Alan Stern wrote: > You didn't include the version number in the Subject: line. Undoubtedly > Greg's automatic error checker will warn you about this. Unless the > version number is clearly marked for each patch, it's difficult for his > programs to tell which email message contains the most recent version. > > On Thu, Sep 14, 2023 at 06:02:56PM +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 is wast trace >> buffer. >> >> Add anonymous union which have one u32 member can be used by trace event >> during fast assign stage to save more entries with same trace ring buffer >> size. >> >> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> >> --- > And you didn't include the version change information here, below the > "---" line. > > Apart from that, this is a _lot_ better than before! I don't know if > Greg will think this change is worth merging, but at least now it's > possible to read the code and understand what's going on. according Steven's comment, maybe will always save data in little endian at trace event fast assign stage. it will add definition of bit field back. > > Alan Stern
On Fri, Sep 15, 2023 at 09:02:48AM +0800, Linyu Yuan wrote: > > On 9/14/2023 10:54 PM, Alan Stern wrote: > > You didn't include the version number in the Subject: line. Undoubtedly > > Greg's automatic error checker will warn you about this. Unless the > > version number is clearly marked for each patch, it's difficult for his > > programs to tell which email message contains the most recent version. > > > > On Thu, Sep 14, 2023 at 06:02:56PM +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 is wast trace > > > buffer. > > > > > > Add anonymous union which have one u32 member can be used by trace event > > > during fast assign stage to save more entries with same trace ring buffer > > > size. > > > > > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> > > > --- > > And you didn't include the version change information here, below the > > "---" line. > > > > Apart from that, this is a _lot_ better than before! I don't know if > > Greg will think this change is worth merging, but at least now it's > > possible to read the code and understand what's going on. > > > according Steven's comment, maybe will always save data in little endian at > trace event > > fast assign stage. > > it will add definition of bit field back. Yes, that would be even better because you wouldn't have to change the definition of struct usb_gadget or struct usb_endpoint at all. The fast assign stage can simply do: __entry->dw1 = (g->sg_supported << 0) | (g->is_otg << 1) | ... and then you can easily access the individual bits in __entry. It wouldn't be as fast but it would still save a lot of space. Alan Stern
On 9/15/2023 9:51 AM, Alan Stern wrote: > On Fri, Sep 15, 2023 at 09:02:48AM +0800, Linyu Yuan wrote: >> On 9/14/2023 10:54 PM, Alan Stern wrote: >>> You didn't include the version number in the Subject: line. Undoubtedly >>> Greg's automatic error checker will warn you about this. Unless the >>> version number is clearly marked for each patch, it's difficult for his >>> programs to tell which email message contains the most recent version. >>> >>> On Thu, Sep 14, 2023 at 06:02:56PM +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 is wast trace >>>> buffer. >>>> >>>> Add anonymous union which have one u32 member can be used by trace event >>>> during fast assign stage to save more entries with same trace ring buffer >>>> size. >>>> >>>> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> >>>> --- >>> And you didn't include the version change information here, below the >>> "---" line. >>> >>> Apart from that, this is a _lot_ better than before! I don't know if >>> Greg will think this change is worth merging, but at least now it's >>> possible to read the code and understand what's going on. >> >> according Steven's comment, maybe will always save data in little endian at >> trace event >> >> fast assign stage. >> >> it will add definition of bit field back. > Yes, that would be even better because you wouldn't have to change the > definition of struct usb_gadget or struct usb_endpoint at all. The fast > assign stage can simply do: > > __entry->dw1 = (g->sg_supported << 0) | > (g->is_otg << 1) | > ... > > and then you can easily access the individual bits in __entry. It > wouldn't be as fast but it would still save a lot of space. how about __entry->dw1 = cpu_to_le32(g->dw1) ? > > Alan Stern
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h index 75bda0783395..4894f256df55 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; + }; void (*complete)(struct usb_ep *ep, struct usb_request *req); @@ -163,13 +169,13 @@ 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; +} __packed; #define USB_EP_CAPS_TYPE_CONTROL 0x01 #define USB_EP_CAPS_TYPE_ISO 0x02 @@ -199,6 +205,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. @@ -228,15 +237,30 @@ 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; + }; + union { + struct { + u32 max_streams:16; + u32 mult:2; + u32 maxburst:5; + } __packed; + u32 dw2; + }; + union { + struct { + struct usb_ep_caps caps; + u8 claimed:1; + u8 enabled:1; + u8 address; + } __packed; + u32 dw3; + }; const struct usb_endpoint_descriptor *desc; const struct usb_ss_ep_comp_descriptor *comp_desc; }; @@ -357,6 +381,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,25 +457,31 @@ 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 { + 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; + }; int irq; int id_number; };
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 is wast trace buffer. Add anonymous union which have one u32 member can be used by trace event during fast assign stage to save more entries with same trace ring buffer size. Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> --- include/linux/usb/gadget.h | 113 +++++++++++++++++++++++-------------- 1 file changed, 72 insertions(+), 41 deletions(-)