On Mon, Oct 18, 2021 at 01:29:30PM +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 most trace events which enable/disable a function,
> it including add/remove configuration, bind/unbind UDC,
> and write/read some attributes.
>
> 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
>
> drivers/usb/gadget/configfs.c | 43 ++++++--
> drivers/usb/gadget/configfs_trace.h | 196 ++++++++++++++++++++++++++++++++++++
> 2 files changed, 230 insertions(+), 9 deletions(-)
> create mode 100644 drivers/usb/gadget/configfs_trace.h
>
> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> index 477e72a..0e7feaa 100644
> --- a/drivers/usb/gadget/configfs.c
> +++ b/drivers/usb/gadget/configfs.c
> @@ -73,6 +73,11 @@ static inline struct config_usb_cfg *to_config_usb_cfg(struct config_item *item)
> group);
> }
>
> +static inline struct gadget_info *cfg_to_gadget_info(struct config_usb_cfg *cfg)
> +{
> + return container_of(cfg->c.cdev, struct gadget_info, cdev);
> +}
> +
> struct gadget_strings {
> struct usb_gadget_strings stringtab_dev;
> struct usb_string strings[USB_GADGET_FIRST_AVAIL_IDX];
> @@ -97,6 +102,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)
> @@ -205,6 +214,7 @@ static ssize_t gadget_dev_desc_bcdDevice_store(struct config_item *item,
> return ret;
>
> to_gadget_info(item)->cdev.desc.bcdDevice = cpu_to_le16(bcdDevice);
> + trace_gadget_dev_desc_bcdDevice_store(to_gadget_info(item));
> return len;
> }
>
> @@ -222,6 +232,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;
> }
>
> @@ -234,6 +245,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);
Why are you traceing things that you have full control over in
userspace? You can read this data out there, why do you need to trace
this here?
> mutex_unlock(&gi->lock);
>
> return ret;
> @@ -243,6 +255,7 @@ static int unregister_gadget(struct gadget_info *gi)
> {
> int ret;
>
> + trace_unregister_gadget(gi);
> if (!gi->composite.gadget_driver.udc_name)
> return -ENODEV;
>
> @@ -289,6 +302,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:
> @@ -302,6 +316,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));
Same here, why trace a configfs show callback? You can get this same
information just by reading the configfs file.
> }
>
> @@ -331,6 +346,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:
> @@ -413,8 +429,7 @@ static int config_usb_cfg_link(
> struct config_item *usb_func_ci)
> {
> struct config_usb_cfg *cfg = to_config_usb_cfg(usb_cfg_ci);
> - struct usb_composite_dev *cdev = cfg->c.cdev;
> - struct gadget_info *gi = container_of(cdev, struct gadget_info, cdev);
> + struct gadget_info *gi = cfg_to_gadget_info(cfg);
>
> struct config_group *group = to_config_group(usb_func_ci);
> struct usb_function_instance *fi = container_of(group,
> @@ -455,6 +470,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;
> }
> @@ -464,8 +480,7 @@ static void config_usb_cfg_unlink(
> struct config_item *usb_func_ci)
> {
> struct config_usb_cfg *cfg = to_config_usb_cfg(usb_cfg_ci);
> - struct usb_composite_dev *cdev = cfg->c.cdev;
> - struct gadget_info *gi = container_of(cdev, struct gadget_info, cdev);
> + struct gadget_info *gi = cfg_to_gadget_info(cfg);
Why not make this a 2 patch series, the first one adds the
cfg_to_gadget_info() function, and the second the traces. That way it's
easier to review.
And why so many versions of the patch so quickly?
thanks,
greg k-h
