[PATCH v5 0/3] usb: gadget: configfs: add some trace event

[PATCH v5 0/3] usb: gadget: configfs: add some trace event

[Linyu Yuan]

this series make some minor change to gadget configfs
which allow common trace from configfs layer.

follow suggestion from Felipe Balbi in link below,
https://lore.kernel.org/linux-usb/1629777281-30188-1-git-send-email-quic_linyyuan@quicinc.com/

v2: fix two issue Reported-by: kernel test robot <lkp@intel.com>
v3: do not move private structure to configfs.h
v4: add missing new file configfs_trace.h
v5: lost some change of v2, add it again

Linyu Yuan (3):
  usb: gadget: configfs: avoid list move operation of usb_function
  usb: gadget: configfs: add gadget_info for config group
  usb: gadget: configfs: add some trace event

 drivers/usb/gadget/configfs.c       | 117 +++++++++++++++++++------
 drivers/usb/gadget/configfs_trace.h | 167 ++++++++++++++++++++++++++++++++++++
 2 files changed, 256 insertions(+), 28 deletions(-)
 create mode 100644 drivers/usb/gadget/configfs_trace.h

-- 
2.7.4

[PATCH v5 1/3] usb: gadget: configfs: avoid list move operation of usb_function

[Linyu Yuan]

add a new list which link all usb_function at configfs layers,
it means that after link a function a configuration,
from configfs layer, we can still found all functions,
it will allow trace all functions from configfs.

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
v2: fix unused cfg variable warning
v3: add struct inside configfs.c
v4: no change
v5: lost v2 fix, add it again

 drivers/usb/gadget/configfs.c | 55 ++++++++++++++++++++++++-------------------
 1 file changed, 31 insertions(+), 24 deletions(-)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 477e72a..5b2e6f9 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -58,6 +58,11 @@ static inline struct gadget_info *to_gadget_info(struct config_item *item)
 	return container_of(to_config_group(item), struct gadget_info, group);
 }
 
