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

[Linyu Yuan <quic_linyyuan@quicinc.com>]

On 9/14/2023 10:16 AM, Alan Stern wrote:
> 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).


got it, it is same concern of Greg.


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


sure, will remove it.


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


this is good idea, it is more simple.

but still one question, why can't put in gadget.h, or not it need in 
every trace file, right ?


>
> Alan Stern