| Message ID | 1630976977-13938-2-git-send-email-quic_linyyuan@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb: gadget: configfs: add some trace event | expand |
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
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Sent: Tuesday, October 5, 2021 7:11 PM > To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com> > Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org > 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? > > > > > 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, Let me explain what I want to do in this change, For original code, when add a function to configuration, it will add function struct usb_function { ... struct list_head list; [1] }; to following list, struct config_usb_cfg { ... struct list_head func_list; [2] }; Then when bind happen, it will move [1] to following list, struct usb_configuration { ... struct list_head functions; [3] }; When unbind, it will move [1] back to [2]. We can see list [1] move between two list head, it is not easy to trace. And when I add trace, I only want to get trace info from structure defined in configfs.c, So I add a new list which ONLY add/remove to head [2] and it represent a function in configfs layer. And original list [1] will ONLY add/remove to head [3]. > > thanks, > > greg k-h
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Sent: Tuesday, October 5, 2021 7:11 PM > To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com> > Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org > 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@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? Thanks, will remove next version. > > > > 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? 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
On Sat, Oct 09, 2021 at 02:26:07AM +0000, Linyu Yuan (QUIC) wrote: > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Sent: Tuesday, October 5, 2021 7:11 PM > > To: Linyu Yuan (QUIC) <quic_linyyuan@quicinc.com> > > Cc: Felipe Balbi <balbi@kernel.org>; linux-usb@vger.kernel.org > > 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? > > > > > > > > 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, > > Let me explain what I want to do in this change, > > For original code, when add a function to configuration, it will add function > struct usb_function { > ... > struct list_head list; [1] > }; > to following list, > struct config_usb_cfg { > ... > struct list_head func_list; [2] > }; > Then when bind happen, it will move [1] to following list, > struct usb_configuration { > ... > struct list_head functions; [3] > }; > > When unbind, it will move [1] back to [2]. > > We can see list [1] move between two list head, it is not easy to trace. > > And when I add trace, I only want to get trace info from structure defined in configfs.c, > > So I add a new list which ONLY add/remove to head [2] and it represent a function in configfs layer. > And original list [1] will ONLY add/remove to head [3]. I am sorry, but I still do not understand. These are different types of things that you are now putting all on the same list? What prevents your trace functions from working today with the existing code? What can you not get access to? All you say here is "it is not easy to trace", but that does not explain _what_ you are missing and why you need to change that. thanks, greg k-h
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; +}; + 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); + if (!cf) { + ret = -ENOMEM; + goto out; + } + INIT_LIST_HEAD(&cf->list); + + cf->f = usb_get_function(fi); + if (IS_ERR(cf->f)) { + ret = PTR_ERR(cf->f); + kfree(cf); goto out; } /* stash the function until we bind it to the gadget */ - list_add_tail(&f->list, &cfg->func_list); + list_add_tail(&cf->list, &cfg->func_list); ret = 0; out: mutex_unlock(&gi->lock); @@ -470,7 +483,7 @@ static void config_usb_cfg_unlink( struct config_group *group = to_config_group(usb_func_ci); struct usb_function_instance *fi = container_of(group, struct usb_function_instance, group); - struct usb_function *f; + struct config_usb_function *cf; /* * ideally I would like to forbid to unlink functions while a gadget is @@ -483,10 +496,11 @@ static void config_usb_cfg_unlink( unregister_gadget(gi); WARN_ON(gi->composite.gadget_driver.udc_name); - list_for_each_entry(f, &cfg->func_list, list) { - if (f->fi == fi) { - list_del(&f->list); - usb_put_function(f); + list_for_each_entry(cf, &cfg->func_list, list) { + if (cf->f->fi == fi) { + list_del(&cf->list); + usb_put_function(cf->f); + kfree(cf); mutex_unlock(&gi->lock); return; } @@ -1257,13 +1271,10 @@ static void purge_configs_funcs(struct gadget_info *gi) list_for_each_entry(c, &gi->cdev.configs, list) { struct usb_function *f, *tmp; - struct config_usb_cfg *cfg; - - cfg = container_of(c, struct config_usb_cfg, c); list_for_each_entry_safe_reverse(f, tmp, &c->functions, list) { - list_move(&f->list, &cfg->func_list); + list_del(&f->list); if (f->unbind) { dev_dbg(&gi->cdev.gadget->dev, "unbind function '%s'/%p\n", @@ -1371,8 +1382,7 @@ static int configfs_composite_bind(struct usb_gadget *gadget, /* Go through all configs, attach all functions */ list_for_each_entry(c, &gi->cdev.configs, list) { struct config_usb_cfg *cfg; - struct usb_function *f; - struct usb_function *tmp; + struct config_usb_function *cf, *tmp; struct gadget_config_name *cn; if (gadget_is_otg(gadget)) @@ -1396,13 +1406,10 @@ static int configfs_composite_bind(struct usb_gadget *gadget, c->iConfiguration = s[0].id; } - list_for_each_entry_safe(f, tmp, &cfg->func_list, list) { - list_del(&f->list); - ret = usb_add_function(c, f); - if (ret) { - list_add(&f->list, &cfg->func_list); + list_for_each_entry_safe(cf, tmp, &cfg->func_list, list) { + ret = usb_add_function(c, cf->f); + if (ret) goto err_purge_funcs; - } } ret = usb_gadget_check_config(cdev->gadget); if (ret)
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. Reported-by: kernel test robot <lkp@intel.com> 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(-)