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

[Alan Stern <stern@rowland.harvard.edu>]

On Thu, Sep 14, 2023 at 09:08:04AM +0800, Linyu Yuan wrote:
> 
> On 9/14/2023 12:02 AM, Alan Stern wrote:
> > On Wed, Sep 13, 2023 at 11:46:12AM +0800, Linyu Yuan wrote:
> > > but Alan Stern have one comment,   do it mean the bit position number is not
> > > expect and we can't use it ?
> > > 
> > > @Alan Stern ,  BIT(0), BIT(1) is not the member we expect ?
> > They might not be.  If you can avoid making this assumption, you should.
> 
> 
> i don't know if it is true or not, seem some driver expect there is no hole
> for this kind of bit field definition.

I didn't say there would be a hole; I said that BIT(0) might not be the 
member you expect.  For example, sg_supported might be BIT(31) instead 
of BIT(0).

> > > > This macro usage is a real mess.  Can't you find a better way to do it?
> > > > 
> > > > For instance, in the code that parses the trace buffer, define a
> > > > temporary usb_gadget structure and copy the dw1 field from the trace
> > > > buffer to the temporary structure.  Then you can access the fields in
> > > > that structure directly by their original names, with no macros.
> > > do it same idea just move it outside of gadget.h ?
> > Keep the anonymous union in gadget.h, but get rid of the macros.
> 
> 
> do you expect below ?
> 
> 
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -357,6 +357,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,30 +433,88 @@ 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 {
> +                       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;
> +               } __packed;
> +
> +               u32                     dw1;
> +       } __aligned(4);
>         int                             irq;
>         int                             id_number;
>  };
>  #define work_to_gadget(w)      (container_of((w), struct usb_gadget, work))

Stop here.  The above is what I expect.  Don't include any of the 
material below.

(BTW, you don't need the __aligned(4) thing, do you?  Since the union 
includes a 32-bit integer field, it will naturally be aligned on a 
4-byte boundary.)

> +#define USB_GADGET_BITFIELD(field)                             \
> +static inline u32 usb_gadget_bit_##field(u32 dw1)              \
> +{                                                              \
> +       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) u;                                       \
> +       BUILD_BUG_ON(sizeof(u) != 4);                           \
> +       u.dw1 = dw1;                                            \
> +       return u.field;                                         \
> +}
> +
> +USB_GADGET_BITFIELD(sg_supported)
> +USB_GADGET_BITFIELD(is_otg)
> +USB_GADGET_BITFIELD(is_a_peripheral)
> +USB_GADGET_BITFIELD(b_hnp_enable)
> +USB_GADGET_BITFIELD(a_hnp_support)
> +USB_GADGET_BITFIELD(a_alt_hnp_support)
> +USB_GADGET_BITFIELD(hnp_polling_support)
> +USB_GADGET_BITFIELD(host_request_flag)
> +USB_GADGET_BITFIELD(quirk_ep_out_aligned_size)
> +USB_GADGET_BITFIELD(quirk_altset_not_supp)
> +USB_GADGET_BITFIELD(quirk_stall_not_supp)
> +USB_GADGET_BITFIELD(quirk_zlp_not_supp)
> +USB_GADGET_BITFIELD(quirk_avoids_skb_reserve)
> +USB_GADGET_BITFIELD(is_selfpowered)
> +USB_GADGET_BITFIELD(deactivated)
> +USB_GADGET_BITFIELD(connected)
> +USB_GADGET_BITFIELD(lpm_capable)
> +USB_GADGET_BITFIELD(wakeup_capable)
> +USB_GADGET_BITFIELD(wakeup_armed)

So ignore all of that.

Now in your patch 4/10, do something that will have this effect:

+	struct usb_gadget g;
+
+	g.dw1 = __entry->dw1;
+
	TP_printk(....
-		__entry->sg_supported ? "sg:" : "",
+		g.sg_supported ? "sg:" : "",
...

You probably can't do it exactly this way, because this won't work with 
the tracing macros, but maybe something that is equivalent will work.

For example, you could try:

+#define USB_GADGET_BITFIELD(field)		\
+	({struct usb_gadget g;			\
+		g.dw1 = __entry->dw1;		\
+		g.field;})

	TP_printk(....
-		__entry->sg_supported ? "sg:" : "",
+		USB_GADGET_BITFIELD(sg_supported) ? "sg:" : "",

Do you get the idea now?

Alan Stern