| Message ID | 1629777281-30188-2-git-send-email-quic_linyyuan@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: gadget: configfs: add three trace entries | expand |
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.
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
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
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.
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
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.
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>
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