+struct config_usb_function {
+	struct list_head list;
+	struct usb_function *f;
+};
+
 struct config_usb_cfg {
 	struct config_group group;
 	struct config_group strings_group;
@@ -420,7 +425,7 @@ static int config_usb_cfg_link(
 	struct usb_function_instance *fi = container_of(group,
 			struct usb_function_instance, group);
 	struct usb_function_instance *a_fi;
-	struct usb_function *f;
+	struct config_usb_function *cf;
 	int ret;
 
 	mutex_lock(&gi->lock);
@@ -438,21 +443,29 @@ static int config_usb_cfg_link(
 		goto out;
 	}
 
-	list_for_each_entry(f, &cfg->func_list, list) {
-		if (f->fi == fi) {
+	list_for_each_entry(cf, &cfg->func_list, list) {
+		if (cf->f->fi == fi) {
 			ret = -EEXIST;
 			goto out;
 		}
 	}
 
-	f = usb_get_function(fi);
-	if (IS_ERR(f)) {
-		ret = PTR_ERR(f);
+	cf = kzalloc(sizeof(*cf), GFP_KERNEL);
+	if (!cf) {
+		ret = -ENOMEM;
+		goto out;
+	}
+	INIT_LIST_HEAD(&cf->list);
+
+	cf->f = usb_get_function(fi);
+	if (IS_ERR(cf->f)) {
+		ret = PTR_ERR(cf->f);
+		kfree(cf);
 		goto out;
 	}
 
 	/* stash the function until we bind it to the gadget */
-	list_add_tail(&f->list, &cfg->func_list);
+	list_add_tail(&cf->list, &cfg->func_list);
 	ret = 0;
 out:
 	mutex_unlock(&gi->lock);
@@ -470,7 +483,7 @@ static void config_usb_cfg_unlink(
 	struct config_group *group = to_config_group(usb_func_ci);
 	struct usb_function_instance *fi = container_of(group,
 			struct usb_function_instance, group);
-	struct usb_function *f;
+	struct config_usb_function *cf;
 
 	/*
 	 * ideally I would like to forbid to unlink functions while a gadget is
@@ -483,10 +496,11 @@ static void config_usb_cfg_unlink(
 		unregister_gadget(gi);
 	WARN_ON(gi->composite.gadget_driver.udc_name);
 
-	list_for_each_entry(f, &cfg->func_list, list) {
-		if (f->fi == fi) {
-			list_del(&f->list);
-			usb_put_function(f);
+	list_for_each_entry(cf, &cfg->func_list, list) {
+		if (cf->f->fi == fi) {
+			list_del(&cf->list);
+			usb_put_function(cf->f);
+			kfree(cf);
 			mutex_unlock(&gi->lock);
 			return;
 		}
@@ -1257,13 +1271,10 @@ static void purge_configs_funcs(struct gadget_info *gi)
 
 	list_for_each_entry(c, &gi->cdev.configs, list) {
 		struct usb_function *f, *tmp;
-		struct config_usb_cfg *cfg;
-
-		cfg = container_of(c, struct config_usb_cfg, c);
 
 		list_for_each_entry_safe_reverse(f, tmp, &c->functions, list) {
 
-			list_move(&f->list, &cfg->func_list);
+			list_del(&f->list);
 			if (f->unbind) {
 				dev_dbg(&gi->cdev.gadget->dev,
 					"unbind function '%s'/%p\n",
@@ -1371,8 +1382,7 @@ static int configfs_composite_bind(struct usb_gadget *gadget,
 	/* Go through all configs, attach all functions */
 	list_for_each_entry(c, &gi->cdev.configs, list) {
 		struct config_usb_cfg *cfg;
-		struct usb_function *f;
-		struct usb_function *tmp;
+		struct config_usb_function *cf, *tmp;
 		struct gadget_config_name *cn;
 
 		if (gadget_is_otg(gadget))
@@ -1396,13 +1406,10 @@ static int configfs_composite_bind(struct usb_gadget *gadget,
 			c->iConfiguration = s[0].id;
 		}
 
-		list_for_each_entry_safe(f, tmp, &cfg->func_list, list) {
-			list_del(&f->list);
-			ret = usb_add_function(c, f);
-			if (ret) {
-				list_add(&f->list, &cfg->func_list);
+		list_for_each_entry_safe(cf, tmp, &cfg->func_list, list) {
+			ret = usb_add_function(c, cf->f);
+			if (ret)
 				goto err_purge_funcs;
-			}
 		}
 		ret = usb_gadget_check_config(cdev->gadget);
 		if (ret)
-- 
2.7.4

[PATCH v5 2/3] usb: gadget: configfs: add gadget_info for config group

[Linyu Yuan]

add gadget_info pointer in struct config_usb_cfg
to allow common gadget info trace from configfs layer.

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
v2: no change
v3: change struct inside configfs.c
v4: no change
v5: no change

 drivers/usb/gadget/configfs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 5b2e6f9..cea12c3 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -70,6 +70,7 @@ struct config_usb_cfg {
 	struct usb_configuration c;
 	struct list_head func_list;
 	struct usb_gadget_strings *gstrings[MAX_USB_STRING_LANGS + 1];
+	struct gadget_info *gi;
 };
 
 static inline struct config_usb_cfg *to_config_usb_cfg(struct config_item *item)
@@ -418,8 +419,7 @@ static int config_usb_cfg_link(
 	struct config_item *usb_func_ci)
 {
 	struct config_usb_cfg *cfg = to_config_usb_cfg(usb_cfg_ci);
-	struct usb_composite_dev *cdev = cfg->c.cdev;
-	struct gadget_info *gi = container_of(cdev, struct gadget_info, cdev);
+	struct gadget_info *gi = cfg->gi;
 
 	struct config_group *group = to_config_group(usb_func_ci);
 	struct usb_function_instance *fi = container_of(group,
@@ -477,8 +477,7 @@ static void config_usb_cfg_unlink(
 	struct config_item *usb_func_ci)
 {
 	struct config_usb_cfg *cfg = to_config_usb_cfg(usb_cfg_ci);
-	struct usb_composite_dev *cdev = cfg->c.cdev;
-	struct gadget_info *gi = container_of(cdev, struct gadget_info, cdev);
+	struct gadget_info *gi = cfg->gi;
 
 	struct config_group *group = to_config_group(usb_func_ci);
 	struct usb_function_instance *fi = container_of(group,
@@ -717,6 +716,7 @@ static struct config_group *config_desc_make(
 	cfg = kzalloc(sizeof(*cfg), GFP_KERNEL);
 	if (!cfg)
 		return ERR_PTR(-ENOMEM);
+	cfg->gi = gi;
 	cfg->c.label = kstrdup(buf, GFP_KERNEL);
 	if (!cfg->c.label) {
 		ret = -ENOMEM;
-- 
2.7.4

[PATCH v5 3/3] usb: gadget: configfs: add some trace event

[Linyu Yuan]

add UDC, cfg link/unlink and some attributes trace
to better trace gadget issues.

Suggested-by: Felipe Balbi <balbi@kernel.org>
Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
v3: build trace inside configfs.c
v4: no change
v5: lost v2 fix, add it again

 drivers/usb/gadget/configfs.c       |  54 ++++++++++++
 drivers/usb/gadget/configfs_trace.h | 167 ++++++++++++++++++++++++++++++++++++
 2 files changed, 221 insertions(+)
 create mode 100644 drivers/usb/gadget/configfs_trace.h

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index cea12c3..61a8908 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -103,6 +103,42 @@ struct gadget_config_name {
 	struct list_head list;
 };
 
+#define MAX_CONFIGURAITON_STR_LEN	512
+
+static char *config_trace_string(struct gadget_info *gi)
+{
+	struct usb_configuration *uc;
+	struct config_usb_cfg *cfg;
+	struct config_usb_function *cf;
+	static char trs[MAX_CONFIGURAITON_STR_LEN];
+	size_t len = MAX_CONFIGURAITON_STR_LEN;
+	int n = 0;
+
+	trs[0] = '\0';
+
+	list_for_each_entry(uc, &gi->cdev.configs, list) {
+		cfg = container_of(uc, struct config_usb_cfg, c);
+
+		n += scnprintf(trs + n, len - n,
+			"group:%s,bConfigurationValue:%d,bmAttributes:%d,"
+			"MaxPower:%d,",
+			config_item_name(&cfg->group.cg_item),
+			uc->bConfigurationValue,
+			uc->bmAttributes,
+			uc->MaxPower);
+
+		n += scnprintf(trs + n, len - n, "function:[");
+		list_for_each_entry(cf, &cfg->func_list, list)
+			n += scnprintf(trs + n, len - n, "%s", cf->f->name);
+		n += scnprintf(trs + n, len - n, "},");
+	}
+
+	return trs;
+}
+
+#define CREATE_TRACE_POINTS
+#include "configfs_trace.h"
+
 #define USB_MAX_STRING_WITH_NULL_LEN	(USB_MAX_STRING_LEN+1)
 
 static int usb_string_copy(const char *s, char **s_copy)
@@ -210,6 +246,7 @@ static ssize_t gadget_dev_desc_bcdDevice_store(struct config_item *item,
 	if (ret)
 		return ret;
 
+	trace_gadget_dev_desc_bcdDevice_store(to_gadget_info(item));
 	to_gadget_info(item)->cdev.desc.bcdDevice = cpu_to_le16(bcdDevice);
 	return len;
 }
@@ -228,6 +265,7 @@ static ssize_t gadget_dev_desc_bcdUSB_store(struct config_item *item,
 		return ret;
 
 	to_gadget_info(item)->cdev.desc.bcdUSB = cpu_to_le16(bcdUSB);
+	trace_gadget_dev_desc_bcdUSB_store(to_gadget_info(item));
 	return len;
 }
 
@@ -240,6 +278,7 @@ static ssize_t gadget_dev_desc_UDC_show(struct config_item *item, char *page)
 	mutex_lock(&gi->lock);
 	udc_name = gi->composite.gadget_driver.udc_name;
 	ret = sprintf(page, "%s\n", udc_name ?: "");
+	trace_gadget_dev_desc_UDC_show(gi);
 	mutex_unlock(&gi->lock);
 
 	return ret;
@@ -249,6 +288,7 @@ static int unregister_gadget(struct gadget_info *gi)
 {
 	int ret;
 
+	trace_unregister_gadget(gi);
 	if (!gi->composite.gadget_driver.udc_name)
 		return -ENODEV;
 
@@ -276,6 +316,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(gi);
+
 	mutex_lock(&gi->lock);
 
 	if (!strlen(name)) {
@@ -296,6 +338,8 @@ static ssize_t gadget_dev_desc_UDC_store(struct config_item *item,
 		}
 	}
 	mutex_unlock(&gi->lock);
+
+	trace_gadget_dev_desc_UDC_store(gi);
 	return len;
 err:
 	kfree(name);
@@ -308,6 +352,7 @@ static ssize_t gadget_dev_desc_max_speed_show(struct config_item *item,
 {
 	enum usb_device_speed speed = to_gadget_info(item)->composite.max_speed;
 
+	trace_gadget_dev_desc_max_speed_show(to_gadget_info(item));
 	return sprintf(page, "%s\n", usb_speed_string(speed));
 }
 
@@ -337,6 +382,8 @@ static ssize_t gadget_dev_desc_max_speed_store(struct config_item *item,
 
 	gi->composite.gadget_driver.max_speed = gi->composite.max_speed;
 
+	trace_gadget_dev_desc_max_speed_store(gi);
+
 	mutex_unlock(&gi->lock);
 	return len;
 err:
@@ -468,6 +515,7 @@ static int config_usb_cfg_link(
 	list_add_tail(&cf->list, &cfg->func_list);
 	ret = 0;
 out:
+	trace_config_usb_cfg_link(gi);
 	mutex_unlock(&gi->lock);
 	return ret;
 }
@@ -500,10 +548,12 @@ static void config_usb_cfg_unlink(
 			list_del(&cf->list);
 			usb_put_function(cf->f);
 			kfree(cf);
+			trace_config_usb_cfg_unlink(gi);
 			mutex_unlock(&gi->lock);
 			return;
 		}
 	}
+	trace_config_usb_cfg_unlink(gi);
 	mutex_unlock(&gi->lock);
 	WARN(1, "Unable to locate function to unbind\n");
 }
@@ -518,6 +568,7 @@ static struct configfs_item_operations gadget_config_item_ops = {
 static ssize_t gadget_config_desc_MaxPower_show(struct config_item *item,
 		char *page)
 {
+	trace_gadget_config_desc_MaxPower_show(to_config_usb_cfg(item)->gi);
 	return sprintf(page, "%u\n", to_config_usb_cfg(item)->c.MaxPower);
 }
 
@@ -532,12 +583,14 @@ static ssize_t gadget_config_desc_MaxPower_store(struct config_item *item,
 	if (DIV_ROUND_UP(val, 8) > 0xff)
 		return -ERANGE;
 	to_config_usb_cfg(item)->c.MaxPower = val;
+	trace_gadget_config_desc_MaxPower_store(to_config_usb_cfg(item)->gi);
 	return len;
 }
 
 static ssize_t gadget_config_desc_bmAttributes_show(struct config_item *item,
 		char *page)
 {
+	trace_gadget_config_desc_bmAttributes_show(to_config_usb_cfg(item)->gi);
 	return sprintf(page, "0x%02x\n",
 		to_config_usb_cfg(item)->c.bmAttributes);
 }
@@ -556,6 +609,7 @@ static ssize_t gadget_config_desc_bmAttributes_store(struct config_item *item,
 				USB_CONFIG_ATT_WAKEUP))
 		return -EINVAL;
 	to_config_usb_cfg(item)->c.bmAttributes = val;
+	trace_gadget_config_desc_bmAttributes_store(to_config_usb_cfg(item)->gi);
 	return len;
 }
 
diff --git a/drivers/usb/gadget/configfs_trace.h b/drivers/usb/gadget/configfs_trace.h
new file mode 100644
index 0000000..59d73d5
--- /dev/null
+++ b/drivers/usb/gadget/configfs_trace.h
@@ -0,0 +1,167 @@
+// 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>
+
+DECLARE_EVENT_CLASS(log_gadget_info,
+	TP_PROTO(struct gadget_info *gi),
+	TP_ARGS(gi),
+	TP_STRUCT__entry(
+		/* gadget_info */
+		__string(gi_group, config_item_name(&gi->group.cg_item))
+		__field(bool, unbind)
+		__field(bool, use_os_desc)
+		__field(char, b_vendor_code)
+
+		/* usb_composite_dev */
+		__field(bool, suspended)
+		__field(bool, setup_pending)
+		__field(bool, os_desc_pending)
+		__field(unsigned, deactivations)
+		__field(int, delayed_status)
+		__field(u16, bcdUSB)
+		__field(u16, bcdDevice)
+		__string(config, gi->cdev.config)
+
+		/* usb_composite_driver */
+		__field(unsigned, max_speed)
+		__field(bool, needs_serial)
+
+		/* usb_gadget_driver */
+		__string(udc_name, gi->composite.gadget_driver.udc_name)
+	),
+	TP_fast_assign(
+		__assign_str(gi_group, config_item_name(&gi->group.cg_item));
+		__entry->unbind = gi->unbind;
+		__entry->use_os_desc = gi->use_os_desc;
+		__entry->b_vendor_code = gi->b_vendor_code;
+
+		__entry->suspended = gi->cdev.suspended;
+		__entry->setup_pending = gi->cdev.setup_pending;
+		__entry->os_desc_pending = gi->cdev.os_desc_pending;
+		__entry->deactivations = gi->cdev.deactivations;
+		__entry->delayed_status = gi->cdev.delayed_status;
+		__entry->bcdUSB = le16_to_cpu(gi->cdev.desc.bcdUSB);
+		__entry->bcdDevice = le16_to_cpu(gi->cdev.desc.bcdDevice);
+		__assign_str(config, config_trace_string(gi));
+
+		__entry->max_speed = gi->composite.max_speed;
+		__entry->needs_serial = gi->composite.needs_serial;
+
+		__assign_str(udc_name, gi->composite.gadget_driver.udc_name);
+	),
+	TP_printk("gi_group:%s,unbind:%d,use_os_desc:%d,b_vendor_code:%d,"
+		" - "
+		"suspended:%d,setup_pending:%d,"
+		"os_desc_pending:%d,deactivations:%d,delayed_status:%d,"
+		"bcdUSB:%04x,bcdDevice:%04x,config:%s,"
+		" - "
+		"max_speed:%d,needs_serial:%d,"
+		" - "
+		"udc_name:%s",
+
+		__get_str(gi_group),
+		__entry->unbind,
+		__entry->use_os_desc,
+		__entry->b_vendor_code,
+
+		__entry->suspended,
+		__entry->setup_pending,
+		__entry->os_desc_pending,
+		__entry->deactivations,
+		__entry->delayed_status,
+		__entry->bcdUSB,
+		__entry->bcdDevice,
+		__get_str(config),
+
+		__entry->max_speed,
+		__entry->needs_serial,
+
+		__get_str(udc_name)
+		)
+);
+
+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_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_UDC_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)
+);
+
+DEFINE_EVENT(log_gadget_info, config_usb_cfg_link,
+	TP_PROTO(struct gadget_info *gi),
+	TP_ARGS(gi)
+);
+
+DEFINE_EVENT(log_gadget_info, config_usb_cfg_unlink,
+	TP_PROTO(struct gadget_info *gi),
+	TP_ARGS(gi)
+);
+
+DEFINE_EVENT(log_gadget_info, gadget_config_desc_MaxPower_show,
+	TP_PROTO(struct gadget_info *gi),
+	TP_ARGS(gi)
+);
+
+DEFINE_EVENT(log_gadget_info, gadget_config_desc_MaxPower_store,
+	TP_PROTO(struct gadget_info *gi),
+	TP_ARGS(gi)
+);
+
+DEFINE_EVENT(log_gadget_info, gadget_config_desc_bmAttributes_show,
+	TP_PROTO(struct gadget_info *gi),
+	TP_ARGS(gi)
+);
+
+DEFINE_EVENT(log_gadget_info, gadget_config_desc_bmAttributes_store,
+	TP_PROTO(struct gadget_info *gi),
+	TP_ARGS(gi)
+);
+
+#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>
-- 
2.7.4

Re: [PATCH v5 1/3] usb: gadget: configfs: avoid list move operation of usb_function

[Greg Kroah-Hartman]

On Tue, Sep 07, 2021 at 09:09:35AM +0800, Linyu Yuan wrote:
> add a new list which link all usb_function at configfs layers,
> it means that after link a function a configuration,
> from configfs layer, we can still found all functions,
> it will allow trace all functions from configfs.

I am sorry, but I do not understand this paragraph.  Can you try
rewording it?

> 
> Reported-by: kernel test robot <lkp@intel.com>

How did the kernel test robot report this?  You are adding a new
function here, it is not a bug you are fixing, right?


> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> ---
> v2: fix unused cfg variable warning
> v3: add struct inside configfs.c
> v4: no change
> v5: lost v2 fix, add it again
> 
>  drivers/usb/gadget/configfs.c | 55 ++++++++++++++++++++++++-------------------
>  1 file changed, 31 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> index 477e72a..5b2e6f9 100644
> --- a/drivers/usb/gadget/configfs.c
> +++ b/drivers/usb/gadget/configfs.c
> @@ -58,6 +58,11 @@ static inline struct gadget_info *to_gadget_info(struct config_item *item)
>  	return container_of(to_config_group(item), struct gadget_info, group);
>  }
>  
> +struct config_usb_function {
> +	struct list_head list;
> +	struct usb_function *f;
> +};

What lock protects this list?

> +
>  struct config_usb_cfg {
>  	struct config_group group;
>  	struct config_group strings_group;
> @@ -420,7 +425,7 @@ static int config_usb_cfg_link(
>  	struct usb_function_instance *fi = container_of(group,
>  			struct usb_function_instance, group);
>  	struct usb_function_instance *a_fi;
> -	struct usb_function *f;
> +	struct config_usb_function *cf;
>  	int ret;
>  
>  	mutex_lock(&gi->lock);
> @@ -438,21 +443,29 @@ static int config_usb_cfg_link(
>  		goto out;
>  	}
>  
> -	list_for_each_entry(f, &cfg->func_list, list) {
> -		if (f->fi == fi) {
> +	list_for_each_entry(cf, &cfg->func_list, list) {
> +		if (cf->f->fi == fi) {
>  			ret = -EEXIST;
>  			goto out;
>  		}
>  	}
>  
> -	f = usb_get_function(fi);
> -	if (IS_ERR(f)) {
> -		ret = PTR_ERR(f);
> +	cf = kzalloc(sizeof(*cf), GFP_KERNEL);

Why "kzalloc" and not "kmalloc"?

I don't understand why you are moving everything to a single list in the
system, what is wrong with the existing per-device one?

thanks,

greg k-h

Re: [PATCH v5 2/3] usb: gadget: configfs: add gadget_info for config group

[Greg Kroah-Hartman]

On Tue, Sep 07, 2021 at 09:09:36AM +0800, Linyu Yuan wrote:
> add gadget_info pointer in struct config_usb_cfg
> to allow common gadget info trace from configfs layer.

Again, I do not understand this description, can you please try to
reword it?

> 
> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> ---
> v2: no change
> v3: change struct inside configfs.c
> v4: no change
> v5: no change
> 
>  drivers/usb/gadget/configfs.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> index 5b2e6f9..cea12c3 100644
> --- a/drivers/usb/gadget/configfs.c
> +++ b/drivers/usb/gadget/configfs.c
> @@ -70,6 +70,7 @@ struct config_usb_cfg {
>  	struct usb_configuration c;
>  	struct list_head func_list;
>  	struct usb_gadget_strings *gstrings[MAX_USB_STRING_LANGS + 1];
> +	struct gadget_info *gi;

You add a pointer, where are you removing it?

thanks,

greg k-h

Re: [PATCH v5 3/3] usb: gadget: configfs: add some trace event

[Greg Kroah-Hartman]

On Tue, Sep 07, 2021 at 09:09:37AM +0800, Linyu Yuan wrote:
> add UDC, cfg link/unlink and some attributes trace
> to better trace gadget issues.

Please document this a lot better.  What do these traces do and who is
supposed to use them and what for?


> 
> Suggested-by: Felipe Balbi <balbi@kernel.org>
> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> ---
> v3: build trace inside configfs.c
> v4: no change
> v5: lost v2 fix, add it again
> 
>  drivers/usb/gadget/configfs.c       |  54 ++++++++++++
>  drivers/usb/gadget/configfs_trace.h | 167 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 221 insertions(+)
>  create mode 100644 drivers/usb/gadget/configfs_trace.h
> 
> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> index cea12c3..61a8908 100644
> --- a/drivers/usb/gadget/configfs.c
> +++ b/drivers/usb/gadget/configfs.c
> @@ -103,6 +103,42 @@ struct gadget_config_name {
>  	struct list_head list;
>  };
>  
> +#define MAX_CONFIGURAITON_STR_LEN	512
> +
> +static char *config_trace_string(struct gadget_info *gi)
> +{
> +	struct usb_configuration *uc;
> +	struct config_usb_cfg *cfg;
> +	struct config_usb_function *cf;
> +	static char trs[MAX_CONFIGURAITON_STR_LEN];

One buffer for all messages?  What locking do you have in place to
handle things when multiple CPUs call this function at the same time?

> +	size_t len = MAX_CONFIGURAITON_STR_LEN;

Should be MAX_CONFIGURAITON_STR_LEN - 1, right?

> +	int n = 0;
> +
> +	trs[0] = '\0';

Why initialize just the first character


> +
> +	list_for_each_entry(uc, &gi->cdev.configs, list) {
> +		cfg = container_of(uc, struct config_usb_cfg, c);
> +
> +		n += scnprintf(trs + n, len - n,
> +			"group:%s,bConfigurationValue:%d,bmAttributes:%d,"

No spaces in the trace message, is that normal?

> +			"MaxPower:%d,",

Please do not split strings across a line.

> +			config_item_name(&cfg->group.cg_item),
> +			uc->bConfigurationValue,
> +			uc->bmAttributes,
> +			uc->MaxPower);
> +
> +		n += scnprintf(trs + n, len - n, "function:[");
> +		list_for_each_entry(cf, &cfg->func_list, list)
> +			n += scnprintf(trs + n, len - n, "%s", cf->f->name);
> +		n += scnprintf(trs + n, len - n, "},");
> +	}
> +
> +	return trs;

Again, you return a pointer to a static structure, yet you have no locks
at all.

> +}
> +
> +#define CREATE_TRACE_POINTS
> +#include "configfs_trace.h"
> +
>  #define USB_MAX_STRING_WITH_NULL_LEN	(USB_MAX_STRING_LEN+1)
>  
>  static int usb_string_copy(const char *s, char **s_copy)
> @@ -210,6 +246,7 @@ static ssize_t gadget_dev_desc_bcdDevice_store(struct config_item *item,
>  	if (ret)
>  		return ret;
>  
> +	trace_gadget_dev_desc_bcdDevice_store(to_gadget_info(item));
>  	to_gadget_info(item)->cdev.desc.bcdDevice = cpu_to_le16(bcdDevice);
>  	return len;
>  }
> @@ -228,6 +265,7 @@ static ssize_t gadget_dev_desc_bcdUSB_store(struct config_item *item,
>  		return ret;
>  
>  	to_gadget_info(item)->cdev.desc.bcdUSB = cpu_to_le16(bcdUSB);
> +	trace_gadget_dev_desc_bcdUSB_store(to_gadget_info(item));
>  	return len;
>  }
>  
> @@ -240,6 +278,7 @@ static ssize_t gadget_dev_desc_UDC_show(struct config_item *item, char *page)
>  	mutex_lock(&gi->lock);
>  	udc_name = gi->composite.gadget_driver.udc_name;
>  	ret = sprintf(page, "%s\n", udc_name ?: "");
> +	trace_gadget_dev_desc_UDC_show(gi);
>  	mutex_unlock(&gi->lock);
>  
>  	return ret;
> @@ -249,6 +288,7 @@ static int unregister_gadget(struct gadget_info *gi)
>  {
>  	int ret;
>  
> +	trace_unregister_gadget(gi);
>  	if (!gi->composite.gadget_driver.udc_name)
>  		return -ENODEV;
>  
> @@ -276,6 +316,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(gi);
> +
>  	mutex_lock(&gi->lock);
>  
>  	if (!strlen(name)) {
> @@ -296,6 +338,8 @@ static ssize_t gadget_dev_desc_UDC_store(struct config_item *item,
>  		}
>  	}
>  	mutex_unlock(&gi->lock);
> +
> +	trace_gadget_dev_desc_UDC_store(gi);
>  	return len;
>  err:
>  	kfree(name);
> @@ -308,6 +352,7 @@ static ssize_t gadget_dev_desc_max_speed_show(struct config_item *item,
>  {
>  	enum usb_device_speed speed = to_gadget_info(item)->composite.max_speed;
>  
> +	trace_gadget_dev_desc_max_speed_show(to_gadget_info(item));
>  	return sprintf(page, "%s\n", usb_speed_string(speed));
>  }
>  
> @@ -337,6 +382,8 @@ static ssize_t gadget_dev_desc_max_speed_store(struct config_item *item,
>  
>  	gi->composite.gadget_driver.max_speed = gi->composite.max_speed;
>  
> +	trace_gadget_dev_desc_max_speed_store(gi);
> +
>  	mutex_unlock(&gi->lock);
>  	return len;
>  err:
> @@ -468,6 +515,7 @@ static int config_usb_cfg_link(
>  	list_add_tail(&cf->list, &cfg->func_list);
>  	ret = 0;
>  out:
> +	trace_config_usb_cfg_link(gi);
>  	mutex_unlock(&gi->lock);
>  	return ret;
>  }
> @@ -500,10 +548,12 @@ static void config_usb_cfg_unlink(
>  			list_del(&cf->list);
>  			usb_put_function(cf->f);
>  			kfree(cf);
> +			trace_config_usb_cfg_unlink(gi);
>  			mutex_unlock(&gi->lock);
>  			return;
>  		}
>  	}
> +	trace_config_usb_cfg_unlink(gi);
>  	mutex_unlock(&gi->lock);
>  	WARN(1, "Unable to locate function to unbind\n");
>  }
> @@ -518,6 +568,7 @@ static struct configfs_item_operations gadget_config_item_ops = {
>  static ssize_t gadget_config_desc_MaxPower_show(struct config_item *item,
>  		char *page)
>  {
> +	trace_gadget_config_desc_MaxPower_show(to_config_usb_cfg(item)->gi);
>  	return sprintf(page, "%u\n", to_config_usb_cfg(item)->c.MaxPower);
>  }
>  
> @@ -532,12 +583,14 @@ static ssize_t gadget_config_desc_MaxPower_store(struct config_item *item,
>  	if (DIV_ROUND_UP(val, 8) > 0xff)
>  		return -ERANGE;
>  	to_config_usb_cfg(item)->c.MaxPower = val;
> +	trace_gadget_config_desc_MaxPower_store(to_config_usb_cfg(item)->gi);
>  	return len;
>  }
>  
>  static ssize_t gadget_config_desc_bmAttributes_show(struct config_item *item,
>  		char *page)
>  {
> +	trace_gadget_config_desc_bmAttributes_show(to_config_usb_cfg(item)->gi);
>  	return sprintf(page, "0x%02x\n",
>  		to_config_usb_cfg(item)->c.bmAttributes);
>  }
> @@ -556,6 +609,7 @@ static ssize_t gadget_config_desc_bmAttributes_store(struct config_item *item,
>  				USB_CONFIG_ATT_WAKEUP))
>  		return -EINVAL;
>  	to_config_usb_cfg(item)->c.bmAttributes = val;
> +	trace_gadget_config_desc_bmAttributes_store(to_config_usb_cfg(item)->gi);
>  	return len;
>  }
>  
> diff --git a/drivers/usb/gadget/configfs_trace.h b/drivers/usb/gadget/configfs_trace.h
> new file mode 100644
> index 0000000..59d73d5
> --- /dev/null
> +++ b/drivers/usb/gadget/configfs_trace.h
> @@ -0,0 +1,167 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2021 Qualcomm Innovation Center, Inc. All rights reserved.

Wrong copyright notice, right?  I could be wrong, but you might want to
check...


thanks,

greg k-h

Re: [PATCH v5 3/3] usb: gadget: configfs: add some trace event

[Jack Pham]

On Tue, Oct 05, 2021 at 01:16:12PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Sep 07, 2021 at 09:09:37AM +0800, Linyu Yuan wrote:

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

> > diff --git a/drivers/usb/gadget/configfs_trace.h b/drivers/usb/gadget/configfs_trace.h
> > new file mode 100644
> > index 0000000..59d73d5
> > --- /dev/null
> > +++ b/drivers/usb/gadget/configfs_trace.h
> > @@ -0,0 +1,167 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2021 Qualcomm Innovation Center, Inc. All rights reserved.
> 
> Wrong copyright notice, right?  I could be wrong, but you might want to
> check...

FYI this is correct.  Qualcomm is currently in transition and some
developers have started sending patches from a @quicinc.com email
address.  New files sent in this manner will be attributed using this
copyright notice (and is approved by our legal team).

Jack

Re: [PATCH v5 3/3] usb: gadget: configfs: add some trace event

[Greg Kroah-Hartman]

On Thu, Oct 07, 2021 at 06:09:52PM -0700, Jack Pham wrote:
> On Tue, Oct 05, 2021 at 01:16:12PM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Sep 07, 2021 at 09:09:37AM +0800, Linyu Yuan wrote:
> 
> > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> 
> > > diff --git a/drivers/usb/gadget/configfs_trace.h b/drivers/usb/gadget/configfs_trace.h
> > > new file mode 100644
> > > index 0000000..59d73d5
> > > --- /dev/null
> > > +++ b/drivers/usb/gadget/configfs_trace.h
> > > @@ -0,0 +1,167 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (c) 2021 Qualcomm Innovation Center, Inc. All rights reserved.
> > 
> > Wrong copyright notice, right?  I could be wrong, but you might want to
> > check...
> 
> FYI this is correct.  Qualcomm is currently in transition and some
> developers have started sending patches from a @quicinc.com email
> address.  New files sent in this manner will be attributed using this
> copyright notice (and is approved by our legal team).

Thanks for letting me know, is there some guideline as to what code gets
what copyright during the "transition" phase?

thanks,

greg k-h

Re: [PATCH v5 3/3] usb: gadget: configfs: add some trace event

[Trilok Soni]

Hi Greg,

On 10/7/2021 10:38 PM, Greg Kroah-Hartman wrote:
> On Thu, Oct 07, 2021 at 06:09:52PM -0700, Jack Pham wrote:
>> On Tue, Oct 05, 2021 at 01:16:12PM +0200, Greg Kroah-Hartman wrote:
>>> On Tue, Sep 07, 2021 at 09:09:37AM +0800, Linyu Yuan wrote:
>>
>>>> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
>>
>>>> diff --git a/drivers/usb/gadget/configfs_trace.h b/drivers/usb/gadget/configfs_trace.h
>>>> new file mode 100644
>>>> index 0000000..59d73d5
>>>> --- /dev/null
>>>> +++ b/drivers/usb/gadget/configfs_trace.h
>>>> @@ -0,0 +1,167 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Copyright (c) 2021 Qualcomm Innovation Center, Inc. All rights reserved.
>>>
>>> Wrong copyright notice, right?  I could be wrong, but you might want to
>>> check...
>>
>> FYI this is correct.  Qualcomm is currently in transition and some
>> developers have started sending patches from a @quicinc.com email
>> address.  New files sent in this manner will be attributed using this
>> copyright notice (and is approved by our legal team).
> 
> Thanks for letting me know, is there some guideline as to what code gets
> what copyright during the "transition" phase?

1. For the new files added by QuIC using the quicinc ID will have this 
new copyright string.

2. If we are modifying the existing files having the LF copyright and if 
the contributions are significant then we will add this new copyright 
string as well. (if the patch is sent using this new quicinc ID).

---Trilok Soni

RE: [PATCH v5 1/3] usb: gadget: configfs: avoid list move operation of usb_function

[Linyu Yuan (QUIC)]

> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Sent: Tuesday, October 5, 2021 7:11 PM
> To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org
> Subject: Re: [PATCH v5 1/3] usb: gadget: configfs: avoid list move operation
> of usb_function
> 
> On Tue, Sep 07, 2021 at 09:09:35AM +0800, Linyu Yuan wrote:
> > add a new list which link all usb_function at configfs layers,
> > it means that after link a function a configuration,
> > from configfs layer, we can still found all functions,
> > it will allow trace all functions from configfs.
> 
> I am sorry, but I do not understand this paragraph.  Can you try
> rewording it?
> 
> >
> > Reported-by: kernel test robot <lkp@intel.com>
> 
> How did the kernel test robot report this?  You are adding a new
> function here, it is not a bug you are fixing, right?
> 
> 
> > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> > ---
> > v2: fix unused cfg variable warning
> > v3: add struct inside configfs.c
> > v4: no change
> > v5: lost v2 fix, add it again
> >
> >  drivers/usb/gadget/configfs.c | 55 ++++++++++++++++++++++++----------
> ---------
> >  1 file changed, 31 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> > index 477e72a..5b2e6f9 100644
> > --- a/drivers/usb/gadget/configfs.c
> > +++ b/drivers/usb/gadget/configfs.c
> > @@ -58,6 +58,11 @@ static inline struct gadget_info
> *to_gadget_info(struct config_item *item)
> >  	return container_of(to_config_group(item), struct gadget_info,
> group);
> >  }
> >
> > +struct config_usb_function {
> > +	struct list_head list;
> > +	struct usb_function *f;
> > +};
> 
> What lock protects this list?
> 
> > +
> >  struct config_usb_cfg {
> >  	struct config_group group;
> >  	struct config_group strings_group;
> > @@ -420,7 +425,7 @@ static int config_usb_cfg_link(
> >  	struct usb_function_instance *fi = container_of(group,
> >  			struct usb_function_instance, group);
> >  	struct usb_function_instance *a_fi;
> > -	struct usb_function *f;
> > +	struct config_usb_function *cf;
> >  	int ret;
> >
> >  	mutex_lock(&gi->lock);
> > @@ -438,21 +443,29 @@ static int config_usb_cfg_link(
> >  		goto out;
> >  	}
> >
> > -	list_for_each_entry(f, &cfg->func_list, list) {
> > -		if (f->fi == fi) {
> > +	list_for_each_entry(cf, &cfg->func_list, list) {
> > +		if (cf->f->fi == fi) {
> >  			ret = -EEXIST;
> >  			goto out;
> >  		}
> >  	}
> >
> > -	f = usb_get_function(fi);
> > -	if (IS_ERR(f)) {
> > -		ret = PTR_ERR(f);
> > +	cf = kzalloc(sizeof(*cf), GFP_KERNEL);
> 
> Why "kzalloc" and not "kmalloc"?
> 
> I don't understand why you are moving everything to a single list in the
> system, what is wrong with the existing per-device one?
Thanks Greg,

Let me explain what I want to do in this  change,

For original code,  when add a function to configuration, it will add function 
struct usb_function {
...
struct list_head		list; [1]
};
to following list,
struct config_usb_cfg {
...
	struct list_head func_list; [2]
};
Then when bind happen, it will move [1] to following list,
struct usb_configuration {
...
struct list_head	functions; [3]
};

When unbind, it will move [1] back to [2].

We can see list [1] move between two list head, it is not easy to trace.

And when I add trace, I only want to get trace info from structure defined in configfs.c,

So I add a new list which ONLY add/remove to head [2] and it represent a function in configfs layer.
And original list [1] will ONLY add/remove to head [3].

> 
> thanks,
> 
> greg k-h

RE: [PATCH v5 1/3] usb: gadget: configfs: avoid list move operation of usb_function

[Linyu Yuan (QUIC)]

> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Sent: Tuesday, October 5, 2021 7:11 PM
> To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org
> Subject: Re: [PATCH v5 1/3] usb: gadget: configfs: avoid list move operation
> of usb_function
> 
> On Tue, Sep 07, 2021 at 09:09:35AM +0800, Linyu Yuan wrote:
> > add a new list which link all usb_function at configfs layers,
> > it means that after link a function a configuration,
> > from configfs layer, we can still found all functions,
> > it will allow trace all functions from configfs.
> 
> I am sorry, but I do not understand this paragraph.  Can you try
> rewording it?
Thanks, will update next version.
> 
> >
> > Reported-by: kernel test robot <lkp@intel.com>
> 
> How did the kernel test robot report this?  You are adding a new
> function here, it is not a bug you are fixing, right?
Thanks, will remove next version.
> 
> 
> > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> > ---
> > v2: fix unused cfg variable warning
> > v3: add struct inside configfs.c
> > v4: no change
> > v5: lost v2 fix, add it again
> >
> >  drivers/usb/gadget/configfs.c | 55 ++++++++++++++++++++++++----------
> ---------
> >  1 file changed, 31 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> > index 477e72a..5b2e6f9 100644
> > --- a/drivers/usb/gadget/configfs.c
> > +++ b/drivers/usb/gadget/configfs.c
> > @@ -58,6 +58,11 @@ static inline struct gadget_info
> *to_gadget_info(struct config_item *item)
> >  	return container_of(to_config_group(item), struct gadget_info,
> group);
> >  }
> >
> > +struct config_usb_function {
> > +	struct list_head list;
> > +	struct usb_function *f;
> > +};
> 
> What lock protects this list?
Currently like string list, there is no protection method,
I guess original author hope it can protect by following lock,
struct gadget_info {
...
	struct mutex lock;
};
> 
> > +
> >  struct config_usb_cfg {
> >  	struct config_group group;
> >  	struct config_group strings_group;
> > @@ -420,7 +425,7 @@ static int config_usb_cfg_link(
> >  	struct usb_function_instance *fi = container_of(group,
> >  			struct usb_function_instance, group);
> >  	struct usb_function_instance *a_fi;
> > -	struct usb_function *f;
> > +	struct config_usb_function *cf;
> >  	int ret;
> >
> >  	mutex_lock(&gi->lock);
> > @@ -438,21 +443,29 @@ static int config_usb_cfg_link(
> >  		goto out;
> >  	}
> >
> > -	list_for_each_entry(f, &cfg->func_list, list) {
> > -		if (f->fi == fi) {
> > +	list_for_each_entry(cf, &cfg->func_list, list) {
> > +		if (cf->f->fi == fi) {
> >  			ret = -EEXIST;
> >  			goto out;
> >  		}
> >  	}
> >
> > -	f = usb_get_function(fi);
> > -	if (IS_ERR(f)) {
> > -		ret = PTR_ERR(f);
> > +	cf = kzalloc(sizeof(*cf), GFP_KERNEL);
> 
> Why "kzalloc" and not "kmalloc"?
Thanks, will change next version.
> 
> I don't understand why you are moving everything to a single list in the
> system, what is wrong with the existing per-device one?
> 
> thanks,
> 
> greg k-h

RE: [PATCH v5 2/3] usb: gadget: configfs: add gadget_info for config group

[Linyu Yuan (QUIC)]

> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Sent: Tuesday, October 5, 2021 7:12 PM
> To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org
> Subject: Re: [PATCH v5 2/3] usb: gadget: configfs: add gadget_info for config
> group
> 
> On Tue, Sep 07, 2021 at 09:09:36AM +0800, Linyu Yuan wrote:
> > add gadget_info pointer in struct config_usb_cfg
> > to allow common gadget info trace from configfs layer.
> 
> Again, I do not understand this description, can you please try to
> reword it?
> 
> >
> > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> > ---
> > v2: no change
> > v3: change struct inside configfs.c
> > v4: no change
> > v5: no change
> >
> >  drivers/usb/gadget/configfs.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> > index 5b2e6f9..cea12c3 100644
> > --- a/drivers/usb/gadget/configfs.c
> > +++ b/drivers/usb/gadget/configfs.c
> > @@ -70,6 +70,7 @@ struct config_usb_cfg {
> >  	struct usb_configuration c;
> >  	struct list_head func_list;
> >  	struct usb_gadget_strings *gstrings[MAX_USB_STRING_LANGS + 1];
> > +	struct gadget_info *gi;
> 
> You add a pointer, where are you removing it?
Currently configfs layer seem do nothing when configfs drop of config group...
(it assume the configuration group will not remove)
Need to test rmdir operation  and send patch if possible later.
> 
> thanks,
> 
> greg k-h

Re: [PATCH v5 1/3] usb: gadget: configfs: avoid list move operation of usb_function

[Greg Kroah-Hartman]

On Sat, Oct 09, 2021 at 02:26:07AM +0000, Linyu Yuan (QUIC) wrote:
> > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Sent: Tuesday, October 5, 2021 7:11 PM
> > To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> > Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org
> > Subject: Re: [PATCH v5 1/3] usb: gadget: configfs: avoid list move operation
> > of usb_function
> > 
> > On Tue, Sep 07, 2021 at 09:09:35AM +0800, Linyu Yuan wrote:
> > > add a new list which link all usb_function at configfs layers,
> > > it means that after link a function a configuration,
> > > from configfs layer, we can still found all functions,
> > > it will allow trace all functions from configfs.
> > 
> > I am sorry, but I do not understand this paragraph.  Can you try
> > rewording it?
> > 
> > >
> > > Reported-by: kernel test robot <lkp@intel.com>
> > 
> > How did the kernel test robot report this?  You are adding a new
> > function here, it is not a bug you are fixing, right?
> > 
> > 
> > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> > > ---
> > > v2: fix unused cfg variable warning
> > > v3: add struct inside configfs.c
> > > v4: no change
> > > v5: lost v2 fix, add it again
> > >
> > >  drivers/usb/gadget/configfs.c | 55 ++++++++++++++++++++++++----------
> > ---------
> > >  1 file changed, 31 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> > > index 477e72a..5b2e6f9 100644
> > > --- a/drivers/usb/gadget/configfs.c
> > > +++ b/drivers/usb/gadget/configfs.c
> > > @@ -58,6 +58,11 @@ static inline struct gadget_info
> > *to_gadget_info(struct config_item *item)
> > >  	return container_of(to_config_group(item), struct gadget_info,
> > group);
> > >  }
> > >
> > > +struct config_usb_function {
> > > +	struct list_head list;
> > > +	struct usb_function *f;
> > > +};
> > 
> > What lock protects this list?
> > 
> > > +
> > >  struct config_usb_cfg {
> > >  	struct config_group group;
> > >  	struct config_group strings_group;
> > > @@ -420,7 +425,7 @@ static int config_usb_cfg_link(
> > >  	struct usb_function_instance *fi = container_of(group,
> > >  			struct usb_function_instance, group);
> > >  	struct usb_function_instance *a_fi;
> > > -	struct usb_function *f;
> > > +	struct config_usb_function *cf;
> > >  	int ret;
> > >
> > >  	mutex_lock(&gi->lock);
> > > @@ -438,21 +443,29 @@ static int config_usb_cfg_link(
> > >  		goto out;
> > >  	}
> > >
> > > -	list_for_each_entry(f, &cfg->func_list, list) {
> > > -		if (f->fi == fi) {
> > > +	list_for_each_entry(cf, &cfg->func_list, list) {
> > > +		if (cf->f->fi == fi) {
> > >  			ret = -EEXIST;
> > >  			goto out;
> > >  		}
> > >  	}
> > >
> > > -	f = usb_get_function(fi);
> > > -	if (IS_ERR(f)) {
> > > -		ret = PTR_ERR(f);
> > > +	cf = kzalloc(sizeof(*cf), GFP_KERNEL);
> > 
> > Why "kzalloc" and not "kmalloc"?
> > 
> > I don't understand why you are moving everything to a single list in the
> > system, what is wrong with the existing per-device one?
> Thanks Greg,
> 
> Let me explain what I want to do in this  change,
> 
> For original code,  when add a function to configuration, it will add function 
> struct usb_function {
> ...
> struct list_head		list; [1]
> };
> to following list,
> struct config_usb_cfg {
> ...
> 	struct list_head func_list; [2]
> };
> Then when bind happen, it will move [1] to following list,
> struct usb_configuration {
> ...
> struct list_head	functions; [3]
> };
> 
> When unbind, it will move [1] back to [2].
> 
> We can see list [1] move between two list head, it is not easy to trace.
> 
> And when I add trace, I only want to get trace info from structure defined in configfs.c,
> 
> So I add a new list which ONLY add/remove to head [2] and it represent a function in configfs layer.
> And original list [1] will ONLY add/remove to head [3].

I am sorry, but I still do not understand.  These are different types of
things that you are now putting all on the same list?

What prevents your trace functions from working today with the existing
code?  What can you not get access to?

All you say here is "it is not easy to trace", but that does not explain
_what_ you are missing and why you need to change that.

thanks,

greg k-h

RE: [PATCH v5 1/3] usb: gadget: configfs: avoid list move operation of usb_function

[Linyu Yuan (QUIC)]

> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Sent: Sunday, October 10, 2021 9:05 PM
> To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org
> Subject: Re: [PATCH v5 1/3] usb: gadget: configfs: avoid list move operation
> of usb_function
> 
> On Sat, Oct 09, 2021 at 02:26:07AM +0000, Linyu Yuan (QUIC) wrote:
> > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Sent: Tuesday, October 5, 2021 7:11 PM
> > > To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> > > Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org
> > > Subject: Re: [PATCH v5 1/3] usb: gadget: configfs: avoid list move
> operation
> > > of usb_function
> > >
> > > On Tue, Sep 07, 2021 at 09:09:35AM +0800, Linyu Yuan wrote:
> > > > add a new list which link all usb_function at configfs layers,
> > > > it means that after link a function a configuration,
> > > > from configfs layer, we can still found all functions,
> > > > it will allow trace all functions from configfs.
> > >
> > > I am sorry, but I do not understand this paragraph.  Can you try
> > > rewording it?
> > >
> > > >
> > > > Reported-by: kernel test robot <lkp@intel.com>
> > >
> > > How did the kernel test robot report this?  You are adding a new
> > > function here, it is not a bug you are fixing, right?
> > >
> > >
> > > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> > > > ---
> > > > v2: fix unused cfg variable warning
> > > > v3: add struct inside configfs.c
> > > > v4: no change
> > > > v5: lost v2 fix, add it again
> > > >
> > > >  drivers/usb/gadget/configfs.c | 55 ++++++++++++++++++++++++------
> ----
> > > ---------
> > > >  1 file changed, 31 insertions(+), 24 deletions(-)
> > > >
> > > > diff --git a/drivers/usb/gadget/configfs.c
> b/drivers/usb/gadget/configfs.c
> > > > index 477e72a..5b2e6f9 100644
> > > > --- a/drivers/usb/gadget/configfs.c
> > > > +++ b/drivers/usb/gadget/configfs.c
> > > > @@ -58,6 +58,11 @@ static inline struct gadget_info
> > > *to_gadget_info(struct config_item *item)
> > > >  	return container_of(to_config_group(item), struct gadget_info,
> > > group);
> > > >  }
> > > >
> > > > +struct config_usb_function {
> > > > +	struct list_head list;
> > > > +	struct usb_function *f;
> > > > +};
> > >
> > > What lock protects this list?
> > >
> > > > +
> > > >  struct config_usb_cfg {
> > > >  	struct config_group group;
> > > >  	struct config_group strings_group;
> > > > @@ -420,7 +425,7 @@ static int config_usb_cfg_link(
> > > >  	struct usb_function_instance *fi = container_of(group,
> > > >  			struct usb_function_instance, group);
> > > >  	struct usb_function_instance *a_fi;
> > > > -	struct usb_function *f;
> > > > +	struct config_usb_function *cf;
> > > >  	int ret;
> > > >
> > > >  	mutex_lock(&gi->lock);
> > > > @@ -438,21 +443,29 @@ static int config_usb_cfg_link(
> > > >  		goto out;
> > > >  	}
> > > >
> > > > -	list_for_each_entry(f, &cfg->func_list, list) {
> > > > -		if (f->fi == fi) {
> > > > +	list_for_each_entry(cf, &cfg->func_list, list) {
> > > > +		if (cf->f->fi == fi) {
> > > >  			ret = -EEXIST;
> > > >  			goto out;
> > > >  		}
> > > >  	}
> > > >
> > > > -	f = usb_get_function(fi);
> > > > -	if (IS_ERR(f)) {
> > > > -		ret = PTR_ERR(f);
> > > > +	cf = kzalloc(sizeof(*cf), GFP_KERNEL);
> > >
> > > Why "kzalloc" and not "kmalloc"?
> > >
> > > I don't understand why you are moving everything to a single list in the
> > > system, what is wrong with the existing per-device one?
> > Thanks Greg,
> >
> > Let me explain what I want to do in this  change,
> >
> > For original code,  when add a function to configuration, it will add function
> > struct usb_function {
> > ...
> > struct list_head		list; [1]
> > };
> > to following list,
> > struct config_usb_cfg {
> > ...
> > 	struct list_head func_list; [2]
> > };
> > Then when bind happen, it will move [1] to following list,
> > struct usb_configuration {
> > ...
> > struct list_head	functions; [3]
> > };
> >
> > When unbind, it will move [1] back to [2].
> >
> > We can see list [1] move between two list head, it is not easy to trace.
> >
> > And when I add trace, I only want to get trace info from structure defined
> in configfs.c,
> >
> > So I add a new list which ONLY add/remove to head [2] and it represent a
> function in configfs layer.
> > And original list [1] will ONLY add/remove to head [3].
> 
> I am sorry, but I still do not understand.  These are different types of
> things that you are now putting all on the same list?
> 
> What prevents your trace functions from working today with the existing
> code?  What can you not get access to?
> 
> All you say here is "it is not easy to trace", but that does not explain
> _what_ you are missing and why you need to change that.

Consider the list is moving between two list heads,
if I trace each function, It may duplicate or missing some function.
This is my simple reason.

> 
> thanks,
> 
> greg k-h

Re: [PATCH v5 1/3] usb: gadget: configfs: avoid list move operation of usb_function

[Greg Kroah-Hartman]

On Tue, Oct 12, 2021 at 03:54:29AM +0000, Linyu Yuan (QUIC) wrote:
> > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Sent: Sunday, October 10, 2021 9:05 PM
> > To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> > Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org
> > Subject: Re: [PATCH v5 1/3] usb: gadget: configfs: avoid list move operation
> > of usb_function
> > 
> > On Sat, Oct 09, 2021 at 02:26:07AM +0000, Linyu Yuan (QUIC) wrote:
> > > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > Sent: Tuesday, October 5, 2021 7:11 PM
> > > > To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> > > > Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org
> > > > Subject: Re: [PATCH v5 1/3] usb: gadget: configfs: avoid list move
> > operation
> > > > of usb_function
> > > >
> > > > On Tue, Sep 07, 2021 at 09:09:35AM +0800, Linyu Yuan wrote:
> > > > > add a new list which link all usb_function at configfs layers,
> > > > > it means that after link a function a configuration,
> > > > > from configfs layer, we can still found all functions,
> > > > > it will allow trace all functions from configfs.
> > > >
> > > > I am sorry, but I do not understand this paragraph.  Can you try
> > > > rewording it?
> > > >
> > > > >
> > > > > Reported-by: kernel test robot <lkp@intel.com>
> > > >
> > > > How did the kernel test robot report this?  You are adding a new
> > > > function here, it is not a bug you are fixing, right?
> > > >
> > > >
> > > > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> > > > > ---
> > > > > v2: fix unused cfg variable warning
> > > > > v3: add struct inside configfs.c
> > > > > v4: no change
> > > > > v5: lost v2 fix, add it again
> > > > >
> > > > >  drivers/usb/gadget/configfs.c | 55 ++++++++++++++++++++++++------
> > ----
> > > > ---------
> > > > >  1 file changed, 31 insertions(+), 24 deletions(-)
> > > > >
> > > > > diff --git a/drivers/usb/gadget/configfs.c
> > b/drivers/usb/gadget/configfs.c
> > > > > index 477e72a..5b2e6f9 100644
> > > > > --- a/drivers/usb/gadget/configfs.c
> > > > > +++ b/drivers/usb/gadget/configfs.c
> > > > > @@ -58,6 +58,11 @@ static inline struct gadget_info
> > > > *to_gadget_info(struct config_item *item)
> > > > >  	return container_of(to_config_group(item), struct gadget_info,
> > > > group);
> > > > >  }
> > > > >
> > > > > +struct config_usb_function {
> > > > > +	struct list_head list;
> > > > > +	struct usb_function *f;
> > > > > +};
> > > >
> > > > What lock protects this list?
> > > >
> > > > > +
> > > > >  struct config_usb_cfg {
> > > > >  	struct config_group group;
> > > > >  	struct config_group strings_group;
> > > > > @@ -420,7 +425,7 @@ static int config_usb_cfg_link(
> > > > >  	struct usb_function_instance *fi = container_of(group,
> > > > >  			struct usb_function_instance, group);
> > > > >  	struct usb_function_instance *a_fi;
> > > > > -	struct usb_function *f;
> > > > > +	struct config_usb_function *cf;
> > > > >  	int ret;
> > > > >
> > > > >  	mutex_lock(&gi->lock);
> > > > > @@ -438,21 +443,29 @@ static int config_usb_cfg_link(
> > > > >  		goto out;
> > > > >  	}
> > > > >
> > > > > -	list_for_each_entry(f, &cfg->func_list, list) {
> > > > > -		if (f->fi == fi) {
> > > > > +	list_for_each_entry(cf, &cfg->func_list, list) {
> > > > > +		if (cf->f->fi == fi) {
> > > > >  			ret = -EEXIST;
> > > > >  			goto out;
> > > > >  		}
> > > > >  	}
> > > > >
> > > > > -	f = usb_get_function(fi);
> > > > > -	if (IS_ERR(f)) {
> > > > > -		ret = PTR_ERR(f);
> > > > > +	cf = kzalloc(sizeof(*cf), GFP_KERNEL);
> > > >
> > > > Why "kzalloc" and not "kmalloc"?
> > > >
> > > > I don't understand why you are moving everything to a single list in the
> > > > system, what is wrong with the existing per-device one?
> > > Thanks Greg,
> > >
> > > Let me explain what I want to do in this  change,
> > >
> > > For original code,  when add a function to configuration, it will add function
> > > struct usb_function {
> > > ...
> > > struct list_head		list; [1]
> > > };
> > > to following list,
> > > struct config_usb_cfg {
> > > ...
> > > 	struct list_head func_list; [2]
> > > };
> > > Then when bind happen, it will move [1] to following list,
> > > struct usb_configuration {
> > > ...
> > > struct list_head	functions; [3]
> > > };
> > >
> > > When unbind, it will move [1] back to [2].
> > >
> > > We can see list [1] move between two list head, it is not easy to trace.
> > >
> > > And when I add trace, I only want to get trace info from structure defined
> > in configfs.c,
> > >
> > > So I add a new list which ONLY add/remove to head [2] and it represent a
> > function in configfs layer.
> > > And original list [1] will ONLY add/remove to head [3].
> > 
> > I am sorry, but I still do not understand.  These are different types of
> > things that you are now putting all on the same list?
> > 
> > What prevents your trace functions from working today with the existing
> > code?  What can you not get access to?
> > 
> > All you say here is "it is not easy to trace", but that does not explain
> > _what_ you are missing and why you need to change that.
> 
> Consider the list is moving between two list heads,
> if I trace each function, It may duplicate or missing some function.
> This is my simple reason.

How can the list move when you are tracing?

I'm sorry, I do not understand.

greg k-h

RE: [PATCH v5 1/3] usb: gadget: configfs: avoid list move operation of usb_function

[Linyu Yuan (QUIC)]

> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Sent: Tuesday, October 12, 2021 3:31 PM
> To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org
> Subject: Re: [PATCH v5 1/3] usb: gadget: configfs: avoid list move operation
> of usb_function
> 
> On Tue, Oct 12, 2021 at 03:54:29AM +0000, Linyu Yuan (QUIC) wrote:
> > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Sent: Sunday, October 10, 2021 9:05 PM
> > > To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> > > Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org
> > > Subject: Re: [PATCH v5 1/3] usb: gadget: configfs: avoid list move
> operation
> > > of usb_function
> > >
> > > On Sat, Oct 09, 2021 at 02:26:07AM +0000, Linyu Yuan (QUIC) wrote:
> > > > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > Sent: Tuesday, October 5, 2021 7:11 PM
> > > > > To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> > > > > Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org
> > > > > Subject: Re: [PATCH v5 1/3] usb: gadget: configfs: avoid list move
> > > operation
> > > > > of usb_function
> > > > >
> > > > > On Tue, Sep 07, 2021 at 09:09:35AM +0800, Linyu Yuan wrote:
> > > > > > add a new list which link all usb_function at configfs layers,
> > > > > > it means that after link a function a configuration,
> > > > > > from configfs layer, we can still found all functions,
> > > > > > it will allow trace all functions from configfs.
> > > > >
> > > > > I am sorry, but I do not understand this paragraph.  Can you try
> > > > > rewording it?
> > > > >
> > > > > >
> > > > > > Reported-by: kernel test robot <lkp@intel.com>
> > > > >
> > > > > How did the kernel test robot report this?  You are adding a new
> > > > > function here, it is not a bug you are fixing, right?
> > > > >
> > > > >
> > > > > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> > > > > > ---
> > > > > > v2: fix unused cfg variable warning
> > > > > > v3: add struct inside configfs.c
> > > > > > v4: no change
> > > > > > v5: lost v2 fix, add it again
> > > > > >
> > > > > >  drivers/usb/gadget/configfs.c | 55 ++++++++++++++++++++++++-
> -----
> > > ----
> > > > > ---------
> > > > > >  1 file changed, 31 insertions(+), 24 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/usb/gadget/configfs.c
> > > b/drivers/usb/gadget/configfs.c
> > > > > > index 477e72a..5b2e6f9 100644
> > > > > > --- a/drivers/usb/gadget/configfs.c
> > > > > > +++ b/drivers/usb/gadget/configfs.c
> > > > > > @@ -58,6 +58,11 @@ static inline struct gadget_info
> > > > > *to_gadget_info(struct config_item *item)
> > > > > >  	return container_of(to_config_group(item), struct
> gadget_info,
> > > > > group);
> > > > > >  }
> > > > > >
> > > > > > +struct config_usb_function {
> > > > > > +	struct list_head list;
> > > > > > +	struct usb_function *f;
> > > > > > +};
> > > > >
> > > > > What lock protects this list?
> > > > >
> > > > > > +
> > > > > >  struct config_usb_cfg {
> > > > > >  	struct config_group group;
> > > > > >  	struct config_group strings_group;
> > > > > > @@ -420,7 +425,7 @@ static int config_usb_cfg_link(
> > > > > >  	struct usb_function_instance *fi = container_of(group,
> > > > > >  			struct usb_function_instance, group);
> > > > > >  	struct usb_function_instance *a_fi;
> > > > > > -	struct usb_function *f;
> > > > > > +	struct config_usb_function *cf;
> > > > > >  	int ret;
> > > > > >
> > > > > >  	mutex_lock(&gi->lock);
> > > > > > @@ -438,21 +443,29 @@ static int config_usb_cfg_link(
> > > > > >  		goto out;
> > > > > >  	}
> > > > > >
> > > > > > -	list_for_each_entry(f, &cfg->func_list, list) {
> > > > > > -		if (f->fi == fi) {
> > > > > > +	list_for_each_entry(cf, &cfg->func_list, list) {
> > > > > > +		if (cf->f->fi == fi) {
> > > > > >  			ret = -EEXIST;
> > > > > >  			goto out;
> > > > > >  		}
> > > > > >  	}
> > > > > >
> > > > > > -	f = usb_get_function(fi);
> > > > > > -	if (IS_ERR(f)) {
> > > > > > -		ret = PTR_ERR(f);
> > > > > > +	cf = kzalloc(sizeof(*cf), GFP_KERNEL);
> > > > >
> > > > > Why "kzalloc" and not "kmalloc"?
> > > > >
> > > > > I don't understand why you are moving everything to a single list in
> the
> > > > > system, what is wrong with the existing per-device one?
> > > > Thanks Greg,
> > > >
> > > > Let me explain what I want to do in this  change,
> > > >
> > > > For original code,  when add a function to configuration, it will add
> function
> > > > struct usb_function {
> > > > ...
> > > > struct list_head		list; [1]
> > > > };
> > > > to following list,
> > > > struct config_usb_cfg {
> > > > ...
> > > > 	struct list_head func_list; [2]
> > > > };
> > > > Then when bind happen, it will move [1] to following list,
> > > > struct usb_configuration {
> > > > ...
> > > > struct list_head	functions; [3]
> > > > };
> > > >
> > > > When unbind, it will move [1] back to [2].
> > > >
> > > > We can see list [1] move between two list head, it is not easy to trace.
> > > >
> > > > And when I add trace, I only want to get trace info from structure
> defined
> > > in configfs.c,
> > > >
> > > > So I add a new list which ONLY add/remove to head [2] and it represent
> a
> > > function in configfs layer.
> > > > And original list [1] will ONLY add/remove to head [3].
> > >
> > > I am sorry, but I still do not understand.  These are different types of
> > > things that you are now putting all on the same list?
> > >
> > > What prevents your trace functions from working today with the existing
> > > code?  What can you not get access to?
> > >
> > > All you say here is "it is not easy to trace", but that does not explain
> > > _what_ you are missing and why you need to change that.
> >
> > Consider the list is moving between two list heads,
> > if I trace each function, It may duplicate or missing some function.
> > This is my simple reason.
> 
> How can the list move when you are tracing?
> 
> I'm sorry, I do not understand.
Thanks, I can consider it as only two list heads and there is no movement between them.
> 
> greg k-h

RE: [PATCH v5 3/3] usb: gadget: configfs: add some trace event

[Linyu Yuan (QUIC)]

> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Sent: Tuesday, October 5, 2021 7:16 PM
> To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com>
> Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org
> Subject: Re: [PATCH v5 3/3] usb: gadget: configfs: add some trace event
> 
> On Tue, Sep 07, 2021 at 09:09:37AM +0800, Linyu Yuan wrote:
> > add UDC, cfg link/unlink and some attributes trace
> > to better trace gadget issues.
> 
> Please document this a lot better.  What do these traces do and who is
> supposed to use them and what for?
> 
> 
> >
> > Suggested-by: Felipe Balbi <balbi@kernel.org>
> > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> > ---
> > v3: build trace inside configfs.c
> > v4: no change
> > v5: lost v2 fix, add it again
> >
> >  drivers/usb/gadget/configfs.c       |  54 ++++++++++++
> >  drivers/usb/gadget/configfs_trace.h | 167
> ++++++++++++++++++++++++++++++++++++
> >  2 files changed, 221 insertions(+)
> >  create mode 100644 drivers/usb/gadget/configfs_trace.h
> >
> > diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> > index cea12c3..61a8908 100644
> > --- a/drivers/usb/gadget/configfs.c
> > +++ b/drivers/usb/gadget/configfs.c
> > @@ -103,6 +103,42 @@ struct gadget_config_name {
> >  	struct list_head list;
> >  };
> >
> > +#define MAX_CONFIGURAITON_STR_LEN	512
> > +
> > +static char *config_trace_string(struct gadget_info *gi)
> > +{
> > +	struct usb_configuration *uc;
> > +	struct config_usb_cfg *cfg;
> > +	struct config_usb_function *cf;
> > +	static char trs[MAX_CONFIGURAITON_STR_LEN];
> 
> One buffer for all messages?  What locking do you have in place to
> handle things when multiple CPUs call this function at the same time?
> 
> > +	size_t len = MAX_CONFIGURAITON_STR_LEN;
> 
> Should be MAX_CONFIGURAITON_STR_LEN - 1, right?
> 
> > +	int n = 0;
> > +
> > +	trs[0] = '\0';
> 
> Why initialize just the first character
> 
> 
> > +
> > +	list_for_each_entry(uc, &gi->cdev.configs, list) {
> > +		cfg = container_of(uc, struct config_usb_cfg, c);
> > +
> > +		n += scnprintf(trs + n, len - n,
> > +
> 	"group:%s,bConfigurationValue:%d,bmAttributes:%d,"
> 
> No spaces in the trace message, is that normal?
> 
> > +			"MaxPower:%d,",
> 
> Please do not split strings across a line.
> 
> > +			config_item_name(&cfg->group.cg_item),
> > +			uc->bConfigurationValue,
> > +			uc->bmAttributes,
> > +			uc->MaxPower);
> > +
> > +		n += scnprintf(trs + n, len - n, "function:[");
> > +		list_for_each_entry(cf, &cfg->func_list, list)
> > +			n += scnprintf(trs + n, len - n, "%s", cf->f->name);
> > +		n += scnprintf(trs + n, len - n, "},");
> > +	}
> > +
> > +	return trs;
> 
> Again, you return a pointer to a static structure, yet you have no locks
> at all.
Seem when trace function called, the preempt disabled.
Do we need to add a lock ?
> 
> > +}
> > +
> > +#define CREATE_TRACE_POINTS
> > +#include "configfs_trace.h"
> > +
> >  #define USB_MAX_STRING_WITH_NULL_LEN
> 	(USB_MAX_STRING_LEN+1)
> >
> >  static int usb_string_copy(const char *s, char **s_copy)
> > @@ -210,6 +246,7 @@ static ssize_t
> gadget_dev_desc_bcdDevice_store(struct config_item *item,
> >  	if (ret)
> >  		return ret;
> >
> > +	trace_gadget_dev_desc_bcdDevice_store(to_gadget_info(item));
> >  	to_gadget_info(item)->cdev.desc.bcdDevice =
> cpu_to_le16(bcdDevice);
> >  	return len;
> >  }
> > @@ -228,6 +265,7 @@ static ssize_t
> gadget_dev_desc_bcdUSB_store(struct config_item *item,
> >  		return ret;
> >
> >  	to_gadget_info(item)->cdev.desc.bcdUSB = cpu_to_le16(bcdUSB);
> > +	trace_gadget_dev_desc_bcdUSB_store(to_gadget_info(item));
> >  	return len;
> >  }
> >
> > @@ -240,6 +278,7 @@ static ssize_t gadget_dev_desc_UDC_show(struct
> config_item *item, char *page)
> >  	mutex_lock(&gi->lock);
> >  	udc_name = gi->composite.gadget_driver.udc_name;
> >  	ret = sprintf(page, "%s\n", udc_name ?: "");
> > +	trace_gadget_dev_desc_UDC_show(gi);
> >  	mutex_unlock(&gi->lock);
> >
> >  	return ret;
> > @@ -249,6 +288,7 @@ static int unregister_gadget(struct gadget_info *gi)
> >  {
> >  	int ret;
> >
> > +	trace_unregister_gadget(gi);
> >  	if (!gi->composite.gadget_driver.udc_name)
> >  		return -ENODEV;
> >
> > @@ -276,6 +316,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(gi);
> > +
> >  	mutex_lock(&gi->lock);
> >
> >  	if (!strlen(name)) {
> > @@ -296,6 +338,8 @@ static ssize_t gadget_dev_desc_UDC_store(struct
> config_item *item,
> >  		}
> >  	}
> >  	mutex_unlock(&gi->lock);
> > +
> > +	trace_gadget_dev_desc_UDC_store(gi);
> >  	return len;
> >  err:
> >  	kfree(name);
> > @@ -308,6 +352,7 @@ static ssize_t
> gadget_dev_desc_max_speed_show(struct config_item *item,
> >  {
> >  	enum usb_device_speed speed = to_gadget_info(item)-
> >composite.max_speed;
> >
> > +	trace_gadget_dev_desc_max_speed_show(to_gadget_info(item));
> >  	return sprintf(page, "%s\n", usb_speed_string(speed));
> >  }
> >
> > @@ -337,6 +382,8 @@ static ssize_t
> gadget_dev_desc_max_speed_store(struct config_item *item,
> >
> >  	gi->composite.gadget_driver.max_speed = gi-
> >composite.max_speed;
> >
> > +	trace_gadget_dev_desc_max_speed_store(gi);
> > +
> >  	mutex_unlock(&gi->lock);
> >  	return len;
> >  err:
> > @@ -468,6 +515,7 @@ static int config_usb_cfg_link(
> >  	list_add_tail(&cf->list, &cfg->func_list);
> >  	ret = 0;
> >  out:
> > +	trace_config_usb_cfg_link(gi);
> >  	mutex_unlock(&gi->lock);
> >  	return ret;
> >  }
> > @@ -500,10 +548,12 @@ static void config_usb_cfg_unlink(
> >  			list_del(&cf->list);
> >  			usb_put_function(cf->f);
> >  			kfree(cf);
> > +			trace_config_usb_cfg_unlink(gi);
> >  			mutex_unlock(&gi->lock);
> >  			return;
> >  		}
> >  	}
> > +	trace_config_usb_cfg_unlink(gi);
> >  	mutex_unlock(&gi->lock);
> >  	WARN(1, "Unable to locate function to unbind\n");
> >  }
> > @@ -518,6 +568,7 @@ static struct configfs_item_operations
> gadget_config_item_ops = {
> >  static ssize_t gadget_config_desc_MaxPower_show(struct config_item
> *item,
> >  		char *page)
> >  {
> > +
> 	trace_gadget_config_desc_MaxPower_show(to_config_usb_cfg(ite
> m)->gi);
> >  	return sprintf(page, "%u\n", to_config_usb_cfg(item)-
> >c.MaxPower);
> >  }
> >
> > @@ -532,12 +583,14 @@ static ssize_t
> gadget_config_desc_MaxPower_store(struct config_item *item,
> >  	if (DIV_ROUND_UP(val, 8) > 0xff)
> >  		return -ERANGE;
> >  	to_config_usb_cfg(item)->c.MaxPower = val;
> > +
> 	trace_gadget_config_desc_MaxPower_store(to_config_usb_cfg(ite
> m)->gi);
> >  	return len;
> >  }
> >
> >  static ssize_t gadget_config_desc_bmAttributes_show(struct config_item
> *item,
> >  		char *page)
> >  {
> > +
> 	trace_gadget_config_desc_bmAttributes_show(to_config_usb_cfg(i
> tem)->gi);
> >  	return sprintf(page, "0x%02x\n",
> >  		to_config_usb_cfg(item)->c.bmAttributes);
> >  }
> > @@ -556,6 +609,7 @@ static ssize_t
> gadget_config_desc_bmAttributes_store(struct config_item *item,
> >  				USB_CONFIG_ATT_WAKEUP))
> >  		return -EINVAL;
> >  	to_config_usb_cfg(item)->c.bmAttributes = val;
> > +
> 	trace_gadget_config_desc_bmAttributes_store(to_config_usb_cfg(i
> tem)->gi);
> >  	return len;
> >  }
> >
> > diff --git a/drivers/usb/gadget/configfs_trace.h
> b/drivers/usb/gadget/configfs_trace.h
> > new file mode 100644
> > index 0000000..59d73d5
> > --- /dev/null
> > +++ b/drivers/usb/gadget/configfs_trace.h
> > @@ -0,0 +1,167 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2021 Qualcomm Innovation Center, Inc. All rights reserved.
> 
> Wrong copyright notice, right?  I could be wrong, but you might want to
> check...
> 
> 
> thanks,
> 
> greg k-h