linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v11 0/3] usb: gadget: configfs: add some trace event
@ 2021-10-19  7:44 Linyu Yuan
  2021-10-19  7:44 ` [PATCH v11 1/3] usb: gadget: configfs: add cfg_to_gadget_info() helper Linyu Yuan
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Linyu Yuan @ 2021-10-19  7:44 UTC (permalink / raw)
  To: Felipe Balbi, Greg Kroah-Hartman; +Cc: linux-usb, Jack Pham, Linyu Yuan

this series make some minor change to gadget configfs and
add some important trace event 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
v6: fix comments from Greg Kroah-Hartman
v7: three minor changes according to coding rules
v8: change two trace location
v9: fix when config is empty
v10: fix wrong api in v9
v11: split to three changes, minor change to trace event print format

trace event will looks like as below,
config_usb_cfg_link: g1: 0 0 0 0 0 0 0 0 0000 0510 6 0 {1 80 2 Function FS Gadget,}; - (null)
gadget_dev_desc_UDC_store: g1: 0 0 0 0 0 0 0 0 0000 0510 6 0 {1 80 2 Function FS Gadget,}; - dummy_udc
unregister_gadget: g1: 0 0 0 0 0 0 0 0 0000 0510 6 0 {1 80 2 Function FS Gadget,}; - dummy_udc
config_usb_cfg_unlink: g1: 0 0 0 0 0 0 0 0 0000 0510 6 0 {1 80 2 }, - (null)


Linyu Yuan (3):
  usb: gadget: configfs: add cfg_to_gadget_info() helper
  usb: gadget: configfs: change config attributes file operation
  usb: gadget: add configfs trace events

 drivers/usb/gadget/configfs.c       |  39 +++++++--
 drivers/usb/gadget/configfs_trace.h | 166 ++++++++++++++++++++++++++++++++++++
 2 files changed, 196 insertions(+), 9 deletions(-)
 create mode 100644 drivers/usb/gadget/configfs_trace.h

-- 
2.7.4


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v11 1/3] usb: gadget: configfs: add cfg_to_gadget_info() helper
  2021-10-19  7:44 [PATCH v11 0/3] usb: gadget: configfs: add some trace event Linyu Yuan
@ 2021-10-19  7:44 ` Linyu Yuan
  2021-10-19  7:44 ` [PATCH v11 2/3] usb: gadget: configfs: change config attributes file operation Linyu Yuan
  2021-10-19  7:44 ` [PATCH v11 3/3] usb: gadget: add configfs trace events Linyu Yuan
  2 siblings, 0 replies; 5+ messages in thread
From: Linyu Yuan @ 2021-10-19  7:44 UTC (permalink / raw)
  To: Felipe Balbi, Greg Kroah-Hartman; +Cc: linux-usb, Jack Pham, Linyu Yuan

add this helper function can simplify code of
config_usb_cfg_link() and config_usb_cfg_unlink().

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
 drivers/usb/gadget/configfs.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 477e72a..58615dc 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -73,6 +73,11 @@ static inline struct config_usb_cfg *to_config_usb_cfg(struct config_item *item)
 			group);
 }
 
