> From: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Sent: Tuesday, October 5, 2021 7:11 PM
> To: Linyu Yuan (QUIC) <quic_linyyuan@xxxxxxxxxxx>
> Cc: Felipe Balbi <balbi@xxxxxxxxxx>; linux-usb@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v5 1/3] usb: gadget: configfs: avoid list move operation
> of usb_function
>
> 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?
Thanks, will update next version.
>
> >
> > Reported-by: kernel test robot <lkp@xxxxxxxxx>
>
> How did the kernel test robot report this? You are adding a new
> function here, it is not a bug you are fixing, right?
Thanks, will remove next version.
>
>
> > Signed-off-by: Linyu Yuan <quic_linyyuan@xxxxxxxxxxx>
> > ---
> > 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?
Currently like string list, there is no protection method,
I guess original author hope it can protect by following lock,
struct gadget_info {
...
struct mutex lock;
};
>
> > +
> > 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"?
Thanks, will change next version.
>
> 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
