diff mbox series

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

Message ID 1629777281-30188-2-git-send-email-quic_linyyuan@quicinc.com
State New
Headers show
Series usb: gadget: configfs: add three trace entries | expand

Commit Message

Linyu Yuan Aug. 24, 2021, 3:54 a.m. UTC
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

Comments

Linyu Yuan Aug. 24, 2021, 10:07 a.m. UTC | #1
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
Felipe Balbi Aug. 24, 2021, 10:41 a.m. UTC | #2
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.
Linyu Yuan Aug. 25, 2021, 9:37 a.m. UTC | #3
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
Felipe Balbi Aug. 25, 2021, 10:50 a.m. UTC | #4
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
diff mbox series

Patch

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>