[PATCH V4 0/3] usb: gadget: configfs: add three trace entries

[PATCH V4 0/3] usb: gadget: configfs: add three trace entries

[Linyu Yuan]

when debug USB gadget issue, it is better to understand
what configuration was done from user space.

v3->v4: split into three changes
v2->v3: add more change log
v1->v2: fix typo tree -> three of subject line

Linyu Yuan (3):
  usb: gadget: configfs: add UDC trace entry
  usb: gadget: configfs: add function link trace entry
  usb: gadget: configfs: add function unlink trace entry

 drivers/usb/gadget/Makefile         |  1 +
 drivers/usb/gadget/configfs.c       | 11 ++++++
 drivers/usb/gadget/configfs_trace.c |  7 ++++
 drivers/usb/gadget/configfs_trace.h | 73 +++++++++++++++++++++++++++++++++++++
 4 files changed, 92 insertions(+)
 create mode 100644 drivers/usb/gadget/configfs_trace.c
 create mode 100644 drivers/usb/gadget/configfs_trace.h

-- 
2.7.4

[PATCH V4 1/3] usb: gadget: configfs: add UDC trace entry

[Linyu Yuan]

add trace in function gadget_dev_desc_UDC_store()
will show when user space enable/disable the gadget.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
 drivers/usb/gadget/Makefile         |  1 +
 drivers/usb/gadget/configfs.c       |  3 +++
 drivers/usb/gadget/configfs_trace.c |  7 +++++++
 drivers/usb/gadget/configfs_trace.h | 39 +++++++++++++++++++++++++++++++++++++
 4 files changed, 50 insertions(+)
 create mode 100644 drivers/usb/gadget/configfs_trace.c
 create mode 100644 drivers/usb/gadget/configfs_trace.h

diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
index 130dad7..8e9c2b8 100644
--- a/drivers/usb/gadget/Makefile
+++ b/drivers/usb/gadget/Makefile
@@ -9,5 +9,6 @@ ccflags-y				+= -I$(srctree)/drivers/usb/gadget/udc
 obj-$(CONFIG_USB_LIBCOMPOSITE)	+= libcomposite.o
 libcomposite-y			:= usbstring.o config.o epautoconf.o
 libcomposite-y			+= composite.o functions.o configfs.o u_f.o
+libcomposite-y			+= configfs_trace.o
 
 obj-$(CONFIG_USB_GADGET)	+= udc/ function/ legacy/
diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 477e72a..f7f3af8 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -9,6 +9,7 @@
 #include "configfs.h"
 #include "u_f.h"
 #include "u_os_desc.h"
+#include "configfs_trace.h"
 
 int check_user_usb_string(const char *name,
 		struct usb_gadget_strings *stringtab_dev)
