linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V4 0/3] usb: gadget: configfs: add three trace entries
@ 2021-08-24  3:54 Linyu Yuan
  2021-08-24  3:54 ` [PATCH V4 1/3] usb: gadget: configfs: add UDC trace entry Linyu Yuan
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Linyu Yuan @ 2021-08-24  3:54 UTC (permalink / raw)
  To: Felipe Balbi, Greg Kroah-Hartman; +Cc: linux-usb, 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH V4 1/3] usb: gadget: configfs: add UDC trace entry
  2021-08-24  3:54 [PATCH V4 0/3] usb: gadget: configfs: add three trace entries Linyu Yuan
@ 2021-08-24  3:54 ` Linyu Yuan
  2021-08-24  8:15   ` Felipe Balbi
  2021-08-24  3:54 ` [PATCH V4 2/3] usb: gadget: configfs: add function link " Linyu Yuan
  2021-08-24  3:54 ` [PATCH V4 3/3] usb: gadget: configfs: add function unlink " Linyu Yuan
  2 siblings, 1 reply; 10+ messages in thread
From: Linyu Yuan @ 2021-08-24  3:54 UTC (permalink / raw)
  To: Felipe Balbi, Greg Kroah-Hartman; +Cc: linux-usb, 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH V4 2/3] usb: gadget: configfs: add function link trace entry
  2021-08-24  3:54 [PATCH V4 0/3] usb: gadget: configfs: add three trace entries Linyu Yuan
  2021-08-24  3:54 ` [PATCH V4 1/3] usb: gadget: configfs: add UDC trace entry Linyu Yuan
@ 2021-08-24  3:54 ` Linyu Yuan
  2021-08-24  3:54 ` [PATCH V4 3/3] usb: gadget: configfs: add function unlink " Linyu Yuan
  2 siblings, 0 replies; 10+ messages in thread
From: Linyu Yuan @ 2021-08-24  3:54 UTC (permalink / raw)
  To: Felipe Balbi, Greg Kroah-Hartman; +Cc: linux-usb, 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH V4 3/3] usb: gadget: configfs: add function unlink trace entry
  2021-08-24  3:54 [PATCH V4 0/3] usb: gadget: configfs: add three trace entries Linyu Yuan
  2021-08-24  3:54 ` [PATCH V4 1/3] usb: gadget: configfs: add UDC trace entry Linyu Yuan
  2021-08-24  3:54 ` [PATCH V4 2/3] usb: gadget: configfs: add function link " Linyu Yuan
@ 2021-08-24  3:54 ` Linyu Yuan
  2 siblings, 0 replies; 10+ messages in thread
From: Linyu Yuan @ 2021-08-24  3:54 UTC (permalink / raw)
  To: Felipe Balbi, Greg Kroah-Hartman; +Cc: linux-usb, 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH V4 1/3] usb: gadget: configfs: add UDC trace entry
  2021-08-24  3:54 ` [PATCH V4 1/3] usb: gadget: configfs: add UDC trace entry Linyu Yuan
@ 2021-08-24  8:15   ` Felipe Balbi
  2021-08-24  8:29     ` Felipe Balbi
  0 siblings, 1 reply; 10+ messages in thread
From: Felipe Balbi @ 2021-08-24  8:15 UTC (permalink / raw)
  To: Linyu Yuan; +Cc: Greg Kroah-Hartman, linux-usb


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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH V4 1/3] usb: gadget: configfs: add UDC trace entry
  2021-08-24  8:15   ` Felipe Balbi
@ 2021-08-24  8:29     ` Felipe Balbi
  2021-08-24 10:07       ` Linyu Yuan (QUIC)
  0 siblings, 1 reply; 10+ messages in thread
From: Felipe Balbi @ 2021-08-24  8:29 UTC (permalink / raw)
  To: Linyu Yuan; +Cc: Greg Kroah-Hartman, linux-usb


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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH V4 1/3] usb: gadget: configfs: add UDC trace entry
  2021-08-24  8:29     ` Felipe Balbi
@ 2021-08-24 10:07       ` Linyu Yuan (QUIC)
  2021-08-24 10:41         ` Felipe Balbi
  0 siblings, 1 reply; 10+ messages in thread
From: Linyu Yuan (QUIC) @ 2021-08-24 10:07 UTC (permalink / raw)
  To: Felipe Balbi, Linyu Yuan (QUIC); +Cc: Greg Kroah-Hartman, linux-usb

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH V4 1/3] usb: gadget: configfs: add UDC trace entry
  2021-08-24 10:07       ` Linyu Yuan (QUIC)
@ 2021-08-24 10:41         ` Felipe Balbi
  2021-08-25  9:37           ` Linyu Yuan (QUIC)
  0 siblings, 1 reply; 10+ messages in thread
From: Felipe Balbi @ 2021-08-24 10:41 UTC (permalink / raw)
  To: Linyu Yuan (QUIC); +Cc: Greg Kroah-Hartman, linux-usb


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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH V4 1/3] usb: gadget: configfs: add UDC trace entry
  2021-08-24 10:41         ` Felipe Balbi
@ 2021-08-25  9:37           ` Linyu Yuan (QUIC)
  2021-08-25 10:50             ` Felipe Balbi
  0 siblings, 1 reply; 10+ messages in thread
From: Linyu Yuan (QUIC) @ 2021-08-25  9:37 UTC (permalink / raw)
  To: Felipe Balbi, Linyu Yuan (QUIC); +Cc: Greg Kroah-Hartman, linux-usb

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH V4 1/3] usb: gadget: configfs: add UDC trace entry
  2021-08-25  9:37           ` Linyu Yuan (QUIC)
@ 2021-08-25 10:50             ` Felipe Balbi
  0 siblings, 0 replies; 10+ messages in thread
From: Felipe Balbi @ 2021-08-25 10:50 UTC (permalink / raw)
  To: Linyu Yuan (QUIC); +Cc: Greg Kroah-Hartman, linux-usb


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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-08-25 10:55 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-24  3:54 [PATCH V4 0/3] usb: gadget: configfs: add three trace entries Linyu Yuan
2021-08-24  3:54 ` [PATCH V4 1/3] usb: gadget: configfs: add UDC trace entry Linyu Yuan
2021-08-24  8:15   ` Felipe Balbi
2021-08-24  8:29     ` Felipe Balbi
2021-08-24 10:07       ` Linyu Yuan (QUIC)
2021-08-24 10:41         ` Felipe Balbi
2021-08-25  9:37           ` Linyu Yuan (QUIC)
2021-08-25 10:50             ` Felipe Balbi
2021-08-24  3:54 ` [PATCH V4 2/3] usb: gadget: configfs: add function link " Linyu Yuan
2021-08-24  3:54 ` [PATCH V4 3/3] usb: gadget: configfs: add function unlink " Linyu Yuan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).