[PATCH v6 0/5] usb: gadget: configfs: new trace events

[PATCH v6 0/5] usb: gadget: configfs: new trace events

[Linyu Yuan]

Last year I try to add trace event support for usb gadget configfs [1],
this time the idea is change a lot, the purpose is trace all user space
operation to gadget configuration, include gadget and it's function.

In usb gadget configfs, mainly user can do mkdir/rmdir a group,
link/unlink a function, change gadget/function attributes,
each operation will touch a struct config_item.

It only have one trace event entry which store string,
provide several API which represent user operation and generate string
from  struct config_item.

example output,
   mkdir-80      [000] .....    44.555106: gadget_configfs: mkdir dummy
      sh-76      [000] .....    44.562609: gadget_configfs: echo dummy/idVendor 0x05C6

   mkdir-81      [000] .....    44.569795: gadget_configfs: mkdir dummy/functions/eem.0
      sh-76      [000] .....    44.600221: gadget_configfs: echo dummy/functions/eem.0/dev_addr 1e:77:46:4b:1e:96

   mkdir-82      [000] .....    44.615542: gadget_configfs: mkdir dummy/configs/dummy.1
      ln-83      [000] .....    44.628997: gadget_configfs: link dummy/configs/dummy.1 dummy/functions/eem.0
      sh-76      [000] .....    44.634506: gadget_configfs: echo dummy/configs/dummy.1/MaxPower 500

   mkdir-84      [000] .....    44.642265: gadget_configfs: mkdir dummy/configs/dummy.1/strings/0x409
      sh-76      [000] .....    44.663886: gadget_configfs: echo dummy/configs/dummy.1/strings/0x409/configuration dummy

   rmdir-85      [000] .....    64.255507: gadget_configfs: rmdir dummy/configs/dummy.1/strings/0x409
      rm-86      [000] .....    64.263926: gadget_configfs: unlink dummy/configs/dummy.1 dummy/functions/eem.0
   rmdir-87      [000] .....    64.279768: gadget_configfs: rmdir dummy/configs/dummy.1
   rmdir-88      [000] .....    64.328124: gadget_configfs: rmdir dummy/functions/eem.0
   rmdir-89      [000] .....    64.992085: gadget_configfs: rmdir dummy


As it is different from last year change, start a new thread.

[1] https://lore.kernel.org/linux-usb/1635229309-2821-1-git-send-email-quic_linyyuan@quicinc.com/

V2: add example output
V3: add trace.c and move all APIs to it
v4: fix memory leak in v3
v5: change return value report by kernel test robot <lkp@intel.com> and
    Dan Carpenter <dan.carpenter@oracle.com>
v6: fix checkpatch warning

Linyu Yuan (5):
  usb: gadget: remove gadgets_type storage type 'static'
  usb: gadget: add trace event of configfs operation
  usb: gadget: add trace event of configfs group operation
  usb: gadget: add trace event of configfs link/unlink operation
  usb: gadget: add trace event of configfs write attributes operation

 drivers/usb/gadget/Makefile                    |   2 +
 drivers/usb/gadget/configfs.c                  |  45 ++++++-
 drivers/usb/gadget/function/f_acm.c            |   1 +
 drivers/usb/gadget/function/f_hid.c            |   4 +
 drivers/usb/gadget/function/f_loopback.c       |   4 +
 drivers/usb/gadget/function/f_mass_storage.c   |  20 +++
 drivers/usb/gadget/function/f_midi.c           |   6 +
 drivers/usb/gadget/function/f_printer.c        |   4 +
 drivers/usb/gadget/function/f_serial.c         |   1 +
 drivers/usb/gadget/function/f_sourcesink.c     |  16 +++
 drivers/usb/gadget/function/f_uac1.c           |   6 +
 drivers/usb/gadget/function/f_uac1_legacy.c    |   4 +
 drivers/usb/gadget/function/f_uac2.c           |   8 ++
 drivers/usb/gadget/function/u_ether_configfs.h |  10 ++
 drivers/usb/gadget/function/uvc_configfs.c     |  42 +++++++
 drivers/usb/gadget/trace.c                     | 163 +++++++++++++++++++++++++
 drivers/usb/gadget/trace.h                     |  39 ++++++
 include/linux/usb/composite.h                  |  18 +++
 include/linux/usb/gadget_configfs.h            |   4 +
 19 files changed, 396 insertions(+), 1 deletion(-)
 create mode 100644 drivers/usb/gadget/trace.c
 create mode 100644 drivers/usb/gadget/trace.h

-- 
2.7.4

[PATCH v6 1/5] usb: gadget: remove gadgets_type storage type 'static'

[Linyu Yuan]

Prepare for new trace event function which will use gadgets_type variable.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
v3: first add this change
v4: no change
v5: no change
v6: no change

 drivers/usb/gadget/configfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 1fb837d..b2beeda 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -1642,7 +1642,7 @@ static struct configfs_group_operations gadgets_ops = {
 	.drop_item      = &gadgets_drop,
 };
 
