> From: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Sent: Friday, October 22, 2021 5:21 PM
> To: Linyu Yuan (QUIC) <quic_linyyuan@xxxxxxxxxxx>
> Cc: Felipe Balbi <balbi@xxxxxxxxxx>; linux-usb@xxxxxxxxxxxxxxx; Jack Pham
> <jackp@xxxxxxxxxxx>
> Subject: Re: [PATCH v12 4/4] usb: gadget: add configfs trace events
>
> On Tue, Oct 19, 2021 at 09:26:37PM +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@xxxxxxxxxx>
> > Signed-off-by: Linyu Yuan <quic_linyyuan@xxxxxxxxxxx>
> > ---
> > v2: fix two issue Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > 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
> > v12: allocate trace string array per gadget driver
> >
> > drivers/usb/gadget/configfs.c | 17 ++++
> > drivers/usb/gadget/configfs_trace.h | 168
> ++++++++++++++++++++++++++++++++++++
> > 2 files changed, 185 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 27aa569..6e7d1b1 100644
> > --- a/drivers/usb/gadget/configfs.c
> > +++ b/drivers/usb/gadget/configfs.c
> > @@ -29,6 +29,7 @@ int check_user_usb_string(const char *name,
> >
> > #define MAX_NAME_LEN 40
> > #define MAX_USB_STRING_LANGS 2
> > +#define MAX_TRACE_STR_LEN 512
> >
> > static const struct usb_descriptor_header *otg_desc[2];
> >
> > @@ -51,6 +52,9 @@ struct gadget_info {
> > char qw_sign[OS_STRING_QW_SIGN_LEN];
> > spinlock_t spinlock;
> > bool unbind;
> > +#ifdef CONFIG_TRACING
> > + char trace_string[MAX_TRACE_STR_LEN];
> > +#endif
> > };
> >
> > static inline struct gadget_info *to_gadget_info(struct config_item *item)
> > @@ -102,6 +106,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)
> > @@ -212,6 +220,7 @@ static ssize_t
> gadget_dev_desc_bcdDevice_store(struct config_item *item,
> >
> > mutex_lock(&gi->lock);
> > gi->cdev.desc.bcdDevice = cpu_to_le16(bcdDevice);
> > + trace_gadget_dev_desc_bcdDevice_store(gi);
> > mutex_unlock(&gi->lock);
> > return len;
> > }
> > @@ -232,6 +241,7 @@ static ssize_t
> gadget_dev_desc_bcdUSB_store(struct config_item *item,
> >
> > mutex_lock(&gi->lock);
> > gi->cdev.desc.bcdUSB = cpu_to_le16(bcdUSB);
> > + trace_gadget_dev_desc_bcdUSB_store(gi);
> > mutex_unlock(&gi->lock);
> > return len;
> > }
> > @@ -254,6 +264,7 @@ static int unregister_gadget(struct gadget_info *gi)
> > {
> > int ret;
> >
> > + trace_unregister_gadget(gi);
> > if (!gi->composite.gadget_driver.udc_name)
> > return -ENODEV;
> >
> > @@ -300,6 +311,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:
> > @@ -342,6 +354,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:
> > @@ -465,6 +478,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;
> > }
> > @@ -496,6 +510,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;
> > }
> > @@ -533,6 +548,7 @@ static ssize_t
> gadget_config_desc_MaxPower_store(struct config_item *item,
> > return -ERANGE;
> > mutex_lock(&gi->lock);
> > cfg->c.MaxPower = val;
> > + trace_gadget_config_desc_MaxPower_store(gi);
> > mutex_unlock(&gi->lock);
> > return len;
> > }
> > @@ -562,6 +578,7 @@ static ssize_t
> gadget_config_desc_bmAttributes_store(struct config_item *item,
> > return -EINVAL;
> > mutex_lock(&gi->lock);
> > cfg->c.bmAttributes = val;
> > + trace_gadget_config_desc_bmAttributes_store(gi);
> > mutex_unlock(&gi->lock);
> > return len;
> > }
> > diff --git a/drivers/usb/gadget/configfs_trace.h
> b/drivers/usb/gadget/configfs_trace.h
> > new file mode 100644
> > index 0000000..d36c61f
> > --- /dev/null
> > +++ b/drivers/usb/gadget/configfs_trace.h
> > @@ -0,0 +1,168 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2021 Qualcomm Innovation Center, Inc. All rights reserved.
> > + */
> > +
> > +#ifdef CONFIGFS_TRACE_STRING
> > +#undef CONFIGFS_TRACE_STRING
> > +
> > +#ifdef CONFIG_TRACING
> > +static __maybe_unused char *configfs_trace_string(struct gadget_info
> *gi)
> > +{
> > + struct usb_configuration *uc;
> > + struct config_usb_cfg *cfg;
> > + struct usb_function *f;
> > + size_t len = sizeof(gi->trace_string) - 1;
> > + int n = 0;
> > +
> > + if (list_empty(&gi->cdev.configs)) {
> > + strcat(gi->trace_string, "empty");
> > + return gi->trace_string;
> > + }
> > +
> > + list_for_each_entry(uc, &gi->cdev.configs, list) {
> > + cfg = container_of(uc, struct config_usb_cfg, c);
> > +
> > + n += scnprintf(gi->trace_string + n, len - n,
> > + "{%d %02x %d ",
> > + uc->bConfigurationValue,
> > + uc->bmAttributes,
> > + uc->MaxPower);
> > +
> > + list_for_each_entry(f, &cfg->func_list, list)
> > + n += scnprintf(gi->trace_string + n,
> > + len - n, "%s,", f->name);
> > +
> > + list_for_each_entry(f, &cfg->c.functions, list)
> > + n += scnprintf(gi->trace_string + n,
> > + len - n, "%s,", f->name);
> > +
> > + n += scnprintf(gi->trace_string + n, len - n, "};");
> > + }
> > +
> > + return gi->trace_string;
>
> I still do not see where you are locking anything to protect the walking
> of these lists.
>
> Where is that now happening?
With patch#3, my design is all data was protect by gi->lock mutex lock.
Now trace_string is allocated per instance, it also protected by gi->lock.
>
> thanks,
>
> greg k-h
