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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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