@@ -270,6 +271,8 @@ static ssize_t gadget_dev_desc_UDC_store(struct config_item *item,
 	if (name[len - 1] == '\n')
 		name[len - 1] = '\0';
 
+	trace_gadget_dev_desc_UDC_store(config_item_name(item), name);
+
 	mutex_lock(&gi->lock);
 
 	if (!strlen(name)) {
diff --git a/drivers/usb/gadget/configfs_trace.c b/drivers/usb/gadget/configfs_trace.c
new file mode 100644
index 0000000..b74ff21
--- /dev/null
+++ b/drivers/usb/gadget/configfs_trace.c
@@ -0,0 +1,7 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2021 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#define CREATE_TRACE_POINTS
+#include "configfs_trace.h"
diff --git a/drivers/usb/gadget/configfs_trace.h b/drivers/usb/gadget/configfs_trace.h
new file mode 100644
index 0000000..f2e17e4
--- /dev/null
+++ b/drivers/usb/gadget/configfs_trace.h
@@ -0,0 +1,39 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2021 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM configfs_gadget
+
+#if !defined(__GADGET_TRACE_H) || defined(TRACE_HEADER_MULTI_READ)
+#define __GADGET_TRACE_H
+
+#include <linux/tracepoint.h>
+
+TRACE_EVENT(gadget_dev_desc_UDC_store,
+	TP_PROTO(char *name, char *udc),
+	TP_ARGS(name, udc),
+	TP_STRUCT__entry(
+		__string(group_name, name)
+		__string(udc_name, udc)
+	),
+	TP_fast_assign(
+		__assign_str(group_name, name);
+		__assign_str(udc_name, udc);
+	),
+	TP_printk("gadget:%s UDC:%s", __get_str(group_name),
+		__get_str(udc_name))
+);
+
+#endif /* __GADGET_TRACE_H */
+
+/* this part has to be here */
+
+#undef TRACE_INCLUDE_PATH
+#define TRACE_INCLUDE_PATH ../../drivers/usb/gadget
+
+#undef TRACE_INCLUDE_FILE
+#define TRACE_INCLUDE_FILE configfs_trace
+
+#include <trace/define_trace.h>
-- 
2.7.4

Re: [PATCH V4 1/3] usb: gadget: configfs: add UDC trace entry

[Felipe Balbi]

Hi,

Linyu Yuan <quic_linyyuan@quicinc.com> writes:
> add trace in function gadget_dev_desc_UDC_store()
> will show when user space enable/disable the gadget.
>
> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> ---
>  drivers/usb/gadget/Makefile         |  1 +
>  drivers/usb/gadget/configfs.c       |  3 +++
>  drivers/usb/gadget/configfs_trace.c |  7 +++++++
>  drivers/usb/gadget/configfs_trace.h | 39 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 50 insertions(+)
>  create mode 100644 drivers/usb/gadget/configfs_trace.c
>  create mode 100644 drivers/usb/gadget/configfs_trace.h
>
> diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
> index 130dad7..8e9c2b8 100644
> --- a/drivers/usb/gadget/Makefile
> +++ b/drivers/usb/gadget/Makefile
> @@ -9,5 +9,6 @@ ccflags-y				+= -I$(srctree)/drivers/usb/gadget/udc
>  obj-$(CONFIG_USB_LIBCOMPOSITE)	+= libcomposite.o
>  libcomposite-y			:= usbstring.o config.o epautoconf.o
>  libcomposite-y			+= composite.o functions.o configfs.o u_f.o
> +libcomposite-y			+= configfs_trace.o
>  
>  obj-$(CONFIG_USB_GADGET)	+= udc/ function/ legacy/
> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> index 477e72a..f7f3af8 100644
> --- a/drivers/usb/gadget/configfs.c
> +++ b/drivers/usb/gadget/configfs.c
> @@ -9,6 +9,7 @@
>  #include "configfs.h"
>  #include "u_f.h"
>  #include "u_os_desc.h"
> +#include "configfs_trace.h"
>  
>  int check_user_usb_string(const char *name,
>  		struct usb_gadget_strings *stringtab_dev)
> @@ -270,6 +271,8 @@ static ssize_t gadget_dev_desc_UDC_store(struct config_item *item,
>  	if (name[len - 1] == '\n')
>  		name[len - 1] = '\0';
>  
> +	trace_gadget_dev_desc_UDC_store(config_item_name(item), name);

why tracing only the names? This gives us no insight into whatever bug
may happen and we may want to use trace events to debug. Rather, try to
think of trace events as tracking the lifetime of the UDC and
gadget. Trace values that may change over time.

I also think that all three patches could be combined into a single
commit, but that's up to discussion.


-- 
balbi

Re: [PATCH V4 1/3] usb: gadget: configfs: add UDC trace entry

[Felipe Balbi]

Hi again,

Felipe Balbi <balbi@kernel.org> writes:
> Linyu Yuan <quic_linyyuan@quicinc.com> writes:
>> add trace in function gadget_dev_desc_UDC_store()
>> will show when user space enable/disable the gadget.
>>
>> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
>> ---
>>  drivers/usb/gadget/Makefile         |  1 +
>>  drivers/usb/gadget/configfs.c       |  3 +++
>>  drivers/usb/gadget/configfs_trace.c |  7 +++++++
>>  drivers/usb/gadget/configfs_trace.h | 39 +++++++++++++++++++++++++++++++++++++
>>  4 files changed, 50 insertions(+)
>>  create mode 100644 drivers/usb/gadget/configfs_trace.c
>>  create mode 100644 drivers/usb/gadget/configfs_trace.h
>>
>> diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
>> index 130dad7..8e9c2b8 100644
>> --- a/drivers/usb/gadget/Makefile
>> +++ b/drivers/usb/gadget/Makefile
>> @@ -9,5 +9,6 @@ ccflags-y				+= -I$(srctree)/drivers/usb/gadget/udc
>>  obj-$(CONFIG_USB_LIBCOMPOSITE)	+= libcomposite.o
>>  libcomposite-y			:= usbstring.o config.o epautoconf.o
>>  libcomposite-y			+= composite.o functions.o configfs.o u_f.o
>> +libcomposite-y			+= configfs_trace.o
>>  
>>  obj-$(CONFIG_USB_GADGET)	+= udc/ function/ legacy/
>> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
>> index 477e72a..f7f3af8 100644
>> --- a/drivers/usb/gadget/configfs.c
>> +++ b/drivers/usb/gadget/configfs.c
>> @@ -9,6 +9,7 @@
>>  #include "configfs.h"
>>  #include "u_f.h"
>>  #include "u_os_desc.h"
>> +#include "configfs_trace.h"
>>  
>>  int check_user_usb_string(const char *name,
>>  		struct usb_gadget_strings *stringtab_dev)
>> @@ -270,6 +271,8 @@ static ssize_t gadget_dev_desc_UDC_store(struct config_item *item,
>>  	if (name[len - 1] == '\n')
>>  		name[len - 1] = '\0';
>>  
>> +	trace_gadget_dev_desc_UDC_store(config_item_name(item), name);
>
> why tracing only the names? This gives us no insight into whatever bug
> may happen and we may want to use trace events to debug. Rather, try to
> think of trace events as tracking the lifetime of the UDC and
> gadget. Trace values that may change over time.
>
> I also think that all three patches could be combined into a single
> commit, but that's up to discussion.

nevermind this last paragraph, just saw that Greg asked you to split ;-)

The first paragraph remains valid, though

-- 
balbi

RE: [PATCH V4 1/3] usb: gadget: configfs: add UDC trace entry

[Linyu Yuan (QUIC)]

Hi,

> -----Original Message-----
> From: Felipe Balbi <balbi@kernel.org>
> Sent: Tuesday, August 24, 2021 4:29 PM
> To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; linux-
> usb@vger.kernel.org
> Subject: Re: [PATCH V4 1/3] usb: gadget: configfs: add UDC trace entry
> 
> 
> Hi again,
> 
> Felipe Balbi <balbi@kernel.org> writes:
> > Linyu Yuan <quic_linyyuan@quicinc.com> writes:
> >> add trace in function gadget_dev_desc_UDC_store()
> >> will show when user space enable/disable the gadget.
> >>
> >> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> >> ---
> >>  drivers/usb/gadget/Makefile         |  1 +
> >>  drivers/usb/gadget/configfs.c       |  3 +++
> >>  drivers/usb/gadget/configfs_trace.c |  7 +++++++
> >>  drivers/usb/gadget/configfs_trace.h | 39
> +++++++++++++++++++++++++++++++++++++
> >>  4 files changed, 50 insertions(+)
> >>  create mode 100644 drivers/usb/gadget/configfs_trace.c
> >>  create mode 100644 drivers/usb/gadget/configfs_trace.h
> >>
> >> diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
> >> index 130dad7..8e9c2b8 100644
> >> --- a/drivers/usb/gadget/Makefile
> >> +++ b/drivers/usb/gadget/Makefile
> >> @@ -9,5 +9,6 @@ ccflags-y				+= -
> I$(srctree)/drivers/usb/gadget/udc
> >>  obj-$(CONFIG_USB_LIBCOMPOSITE)	+= libcomposite.o
> >>  libcomposite-y			:= usbstring.o config.o epautoconf.o
> >>  libcomposite-y			+= composite.o functions.o configfs.o
> u_f.o
> >> +libcomposite-y			+= configfs_trace.o
> >>
> >>  obj-$(CONFIG_USB_GADGET)	+= udc/ function/ legacy/
> >> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> >> index 477e72a..f7f3af8 100644
> >> --- a/drivers/usb/gadget/configfs.c
> >> +++ b/drivers/usb/gadget/configfs.c
> >> @@ -9,6 +9,7 @@
> >>  #include "configfs.h"
> >>  #include "u_f.h"
> >>  #include "u_os_desc.h"
> >> +#include "configfs_trace.h"
> >>
> >>  int check_user_usb_string(const char *name,
> >>  		struct usb_gadget_strings *stringtab_dev)
> >> @@ -270,6 +271,8 @@ static ssize_t gadget_dev_desc_UDC_store(struct
> config_item *item,
> >>  	if (name[len - 1] == '\n')
> >>  		name[len - 1] = '\0';
> >>
> >> +	trace_gadget_dev_desc_UDC_store(config_item_name(item),
> name);
> >
> > why tracing only the names? This gives us no insight into whatever bug
This patch only trace user space operation when enable a composition like below of android or similar thing in another way,

on property:sys.usb.config=mtp && property:sys.usb.configfs=1
    write /config/usb_gadget/g1/configs/b.1/strings/0x409/configuration "mtp"
    symlink /config/usb_gadget/g1/functions/mtp.gs0 /config/usb_gadget/g1/configs/b.1/f1
    write /config/usb_gadget/g1/UDC ${sys.usb.controller}

> > may happen and we may want to use trace events to debug. Rather, try to
> > think of trace events as tracking the lifetime of the UDC and
> > gadget. Trace values that may change over time.

UDC already have trace  https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/tree/drivers/usb/gadget/udc/trace.h?h=usb-linus
I can't confirm if I understand your comment correctly ?

> >
> > I also think that all three patches could be combined into a single
> > commit, but that's up to discussion.
> 
> nevermind this last paragraph, just saw that Greg asked you to split ;-)
> 
> The first paragraph remains valid, though
> 
> --
> balbi

Re: [PATCH V4 1/3] usb: gadget: configfs: add UDC trace entry

[Felipe Balbi]

Hi,

"Linyu Yuan (QUIC)" <quic_linyyuan@quicinc.com> writes:
>> Felipe Balbi <balbi@kernel.org> writes:
>> > Linyu Yuan <quic_linyyuan@quicinc.com> writes:
>> >> add trace in function gadget_dev_desc_UDC_store()
>> >> will show when user space enable/disable the gadget.
>> >>
>> >> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
>> >> ---
>> >>  drivers/usb/gadget/Makefile         |  1 +
>> >>  drivers/usb/gadget/configfs.c       |  3 +++
>> >>  drivers/usb/gadget/configfs_trace.c |  7 +++++++
>> >>  drivers/usb/gadget/configfs_trace.h | 39
>> +++++++++++++++++++++++++++++++++++++
>> >>  4 files changed, 50 insertions(+)
>> >>  create mode 100644 drivers/usb/gadget/configfs_trace.c
>> >>  create mode 100644 drivers/usb/gadget/configfs_trace.h
>> >>
>> >> diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
>> >> index 130dad7..8e9c2b8 100644
>> >> --- a/drivers/usb/gadget/Makefile
>> >> +++ b/drivers/usb/gadget/Makefile
>> >> @@ -9,5 +9,6 @@ ccflags-y				+= -
>> I$(srctree)/drivers/usb/gadget/udc
>> >>  obj-$(CONFIG_USB_LIBCOMPOSITE)	+= libcomposite.o
>> >>  libcomposite-y			:= usbstring.o config.o epautoconf.o
>> >>  libcomposite-y			+= composite.o functions.o configfs.o
>> u_f.o
>> >> +libcomposite-y			+= configfs_trace.o
>> >>
>> >>  obj-$(CONFIG_USB_GADGET)	+= udc/ function/ legacy/
>> >> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
>> >> index 477e72a..f7f3af8 100644
>> >> --- a/drivers/usb/gadget/configfs.c
>> >> +++ b/drivers/usb/gadget/configfs.c
>> >> @@ -9,6 +9,7 @@
>> >>  #include "configfs.h"
>> >>  #include "u_f.h"
>> >>  #include "u_os_desc.h"
>> >> +#include "configfs_trace.h"
>> >>
>> >>  int check_user_usb_string(const char *name,
>> >>  		struct usb_gadget_strings *stringtab_dev)
>> >> @@ -270,6 +271,8 @@ static ssize_t gadget_dev_desc_UDC_store(struct
>> config_item *item,
>> >>  	if (name[len - 1] == '\n')
>> >>  		name[len - 1] = '\0';
>> >>
>> >> +	trace_gadget_dev_desc_UDC_store(config_item_name(item),
>> name);
>> >
>> > why tracing only the names? This gives us no insight into whatever bug
> This patch only trace user space operation when enable a composition
> like below of android or similar thing in another way,
>
> on property:sys.usb.config=mtp && property:sys.usb.configfs=1
>     write /config/usb_gadget/g1/configs/b.1/strings/0x409/configuration "mtp"
>     symlink /config/usb_gadget/g1/functions/mtp.gs0 /config/usb_gadget/g1/configs/b.1/f1
>     write /config/usb_gadget/g1/UDC ${sys.usb.controller}

yeah, that's fine. I'm simply stating that you're missing an opportunity
to get more data which may be relevant in the future. If you merely
print the name of the UDC, you get zero information about the state of
the UDC when the gadget started.

You see, from that UDC_store function, you have access to the
gadget_info, which gives you access to the usb_composite_driver and
usb_composite_dev. Both of which contain valuable information about the
state of the UDC.

Instead of making a single trace that prints the name of the UDC when
you call store, make a trace event class that takes a struct gadget_info
pointer and extracts the information from it. Something like so:

DECLARE_EVENT_CLASS(log_gadget_info,
	TP_PROTO(struct gadget_info *gi),
        TP_ARGS(gi),
        TP_STRUCT__entry(
                __string(drv_name, gi->composite->name)
                __string(udc_name, gi->cdev->gadget->name)

        	__field(bool, use_os_desc)
                __field(char, b_vendor_code)
                __field(bool, unbind)
                __field(bool, sg_supported)
                __field(bool, is_otg)
                __field(bool, is_a_peripheral)
                __field(bool, b_hnp_enable)

		/*
                 * and so on, anything that may come in handy should a
		 * bug happen here
                 */
	),
	TP_fast_assign(
        	__assign_str(drv_name, gi->composite->name)
                __assign_srt(udc_name, gi->cdev->gadget->name)

		__entry->use_os_desc = gi->use_os_desc;
                /* and so on */
	),
        TP_printk("%s [%s]: ....",
        	__get_str(udc_name), __get_str(drv_name), ....)
);
                
Then you can easily add traces to several functions that use a similar
argument:

DEFINE_EVENT(log_gadget_info, gadget_dev_desc_UDC_store,
	TP_PROTO(struct gadget_info *gi),
        TP_ARGS(gi)
);

DEFINE_EVENT(log_gadget_info, gadget_dev_desc_UDC_show,
	TP_PROTO(struct gadget_info *gi),
        TP_ARGS(gi)
);

DEFINE_EVENT(log_gadget_info, unregister_gadget,
	TP_PROTO(struct gadget_info *gi),
        TP_ARGS(gi)
);

DEFINE_EVENT(log_gadget_info, gadget_dev_desc_max_speed_show,
	TP_PROTO(struct gadget_info *gi),
        TP_ARGS(gi)
);

DEFINE_EVENT(log_gadget_info, gadget_dev_desc_bcdDevice_store,
	TP_PROTO(struct gadget_info *gi),
        TP_ARGS(gi)
);

DEFINE_EVENT(log_gadget_info, gadget_dev_desc_bcdUSB_store,
	TP_PROTO(struct gadget_info *gi),
        TP_ARGS(gi)
);

DEFINE_EVENT(log_gadget_info, gadget_dev_desc_max_speed_show,
	TP_PROTO(struct gadget_info *gi),
        TP_ARGS(gi)
);

DEFINE_EVENT(log_gadget_info, gadget_dev_desc_max_speed_store,
	TP_PROTO(struct gadget_info *gi),
        TP_ARGS(gi)
);


and so on, for any other function that has direct access to a
gadget_info pointer.

-- 
balbi

RE: [PATCH V4 1/3] usb: gadget: configfs: add UDC trace entry

[Linyu Yuan (QUIC)]

Hi,

> From: Felipe Balbi <balbi@kernel.org>
> Sent: Tuesday, August 24, 2021 6:42 PM
> To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; linux-
> usb@vger.kernel.org
> Subject: Re: [PATCH V4 1/3] usb: gadget: configfs: add UDC trace entry
> 
> 
> Hi,
> 
> "Linyu Yuan (QUIC)" <quic_linyyuan@quicinc.com> writes:
> >> Felipe Balbi <balbi@kernel.org> writes:
> >> > Linyu Yuan <quic_linyyuan@quicinc.com> writes:
> >> >> add trace in function gadget_dev_desc_UDC_store()
> >> >> will show when user space enable/disable the gadget.
> >> >>
> >> >> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> >> >> ---
> >> >>  drivers/usb/gadget/Makefile         |  1 +
> >> >>  drivers/usb/gadget/configfs.c       |  3 +++
> >> >>  drivers/usb/gadget/configfs_trace.c |  7 +++++++
> >> >>  drivers/usb/gadget/configfs_trace.h | 39
> >> +++++++++++++++++++++++++++++++++++++
> >> >>  4 files changed, 50 insertions(+)
> >> >>  create mode 100644 drivers/usb/gadget/configfs_trace.c
> >> >>  create mode 100644 drivers/usb/gadget/configfs_trace.h
> >> >>
> >> >> diff --git a/drivers/usb/gadget/Makefile
> b/drivers/usb/gadget/Makefile
> >> >> index 130dad7..8e9c2b8 100644
> >> >> --- a/drivers/usb/gadget/Makefile
> >> >> +++ b/drivers/usb/gadget/Makefile
> >> >> @@ -9,5 +9,6 @@ ccflags-y				+= -
> >> I$(srctree)/drivers/usb/gadget/udc
> >> >>  obj-$(CONFIG_USB_LIBCOMPOSITE)	+= libcomposite.o
> >> >>  libcomposite-y			:= usbstring.o config.o epautoconf.o
> >> >>  libcomposite-y			+= composite.o functions.o configfs.o
> >> u_f.o
> >> >> +libcomposite-y			+= configfs_trace.o
> >> >>
> >> >>  obj-$(CONFIG_USB_GADGET)	+= udc/ function/ legacy/
> >> >> diff --git a/drivers/usb/gadget/configfs.c
> b/drivers/usb/gadget/configfs.c
> >> >> index 477e72a..f7f3af8 100644
> >> >> --- a/drivers/usb/gadget/configfs.c
> >> >> +++ b/drivers/usb/gadget/configfs.c
> >> >> @@ -9,6 +9,7 @@
> >> >>  #include "configfs.h"
> >> >>  #include "u_f.h"
> >> >>  #include "u_os_desc.h"
> >> >> +#include "configfs_trace.h"
> >> >>
> >> >>  int check_user_usb_string(const char *name,
> >> >>  		struct usb_gadget_strings *stringtab_dev)
> >> >> @@ -270,6 +271,8 @@ static ssize_t
> gadget_dev_desc_UDC_store(struct
> >> config_item *item,
> >> >>  	if (name[len - 1] == '\n')
> >> >>  		name[len - 1] = '\0';
> >> >>
> >> >> +	trace_gadget_dev_desc_UDC_store(config_item_name(item),
> >> name);
> >> >
> >> > why tracing only the names? This gives us no insight into whatever bug
> > This patch only trace user space operation when enable a composition
> > like below of android or similar thing in another way,
> >
> > on property:sys.usb.config=mtp && property:sys.usb.configfs=1
> >     write /config/usb_gadget/g1/configs/b.1/strings/0x409/configuration
> "mtp"
> >     symlink /config/usb_gadget/g1/functions/mtp.gs0
> /config/usb_gadget/g1/configs/b.1/f1
> >     write /config/usb_gadget/g1/UDC ${sys.usb.controller}
> 
> yeah, that's fine. I'm simply stating that you're missing an opportunity
> to get more data which may be relevant in the future. If you merely
> print the name of the UDC, you get zero information about the state of
> the UDC when the gadget started.
> 
> You see, from that UDC_store function, you have access to the
> gadget_info, which gives you access to the usb_composite_driver and
> usb_composite_dev. Both of which contain valuable information about the
> state of the UDC.
> 
> Instead of making a single trace that prints the name of the UDC when
> you call store, make a trace event class that takes a struct gadget_info
> pointer and extracts the information from it. Something like so:
> 
> DECLARE_EVENT_CLASS(log_gadget_info,
> 	TP_PROTO(struct gadget_info *gi),
>         TP_ARGS(gi),
>         TP_STRUCT__entry(
>                 __string(drv_name, gi->composite->name)
>                 __string(udc_name, gi->cdev->gadget->name)
> 

Do we need following two ?
>         	__field(bool, use_os_desc)
>                 __field(char, b_vendor_code)

>                 __field(bool, unbind)

why do you suggest following 4 fields ? it is not exist in gadget_info.
>                 __field(bool, sg_supported)
>                 __field(bool, is_otg)
>                 __field(bool, is_a_peripheral)
>                 __field(bool, b_hnp_enable)
> 
> 		/*
>                  * and so on, anything that may come in handy should a
> 		 * bug happen here
>                  */
> 	),
> 	TP_fast_assign(
>         	__assign_str(drv_name, gi->composite->name)
>                 __assign_srt(udc_name, gi->cdev->gadget->name)
> 
> 		__entry->use_os_desc = gi->use_os_desc;
>                 /* and so on */
> 	),
>         TP_printk("%s [%s]: ....",
>         	__get_str(udc_name), __get_str(drv_name), ....)
> );

the gadget_info have few info to trace, from my view only
struct gadget_info {
	struct config_group group;
	struct config_group functions_group;
	struct config_group configs_group;
	struct config_group strings_group;
	struct config_group os_desc_group;

	struct mutex lock;
	struct usb_gadget_strings *gstrings[MAX_USB_STRING_LANGS + 1];
	struct list_head string_list;
	struct list_head available_func;

	struct usb_composite_driver composite;
	struct usb_composite_dev cdev;
	bool use_os_desc;
	char b_vendor_code;
	char qw_sign[OS_STRING_QW_SIGN_LEN];
	spinlock_t spinlock;
	bool unbind;
};
> 
> Then you can easily add traces to several functions that use a similar
> argument:
> 
> DEFINE_EVENT(log_gadget_info, gadget_dev_desc_UDC_store,
> 	TP_PROTO(struct gadget_info *gi),
>         TP_ARGS(gi)
> );
> 

It is needed for show operation ?
> DEFINE_EVENT(log_gadget_info, gadget_dev_desc_UDC_show,
> 	TP_PROTO(struct gadget_info *gi),
>         TP_ARGS(gi)
> );
> 
> DEFINE_EVENT(log_gadget_info, unregister_gadget,
> 	TP_PROTO(struct gadget_info *gi),
>         TP_ARGS(gi)
> );
> 

Following operation also not needed, right ? according to my experience, it is not change in project.
> DEFINE_EVENT(log_gadget_info, gadget_dev_desc_max_speed_show,
> 	TP_PROTO(struct gadget_info *gi),
>         TP_ARGS(gi)
> );
> 
> DEFINE_EVENT(log_gadget_info, gadget_dev_desc_bcdDevice_store,
> 	TP_PROTO(struct gadget_info *gi),
>         TP_ARGS(gi)
> );
> 
> DEFINE_EVENT(log_gadget_info, gadget_dev_desc_bcdUSB_store,
> 	TP_PROTO(struct gadget_info *gi),
>         TP_ARGS(gi)
> );
> 
> DEFINE_EVENT(log_gadget_info, gadget_dev_desc_max_speed_show,
> 	TP_PROTO(struct gadget_info *gi),
>         TP_ARGS(gi)
> );
> 
> DEFINE_EVENT(log_gadget_info, gadget_dev_desc_max_speed_store,
> 	TP_PROTO(struct gadget_info *gi),
>         TP_ARGS(gi)
> );
> 
> 
> and so on, for any other function that has direct access to a
> gadget_info pointer.
> 
> --
> balbi

Re: [PATCH V4 1/3] usb: gadget: configfs: add UDC trace entry

[Felipe Balbi]

Hi,

"Linyu Yuan (QUIC)" <quic_linyyuan@quicinc.com> writes:
>> >> > why tracing only the names? This gives us no insight into whatever bug
>> > This patch only trace user space operation when enable a composition
>> > like below of android or similar thing in another way,
>> >
>> > on property:sys.usb.config=mtp && property:sys.usb.configfs=1
>> >     write /config/usb_gadget/g1/configs/b.1/strings/0x409/configuration
>> "mtp"
>> >     symlink /config/usb_gadget/g1/functions/mtp.gs0
>> /config/usb_gadget/g1/configs/b.1/f1
>> >     write /config/usb_gadget/g1/UDC ${sys.usb.controller}
>> 
>> yeah, that's fine. I'm simply stating that you're missing an opportunity
>> to get more data which may be relevant in the future. If you merely
>> print the name of the UDC, you get zero information about the state of
>> the UDC when the gadget started.
>> 
>> You see, from that UDC_store function, you have access to the
>> gadget_info, which gives you access to the usb_composite_driver and
>> usb_composite_dev. Both of which contain valuable information about the
>> state of the UDC.
>> 
>> Instead of making a single trace that prints the name of the UDC when
>> you call store, make a trace event class that takes a struct gadget_info
>> pointer and extracts the information from it. Something like so:
>> 
>> DECLARE_EVENT_CLASS(log_gadget_info,
>> 	TP_PROTO(struct gadget_info *gi),
>>         TP_ARGS(gi),
>>         TP_STRUCT__entry(
>>                 __string(drv_name, gi->composite->name)
>>                 __string(udc_name, gi->cdev->gadget->name)
>> 
>
> Do we need following two ?

say your QA team tells you that a particular situation is failing. You
ask them to collect trace events. You'll be glad to see a lot of
information available so you can understand how the device changed
stated as you used it.

>>         	__field(bool, use_os_desc)
>>                 __field(char, b_vendor_code)
>
>>                 __field(bool, unbind)
>
> why do you suggest following 4 fields ? it is not exist in gadget_info.

They are part of composite_dev, IIRC. They tell you important
information about what is supported by the UDC.

>>                 __field(bool, sg_supported)
>>                 __field(bool, is_otg)
>>                 __field(bool, is_a_peripheral)
>>                 __field(bool, b_hnp_enable)
>> 
>> 		/*
>>                  * and so on, anything that may come in handy should a
>> 		 * bug happen here
>>                  */
>> 	),
>> 	TP_fast_assign(
>>         	__assign_str(drv_name, gi->composite->name)
>>                 __assign_srt(udc_name, gi->cdev->gadget->name)
>> 
>> 		__entry->use_os_desc = gi->use_os_desc;
>>                 /* and so on */
>> 	),
>>         TP_printk("%s [%s]: ....",
>>         	__get_str(udc_name), __get_str(drv_name), ....)
>> );
>
> the gadget_info have few info to trace, from my view only

right...

> struct gadget_info {
> 	struct config_group group;
> 	struct config_group functions_group;
> 	struct config_group configs_group;
> 	struct config_group strings_group;
> 	struct config_group os_desc_group;
>
> 	struct mutex lock;
> 	struct usb_gadget_strings *gstrings[MAX_USB_STRING_LANGS + 1];
> 	struct list_head string_list;
> 	struct list_head available_func;
>
> 	struct usb_composite_driver composite;
> 	struct usb_composite_dev cdev;

... Then you can access the composite driver and the composite dev to
get more information which may be super useful when debugging some
issues.

Also keep in mind that changing trace events is not so easy since it
sort of becomes an ABI to userspace. Once we expose it, it's a little
harder to modify as there may be parsers depending on the format
(although they shouldn't).

> 	bool use_os_desc;
> 	char b_vendor_code;
> 	char qw_sign[OS_STRING_QW_SIGN_LEN];
> 	spinlock_t spinlock;
> 	bool unbind;
> };
>> 
>> Then you can easily add traces to several functions that use a similar
>> argument:
>> 
>> DEFINE_EVENT(log_gadget_info, gadget_dev_desc_UDC_store,
>> 	TP_PROTO(struct gadget_info *gi),
>>         TP_ARGS(gi)
>> );
>> 
>
> It is needed for show operation ?

you want to track both show and store.

>> DEFINE_EVENT(log_gadget_info, gadget_dev_desc_UDC_show,
>> 	TP_PROTO(struct gadget_info *gi),
>>         TP_ARGS(gi)
>> );
>> 
>> DEFINE_EVENT(log_gadget_info, unregister_gadget,
>> 	TP_PROTO(struct gadget_info *gi),
>>         TP_ARGS(gi)
>> );
>> 
>
> Following operation also not needed, right ? according to my
> experience, it is not change in project.

What if something changes some internal state behind our backs? We'd
like to see that. One way to notice is if some value changes even if
you're just calling the different show methods.

-- 
balbi

[PATCH V4 2/3] usb: gadget: configfs: add function link trace entry

[Linyu Yuan]

add trace in function config_usb_cfg_link()
will show which function was added to gadget configuration.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
 drivers/usb/gadget/configfs.c       |  4 ++++
 drivers/usb/gadget/configfs_trace.h | 17 +++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index f7f3af8..8c64640 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -426,6 +426,10 @@ static int config_usb_cfg_link(
 	struct usb_function *f;
 	int ret;
 
+	trace_config_usb_cfg_link(config_item_name(&gi->group.cg_item),
+				config_item_name(usb_cfg_ci),
+				config_item_name(usb_func_ci));
+
 	mutex_lock(&gi->lock);
 	/*
 	 * Make sure this function is from within our _this_ gadget and not
diff --git a/drivers/usb/gadget/configfs_trace.h b/drivers/usb/gadget/configfs_trace.h
index f2e17e4..5c666f1 100644
--- a/drivers/usb/gadget/configfs_trace.h
+++ b/drivers/usb/gadget/configfs_trace.h
@@ -26,6 +26,23 @@ TRACE_EVENT(gadget_dev_desc_UDC_store,
 		__get_str(udc_name))
 );
 
+TRACE_EVENT(config_usb_cfg_link,
+	TP_PROTO(char *name, char *cfg, char *func),
+	TP_ARGS(name, cfg, func),
+	TP_STRUCT__entry(
+		__string(group_name, name)
+		__string(cfg_name, cfg)
+		__string(func_name, func)
+	),
+	TP_fast_assign(
+		__assign_str(group_name, name);
+		__assign_str(cfg_name, cfg);
+		__assign_str(func_name, func)
+	),
+	TP_printk("gadget:%s cfg:%s link func:%s", __get_str(group_name),
+		__get_str(cfg_name), __get_str(func_name))
+);
+
 #endif /* __GADGET_TRACE_H */
 
 /* this part has to be here */
-- 
2.7.4

[PATCH V4 3/3] usb: gadget: configfs: add function unlink trace entry

[Linyu Yuan]

add trace in function config_usb_cfg_unlink()
whill show which function was removed from gadget configuraion.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
 drivers/usb/gadget/configfs.c       |  4 ++++
 drivers/usb/gadget/configfs_trace.h | 17 +++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 8c64640..4024f9b 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -479,6 +479,10 @@ static void config_usb_cfg_unlink(
 			struct usb_function_instance, group);
 	struct usb_function *f;
 
+	trace_config_usb_cfg_unlink(config_item_name(&gi->group.cg_item),
+				config_item_name(usb_cfg_ci),
+				config_item_name(usb_func_ci));
+
 	/*
 	 * ideally I would like to forbid to unlink functions while a gadget is
 	 * bound to an UDC. Since this isn't possible at the moment, we simply
diff --git a/drivers/usb/gadget/configfs_trace.h b/drivers/usb/gadget/configfs_trace.h
index 5c666f1..82e0121 100644
--- a/drivers/usb/gadget/configfs_trace.h
+++ b/drivers/usb/gadget/configfs_trace.h
@@ -43,6 +43,23 @@ TRACE_EVENT(config_usb_cfg_link,
 		__get_str(cfg_name), __get_str(func_name))
 );
 
+TRACE_EVENT(config_usb_cfg_unlink,
+	TP_PROTO(char *name, char *cfg, char *func),
+	TP_ARGS(name, cfg, func),
+	TP_STRUCT__entry(
+		__string(group_name, name)
+		__string(cfg_name, cfg)
+		__string(func_name, func)
+	),
+	TP_fast_assign(
+		__assign_str(group_name, name);
+		__assign_str(cfg_name, cfg);
+		__assign_str(func_name, func)
+	),
+	TP_printk("gadget:%s cfg:%s unlink func:%s", __get_str(group_name),
+		__get_str(cfg_name), __get_str(func_name))
+);
+
 #endif /* __GADGET_TRACE_H */
 
 /* this part has to be here */
-- 
2.7.4