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?
thanks,
greg k-h
