> From: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Sent: Tuesday, October 5, 2021 7:16 PM
> To: Linyu Yuan (QUIC) <quic_linyyuan@xxxxxxxxxxx>
> Cc: Felipe Balbi <balbi@xxxxxxxxxx>; linux-usb@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxx>
> > Signed-off-by: Linyu Yuan <quic_linyyuan@xxxxxxxxxxx>
> > ---
> > 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
