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:25 AM, Linyu Yuan wrote:
>
> 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 ?


i will add new trace event macro to cover this special case.

will send next version.


>
>
>>
>> Alan Stern