+static inline struct gadget_info *cfg_to_gadget_info(struct config_usb_cfg *cfg)
+{
+	return container_of(cfg->c.cdev, struct gadget_info, cdev);
+}
+
 struct gadget_strings {
 	struct usb_gadget_strings stringtab_dev;
 	struct usb_string strings[USB_GADGET_FIRST_AVAIL_IDX];
@@ -413,8 +418,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_to_gadget_info(cfg);
 
 	struct config_group *group = to_config_group(usb_func_ci);
 	struct usb_function_instance *fi = container_of(group,
@@ -464,8 +468,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_to_gadget_info(cfg);
 
 	struct config_group *group = to_config_group(usb_func_ci);
 	struct usb_function_instance *fi = container_of(group,
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH v11 2/3] usb: gadget: configfs: change config attributes file operation
  2021-10-19  7:44 [PATCH v11 0/3] usb: gadget: configfs: add some trace event Linyu Yuan
  2021-10-19  7:44 ` [PATCH v11 1/3] usb: gadget: configfs: add cfg_to_gadget_info() helper Linyu Yuan
@ 2021-10-19  7:44 ` Linyu Yuan
  2021-10-19  7:44 ` [PATCH v11 3/3] usb: gadget: add configfs trace events Linyu Yuan
  2 siblings, 0 replies; 5+ messages in thread
From: Linyu Yuan @ 2021-10-19  7:44 UTC (permalink / raw)
  To: Felipe Balbi, Greg Kroah-Hartman; +Cc: linux-usb, Jack Pham, Linyu Yuan

in order to add trace event in configfs function with same
struct gadget_info *gi parameter,
add struct config_usb_cfg *cfg variable in below functions,
gadget_config_desc_MaxPower_show(),
gadget_config_desc_MaxPower_store(),
gadget_config_desc_bmAttributes_show(),
gadget_config_desc_bmAttributes_store(),
this allow following patch easy change cfg to gi with helper function
cfg_to_gadget_info().

Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
---
 drivers/usb/gadget/configfs.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 58615dc..36c611d 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -508,12 +508,15 @@ static struct configfs_item_operations gadget_config_item_ops = {
 static ssize_t gadget_config_desc_MaxPower_show(struct config_item *item,
 		char *page)
 {
-	return sprintf(page, "%u\n", to_config_usb_cfg(item)->c.MaxPower);
+	struct config_usb_cfg *cfg = to_config_usb_cfg(item);
+
+	return sprintf(page, "%u\n", cfg->c.MaxPower);
 }
 
 static ssize_t gadget_config_desc_MaxPower_store(struct config_item *item,
 		const char *page, size_t len)
 {
+	struct config_usb_cfg *cfg = to_config_usb_cfg(item);
 	u16 val;
 	int ret;
 	ret = kstrtou16(page, 0, &val);
@@ -521,20 +524,22 @@ static ssize_t gadget_config_desc_MaxPower_store(struct config_item *item,
 		return ret;
 	if (DIV_ROUND_UP(val, 8) > 0xff)
 		return -ERANGE;
-	to_config_usb_cfg(item)->c.MaxPower = val;
+	cfg->c.MaxPower = val;
 	return len;
 }
 
 static ssize_t gadget_config_desc_bmAttributes_show(struct config_item *item,
 		char *page)
 {
-	return sprintf(page, "0x%02x\n",
-		to_config_usb_cfg(item)->c.bmAttributes);
+	struct config_usb_cfg *cfg = to_config_usb_cfg(item);
+
+	return sprintf(page, "0x%02x\n", cfg->c.bmAttributes);
 }
 
 static ssize_t gadget_config_desc_bmAttributes_store(struct config_item *item,
 		const char *page, size_t len)
 {
+	struct config_usb_cfg *cfg = to_config_usb_cfg(item);
 	u8 val;
 	int ret;
 	ret = kstrtou8(page, 0, &val);
@@ -545,7 +550,7 @@ static ssize_t gadget_config_desc_bmAttributes_store(struct config_item *item,
 	if (val & ~(USB_CONFIG_ATT_ONE | USB_CONFIG_ATT_SELFPOWER |
 				USB_CONFIG_ATT_WAKEUP))
 		return -EINVAL;
-	to_config_usb_cfg(item)->c.bmAttributes = val;
+	cfg->c.bmAttributes = val;
 	return len;
 }
 
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH v11 3/3] usb: gadget: add configfs trace events
  2021-10-19  7:44 [PATCH v11 0/3] usb: gadget: configfs: add some trace event Linyu Yuan
  2021-10-19  7:44 ` [PATCH v11 1/3] usb: gadget: configfs: add cfg_to_gadget_info() helper Linyu Yuan
  2021-10-19  7:44 ` [PATCH v11 2/3] usb: gadget: configfs: change config attributes file operation Linyu Yuan
@ 2021-10-19  7:44 ` Linyu Yuan
  2021-10-19  9:39   ` Greg Kroah-Hartman
  2 siblings, 1 reply; 5+ messages in thread
From: Linyu Yuan @ 2021-10-19  7:44 UTC (permalink / raw)
  To: Felipe Balbi, Greg Kroah-Hartman; +Cc: linux-usb, Jack Pham, Linyu Yuan

in case of USB Gadget functions configure through configfs from
a complicated user space program, when switch function from one to another,
if it failed, it is better to find out what action was done to configfs
from user space program.

this change add some trace events which enable/disable a function,
it including add/remove configuration, bind/unbind UDC,
and some attribute files write operation.

Suggested-by: Felipe Balbi <balbi@kernel.org>
Signed-off-by: Linyu Yuan <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
v6: fix comments from Greg Kroah-Hartman
v7: three minor changes according to coding rules
v8: change two trace location
v9: fix when config is empty
v10: fix wrong api in v9
v11: split to 3 changes, remove read trace, change trace event print format

 drivers/usb/gadget/configfs.c       |  13 +++
 drivers/usb/gadget/configfs_trace.h | 166 ++++++++++++++++++++++++++++++++++++
 2 files changed, 179 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 36c611d..9b063bb 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -102,6 +102,10 @@ struct gadget_config_name {
 	struct list_head list;
 };
 
+#define CONFIGFS_TRACE_STRING
+#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 +214,7 @@ static ssize_t gadget_dev_desc_bcdDevice_store(struct config_item *item,
 		return ret;
 
 	to_gadget_info(item)->cdev.desc.bcdDevice = cpu_to_le16(bcdDevice);
+	trace_gadget_dev_desc_bcdDevice_store(to_gadget_info(item));
 	return len;
 }
 
@@ -227,6 +232,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;
 }
 
@@ -248,6 +254,7 @@ static int unregister_gadget(struct gadget_info *gi)
 {
 	int ret;
 
+	trace_unregister_gadget(gi);
 	if (!gi->composite.gadget_driver.udc_name)
 		return -ENODEV;
 
@@ -294,6 +301,7 @@ static ssize_t gadget_dev_desc_UDC_store(struct config_item *item,
 			goto err;
 		}
 	}
+	trace_gadget_dev_desc_UDC_store(gi);
 	mutex_unlock(&gi->lock);
 	return len;
 err:
@@ -336,6 +344,7 @@ 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:
@@ -459,6 +468,7 @@ static int config_usb_cfg_link(
 	list_add_tail(&f->list, &cfg->func_list);
 	ret = 0;
 out:
+	trace_config_usb_cfg_link(gi);
 	mutex_unlock(&gi->lock);
 	return ret;
 }
@@ -490,6 +500,7 @@ static void config_usb_cfg_unlink(
 		if (f->fi == fi) {
 			list_del(&f->list);
 			usb_put_function(f);
+			trace_config_usb_cfg_unlink(gi);
 			mutex_unlock(&gi->lock);
 			return;
 		}
@@ -525,6 +536,7 @@ static ssize_t gadget_config_desc_MaxPower_store(struct config_item *item,
 	if (DIV_ROUND_UP(val, 8) > 0xff)
 		return -ERANGE;
 	cfg->c.MaxPower = val;
+	trace_gadget_config_desc_MaxPower_store(cfg_to_gadget_info(cfg));
 	return len;
 }
 
@@ -551,6 +563,7 @@ static ssize_t gadget_config_desc_bmAttributes_store(struct config_item *item,
 				USB_CONFIG_ATT_WAKEUP))
 		return -EINVAL;
 	cfg->c.bmAttributes = val;
+	trace_gadget_config_desc_bmAttributes_store(cfg_to_gadget_info(cfg));
 	return len;
 }
 
diff --git a/drivers/usb/gadget/configfs_trace.h b/drivers/usb/gadget/configfs_trace.h
new file mode 100644
index 0000000..74283f0
--- /dev/null
+++ b/drivers/usb/gadget/configfs_trace.h
@@ -0,0 +1,166 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2021 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#ifdef CONFIGFS_TRACE_STRING
+#undef CONFIGFS_TRACE_STRING
+
+#define MAX_CONFIGURAITON_STR_LEN	512
+static __maybe_unused char *configfs_trace_string(struct gadget_info *gi)
+{
+	struct usb_configuration *uc;
+	struct config_usb_cfg *cfg;
+	struct usb_function *f;
+	static char trs[MAX_CONFIGURAITON_STR_LEN];
+	size_t len = MAX_CONFIGURAITON_STR_LEN - 1;
+	int n = 0;
+
+	if (list_empty(&gi->cdev.configs)) {
+		strcat(trs, "empty");
+		return trs;
+	}
+
+	list_for_each_entry(uc, &gi->cdev.configs, list) {
+		cfg = container_of(uc, struct config_usb_cfg, c);
+
+		n += scnprintf(trs + n, len - n,
+			"{%d %02x %d ",
+			uc->bConfigurationValue,
+			uc->bmAttributes,
+			uc->MaxPower);
+
+		list_for_each_entry(f, &cfg->func_list, list)
+			n += scnprintf(trs + n, len - n, "%s,", f->name);
+
+		list_for_each_entry(f, &cfg->c.functions, list)
+			n += scnprintf(trs + n, len - n, "%s,", f->name);
+
+		n += scnprintf(trs + n, len - n, "};");
+	}
+
+	return trs;
+}
+
+#endif /* CONFIGFS_TRACE_STRING */
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM configfs_gadget
+
+#if !defined(__CONFIGFS_GADGET_TRACE_H) || defined(TRACE_HEADER_MULTI_READ)
+#define __CONFIGFS_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(
+		__string(gi_group, config_item_name(&gi->group.cg_item))
+		__field(bool, unbind)
+		__field(bool, use_os_desc)
+		__field(char, b_vendor_code)
+		__field(bool, suspended)
+		__field(bool, setup_pending)
+		__field(bool, os_desc_pending)
+		__field(unsigned int, deactivations)
+		__field(int, delayed_status)
+		__field(u16, bcdUSB)
+		__field(u16, bcdDevice)
+		__string(config, configfs_trace_string(gi))
+		__field(unsigned int, max_speed)
+		__field(bool, needs_serial)
+		__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, configfs_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("%s: %d %d %d %d %d %d %d %d %04x %04x %d %d %s - %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,
+		__entry->max_speed,
+		__entry->needs_serial,
+		__get_str(config),
+		__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, 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_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_store,
+	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 /* __CONFIGFS_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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v11 3/3] usb: gadget: add configfs trace events
  2021-10-19  7:44 ` [PATCH v11 3/3] usb: gadget: add configfs trace events Linyu Yuan
@ 2021-10-19  9:39   ` Greg Kroah-Hartman
  0 siblings, 0 replies; 5+ messages in thread
From: Greg Kroah-Hartman @ 2021-10-19  9:39 UTC (permalink / raw)
  To: Linyu Yuan; +Cc: Felipe Balbi, linux-usb, Jack Pham

On Tue, Oct 19, 2021 at 03:44:32PM +0800, Linyu Yuan wrote:
> in case of USB Gadget functions configure through configfs from
> a complicated user space program, when switch function from one to another,
> if it failed, it is better to find out what action was done to configfs
> from user space program.
> 
> this change add some trace events which enable/disable a function,
> it including add/remove configuration, bind/unbind UDC,
> and some attribute files write operation.
> 
> Suggested-by: Felipe Balbi <balbi@kernel.org>
> Signed-off-by: Linyu Yuan <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
> v6: fix comments from Greg Kroah-Hartman
> v7: three minor changes according to coding rules
> v8: change two trace location
> v9: fix when config is empty
> v10: fix wrong api in v9
> v11: split to 3 changes, remove read trace, change trace event print format
> 
>  drivers/usb/gadget/configfs.c       |  13 +++
>  drivers/usb/gadget/configfs_trace.h | 166 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 179 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 36c611d..9b063bb 100644
> --- a/drivers/usb/gadget/configfs.c
> +++ b/drivers/usb/gadget/configfs.c
> @@ -102,6 +102,10 @@ struct gadget_config_name {
>  	struct list_head list;
>  };
>  
> +#define CONFIGFS_TRACE_STRING
> +#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 +214,7 @@ static ssize_t gadget_dev_desc_bcdDevice_store(struct config_item *item,
>  		return ret;
>  
>  	to_gadget_info(item)->cdev.desc.bcdDevice = cpu_to_le16(bcdDevice);
> +	trace_gadget_dev_desc_bcdDevice_store(to_gadget_info(item));
>  	return len;
>  }
>  
> @@ -227,6 +232,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;
>  }
>  
> @@ -248,6 +254,7 @@ static int unregister_gadget(struct gadget_info *gi)
>  {
>  	int ret;
>  
> +	trace_unregister_gadget(gi);
>  	if (!gi->composite.gadget_driver.udc_name)
>  		return -ENODEV;
>  
> @@ -294,6 +301,7 @@ static ssize_t gadget_dev_desc_UDC_store(struct config_item *item,
>  			goto err;
>  		}
>  	}
> +	trace_gadget_dev_desc_UDC_store(gi);
>  	mutex_unlock(&gi->lock);
>  	return len;
>  err:
> @@ -336,6 +344,7 @@ 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:
> @@ -459,6 +468,7 @@ static int config_usb_cfg_link(
>  	list_add_tail(&f->list, &cfg->func_list);
>  	ret = 0;
>  out:
> +	trace_config_usb_cfg_link(gi);
>  	mutex_unlock(&gi->lock);
>  	return ret;
>  }
> @@ -490,6 +500,7 @@ static void config_usb_cfg_unlink(
>  		if (f->fi == fi) {
>  			list_del(&f->list);
>  			usb_put_function(f);
> +			trace_config_usb_cfg_unlink(gi);
>  			mutex_unlock(&gi->lock);
>  			return;
>  		}
> @@ -525,6 +536,7 @@ static ssize_t gadget_config_desc_MaxPower_store(struct config_item *item,
>  	if (DIV_ROUND_UP(val, 8) > 0xff)
>  		return -ERANGE;
>  	cfg->c.MaxPower = val;
> +	trace_gadget_config_desc_MaxPower_store(cfg_to_gadget_info(cfg));
>  	return len;
>  }
>  
> @@ -551,6 +563,7 @@ static ssize_t gadget_config_desc_bmAttributes_store(struct config_item *item,
>  				USB_CONFIG_ATT_WAKEUP))
>  		return -EINVAL;
>  	cfg->c.bmAttributes = val;
> +	trace_gadget_config_desc_bmAttributes_store(cfg_to_gadget_info(cfg));
>  	return len;
>  }
>  
> diff --git a/drivers/usb/gadget/configfs_trace.h b/drivers/usb/gadget/configfs_trace.h
> new file mode 100644
> index 0000000..74283f0
> --- /dev/null
> +++ b/drivers/usb/gadget/configfs_trace.h
> @@ -0,0 +1,166 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2021 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#ifdef CONFIGFS_TRACE_STRING
> +#undef CONFIGFS_TRACE_STRING
> +
> +#define MAX_CONFIGURAITON_STR_LEN	512
> +static __maybe_unused char *configfs_trace_string(struct gadget_info *gi)
> +{
> +	struct usb_configuration *uc;
> +	struct config_usb_cfg *cfg;
> +	struct usb_function *f;
> +	static char trs[MAX_CONFIGURAITON_STR_LEN];
> +	size_t len = MAX_CONFIGURAITON_STR_LEN - 1;
> +	int n = 0;
> +
> +	if (list_empty(&gi->cdev.configs)) {
> +		strcat(trs, "empty");
> +		return trs;
> +	}

What protects this list from changing why you are accessing it?  Where
is the lock here?

> +
> +	list_for_each_entry(uc, &gi->cdev.configs, list) {
> +		cfg = container_of(uc, struct config_usb_cfg, c);
> +
> +		n += scnprintf(trs + n, len - n,
> +			"{%d %02x %d ",
> +			uc->bConfigurationValue,
> +			uc->bmAttributes,
> +			uc->MaxPower);
> +
> +		list_for_each_entry(f, &cfg->func_list, list)
> +			n += scnprintf(trs + n, len - n, "%s,", f->name);
> +
> +		list_for_each_entry(f, &cfg->c.functions, list)
> +			n += scnprintf(trs + n, len - n, "%s,", f->name);

Same for all of these list entries, what happens if you are tracing and
remove a device at the same time?  Or add a device?

No locks feels very very wrong to me, I need some "proof" this is safe
to do.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-10-19  9:39 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-19  7:44 [PATCH v11 0/3] usb: gadget: configfs: add some trace event Linyu Yuan
2021-10-19  7:44 ` [PATCH v11 1/3] usb: gadget: configfs: add cfg_to_gadget_info() helper Linyu Yuan
2021-10-19  7:44 ` [PATCH v11 2/3] usb: gadget: configfs: change config attributes file operation Linyu Yuan
2021-10-19  7:44 ` [PATCH v11 3/3] usb: gadget: add configfs trace events Linyu Yuan
2021-10-19  9:39   ` Greg Kroah-Hartman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).