Re: [PATCH v12 4/4] usb: gadget: add configfs trace events

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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@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
> 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