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
