Re: [PATCH v5 3/3] usb: gadget: configfs: add some trace event
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Date: 2021-10-05 11:16:16
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?
quoted hunk ↗ jump to hunk
Suggested-by: Felipe Balbi <balbi@kernel.org> Signed-off-by: Linyu Yuan <redacted> --- 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.hdiff --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.
quoted hunk ↗ jump to hunk
+} + +#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