Hi,
> From: Felipe Balbi <balbi@xxxxxxxxxx>
> Sent: Tuesday, August 24, 2021 6:42 PM
> To: Linyu Yuan (QUIC) <quic_linyyuan@xxxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; linux-
> usb@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH V4 1/3] usb: gadget: configfs: add UDC trace entry
>
>
> Hi,
>
> "Linyu Yuan (QUIC)" <quic_linyyuan@xxxxxxxxxxx> writes:
> >> Felipe Balbi <balbi@xxxxxxxxxx> writes:
> >> > Linyu Yuan <quic_linyyuan@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> >> >> ---
> >> >> 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
