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

[Felipe Balbi <balbi@kernel.org>]

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