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
