Re: [PATCH v5 1/3] usb: gadget: configfs: avoid list move operation of usb_function

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

On Tue, Sep 07, 2021 at 09:09:35AM +0800, Linyu Yuan wrote:
> add a new list which link all usb_function at configfs layers,
> it means that after link a function a configuration,
> from configfs layer, we can still found all functions,
> it will allow trace all functions from configfs.

I am sorry, but I do not understand this paragraph.  Can you try
rewording it?

> 
> Reported-by: kernel test robot <lkp@intel.com>

How did the kernel test robot report this?  You are adding a new
function here, it is not a bug you are fixing, right?


> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com>
> ---
> v2: fix unused cfg variable warning
> v3: add struct inside configfs.c
> v4: no change
> v5: lost v2 fix, add it again
> 
>  drivers/usb/gadget/configfs.c | 55 ++++++++++++++++++++++++-------------------
>  1 file changed, 31 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> index 477e72a..5b2e6f9 100644
> --- a/drivers/usb/gadget/configfs.c
> +++ b/drivers/usb/gadget/configfs.c
> @@ -58,6 +58,11 @@ static inline struct gadget_info *to_gadget_info(struct config_item *item)
>  	return container_of(to_config_group(item), struct gadget_info, group);
>  }
>  
> +struct config_usb_function {
> +	struct list_head list;
> +	struct usb_function *f;
> +};

What lock protects this list?

> +
>  struct config_usb_cfg {
>  	struct config_group group;
>  	struct config_group strings_group;
> @@ -420,7 +425,7 @@ static int config_usb_cfg_link(
>  	struct usb_function_instance *fi = container_of(group,
>  			struct usb_function_instance, group);
>  	struct usb_function_instance *a_fi;
> -	struct usb_function *f;
> +	struct config_usb_function *cf;
>  	int ret;
>  
>  	mutex_lock(&gi->lock);
> @@ -438,21 +443,29 @@ static int config_usb_cfg_link(
>  		goto out;
>  	}
>  
> -	list_for_each_entry(f, &cfg->func_list, list) {
> -		if (f->fi == fi) {
> +	list_for_each_entry(cf, &cfg->func_list, list) {
> +		if (cf->f->fi == fi) {
>  			ret = -EEXIST;
>  			goto out;
>  		}
>  	}
>  
> -	f = usb_get_function(fi);
> -	if (IS_ERR(f)) {
> -		ret = PTR_ERR(f);
> +	cf = kzalloc(sizeof(*cf), GFP_KERNEL);

Why "kzalloc" and not "kmalloc"?

I don't understand why you are moving everything to a single list in the
system, what is wrong with the existing per-device one?

thanks,

greg k-h