-static const struct config_item_type gadgets_type = {
+const struct config_item_type gadgets_type = {
 	.ct_group_ops   = &gadgets_ops,
 	.ct_owner       = THIS_MODULE,
 };
-- 
2.7.4

[PATCH v6 2/5] usb: gadget: add trace event of configfs operation

[Linyu Yuan]

Add a common trace event entry which have only one __string field,
it allow create APIs base on it to add trace events for
usb gadget and function layer, then it will cover all user input
like make configfs group/item, drop item, write attribute, allow/drop link.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
v2: no change
v3: add trace.c and configfs.c will not include trace.h
v4: no change
v5: no change
v6: change license start with // in trace.c

 drivers/usb/gadget/Makefile |  2 ++
 drivers/usb/gadget/trace.c  |  7 +++++++
 drivers/usb/gadget/trace.h  | 39 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 48 insertions(+)
 create mode 100644 drivers/usb/gadget/trace.c
 create mode 100644 drivers/usb/gadget/trace.h

diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
index 33f1ef9..b426f5c 100644
--- a/drivers/usb/gadget/Makefile
+++ b/drivers/usb/gadget/Makefile
@@ -8,5 +8,7 @@ subdir-ccflags-$(CONFIG_USB_GADGET_VERBOSE)	+= -DVERBOSE_DEBUG
 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
+CFLAGS_trace.o			:= -I$(src)
+libcomposite-y			+= trace.o
 
 obj-$(CONFIG_USB_GADGET)	+= udc/ function/ legacy/
diff --git a/drivers/usb/gadget/trace.c b/drivers/usb/gadget/trace.c
new file mode 100644
index 0000000..108c1c8
--- /dev/null
+++ b/drivers/usb/gadget/trace.c
@@ -0,0 +1,7 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#define CREATE_TRACE_POINTS
+#include "trace.h"
diff --git a/drivers/usb/gadget/trace.h b/drivers/usb/gadget/trace.h
new file mode 100644
index 0000000..d556580
--- /dev/null
+++ b/drivers/usb/gadget/trace.h
@@ -0,0 +1,39 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM gadget_configfs
+
+
+#if !defined(_GADGET_TRACE_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _GADGET_TRACE_H
+
+#include <linux/tracepoint.h>
+
+TRACE_EVENT(gadget_configfs,
+	TP_PROTO(char *info),
+	TP_ARGS(info),
+	TP_STRUCT__entry(
+		__string(info, info)
+	),
+
+	TP_fast_assign(
+		__assign_str(info, info);
+	),
+
+	TP_printk("%s", __get_str(info))
+);
+
+#endif /* _GADGET_TRACE_H */
+
+/* this part has to be here */
+
+#undef TRACE_INCLUDE_PATH
+#define TRACE_INCLUDE_PATH .
+
+#undef TRACE_INCLUDE_FILE
+#define TRACE_INCLUDE_FILE trace
+
+#include <trace/define_trace.h>
-- 
2.7.4

[PATCH v6 3/5] usb: gadget: add trace event of configfs group operation

[Linyu Yuan]

Add API trace_usb_configfs_make_group() and trace_usb_configfs_drop_group()
to trace user create groups like gadget/function/configuration,
it also trace group create in a specific function.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
v2: no change
v3: add API in trace.c
v4: fix memory leak
v5: change return value which report by kernel test robot <lkp@intel.com> and
    Dan Carpenter <dan.carpenter@oracle.com>
v6: fix checkpatch warning to add one space after while keyworkd

 drivers/usb/gadget/configfs.c                | 11 ++++
 drivers/usb/gadget/function/f_mass_storage.c |  4 ++
 drivers/usb/gadget/function/uvc_configfs.c   | 12 ++++
 drivers/usb/gadget/trace.c                   | 84 ++++++++++++++++++++++++++++
 include/linux/usb/composite.h                |  9 +++
 include/linux/usb/gadget_configfs.h          |  2 +
 6 files changed, 122 insertions(+)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index b2beeda..a0599fb 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -621,6 +621,8 @@ static struct config_group *function_make(
 
 	gi = container_of(group, struct gadget_info, functions_group);
 
+	trace_usb_configfs_make_group(&group->cg_item, &fi->group.cg_item);
+
 	mutex_lock(&gi->lock);
 	list_add_tail(&fi->cfs_list, &gi->available_func);
 	mutex_unlock(&gi->lock);
@@ -634,6 +636,8 @@ static void function_drop(
 	struct usb_function_instance *fi = to_usb_function_instance(item);
 	struct gadget_info *gi;
 
+	trace_usb_configfs_drop_group(&group->cg_item, item);
+
 	gi = container_of(group, struct gadget_info, functions_group);
 
 	mutex_lock(&gi->lock);
@@ -729,6 +733,7 @@ static struct config_group *config_desc_make(
 	if (ret)
 		goto err;
 
+	trace_usb_configfs_make_group(&group->cg_item, &cfg->group.cg_item);
 	return &cfg->group;
 err:
 	kfree(cfg->c.label);
@@ -740,6 +745,7 @@ static void config_desc_drop(
 		struct config_group *group,
 		struct config_item *item)
 {
+	trace_usb_configfs_drop_group(&group->cg_item, item);
 	config_item_put(item);
 }
 
@@ -1083,6 +1089,7 @@ static struct config_item *ext_prop_make(
 	ext_prop_type->ct_owner = desc->owner;
 
 	config_item_init_type_name(&ext_prop->item, name, ext_prop_type);
+	trace_usb_configfs_make_group(&group->cg_item, &ext_prop->item);
 
 	ext_prop->name = kstrdup(name, GFP_KERNEL);
 	if (!ext_prop->name) {
@@ -1107,6 +1114,8 @@ static void ext_prop_drop(struct config_group *group, struct config_item *item)
 	struct usb_os_desc_ext_prop *ext_prop = to_usb_os_desc_ext_prop(item);
 	struct usb_os_desc *desc = to_usb_os_desc(&group->cg_item);
 
+	trace_usb_configfs_drop_group(&group->cg_item, item);
+
 	if (desc->opts_mutex)
 		mutex_lock(desc->opts_mutex);
 	list_del(&ext_prop->entry);
@@ -1626,6 +1635,7 @@ static struct config_group *gadgets_make(
 	if (!gi->composite.gadget_driver.function)
 		goto err;
 
+	trace_usb_configfs_make_group(&group->cg_item, &gi->group.cg_item);
 	return &gi->group;
 err:
 	kfree(gi);
@@ -1634,6 +1644,7 @@ static struct config_group *gadgets_make(
 
 static void gadgets_drop(struct config_group *group, struct config_item *item)
 {
+	trace_usb_configfs_drop_group(&group->cg_item, item);
 	config_item_put(item);
 }
 
diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index 3a77bca..a96eca9 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -3289,6 +3289,8 @@ static struct config_group *fsg_lun_make(struct config_group *group,
 
 	config_group_init_type_name(&opts->group, name, &fsg_lun_type);
 
+	trace_usb_configfs_make_group(&group->cg_item, &opts->group.cg_item);
+
 	return &opts->group;
 out:
 	mutex_unlock(&fsg_opts->lock);
@@ -3300,6 +3302,8 @@ static void fsg_lun_drop(struct config_group *group, struct config_item *item)
 	struct fsg_lun_opts *lun_opts;
 	struct fsg_opts *fsg_opts;
 
+	trace_usb_configfs_drop_group(&group->cg_item, item);
+
 	lun_opts = to_fsg_lun_opts(item);
 	fsg_opts = to_fsg_opts(&group->cg_item);
 
diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
index 77d6403..cc0f2eb 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -236,6 +236,8 @@ static struct config_item *uvcg_control_header_make(struct config_group *group,
 
 	config_item_init_type_name(&h->item, name, &uvcg_control_header_type);
 
+	trace_usb_configfs_make_group(&group->cg_item, &h->item);
+
 	return &h->item;
 }
 
@@ -1039,6 +1041,8 @@ static struct config_item
 
 	config_item_init_type_name(&h->item, name, &uvcg_streaming_header_type);
 
+	trace_usb_configfs_make_group(&group->cg_item, &h->item);
+
 	return &h->item;
 }
 
@@ -1380,6 +1384,8 @@ static struct config_item *uvcg_frame_make(struct config_group *group,
 
 	config_item_init_type_name(&h->item, name, &uvcg_frame_type);
 
+	trace_usb_configfs_make_group(&group->cg_item, &h->item);
+
 	return &h->item;
 }
 
@@ -1389,6 +1395,8 @@ static void uvcg_frame_drop(struct config_group *group, struct config_item *item
 	struct f_uvc_opts *opts;
 	struct config_item *opts_item;
 
+	trace_usb_configfs_drop_group(&group->cg_item, item);
+
 	opts_item = group->cg_item.ci_parent->ci_parent->ci_parent;
 	opts = to_f_uvc_opts(opts_item);
 
@@ -1649,6 +1657,8 @@ static struct config_group *uvcg_uncompressed_make(struct config_group *group,
 	config_group_init_type_name(&h->fmt.group, name,
 				    &uvcg_uncompressed_type);
 
+	trace_usb_configfs_make_group(&group->cg_item, &h->fmt.group.cg_item);
+
 	return &h->fmt.group;
 }
 
@@ -1835,6 +1845,8 @@ static struct config_group *uvcg_mjpeg_make(struct config_group *group,
 	config_group_init_type_name(&h->fmt.group, name,
 				    &uvcg_mjpeg_type);
 
+	trace_usb_configfs_make_group(&group->cg_item, &h->fmt.group.cg_item);
+
 	return &h->fmt.group;
 }
 
diff --git a/drivers/usb/gadget/trace.c b/drivers/usb/gadget/trace.c
index 108c1c8..65b328f 100644
--- a/drivers/usb/gadget/trace.c
+++ b/drivers/usb/gadget/trace.c
@@ -5,3 +5,87 @@
 
 #define CREATE_TRACE_POINTS
 #include "trace.h"
+
+#include <linux/configfs.h>
+#include <linux/usb/composite.h>
+
+extern const struct config_item_type gadgets_type;
+
+#ifdef CONFIG_TRACEPOINTS
+#define GROUP_LEN	128
+static int gadget_configfs_group(char *group, struct config_item *item)
+{
+	struct config_item *parent;
+	char *tmpgroup;
+
+	if (!item)
+		return -EINVAL;
+
+	tmpgroup = kzalloc(GROUP_LEN, GFP_KERNEL);
+	if (!tmpgroup)
+		return -ENOMEM;
+
+	for (parent = item->ci_parent; parent;
+			item = parent, parent = item->ci_parent) {
+		if (item->ci_type == &gadgets_type) {
+			kfree(tmpgroup);
+			return 0;
+		}
+
+		if (tmpgroup[0] == '\0')
+			snprintf(group, GROUP_LEN, "%s",
+					config_item_name(item));
+		else
+			snprintf(group, GROUP_LEN, "%s/%s",
+					config_item_name(item), tmpgroup);
+
+		strcpy(tmpgroup, group);
+	}
+
+	kfree(tmpgroup);
+	return -EINVAL;
+}
+
+static void trace_usb_configfs_make_drop_group(struct config_item *parent,
+		struct config_item *item, char *make_drop)
+{
+	char *group, *parent_group;
+	int ret;
+
+	group = kzalloc(2 * GROUP_LEN, GFP_KERNEL);
+	if (!group)
+		return;
+
+	parent_group = group + GROUP_LEN;
+	ret = gadget_configfs_group(parent_group, parent);
+	if (ret) {
+		kfree(group);
+		return;
+	}
+
+	if (parent_group[0] == '\0')
+		snprintf(group, GROUP_LEN, "%s %s", make_drop,
+				config_item_name(item));
+	else
+		snprintf(group, GROUP_LEN, "%s %s/%s", make_drop, parent_group,
+				config_item_name(item));
+
+	trace_gadget_configfs(group);
+
+	kfree(group);
+}
+
+void trace_usb_configfs_make_group(struct config_item *parent,
+		struct config_item *item)
+{
+	trace_usb_configfs_make_drop_group(parent, item, "mkdir");
+}
+EXPORT_SYMBOL(trace_usb_configfs_make_group);
+
+void trace_usb_configfs_drop_group(struct config_item *parent,
+		struct config_item *item)
+{
+	trace_usb_configfs_make_drop_group(parent, item, "rmdir");
+}
+EXPORT_SYMBOL(trace_usb_configfs_drop_group);
+#endif
diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
index 9d27622..7bf6574 100644
--- a/include/linux/usb/composite.h
+++ b/include/linux/usb/composite.h
@@ -603,6 +603,15 @@ void usb_put_function_instance(struct usb_function_instance *fi);
 void usb_put_function(struct usb_function *f);
 struct usb_function_instance *usb_get_function_instance(const char *name);
 struct usb_function *usb_get_function(struct usb_function_instance *fi);
+#ifdef CONFIG_TRACEPOINTS
+void trace_usb_configfs_make_group(struct config_item *parent,
+		struct config_item *item);
+void trace_usb_configfs_drop_group(struct config_item *parent,
+		struct config_item *item);
+#else
+#define trace_usb_configfs_make_group(parent, item) do {} while (0)
+#define trace_usb_configfs_drop_group(parent, item) do {} while (0)
+#endif
 
 struct usb_configuration *usb_get_config(struct usb_composite_dev *cdev,
 		int val);
diff --git a/include/linux/usb/gadget_configfs.h b/include/linux/usb/gadget_configfs.h
index d61aebd..a89f177 100644
--- a/include/linux/usb/gadget_configfs.h
+++ b/include/linux/usb/gadget_configfs.h
@@ -63,6 +63,7 @@ static struct config_item_type struct_in##_langid_type = {		\
 		goto err;						\
 	config_group_init_type_name(&new->group, name,			\
 			&struct_in##_langid_type);			\
+	trace_usb_configfs_make_group(&group->cg_item, &new->group.cg_item); \
 									\
 	gi = container_of(group, struct struct_member, strings_group);	\
 	ret = -EEXIST;							\
@@ -86,6 +87,7 @@ static void struct_in##_strings_drop(					\
 		struct config_group *group,				\
 		struct config_item *item)				\
 {									\
+	trace_usb_configfs_drop_group(&group->cg_item, item);		\
 	config_item_put(item);						\
 }									\
 									\
-- 
2.7.4

[PATCH v6 4/5] usb: gadget: add trace event of configfs link/unlink operation

[Linyu Yuan]

Add API trace_usb_configfs_link/unlink_group() to trace user space
link/unlink a group like add function to configuration or
remove function from configuration.
If a specific function need link/unlink, it also can be used.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
v2: no change
v3: add API in trace.c
v4: fix memory leak
v5: no change
v6: fix checkpatch warning

 drivers/usb/gadget/configfs.c              |  8 ++++++
 drivers/usb/gadget/function/uvc_configfs.c | 12 ++++++++
 drivers/usb/gadget/trace.c                 | 46 ++++++++++++++++++++++++++++++
 include/linux/usb/composite.h              |  6 ++++
 4 files changed, 72 insertions(+)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index a0599fb..a304d29 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -422,6 +422,8 @@ static int config_usb_cfg_link(
 	struct usb_function *f;
 	int ret;
 
+	trace_usb_configfs_link_group(usb_cfg_ci, usb_func_ci);
+
 	mutex_lock(&gi->lock);
 	/*
 	 * Make sure this function is from within our _this_ gadget and not
@@ -471,6 +473,8 @@ static void config_usb_cfg_unlink(
 			to_usb_function_instance(usb_func_ci);
 	struct usb_function *f;
 
+	trace_usb_configfs_unlink_group(usb_cfg_ci, 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
@@ -893,6 +897,8 @@ static int os_desc_link(struct config_item *os_desc_ci,
 	struct usb_configuration *c = NULL, *iter;
 	int ret;
 
+	trace_usb_configfs_link_group(os_desc_ci, usb_cfg_ci);
+
 	mutex_lock(&gi->lock);
 	list_for_each_entry(iter, &cdev->configs, list) {
 		if (iter != &c_target->c)
@@ -924,6 +930,8 @@ static void os_desc_unlink(struct config_item *os_desc_ci,
 	struct gadget_info *gi = os_desc_item_to_gadget_info(os_desc_ci);
 	struct usb_composite_dev *cdev = &gi->cdev;
 
+	trace_usb_configfs_unlink_group(os_desc_ci, usb_cfg_ci);
+
 	mutex_lock(&gi->lock);
 	if (gi->composite.gadget_driver.udc_name)
 		unregister_gadget(gi);
diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
index cc0f2eb..fc139f3 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -598,6 +598,8 @@ static int uvcg_control_class_allow_link(struct config_item *src,
 	struct uvcg_control_header *target_hdr;
 	int ret = -EINVAL;
 
+	trace_usb_configfs_link_group(src, target);
+
 	mutex_lock(su_mutex); /* for navigating configfs hierarchy */
 
 	control = src->ci_parent->ci_parent;
@@ -639,6 +641,8 @@ static void uvcg_control_class_drop_link(struct config_item *src,
 	struct uvc_descriptor_header **class_array;
 	struct uvcg_control_header *target_hdr;
 
+	trace_usb_configfs_unlink_group(src, target);
+
 	mutex_lock(su_mutex); /* for navigating configfs hierarchy */
 
 	control = src->ci_parent->ci_parent;
@@ -883,6 +887,8 @@ static int uvcg_streaming_header_allow_link(struct config_item *src,
 	struct uvcg_format_ptr *format_ptr;
 	int i, ret = -EINVAL;
 
+	trace_usb_configfs_link_group(src, target);
+
 	src_hdr = to_uvcg_streaming_header(src);
 	mutex_lock(su_mutex); /* for navigating configfs hierarchy */
 
@@ -946,6 +952,8 @@ static void uvcg_streaming_header_drop_link(struct config_item *src,
 	struct uvcg_format *target_fmt = NULL;
 	struct uvcg_format_ptr *format_ptr, *tmp;
 
+	trace_usb_configfs_unlink_group(src, target);
+
 	src_hdr = to_uvcg_streaming_header(src);
 	mutex_lock(su_mutex); /* for navigating configfs hierarchy */
 
@@ -2171,6 +2179,8 @@ static int uvcg_streaming_class_allow_link(struct config_item *src,
 	size_t size = 0, count = 0;
 	int ret = -EINVAL;
 
+	trace_usb_configfs_link_group(src, target);
+
 	mutex_lock(su_mutex); /* for navigating configfs hierarchy */
 
 	streaming = src->ci_parent->ci_parent;
@@ -2242,6 +2252,8 @@ static void uvcg_streaming_class_drop_link(struct config_item *src,
 	struct uvc_descriptor_header ***class_array;
 	struct uvcg_streaming_header *target_hdr;
 
+	trace_usb_configfs_unlink_group(src, target);
+
 	mutex_lock(su_mutex); /* for navigating configfs hierarchy */
 
 	streaming = src->ci_parent->ci_parent;
diff --git a/drivers/usb/gadget/trace.c b/drivers/usb/gadget/trace.c
index 65b328f..9b33b30 100644
--- a/drivers/usb/gadget/trace.c
+++ b/drivers/usb/gadget/trace.c
@@ -88,4 +88,50 @@ void trace_usb_configfs_drop_group(struct config_item *parent,
 	trace_usb_configfs_make_drop_group(parent, item, "rmdir");
 }
 EXPORT_SYMBOL(trace_usb_configfs_drop_group);
+
+static void trace_usb_configfs_link_unlink_group(struct config_item *dest,
+		struct config_item *src, char *link_unlink)
+{
+	char *group, *dest_group, *src_group;
+	int ret;
+
+	group = kzalloc(4 * GROUP_LEN, GFP_KERNEL);
+	if (!group)
+		return;
+
+	dest_group = group + 2 * GROUP_LEN;
+	ret = gadget_configfs_group(dest_group, dest);
+	if (ret) {
+		kfree(group);
+		return;
+	}
+
+	src_group = group + 3 * GROUP_LEN;
+	ret = gadget_configfs_group(src_group, src);
+	if (ret) {
+		kfree(group);
+		return;
+	}
+
+	snprintf(group, 2 * GROUP_LEN, "%s %s %s",
+			link_unlink, dest_group, src_group);
+
+	trace_gadget_configfs(group);
+
+	kfree(group);
+}
+
+void trace_usb_configfs_link_group(struct config_item *dest,
+		struct config_item *src)
+{
+	trace_usb_configfs_link_unlink_group(dest, src, "link");
+}
+EXPORT_SYMBOL(trace_usb_configfs_link_group);
+
+void trace_usb_configfs_unlink_group(struct config_item *dest,
+		struct config_item *src)
+{
+	trace_usb_configfs_link_unlink_group(dest, src, "unlink");
+}
+EXPORT_SYMBOL(trace_usb_configfs_unlink_group);
 #endif
diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
index 7bf6574..65dc26e 100644
--- a/include/linux/usb/composite.h
+++ b/include/linux/usb/composite.h
@@ -608,9 +608,15 @@ void trace_usb_configfs_make_group(struct config_item *parent,
 		struct config_item *item);
 void trace_usb_configfs_drop_group(struct config_item *parent,
 		struct config_item *item);
+void trace_usb_configfs_link_group(struct config_item *dest,
+		struct config_item *src);
+void trace_usb_configfs_unlink_group(struct config_item *dest,
+		struct config_item *src);
 #else
 #define trace_usb_configfs_make_group(parent, item) do {} while (0)
 #define trace_usb_configfs_drop_group(parent, item) do {} while (0)
+#define trace_usb_configfs_link_group(dest, src) do {} while (0)
+#define trace_usb_configfs_unlink_group(dest, src) do {} while (0)
 #endif
 
 struct usb_configuration *usb_get_config(struct usb_composite_dev *cdev,
-- 
2.7.4

[PATCH v6 5/5] usb: gadget: add trace event of configfs write attributes operation

[Linyu Yuan]

Add API trace_usb_configfs_write_attr() to trace user change gadget or
function attributes.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
v2: no change
v3: add API in trace.c
v4: fix memory leak
v5: no change
v6: fix checkpatch warning

 drivers/usb/gadget/configfs.c                  | 24 ++++++++++++++++++++++++
 drivers/usb/gadget/function/f_acm.c            |  1 +
 drivers/usb/gadget/function/f_hid.c            |  4 ++++
 drivers/usb/gadget/function/f_loopback.c       |  4 ++++
 drivers/usb/gadget/function/f_mass_storage.c   | 16 ++++++++++++++++
 drivers/usb/gadget/function/f_midi.c           |  6 ++++++
 drivers/usb/gadget/function/f_printer.c        |  4 ++++
 drivers/usb/gadget/function/f_serial.c         |  1 +
 drivers/usb/gadget/function/f_sourcesink.c     | 16 ++++++++++++++++
 drivers/usb/gadget/function/f_uac1.c           |  6 ++++++
 drivers/usb/gadget/function/f_uac1_legacy.c    |  4 ++++
 drivers/usb/gadget/function/f_uac2.c           |  8 ++++++++
 drivers/usb/gadget/function/u_ether_configfs.h | 10 ++++++++++
 drivers/usb/gadget/function/uvc_configfs.c     | 18 ++++++++++++++++++
 drivers/usb/gadget/trace.c                     | 26 ++++++++++++++++++++++++++
 include/linux/usb/composite.h                  |  3 +++
 include/linux/usb/gadget_configfs.h            |  2 ++
 17 files changed, 153 insertions(+)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index a304d29..a9ea331 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -146,6 +146,8 @@ static ssize_t gadget_dev_desc_##_name##_store(struct config_item *item, \
 {							\
 	u8 val;						\
 	int ret;					\
+							\
+	trace_usb_configfs_write_attr(item, #_name, page);	\
 	ret = kstrtou8(page, 0, &val);			\
 	if (ret)					\
 		return ret;				\
@@ -159,6 +161,8 @@ static ssize_t gadget_dev_desc_##_name##_store(struct config_item *item, \
 {							\
 	u16 val;					\
 	int ret;					\
+							\
+	trace_usb_configfs_write_attr(item, #_name, page);	\
 	ret = kstrtou16(page, 0, &val);			\
 	if (ret)					\
 		return ret;				\
@@ -198,6 +202,8 @@ static ssize_t gadget_dev_desc_bcdDevice_store(struct config_item *item,
 	u16 bcdDevice;
 	int ret;
 
+	trace_usb_configfs_write_attr(item, "bcdDevice", page);
+
 	ret = kstrtou16(page, 0, &bcdDevice);
 	if (ret)
 		return ret;
@@ -215,6 +221,8 @@ static ssize_t gadget_dev_desc_bcdUSB_store(struct config_item *item,
 	u16 bcdUSB;
 	int ret;
 
+	trace_usb_configfs_write_attr(item, "bcdUSB", page);
+
 	ret = kstrtou16(page, 0, &bcdUSB);
 	if (ret)
 		return ret;
@@ -262,6 +270,8 @@ static ssize_t gadget_dev_desc_UDC_store(struct config_item *item,
 	char *name;
 	int ret;
 
+	trace_usb_configfs_write_attr(item, "UDC", page);
+
 	if (strlen(page) < len)
 		return -EOVERFLOW;
 
@@ -311,6 +321,8 @@ static ssize_t gadget_dev_desc_max_speed_store(struct config_item *item,
 {
 	struct gadget_info *gi = to_gadget_info(item);
 
+	trace_usb_configfs_write_attr(item, "max_speed", page);
+
 	mutex_lock(&gi->lock);
 
 	/* Prevent changing of max_speed after the driver is binded */
@@ -519,6 +531,9 @@ static ssize_t gadget_config_desc_MaxPower_store(struct config_item *item,
 	struct config_usb_cfg *cfg = to_config_usb_cfg(item);
 	u16 val;
 	int ret;
+
+	trace_usb_configfs_write_attr(item, "MaxPower", page);
+
 	ret = kstrtou16(page, 0, &val);
 	if (ret)
 		return ret;
@@ -542,6 +557,9 @@ static ssize_t gadget_config_desc_bmAttributes_store(struct config_item *item,
 	struct config_usb_cfg *cfg = to_config_usb_cfg(item);
 	u8 val;
 	int ret;
+
+	trace_usb_configfs_write_attr(item, "bmAttributes", page);
+
 	ret = kstrtou8(page, 0, &val);
 	if (ret)
 		return ret;
@@ -809,6 +827,8 @@ static ssize_t os_desc_use_store(struct config_item *item, const char *page,
 	int ret;
 	bool use;
 
+	trace_usb_configfs_write_attr(item, "use", page);
+
 	mutex_lock(&gi->lock);
 	ret = strtobool(page, &use);
 	if (!ret) {
@@ -833,6 +853,8 @@ static ssize_t os_desc_b_vendor_code_store(struct config_item *item,
 	int ret;
 	u8 b_vendor_code;
 
+	trace_usb_configfs_write_attr(item, "b_vendor_code", page);
+
 	mutex_lock(&gi->lock);
 	ret = kstrtou8(page, 0, &b_vendor_code);
 	if (!ret) {
@@ -862,6 +884,8 @@ static ssize_t os_desc_qw_sign_store(struct config_item *item, const char *page,
 	struct gadget_info *gi = os_desc_item_to_gadget_info(item);
 	int res, l;
 
+	trace_usb_configfs_write_attr(item, "qw_sign", page);
+
 	l = min((int)len, OS_STRING_QW_SIGN_LEN >> 1);
 	if (page[l - 1] == '\n')
 		--l;
diff --git a/drivers/usb/gadget/function/f_acm.c b/drivers/usb/gadget/function/f_acm.c
index 349945e..d48f666 100644
--- a/drivers/usb/gadget/function/f_acm.c
+++ b/drivers/usb/gadget/function/f_acm.c
@@ -794,6 +794,7 @@ static struct configfs_item_operations acm_item_ops = {
 static ssize_t f_acm_console_store(struct config_item *item,
 		const char *page, size_t count)
 {
+	trace_usb_configfs_write_attr(item, "console", page);
 	return gserial_set_console(to_f_serial_opts(item)->port_num,
 				   page, count);
 }
diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index ca0a7d9..c54af8d 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -1074,6 +1074,8 @@ static ssize_t f_hid_opts_##name##_store(struct config_item *item,	\
 	int ret;							\
 	u##prec num;							\
 									\
+	trace_usb_configfs_write_attr(item, #name, page);		\
+									\
 	mutex_lock(&opts->lock);					\
 	if (opts->refcnt) {						\
 		ret = -EBUSY;						\
@@ -1123,6 +1125,8 @@ static ssize_t f_hid_opts_report_desc_store(struct config_item *item,
 	int ret = -EBUSY;
 	char *d;
 
+	trace_usb_configfs_write_attr(item, "report_desc", page);
+
 	mutex_lock(&opts->lock);
 
 	if (opts->refcnt)
diff --git a/drivers/usb/gadget/function/f_loopback.c b/drivers/usb/gadget/function/f_loopback.c
index ae41f55..42f6061 100644
--- a/drivers/usb/gadget/function/f_loopback.c
+++ b/drivers/usb/gadget/function/f_loopback.c
@@ -489,6 +489,8 @@ static ssize_t f_lb_opts_qlen_store(struct config_item *item,
 	int ret;
 	u32 num;
 
+	trace_usb_configfs_write_attr(item, "qlen", page);
+
 	mutex_lock(&opts->lock);
 	if (opts->refcnt) {
 		ret = -EBUSY;
@@ -527,6 +529,8 @@ static ssize_t f_lb_opts_bulk_buflen_store(struct config_item *item,
 	int ret;
 	u32 num;
 
+	trace_usb_configfs_write_attr(item, "bulk_buflen", page);
+
 	mutex_lock(&opts->lock);
 	if (opts->refcnt) {
 		ret = -EBUSY;
diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index a96eca9..295966b 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -3141,6 +3141,8 @@ static ssize_t fsg_lun_opts_file_store(struct config_item *item,
 	struct fsg_lun_opts *opts = to_fsg_lun_opts(item);
 	struct fsg_opts *fsg_opts = to_fsg_opts(opts->group.cg_item.ci_parent);
 
+	trace_usb_configfs_write_attr(item, "file", page);
+
 	return fsg_store_file(opts->lun, &fsg_opts->common->filesem, page, len);
 }
 
@@ -3157,6 +3159,8 @@ static ssize_t fsg_lun_opts_ro_store(struct config_item *item,
 	struct fsg_lun_opts *opts = to_fsg_lun_opts(item);
 	struct fsg_opts *fsg_opts = to_fsg_opts(opts->group.cg_item.ci_parent);
 
+	trace_usb_configfs_write_attr(item, "ro", page);
+
 	return fsg_store_ro(opts->lun, &fsg_opts->common->filesem, page, len);
 }
 
@@ -3171,6 +3175,8 @@ static ssize_t fsg_lun_opts_removable_show(struct config_item *item,
 static ssize_t fsg_lun_opts_removable_store(struct config_item *item,
 				       const char *page, size_t len)
 {
+	trace_usb_configfs_write_attr(item, "removable", page);
+
 	return fsg_store_removable(to_fsg_lun_opts(item)->lun, page, len);
 }
 
@@ -3187,6 +3193,8 @@ static ssize_t fsg_lun_opts_cdrom_store(struct config_item *item,
 	struct fsg_lun_opts *opts = to_fsg_lun_opts(item);
 	struct fsg_opts *fsg_opts = to_fsg_opts(opts->group.cg_item.ci_parent);
 
+	trace_usb_configfs_write_attr(item, "cdrom", page);
+
 	return fsg_store_cdrom(opts->lun, &fsg_opts->common->filesem, page,
 			       len);
 }
@@ -3201,6 +3209,8 @@ static ssize_t fsg_lun_opts_nofua_show(struct config_item *item, char *page)
 static ssize_t fsg_lun_opts_nofua_store(struct config_item *item,
 				       const char *page, size_t len)
 {
+	trace_usb_configfs_write_attr(item, "nofua", page);
+
 	return fsg_store_nofua(to_fsg_lun_opts(item)->lun, page, len);
 }
 
@@ -3215,6 +3225,8 @@ static ssize_t fsg_lun_opts_inquiry_string_show(struct config_item *item,
 static ssize_t fsg_lun_opts_inquiry_string_store(struct config_item *item,
 						 const char *page, size_t len)
 {
+	trace_usb_configfs_write_attr(item, "inquiry_string", page);
+
 	return fsg_store_inquiry_string(to_fsg_lun_opts(item)->lun, page, len);
 }
 
@@ -3353,6 +3365,8 @@ static ssize_t fsg_opts_stall_store(struct config_item *item, const char *page,
 	int ret;
 	bool stall;
 
+	trace_usb_configfs_write_attr(item, "stall", page);
+
 	mutex_lock(&opts->lock);
 
 	if (opts->refcnt) {
@@ -3393,6 +3407,8 @@ static ssize_t fsg_opts_num_buffers_store(struct config_item *item,
 	int ret;
 	u8 num;
 
+	trace_usb_configfs_write_attr(item, "num_buffers", page);
+
 	mutex_lock(&opts->lock);
 	if (opts->refcnt) {
 		ret = -EBUSY;
diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index fddf539..ebb2d7b 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -1110,6 +1110,8 @@ static ssize_t f_midi_opts_##name##_store(struct config_item *item,	\
 	int ret;							\
 	u32 num;							\
 									\
+	trace_usb_configfs_write_attr(item, #name, page);		\
+									\
 	mutex_lock(&opts->lock);					\
 	if (opts->refcnt > 1) {						\
 		ret = -EBUSY;						\
@@ -1154,6 +1156,8 @@ static ssize_t f_midi_opts_##name##_store(struct config_item *item,	\
 	int ret;							\
 	s32 num;							\
 									\
+	trace_usb_configfs_write_attr(item, #name, page);		\
+									\
 	mutex_lock(&opts->lock);					\
 	if (opts->refcnt > 1) {						\
 		ret = -EBUSY;						\
@@ -1209,6 +1213,8 @@ static ssize_t f_midi_opts_id_store(struct config_item *item,
 	int ret;
 	char *c;
 
+	trace_usb_configfs_write_attr(item, "id", page);
+
 	mutex_lock(&opts->lock);
 	if (opts->refcnt > 1) {
 		ret = -EBUSY;
diff --git a/drivers/usb/gadget/function/f_printer.c b/drivers/usb/gadget/function/f_printer.c
index abec5c5..c071574 100644
--- a/drivers/usb/gadget/function/f_printer.c
+++ b/drivers/usb/gadget/function/f_printer.c
@@ -1239,6 +1239,8 @@ static ssize_t f_printer_opts_pnp_string_store(struct config_item *item,
 	char *new_pnp;
 	int result;
 
+	trace_usb_configfs_write_attr(item, "pnp_string", page);
+
 	mutex_lock(&opts->lock);
 
 	new_pnp = kstrndup(page, len, GFP_KERNEL);
@@ -1281,6 +1283,8 @@ static ssize_t f_printer_opts_q_len_store(struct config_item *item,
 	int ret;
 	u16 num;
 
+	trace_usb_configfs_write_attr(item, "q_len", page);
+
 	mutex_lock(&opts->lock);
 	if (opts->refcnt) {
 		ret = -EBUSY;
diff --git a/drivers/usb/gadget/function/f_serial.c b/drivers/usb/gadget/function/f_serial.c
index a9480b9..58f69a7 100644
--- a/drivers/usb/gadget/function/f_serial.c
+++ b/drivers/usb/gadget/function/f_serial.c
@@ -271,6 +271,7 @@ static struct configfs_item_operations serial_item_ops = {
 static ssize_t f_serial_console_store(struct config_item *item,
 		const char *page, size_t count)
 {
+	trace_usb_configfs_write_attr(item, "console", page);
 	return gserial_set_console(to_f_serial_opts(item)->port_num,
 				   page, count);
 }
diff --git a/drivers/usb/gadget/function/f_sourcesink.c b/drivers/usb/gadget/function/f_sourcesink.c
index 6803cd6..4e6acd7 100644
--- a/drivers/usb/gadget/function/f_sourcesink.c
+++ b/drivers/usb/gadget/function/f_sourcesink.c
@@ -907,6 +907,8 @@ static ssize_t f_ss_opts_pattern_store(struct config_item *item,
 	int ret;
 	u8 num;
 
+	trace_usb_configfs_write_attr(item, "pattern", page);
+
 	mutex_lock(&opts->lock);
 	if (opts->refcnt) {
 		ret = -EBUSY;
@@ -950,6 +952,8 @@ static ssize_t f_ss_opts_isoc_interval_store(struct config_item *item,
 	int ret;
 	u8 num;
 
+	trace_usb_configfs_write_attr(item, "isoc_interval", page);
+
 	mutex_lock(&opts->lock);
 	if (opts->refcnt) {
 		ret = -EBUSY;
@@ -993,6 +997,8 @@ static ssize_t f_ss_opts_isoc_maxpacket_store(struct config_item *item,
 	int ret;
 	u16 num;
 
+	trace_usb_configfs_write_attr(item, "isoc_maxpacket", page);
+
 	mutex_lock(&opts->lock);
 	if (opts->refcnt) {
 		ret = -EBUSY;
@@ -1036,6 +1042,8 @@ static ssize_t f_ss_opts_isoc_mult_store(struct config_item *item,
 	int ret;
 	u8 num;
 
+	trace_usb_configfs_write_attr(item, "isoc_mult", page);
+
 	mutex_lock(&opts->lock);
 	if (opts->refcnt) {
 		ret = -EBUSY;
@@ -1079,6 +1087,8 @@ static ssize_t f_ss_opts_isoc_maxburst_store(struct config_item *item,
 	int ret;
 	u8 num;
 
+	trace_usb_configfs_write_attr(item, "isoc_maxburst", page);
+
 	mutex_lock(&opts->lock);
 	if (opts->refcnt) {
 		ret = -EBUSY;
@@ -1122,6 +1132,8 @@ static ssize_t f_ss_opts_bulk_buflen_store(struct config_item *item,
 	int ret;
 	u32 num;
 
+	trace_usb_configfs_write_attr(item, "bulk_buflen", page);
+
 	mutex_lock(&opts->lock);
 	if (opts->refcnt) {
 		ret = -EBUSY;
@@ -1160,6 +1172,8 @@ static ssize_t f_ss_opts_bulk_qlen_store(struct config_item *item,
 	int ret;
 	u32 num;
 
+	trace_usb_configfs_write_attr(item, "bulk_qlen", page);
+
 	mutex_lock(&opts->lock);
 	if (opts->refcnt) {
 		ret = -EBUSY;
@@ -1198,6 +1212,8 @@ static ssize_t f_ss_opts_iso_qlen_store(struct config_item *item,
 	int ret;
 	u32 num;
 
+	trace_usb_configfs_write_attr(item, "iso_qlen", page);
+
 	mutex_lock(&opts->lock);
 	if (opts->refcnt) {
 		ret = -EBUSY;
diff --git a/drivers/usb/gadget/function/f_uac1.c b/drivers/usb/gadget/function/f_uac1.c
index 6f0e1d8..efbf45f 100644
--- a/drivers/usb/gadget/function/f_uac1.c
+++ b/drivers/usb/gadget/function/f_uac1.c
@@ -1474,6 +1474,8 @@ static ssize_t f_uac1_opts_##name##_store(				\
 	int ret;							\
 	type num;							\
 									\
+	trace_usb_configfs_write_attr(item, #name, page);		\
+									\
 	mutex_lock(&opts->lock);					\
 	if (opts->refcnt) {						\
 		ret = -EBUSY;						\
@@ -1527,6 +1529,8 @@ static ssize_t f_uac1_opts_##name##_store(struct config_item *item,	\
 	u32 num;							\
 	int i;								\
 									\
+	trace_usb_configfs_write_attr(item, #name, page);		\
+									\
 	mutex_lock(&opts->lock);					\
 	if (opts->refcnt) {						\
 		ret = -EBUSY;						\
@@ -1573,6 +1577,8 @@ static ssize_t f_uac1_opts_##name##_store(struct config_item *item,	\
 	struct f_uac1_opts *opts = to_f_uac1_opts(item);		\
 	int ret = 0;							\
 									\
+	trace_usb_configfs_write_attr(item, #name, page);		\
+									\
 	mutex_lock(&opts->lock);					\
 	if (opts->refcnt) {						\
 		ret = -EBUSY;						\
diff --git a/drivers/usb/gadget/function/f_uac1_legacy.c b/drivers/usb/gadget/function/f_uac1_legacy.c
index e2d7f69..a65917b 100644
--- a/drivers/usb/gadget/function/f_uac1_legacy.c
+++ b/drivers/usb/gadget/function/f_uac1_legacy.c
@@ -837,6 +837,8 @@ static ssize_t f_uac1_opts_##name##_store(struct config_item *item,		\
 	int ret;							\
 	u32 num;							\
 									\
+	trace_usb_configfs_write_attr(item, #name, page);		\
+									\
 	mutex_lock(&opts->lock);					\
 	if (opts->refcnt) {						\
 		ret = -EBUSY;						\
@@ -882,6 +884,8 @@ static ssize_t f_uac1_opts_##name##_store(struct config_item *item,	\
 	int ret = -EBUSY;						\
 	char *tmp;							\
 									\
+	trace_usb_configfs_write_attr(item, #name, page);		\
+									\
 	mutex_lock(&opts->lock);					\
 	if (opts->refcnt)						\
 		goto end;						\
diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c
index 1905a8d..1849f3b 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -1886,6 +1886,8 @@ static ssize_t f_uac2_opts_##name##_store(struct config_item *item,	\
 	int ret;							\
 	type num;							\
 									\
+	trace_usb_configfs_write_attr(item, #name, page);		\
+									\
 	mutex_lock(&opts->lock);					\
 	if (opts->refcnt) {						\
 		ret = -EBUSY;						\
@@ -1938,6 +1940,8 @@ static ssize_t f_uac2_opts_##name##_store(struct config_item *item,	\
 	struct f_uac2_opts *opts = to_f_uac2_opts(item);		\
 	int ret = 0;							\
 									\
+	trace_usb_configfs_write_attr(item, #name, page);		\
+									\
 	mutex_lock(&opts->lock);					\
 	if (opts->refcnt) {						\
 		ret = -EBUSY;						\
@@ -1995,6 +1999,8 @@ static ssize_t f_uac2_opts_##name##_store(struct config_item *item,	\
 	u32 num;							\
 	int i;								\
 									\
+	trace_usb_configfs_write_attr(item, #name, page);		\
+									\
 	mutex_lock(&opts->lock);					\
 	if (opts->refcnt) {						\
 		ret = -EBUSY;						\
@@ -2041,6 +2047,8 @@ static ssize_t f_uac2_opts_##name##_store(struct config_item *item,	\
 	struct f_uac2_opts *opts = to_f_uac2_opts(item);		\
 	int ret = 0;							\
 									\
+	trace_usb_configfs_write_attr(item, #name, page);		\
+									\
 	mutex_lock(&opts->lock);					\
 	if (opts->refcnt) {						\
 		ret = -EBUSY;						\
diff --git a/drivers/usb/gadget/function/u_ether_configfs.h b/drivers/usb/gadget/function/u_ether_configfs.h
index f558c31..05578be 100644
--- a/drivers/usb/gadget/function/u_ether_configfs.h
+++ b/drivers/usb/gadget/function/u_ether_configfs.h
@@ -45,6 +45,8 @@
 		struct f_##_f_##_opts *opts = to_f_##_f_##_opts(item);	\
 		int ret;						\
 									\
+		trace_usb_configfs_write_attr(item, "dev_addr", page);	\
+									\
 		mutex_lock(&opts->lock);				\
 		if (opts->refcnt) {					\
 			mutex_unlock(&opts->lock);			\
@@ -80,6 +82,8 @@
 		struct f_##_f_##_opts *opts = to_f_##_f_##_opts(item);	\
 		int ret;						\
 									\
+		trace_usb_configfs_write_attr(item, "host_addr", page);	\
+									\
 		mutex_lock(&opts->lock);				\
 		if (opts->refcnt) {					\
 			mutex_unlock(&opts->lock);			\
@@ -115,6 +119,8 @@
 		u8 val;							\
 		int ret;						\
 									\
+		trace_usb_configfs_write_attr(item, "qmult", page);	\
+									\
 		mutex_lock(&opts->lock);				\
 		if (opts->refcnt) {					\
 			ret = -EBUSY;					\
@@ -154,6 +160,8 @@ out:									\
 		struct f_##_f_##_opts *opts = to_f_##_f_##_opts(item);	\
 		int ret = -EBUSY;					\
 									\
+		trace_usb_configfs_write_attr(item, "ifname", page);	\
+									\
 		mutex_lock(&opts->lock);				\
 		if (!opts->refcnt)					\
 			ret = gether_set_ifname(opts->net, page, len);	\
@@ -185,6 +193,8 @@ out:									\
 		int ret = -EINVAL;					\
 		u8 val;							\
 									\
+		trace_usb_configfs_write_attr(item, #_n_, page);		\
+									\
 		mutex_lock(&opts->lock);				\
 		if (sscanf(page, "%02hhx", &val) > 0) {			\
 			opts->_n_ = val;				\
diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
index fc139f3..6556e42 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -172,6 +172,8 @@ uvcg_control_header_##cname##_store(struct config_item *item,		\
 	int ret;							\
 	u##bits num;							\
 									\
+	trace_usb_configfs_write_attr(item, #cname, page);		\
+									\
 	mutex_lock(su_mutex); /* for navigating configfs hierarchy */	\
 									\
 	opts_item = ch->item.ci_parent->ci_parent->ci_parent;		\
@@ -1129,6 +1131,8 @@ static ssize_t  uvcg_frame_##cname##_store(struct config_item *item,	\
 	typeof(f->frame.cname) num;					\
 	int ret;							\
 									\
+	trace_usb_configfs_write_attr(item, #aname, page);		\
+									\
 	ret = kstrtou##bits(page, 0, &num);				\
 	if (ret)							\
 		return ret;						\
@@ -1288,6 +1292,8 @@ static ssize_t uvcg_frame_dw_frame_interval_store(struct config_item *item,
 	int ret = 0, n = 0;
 	u32 *frm_intrv, *tmp;
 
+	trace_usb_configfs_write_attr(item, "dwFrameInterval", page);
+
 	mutex_lock(su_mutex); /* for navigating configfs hierarchy */
 
 	opts_item = ch->item.ci_parent->ci_parent->ci_parent->ci_parent;
@@ -1484,6 +1490,8 @@ static ssize_t uvcg_uncompressed_guid_format_store(struct config_item *item,
 	struct mutex *su_mutex = &ch->fmt.group.cg_subsys->su_mutex;
 	int ret;
 
+	trace_usb_configfs_write_attr(item, "guidFormat", page);
+
 	mutex_lock(su_mutex); /* for navigating configfs hierarchy */
 
 	opts_item = ch->fmt.group.cg_item.ci_parent->ci_parent->ci_parent;
@@ -1566,6 +1574,8 @@ uvcg_uncompressed_##cname##_store(struct config_item *item,		\
 	int ret;							\
 	u8 num;								\
 									\
+	trace_usb_configfs_write_attr(item, #aname, page);		\
+									\
 	mutex_lock(su_mutex); /* for navigating configfs hierarchy */	\
 									\
 	opts_item = u->fmt.group.cg_item.ci_parent->ci_parent->ci_parent;\
@@ -1613,6 +1623,8 @@ uvcg_uncompressed_bma_controls_store(struct config_item *item,
 				     const char *page, size_t len)
 {
 	struct uvcg_uncompressed *unc = to_uvcg_uncompressed(item);
+
+	trace_usb_configfs_write_attr(item, "bmaControls", page);
 	return uvcg_format_bma_controls_store(&unc->fmt, page, len);
 }
 
@@ -1761,6 +1773,8 @@ uvcg_mjpeg_##cname##_store(struct config_item *item,			\
 	int ret;							\
 	u8 num;								\
 									\
+	trace_usb_configfs_write_attr(item, #aname, page);		\
+									\
 	mutex_lock(su_mutex); /* for navigating configfs hierarchy */	\
 									\
 	opts_item = u->fmt.group.cg_item.ci_parent->ci_parent->ci_parent;\
@@ -1808,6 +1822,8 @@ uvcg_mjpeg_bma_controls_store(struct config_item *item,
 				     const char *page, size_t len)
 {
 	struct uvcg_mjpeg *u = to_uvcg_mjpeg(item);
+
+	trace_usb_configfs_write_attr(item, "bmaControls", page);
 	return uvcg_format_bma_controls_store(&u->fmt, page, len);
 }
 
@@ -2420,6 +2436,8 @@ f_uvc_opts_##cname##_store(struct config_item *item,			\
 	unsigned int num;						\
 	int ret;							\
 									\
+	trace_usb_configfs_write_attr(item, #aname, page);		\
+									\
 	mutex_lock(&opts->lock);					\
 	if (opts->refcnt) {						\
 		ret = -EBUSY;						\
diff --git a/drivers/usb/gadget/trace.c b/drivers/usb/gadget/trace.c
index 9b33b30..44bab9a 100644
--- a/drivers/usb/gadget/trace.c
+++ b/drivers/usb/gadget/trace.c
@@ -134,4 +134,30 @@ void trace_usb_configfs_unlink_group(struct config_item *dest,
 	trace_usb_configfs_link_unlink_group(dest, src, "unlink");
 }
 EXPORT_SYMBOL(trace_usb_configfs_unlink_group);
+
+void trace_usb_configfs_write_attr(struct config_item *item,
+		const char *attr, const char *page)
+{
+	char *info, *group;
+	int ret;
+
+	info = kzalloc(2 * PAGE_SIZE, GFP_KERNEL);
+	if (!info)
+		return;
+
+	group = info + 2 * PAGE_SIZE - GROUP_LEN;
+	ret = gadget_configfs_group(group, item);
+	if (ret) {
+		kfree(info);
+		return;
+	}
+
+	snprintf(info, 2 * PAGE_SIZE - GROUP_LEN,
+			"echo %s/%s %s", group, attr, page);
+
+	trace_gadget_configfs(info);
+
+	kfree(info);
+}
+EXPORT_SYMBOL(trace_usb_configfs_write_attr);
 #endif
diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
index 65dc26e..0170455 100644
--- a/include/linux/usb/composite.h
+++ b/include/linux/usb/composite.h
@@ -612,11 +612,14 @@ void trace_usb_configfs_link_group(struct config_item *dest,
 		struct config_item *src);
 void trace_usb_configfs_unlink_group(struct config_item *dest,
 		struct config_item *src);
+void trace_usb_configfs_write_attr(struct config_item *item,
+		const char *attr, const char *page);
 #else
 #define trace_usb_configfs_make_group(parent, item) do {} while (0)
 #define trace_usb_configfs_drop_group(parent, item) do {} while (0)
 #define trace_usb_configfs_link_group(dest, src) do {} while (0)
 #define trace_usb_configfs_unlink_group(dest, src) do {} while (0)
+#define trace_usb_configfs_write_attr(item, attr, page) do {} while (0)
 #endif
 
 struct usb_configuration *usb_get_config(struct usb_composite_dev *cdev,
diff --git a/include/linux/usb/gadget_configfs.h b/include/linux/usb/gadget_configfs.h
index a89f177..2c0663e 100644
--- a/include/linux/usb/gadget_configfs.h
+++ b/include/linux/usb/gadget_configfs.h
@@ -14,6 +14,8 @@ static ssize_t __struct##_##__name##_store(struct config_item *item, \
 	struct __struct *gs = to_##__struct(item);	\
 	int ret;					\
 							\
+	trace_usb_configfs_write_attr(item, #__name, page);	\
+							\
 	ret = usb_string_copy(page, &gs->__name);	\
 	if (ret)					\
 		return ret;				\
-- 
2.7.4

Re: [PATCH v6 3/5] usb: gadget: add trace event of configfs group operation

[Greg Kroah-Hartman]

On Thu, Apr 07, 2022 at 09:27:43AM +0800, Linyu Yuan wrote:
> Add API trace_usb_configfs_make_group() and trace_usb_configfs_drop_group()
> to trace user create groups like gadget/function/configuration,
> it also trace group create in a specific function.

I'm sorry, but I do not understand what you are really adding here or
why.  Can you expand on this please?

> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>

But you are not using the functions you added in patch 2/5 here, right?
Or did I miss that?

> ---
> v2: no change
> v3: add API in trace.c
> v4: fix memory leak
> v5: change return value which report by kernel test robot <lkp@intel.com> and
>     Dan Carpenter <dan.carpenter@oracle.com>
> v6: fix checkpatch warning to add one space after while keyworkd
> 
>  drivers/usb/gadget/configfs.c                | 11 ++++
>  drivers/usb/gadget/function/f_mass_storage.c |  4 ++
>  drivers/usb/gadget/function/uvc_configfs.c   | 12 ++++
>  drivers/usb/gadget/trace.c                   | 84 ++++++++++++++++++++++++++++
>  include/linux/usb/composite.h                |  9 +++
>  include/linux/usb/gadget_configfs.h          |  2 +
>  6 files changed, 122 insertions(+)
> 
> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> index b2beeda..a0599fb 100644
> --- a/drivers/usb/gadget/configfs.c
> +++ b/drivers/usb/gadget/configfs.c
> @@ -621,6 +621,8 @@ static struct config_group *function_make(
>  
>  	gi = container_of(group, struct gadget_info, functions_group);
>  
> +	trace_usb_configfs_make_group(&group->cg_item, &fi->group.cg_item);
> +
>  	mutex_lock(&gi->lock);
>  	list_add_tail(&fi->cfs_list, &gi->available_func);
>  	mutex_unlock(&gi->lock);
> @@ -634,6 +636,8 @@ static void function_drop(
>  	struct usb_function_instance *fi = to_usb_function_instance(item);
>  	struct gadget_info *gi;
>  
> +	trace_usb_configfs_drop_group(&group->cg_item, item);
> +
>  	gi = container_of(group, struct gadget_info, functions_group);
>  
>  	mutex_lock(&gi->lock);
> @@ -729,6 +733,7 @@ static struct config_group *config_desc_make(
>  	if (ret)
>  		goto err;
>  
> +	trace_usb_configfs_make_group(&group->cg_item, &cfg->group.cg_item);
>  	return &cfg->group;
>  err:
>  	kfree(cfg->c.label);
> @@ -740,6 +745,7 @@ static void config_desc_drop(
>  		struct config_group *group,
>  		struct config_item *item)
>  {
> +	trace_usb_configfs_drop_group(&group->cg_item, item);
>  	config_item_put(item);
>  }
>  
> @@ -1083,6 +1089,7 @@ static struct config_item *ext_prop_make(
>  	ext_prop_type->ct_owner = desc->owner;
>  
>  	config_item_init_type_name(&ext_prop->item, name, ext_prop_type);
> +	trace_usb_configfs_make_group(&group->cg_item, &ext_prop->item);
>  
>  	ext_prop->name = kstrdup(name, GFP_KERNEL);
>  	if (!ext_prop->name) {
> @@ -1107,6 +1114,8 @@ static void ext_prop_drop(struct config_group *group, struct config_item *item)
>  	struct usb_os_desc_ext_prop *ext_prop = to_usb_os_desc_ext_prop(item);
>  	struct usb_os_desc *desc = to_usb_os_desc(&group->cg_item);
>  
> +	trace_usb_configfs_drop_group(&group->cg_item, item);
> +
>  	if (desc->opts_mutex)
>  		mutex_lock(desc->opts_mutex);
>  	list_del(&ext_prop->entry);
> @@ -1626,6 +1635,7 @@ static struct config_group *gadgets_make(
>  	if (!gi->composite.gadget_driver.function)
>  		goto err;
>  
> +	trace_usb_configfs_make_group(&group->cg_item, &gi->group.cg_item);
>  	return &gi->group;
>  err:
>  	kfree(gi);
> @@ -1634,6 +1644,7 @@ static struct config_group *gadgets_make(
>  
>  static void gadgets_drop(struct config_group *group, struct config_item *item)
>  {
> +	trace_usb_configfs_drop_group(&group->cg_item, item);
>  	config_item_put(item);
>  }
>  
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
> index 3a77bca..a96eca9 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -3289,6 +3289,8 @@ static struct config_group *fsg_lun_make(struct config_group *group,
>  
>  	config_group_init_type_name(&opts->group, name, &fsg_lun_type);
>  
> +	trace_usb_configfs_make_group(&group->cg_item, &opts->group.cg_item);
> +
>  	return &opts->group;
>  out:
>  	mutex_unlock(&fsg_opts->lock);
> @@ -3300,6 +3302,8 @@ static void fsg_lun_drop(struct config_group *group, struct config_item *item)
>  	struct fsg_lun_opts *lun_opts;
>  	struct fsg_opts *fsg_opts;
>  
> +	trace_usb_configfs_drop_group(&group->cg_item, item);
> +
>  	lun_opts = to_fsg_lun_opts(item);
>  	fsg_opts = to_fsg_opts(&group->cg_item);
>  
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
> index 77d6403..cc0f2eb 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -236,6 +236,8 @@ static struct config_item *uvcg_control_header_make(struct config_group *group,
>  
>  	config_item_init_type_name(&h->item, name, &uvcg_control_header_type);
>  
> +	trace_usb_configfs_make_group(&group->cg_item, &h->item);
> +
>  	return &h->item;
>  }
>  
> @@ -1039,6 +1041,8 @@ static struct config_item
>  
>  	config_item_init_type_name(&h->item, name, &uvcg_streaming_header_type);
>  
> +	trace_usb_configfs_make_group(&group->cg_item, &h->item);
> +
>  	return &h->item;
>  }
>  
> @@ -1380,6 +1384,8 @@ static struct config_item *uvcg_frame_make(struct config_group *group,
>  
>  	config_item_init_type_name(&h->item, name, &uvcg_frame_type);
>  
> +	trace_usb_configfs_make_group(&group->cg_item, &h->item);
> +
>  	return &h->item;
>  }
>  
> @@ -1389,6 +1395,8 @@ static void uvcg_frame_drop(struct config_group *group, struct config_item *item
>  	struct f_uvc_opts *opts;
>  	struct config_item *opts_item;
>  
> +	trace_usb_configfs_drop_group(&group->cg_item, item);
> +
>  	opts_item = group->cg_item.ci_parent->ci_parent->ci_parent;
>  	opts = to_f_uvc_opts(opts_item);
>  
> @@ -1649,6 +1657,8 @@ static struct config_group *uvcg_uncompressed_make(struct config_group *group,
>  	config_group_init_type_name(&h->fmt.group, name,
>  				    &uvcg_uncompressed_type);
>  
> +	trace_usb_configfs_make_group(&group->cg_item, &h->fmt.group.cg_item);
> +
>  	return &h->fmt.group;
>  }
>  
> @@ -1835,6 +1845,8 @@ static struct config_group *uvcg_mjpeg_make(struct config_group *group,
>  	config_group_init_type_name(&h->fmt.group, name,
>  				    &uvcg_mjpeg_type);
>  
> +	trace_usb_configfs_make_group(&group->cg_item, &h->fmt.group.cg_item);
> +
>  	return &h->fmt.group;
>  }
>  
> diff --git a/drivers/usb/gadget/trace.c b/drivers/usb/gadget/trace.c
> index 108c1c8..65b328f 100644
> --- a/drivers/usb/gadget/trace.c
> +++ b/drivers/usb/gadget/trace.c
> @@ -5,3 +5,87 @@
>  
>  #define CREATE_TRACE_POINTS
>  #include "trace.h"
> +
> +#include <linux/configfs.h>
> +#include <linux/usb/composite.h>
> +
> +extern const struct config_item_type gadgets_type;

That should not be in a .c file.

> +
> +#ifdef CONFIG_TRACEPOINTS
> +#define GROUP_LEN	128

What is this the length of?

> +static int gadget_configfs_group(char *group, struct config_item *item)

I do not understand what this function does, sorry.

> +{
> +	struct config_item *parent;
> +	char *tmpgroup;
> +
> +	if (!item)
> +		return -EINVAL;
> +
> +	tmpgroup = kzalloc(GROUP_LEN, GFP_KERNEL);
> +	if (!tmpgroup)
> +		return -ENOMEM;
> +
> +	for (parent = item->ci_parent; parent;
> +			item = parent, parent = item->ci_parent) {
> +		if (item->ci_type == &gadgets_type) {
> +			kfree(tmpgroup);
> +			return 0;
> +		}
> +
> +		if (tmpgroup[0] == '\0')
> +			snprintf(group, GROUP_LEN, "%s",

How do you know that group is GROUP_LEN long?  Shouldn't you have a size
parameter instead?

> +					config_item_name(item));
> +		else
> +			snprintf(group, GROUP_LEN, "%s/%s",
> +					config_item_name(item), tmpgroup);

Why 2 different ways to create this string?

> +
> +		strcpy(tmpgroup, group);
> +	}
> +
> +	kfree(tmpgroup);
> +	return -EINVAL;
> +}
> +
> +static void trace_usb_configfs_make_drop_group(struct config_item *parent,
> +		struct config_item *item, char *make_drop)
> +{
> +	char *group, *parent_group;
> +	int ret;
> +
> +	group = kzalloc(2 * GROUP_LEN, GFP_KERNEL);

Why 2 *?

> +	if (!group)
> +		return;
> +
> +	parent_group = group + GROUP_LEN;
> +	ret = gadget_configfs_group(parent_group, parent);
> +	if (ret) {
> +		kfree(group);
> +		return;
> +	}
> +
> +	if (parent_group[0] == '\0')
> +		snprintf(group, GROUP_LEN, "%s %s", make_drop,
> +				config_item_name(item));
> +	else
> +		snprintf(group, GROUP_LEN, "%s %s/%s", make_drop, parent_group,
> +				config_item_name(item));
> +
> +	trace_gadget_configfs(group);
> +
> +	kfree(group);
> +}
> +
> +void trace_usb_configfs_make_group(struct config_item *parent,
> +		struct config_item *item)
> +{
> +	trace_usb_configfs_make_drop_group(parent, item, "mkdir");

Why?

> +}
> +EXPORT_SYMBOL(trace_usb_configfs_make_group);

EXPORT_SYMBOL_GPL() please.

And this is a gadget thing, not a usb_configfs thing.

> +
> +void trace_usb_configfs_drop_group(struct config_item *parent,
> +		struct config_item *item)
> +{
> +	trace_usb_configfs_make_drop_group(parent, item, "rmdir");
> +}
> +EXPORT_SYMBOL(trace_usb_configfs_drop_group);

_GPL please.  And same on the name.


> +#endif
> diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
> index 9d27622..7bf6574 100644
> --- a/include/linux/usb/composite.h
> +++ b/include/linux/usb/composite.h
> @@ -603,6 +603,15 @@ void usb_put_function_instance(struct usb_function_instance *fi);
>  void usb_put_function(struct usb_function *f);
>  struct usb_function_instance *usb_get_function_instance(const char *name);
>  struct usb_function *usb_get_function(struct usb_function_instance *fi);
> +#ifdef CONFIG_TRACEPOINTS
> +void trace_usb_configfs_make_group(struct config_item *parent,
> +		struct config_item *item);
> +void trace_usb_configfs_drop_group(struct config_item *parent,
> +		struct config_item *item);
> +#else
> +#define trace_usb_configfs_make_group(parent, item) do {} while (0)
> +#define trace_usb_configfs_drop_group(parent, item) do {} while (0)

Make these inline functions so that if someone uses them wrong, the
build will complain properly.

> +#endif
>  
>  struct usb_configuration *usb_get_config(struct usb_composite_dev *cdev,
>  		int val);
> diff --git a/include/linux/usb/gadget_configfs.h b/include/linux/usb/gadget_configfs.h
> index d61aebd..a89f177 100644
> --- a/include/linux/usb/gadget_configfs.h
> +++ b/include/linux/usb/gadget_configfs.h
> @@ -63,6 +63,7 @@ static struct config_item_type struct_in##_langid_type = {		\
>  		goto err;						\
>  	config_group_init_type_name(&new->group, name,			\
>  			&struct_in##_langid_type);			\
> +	trace_usb_configfs_make_group(&group->cg_item, &new->group.cg_item); \

What is this showing you?

>  									\
>  	gi = container_of(group, struct struct_member, strings_group);	\
>  	ret = -EEXIST;							\
> @@ -86,6 +87,7 @@ static void struct_in##_strings_drop(					\
>  		struct config_group *group,				\
>  		struct config_item *item)				\
>  {									\
> +	trace_usb_configfs_drop_group(&group->cg_item, item);		\

Why do you need to know this?

thanks,

greg k-h

Re: [PATCH v6 4/5] usb: gadget: add trace event of configfs link/unlink operation

[Greg Kroah-Hartman]

On Thu, Apr 07, 2022 at 09:27:44AM +0800, Linyu Yuan wrote:
> Add API trace_usb_configfs_link/unlink_group() to trace user space
> link/unlink a group like add function to configuration or
> remove function from configuration.
> If a specific function need link/unlink, it also can be used.

Doesn't configfs have generic trace functions like this?  What is this
going to show you?  Userspace is the thing causing these events to
happen, why do you need trace events for any of these configfs things?

Who is going to use this, and what are they all going to be used for?

thanks,

greg k-h

Re: [PATCH v6 5/5] usb: gadget: add trace event of configfs write attributes operation

[Greg Kroah-Hartman]

On Thu, Apr 07, 2022 at 09:27:45AM +0800, Linyu Yuan wrote:
> Add API trace_usb_configfs_write_attr() to trace user change gadget or
> function attributes.

Why?  Again, userspace is doing this already, why do we need to trace
what it is doing back to userspace again?

> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> ---
> v2: no change
> v3: add API in trace.c
> v4: fix memory leak
> v5: no change
> v6: fix checkpatch warning
> 
>  drivers/usb/gadget/configfs.c                  | 24 ++++++++++++++++++++++++
>  drivers/usb/gadget/function/f_acm.c            |  1 +
>  drivers/usb/gadget/function/f_hid.c            |  4 ++++
>  drivers/usb/gadget/function/f_loopback.c       |  4 ++++
>  drivers/usb/gadget/function/f_mass_storage.c   | 16 ++++++++++++++++
>  drivers/usb/gadget/function/f_midi.c           |  6 ++++++
>  drivers/usb/gadget/function/f_printer.c        |  4 ++++
>  drivers/usb/gadget/function/f_serial.c         |  1 +
>  drivers/usb/gadget/function/f_sourcesink.c     | 16 ++++++++++++++++
>  drivers/usb/gadget/function/f_uac1.c           |  6 ++++++
>  drivers/usb/gadget/function/f_uac1_legacy.c    |  4 ++++
>  drivers/usb/gadget/function/f_uac2.c           |  8 ++++++++
>  drivers/usb/gadget/function/u_ether_configfs.h | 10 ++++++++++
>  drivers/usb/gadget/function/uvc_configfs.c     | 18 ++++++++++++++++++
>  drivers/usb/gadget/trace.c                     | 26 ++++++++++++++++++++++++++
>  include/linux/usb/composite.h                  |  3 +++
>  include/linux/usb/gadget_configfs.h            |  2 ++
>  17 files changed, 153 insertions(+)
> 
> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> index a304d29..a9ea331 100644
> --- a/drivers/usb/gadget/configfs.c
> +++ b/drivers/usb/gadget/configfs.c
> @@ -146,6 +146,8 @@ static ssize_t gadget_dev_desc_##_name##_store(struct config_item *item, \
>  {							\
>  	u8 val;						\
>  	int ret;					\
> +							\
> +	trace_usb_configfs_write_attr(item, #_name, page);	\
>  	ret = kstrtou8(page, 0, &val);			\
>  	if (ret)					\
>  		return ret;				\
> @@ -159,6 +161,8 @@ static ssize_t gadget_dev_desc_##_name##_store(struct config_item *item, \
>  {							\
>  	u16 val;					\
>  	int ret;					\
> +							\
> +	trace_usb_configfs_write_attr(item, #_name, page);	\
>  	ret = kstrtou16(page, 0, &val);			\
>  	if (ret)					\
>  		return ret;				\
> @@ -198,6 +202,8 @@ static ssize_t gadget_dev_desc_bcdDevice_store(struct config_item *item,
>  	u16 bcdDevice;
>  	int ret;
>  
> +	trace_usb_configfs_write_attr(item, "bcdDevice", page);

Where did "bcdDevice" come from?  Shouldn't this all just come from
configfs instead of having to add it to each individual function?

And again, why?

thanks,

greg k-h

Re: [PATCH v6 0/5] usb: gadget: configfs: new trace events

[Greg Kroah-Hartman]

On Thu, Apr 07, 2022 at 09:27:40AM +0800, Linyu Yuan wrote:
> Last year I try to add trace event support for usb gadget configfs [1],
> this time the idea is change a lot, the purpose is trace all user space
> operation to gadget configuration, include gadget and it's function.

But why?  Who will use this, and what for?

> In usb gadget configfs, mainly user can do mkdir/rmdir a group,
> link/unlink a function, change gadget/function attributes,
> each operation will touch a struct config_item.

As userspace is the thing doing this, why do you need to tell userspace
again that this happened?

> It only have one trace event entry which store string,
> provide several API which represent user operation and generate string
> from  struct config_item.
> 
> example output,
>    mkdir-80      [000] .....    44.555106: gadget_configfs: mkdir dummy
>       sh-76      [000] .....    44.562609: gadget_configfs: echo dummy/idVendor 0x05C6
> 
>    mkdir-81      [000] .....    44.569795: gadget_configfs: mkdir dummy/functions/eem.0
>       sh-76      [000] .....    44.600221: gadget_configfs: echo dummy/functions/eem.0/dev_addr 1e:77:46:4b:1e:96
> 
>    mkdir-82      [000] .....    44.615542: gadget_configfs: mkdir dummy/configs/dummy.1
>       ln-83      [000] .....    44.628997: gadget_configfs: link dummy/configs/dummy.1 dummy/functions/eem.0
>       sh-76      [000] .....    44.634506: gadget_configfs: echo dummy/configs/dummy.1/MaxPower 500
> 
>    mkdir-84      [000] .....    44.642265: gadget_configfs: mkdir dummy/configs/dummy.1/strings/0x409
>       sh-76      [000] .....    44.663886: gadget_configfs: echo dummy/configs/dummy.1/strings/0x409/configuration dummy
> 
>    rmdir-85      [000] .....    64.255507: gadget_configfs: rmdir dummy/configs/dummy.1/strings/0x409
>       rm-86      [000] .....    64.263926: gadget_configfs: unlink dummy/configs/dummy.1 dummy/functions/eem.0
>    rmdir-87      [000] .....    64.279768: gadget_configfs: rmdir dummy/configs/dummy.1
>    rmdir-88      [000] .....    64.328124: gadget_configfs: rmdir dummy/functions/eem.0
>    rmdir-89      [000] .....    64.992085: gadget_configfs: rmdir dummy

As I said in other places, why not just add this to configfs directly
instead of all over the individual users of this one subsystem?

And again, why?

thanks,

greg k-h

RE: [PATCH v6 0/5] usb: gadget: configfs: new trace events

[Linyu Yuan (QUIC)]

> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Sent: Friday, April 22, 2022 4:43 PM
> To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org; Jack Pham
> (QUIC) <quic_jackp@quicinc.com>
> Subject: Re: [PATCH v6 0/5] usb: gadget: configfs: new trace events
> 
> On Thu, Apr 07, 2022 at 09:27:40AM +0800, Linyu Yuan wrote:
> > Last year I try to add trace event support for usb gadget configfs [1],
> > this time the idea is change a lot, the purpose is trace all user space
> > operation to gadget configuration, include gadget and it's function.
> 
> But why?  Who will use this, and what for?

Thanks for review it.
It is not used by user space, just for kernel gadget issue debugging.
Like android, the user space is complicate to kernel developers,
With this trace events, kernel development will understand
What is simplified action happen from user space.

> 
> > In usb gadget configfs, mainly user can do mkdir/rmdir a group,
> > link/unlink a function, change gadget/function attributes,
> > each operation will touch a struct config_item.
> 
> As userspace is the thing doing this, why do you need to tell userspace
> again that this happened?
> 
> > It only have one trace event entry which store string,
> > provide several API which represent user operation and generate string
> > from  struct config_item.
> >
> > example output,
> >    mkdir-80      [000] .....    44.555106: gadget_configfs: mkdir dummy
> >       sh-76      [000] .....    44.562609: gadget_configfs: echo dummy/idVendor
> 0x05C6
> >
> >    mkdir-81      [000] .....    44.569795: gadget_configfs: mkdir
> dummy/functions/eem.0
> >       sh-76      [000] .....    44.600221: gadget_configfs: echo
> dummy/functions/eem.0/dev_addr 1e:77:46:4b:1e:96
> >
> >    mkdir-82      [000] .....    44.615542: gadget_configfs: mkdir
> dummy/configs/dummy.1
> >       ln-83      [000] .....    44.628997: gadget_configfs: link
> dummy/configs/dummy.1 dummy/functions/eem.0
> >       sh-76      [000] .....    44.634506: gadget_configfs: echo
> dummy/configs/dummy.1/MaxPower 500
> >
> >    mkdir-84      [000] .....    44.642265: gadget_configfs: mkdir
> dummy/configs/dummy.1/strings/0x409
> >       sh-76      [000] .....    44.663886: gadget_configfs: echo
> dummy/configs/dummy.1/strings/0x409/configuration dummy
> >
> >    rmdir-85      [000] .....    64.255507: gadget_configfs: rmdir
> dummy/configs/dummy.1/strings/0x409
> >       rm-86      [000] .....    64.263926: gadget_configfs: unlink
> dummy/configs/dummy.1 dummy/functions/eem.0
> >    rmdir-87      [000] .....    64.279768: gadget_configfs: rmdir
> dummy/configs/dummy.1
> >    rmdir-88      [000] .....    64.328124: gadget_configfs: rmdir
> dummy/functions/eem.0
> >    rmdir-89      [000] .....    64.992085: gadget_configfs: rmdir dummy
> 
> As I said in other places, why not just add this to configfs directly
> instead of all over the individual users of this one subsystem?
> 
> And again, why?

I consider it also, but I found most of fs have no such trace event support.
I will try that.

Thanks very much again.

> 
> thanks,
> 
> greg k-h

Re: [PATCH v6 0/5] usb: gadget: configfs: new trace events

[Greg Kroah-Hartman]

On Fri, Apr 22, 2022 at 09:01:13AM +0000, Linyu Yuan (QUIC) wrote:
> > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Sent: Friday, April 22, 2022 4:43 PM
> > To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> > Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org; Jack Pham
> > (QUIC) <quic_jackp@quicinc.com>
> > Subject: Re: [PATCH v6 0/5] usb: gadget: configfs: new trace events
> > 
> > On Thu, Apr 07, 2022 at 09:27:40AM +0800, Linyu Yuan wrote:
> > > Last year I try to add trace event support for usb gadget configfs [1],
> > > this time the idea is change a lot, the purpose is trace all user space
> > > operation to gadget configuration, include gadget and it's function.
> > 
> > But why?  Who will use this, and what for?
> 
> Thanks for review it.
> It is not used by user space, just for kernel gadget issue debugging.

So you use it in userspace?  How can you use a tracepoint from somewhere
else in the kernel?

> Like android, the user space is complicate to kernel developers,
> With this trace events, kernel development will understand
> What is simplified action happen from user space.

They do not need this, they can just use ftrace today.  Most of these
tracepoints you are putting in here are just for a "got to this
function!" type of thing, which ftrace can show you already.

What is the added benefit of these over ftrace?

thanks,

greg k-h

RE: [PATCH v6 0/5] usb: gadget: configfs: new trace events

[Linyu Yuan (QUIC)]

> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Sent: Friday, April 22, 2022 8:17 PM
> To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org; Jack Pham
> (QUIC) <quic_jackp@quicinc.com>
> Subject: Re: [PATCH v6 0/5] usb: gadget: configfs: new trace events
> 
> On Fri, Apr 22, 2022 at 09:01:13AM +0000, Linyu Yuan (QUIC) wrote:
> > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Sent: Friday, April 22, 2022 4:43 PM
> > > To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> > > Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org; Jack
> Pham
> > > (QUIC) <quic_jackp@quicinc.com>
> > > Subject: Re: [PATCH v6 0/5] usb: gadget: configfs: new trace events
> > >
> > > On Thu, Apr 07, 2022 at 09:27:40AM +0800, Linyu Yuan wrote:
> > > > Last year I try to add trace event support for usb gadget configfs [1],
> > > > this time the idea is change a lot, the purpose is trace all user space
> > > > operation to gadget configuration, include gadget and it's function.
> > >
> > > But why?  Who will use this, and what for?
> >
> > Thanks for review it.
> > It is not used by user space, just for kernel gadget issue debugging.
> 
> So you use it in userspace?  How can you use a tracepoint from somewhere
> else in the kernel?
> 
> > Like android, the user space is complicate to kernel developers,
> > With this trace events, kernel development will understand
> > What is simplified action happen from user space.
> 
> They do not need this, they can just use ftrace today.  Most of these
> tracepoints you are putting in here are just for a "got to this
> function!" type of thing, which ftrace can show you already.
> 
> What is the added benefit of these over ftrace?

Thanks, please discard this changes,   will use kprobe event.

> 
> thanks,
> 
> greg k-h