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

[Linyu Yuan <quic_linyyuan@quicinc.com>]

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.


>
>>> 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))

+#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)
+



>
> Alan Stern