| Message ID | 20230801045449.156348-1-zhouscd@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | USB: gadget: Fix the function name error in sourcesink/loopback. | expand |
On Mon, Jul 31, 2023 at 09:54:49PM -0700, zhouscd wrote: > Inconsistent function names can cause the USB config FS not work. I do not understand this text at all, sorry. What exactly is broken and what is changed here to resolve the issue? > > Signed-off-by: zhouscd <zhouscd@gmail.com> What commit caused the problem? And please use your full name for patches. > --- > drivers/usb/gadget/function/f_loopback.c | 13 +---------- > drivers/usb/gadget/function/f_sourcesink.c | 25 ++-------------------- > drivers/usb/gadget/function/g_zero.h | 3 --- > 3 files changed, 3 insertions(+), 38 deletions(-) > > diff --git a/drivers/usb/gadget/function/f_loopback.c b/drivers/usb/gadget/function/f_loopback.c > index ae41f556eb75..45f542b5ff55 100644 > --- a/drivers/usb/gadget/function/f_loopback.c > +++ b/drivers/usb/gadget/function/f_loopback.c > @@ -583,16 +583,5 @@ static struct usb_function_instance *loopback_alloc_instance(void) > > return &lb_opts->func_inst; > } > -DECLARE_USB_FUNCTION(Loopback, loopback_alloc_instance, loopback_alloc); > - > -int __init lb_modinit(void) > -{ > - return usb_function_register(&Loopbackusb_func); > -} > - > -void __exit lb_modexit(void) > -{ > - usb_function_unregister(&Loopbackusb_func); > -} > - > +DECLARE_USB_FUNCTION_INIT(loopback, loopback_alloc_instance, loopback_alloc); > MODULE_LICENSE("GPL"); > diff --git a/drivers/usb/gadget/function/f_sourcesink.c b/drivers/usb/gadget/function/f_sourcesink.c > index 6803cd60cc6d..f6d1c095aa2c 100644 > --- a/drivers/usb/gadget/function/f_sourcesink.c > +++ b/drivers/usb/gadget/function/f_sourcesink.c > @@ -858,7 +858,7 @@ static struct usb_function *source_sink_alloc_func( > ss->bulk_qlen = ss_opts->bulk_qlen; > ss->iso_qlen = ss_opts->iso_qlen; > > - ss->function.name = "source/sink"; > + ss->function.name = "sourcesink"; You just changed a user-visable api, right? Where did you document this and what will it affect? thanks, greg k-h
Hi, Greg KH > I do not understand this text at all, sorry. > What exactly is broken and what is changed here to resolve the issue? The reason for the problem is that the value of struct usb_function.name is "loopback", while struct usb_function_driver.name is "Loopback". The same issue exists for sourcesink. When using USB Config FS, it won't be possible to enable these two functions. > And please use your full name for patches. I'm sorry, this is my first time sending kernel patch. How should I modify my name for the patch that has already been sent? Or should I resend a new patch? > You just changed a user-visable api, right? Where did you document this > and what will it affect? Yes, I removed lb_modexit and lb_modinit and used a simpler method for function initialization. This does not affect any other functionalities. In the old way, the loopback function was called by sslb_modinit in sourcesink. I believe this is not a good approach as the loopback and sourcesink should be independent functionalities. Their purpose is to provide simple examples for USB beginners like myself. Greg KH <gregkh@linuxfoundation.org> 于2023年8月1日周二 13:06写道： > > On Mon, Jul 31, 2023 at 09:54:49PM -0700, zhouscd wrote: > > Inconsistent function names can cause the USB config FS not work. > > I do not understand this text at all, sorry. > > What exactly is broken and what is changed here to resolve the issue? > > > > > Signed-off-by: zhouscd <zhouscd@gmail.com> > > What commit caused the problem? > > And please use your full name for patches. > > > --- > > drivers/usb/gadget/function/f_loopback.c | 13 +---------- > > drivers/usb/gadget/function/f_sourcesink.c | 25 ++-------------------- > > drivers/usb/gadget/function/g_zero.h | 3 --- > > 3 files changed, 3 insertions(+), 38 deletions(-) > > > > diff --git a/drivers/usb/gadget/function/f_loopback.c b/drivers/usb/gadget/function/f_loopback.c > > index ae41f556eb75..45f542b5ff55 100644 > > --- a/drivers/usb/gadget/function/f_loopback.c > > +++ b/drivers/usb/gadget/function/f_loopback.c > > @@ -583,16 +583,5 @@ static struct usb_function_instance *loopback_alloc_instance(void) > > > > return &lb_opts->func_inst; > > } > > -DECLARE_USB_FUNCTION(Loopback, loopback_alloc_instance, loopback_alloc); > > - > > -int __init lb_modinit(void) > > -{ > > - return usb_function_register(&Loopbackusb_func); > > -} > > - > > -void __exit lb_modexit(void) > > -{ > > - usb_function_unregister(&Loopbackusb_func); > > -} > > - > > +DECLARE_USB_FUNCTION_INIT(loopback, loopback_alloc_instance, loopback_alloc); > > MODULE_LICENSE("GPL"); > > diff --git a/drivers/usb/gadget/function/f_sourcesink.c b/drivers/usb/gadget/function/f_sourcesink.c > > index 6803cd60cc6d..f6d1c095aa2c 100644 > > --- a/drivers/usb/gadget/function/f_sourcesink.c > > +++ b/drivers/usb/gadget/function/f_sourcesink.c > > @@ -858,7 +858,7 @@ static struct usb_function *source_sink_alloc_func( > > ss->bulk_qlen = ss_opts->bulk_qlen; > > ss->iso_qlen = ss_opts->iso_qlen; > > > > - ss->function.name = "source/sink"; > > + ss->function.name = "sourcesink"; > > You just changed a user-visable api, right? Where did you document this > and what will it affect? > > thanks, > > greg k-h
On Tue, Aug 01, 2023 at 02:15:50PM +0800, 周城东 wrote: > Hi, Greg KH > > > I do not understand this text at all, sorry. > > What exactly is broken and what is changed here to resolve the issue? > > The reason for the problem is that the value of struct > usb_function.name is "loopback", while struct usb_function_driver.name > is "Loopback". The same issue exists for sourcesink. When using USB > Config FS, it won't be possible to enable these two functions. Please document this in the changelog text. > > And please use your full name for patches. > > I'm sorry, this is my first time sending kernel patch. How should I > modify my name for the patch that has already been sent? Or should I > resend a new patch? Yes, you need to send a new version, please read the section entitled "The canonical patch format" in the kernel file, Documentation/process/submitting-patches.rst for what needs to be done here. > > You just changed a user-visable api, right? Where did you document this > > and what will it affect? > > Yes, I removed lb_modexit and lb_modinit and used a simpler method for > function initialization. This does not affect any other > functionalities. In the old way, the loopback function was called by > sslb_modinit in sourcesink. I believe this is not a good approach as > the loopback and sourcesink should be independent functionalities. > Their purpose is to provide simple examples for USB beginners like myself. But you changed the name: > > > - ss->function.name = "source/sink"; > > > + ss->function.name = "sourcesink"; isn't that visable to userspace? thanks, greg k-h
Thank you for your patient response. On Tue, Aug 1, 2023 at 2:23 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > Please document this in the changelog text. But I can't find the changelog text anywhere. > > > But you changed the name: > > > > > - ss->function.name = "source/sink"; > > > > + ss->function.name = "sourcesink"; > > isn't that visable to userspace? Yes, I removed the "/". Because the macro definition DECLARE_USB_FUNCTION_INIT does not support "/". Should I stick with the original "SourceSink"? I think using the Linux-style "sourcesink" is better. By the way, due to the current bug, no one should be able to use "source/sink" in userspace. thanks.
On Tue, Aug 01, 2023 at 03:27:04PM +0800, chengdong zhou wrote: > Thank you for your patient response. > > On Tue, Aug 1, 2023 at 2:23 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > > Please document this in the changelog text. > > But I can't find the changelog text anywhere. That is what you are writing here for the commit. Please read the kernel documentation for how to submit a patch, it will explain this. > > But you changed the name: > > > > > > > - ss->function.name = "source/sink"; > > > > > + ss->function.name = "sourcesink"; > > > > isn't that visable to userspace? > > Yes, I removed the "/". Because the macro definition > DECLARE_USB_FUNCTION_INIT does not support "/". > Should I stick with the original "SourceSink"? I think using the > Linux-style "sourcesink" is better. By the way, due to the current > bug, no one should be able to use "source/sink" in userspace. But doesn't the '/' mean that you have a subdirectory here? What did userspace look like before this, and what does it look like now? thanks, greg k-h
On Tue, Aug 1, 2023 at 3:35 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > But you changed the name: > > > > > > > > > - ss->function.name = "source/sink"; > > > > > > + ss->function.name = "sourcesink"; > > > > > > isn't that visable to userspace? > > > > Yes, I removed the "/". Because the macro definition > > DECLARE_USB_FUNCTION_INIT does not support "/". > > Should I stick with the original "SourceSink"? I think using the > > Linux-style "sourcesink" is better. By the way, due to the current > > bug, no one should be able to use "source/sink" in userspace. > > But doesn't the '/' mean that you have a subdirectory here? What did > userspace look like before this, and what does it look like now? > Because usb_function.name and usb_function_driver.name are not the same, this function could not be exported to userspace through the USB config fs before. Previously, the source/sink was used by g_zero legacy, so I will make adjustments in the next patch version by changing the function name to "sourcesink". Do you think this is okay?
Hi zhouscd, kernel test robot noticed the following build errors: [auto build test ERROR on usb/usb-testing] [also build test ERROR on usb/usb-next usb/usb-linus linus/master v6.5-rc4 next-20230801] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/zhouscd/USB-gadget-Fix-the-function-name-error-in-sourcesink-loopback/20230801-125745 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing patch link: https://lore.kernel.org/r/20230801045449.156348-1-zhouscd%40gmail.com patch subject: [PATCH] USB: gadget: Fix the function name error in sourcesink/loopback. config: sparc-randconfig-r015-20230731 (https://download.01.org/0day-ci/archive/20230801/202308012224.vyXfjJMA-lkp@intel.com/config) compiler: sparc64-linux-gcc (GCC) 12.3.0 reproduce: (https://download.01.org/0day-ci/archive/20230801/202308012224.vyXfjJMA-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202308012224.vyXfjJMA-lkp@intel.com/ All errors (new ones prefixed by >>): sparc64-linux-ld: drivers/usb/gadget/function/f_sourcesink.o: in function `sourcesinkmod_init': >> f_sourcesink.c:(.init.text+0x0): multiple definition of `init_module'; drivers/usb/gadget/function/f_loopback.o:f_loopback.c:(.init.text+0x0): first defined here sparc64-linux-ld: drivers/usb/gadget/function/f_sourcesink.o: in function `sourcesinkmod_exit': >> f_sourcesink.c:(.exit.text+0x0): multiple definition of `cleanup_module'; drivers/usb/gadget/function/f_loopback.o:f_loopback.c:(.exit.text+0x0): first defined here
Hi zhouscd, kernel test robot noticed the following build errors: [auto build test ERROR on usb/usb-testing] [also build test ERROR on usb/usb-next usb/usb-linus linus/master v6.5-rc4 next-20230801] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/zhouscd/USB-gadget-Fix-the-function-name-error-in-sourcesink-loopback/20230801-125745 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing patch link: https://lore.kernel.org/r/20230801045449.156348-1-zhouscd%40gmail.com patch subject: [PATCH] USB: gadget: Fix the function name error in sourcesink/loopback. config: x86_64-buildonly-randconfig-r003-20230731 (https://download.01.org/0day-ci/archive/20230802/202308020504.tNlWrHXN-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce: (https://download.01.org/0day-ci/archive/20230802/202308020504.tNlWrHXN-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202308020504.tNlWrHXN-lkp@intel.com/ All errors (new ones prefixed by >>): ld: drivers/usb/gadget/function/f_sourcesink.o: in function `sourcesinkmod_init': >> drivers/usb/gadget/function/f_sourcesink.c:1266: multiple definition of `init_module'; drivers/usb/gadget/function/f_loopback.o:drivers/usb/gadget/function/f_loopback.c:586: first defined here ld: drivers/usb/gadget/function/f_sourcesink.o: in function `sourcesinkmod_exit': >> drivers/usb/gadget/function/f_sourcesink.c:1266: multiple definition of `cleanup_module'; drivers/usb/gadget/function/f_loopback.o:drivers/usb/gadget/function/f_loopback.c:586: first defined here vim +1266 drivers/usb/gadget/function/f_sourcesink.c 1245 1246 static struct usb_function_instance *source_sink_alloc_inst(void) 1247 { 1248 struct f_ss_opts *ss_opts; 1249 1250 ss_opts = kzalloc(sizeof(*ss_opts), GFP_KERNEL); 1251 if (!ss_opts) 1252 return ERR_PTR(-ENOMEM); 1253 mutex_init(&ss_opts->lock); 1254 ss_opts->func_inst.free_func_inst = source_sink_free_instance; 1255 ss_opts->isoc_interval = GZERO_ISOC_INTERVAL; 1256 ss_opts->isoc_maxpacket = GZERO_ISOC_MAXPACKET; 1257 ss_opts->bulk_buflen = GZERO_BULK_BUFLEN; 1258 ss_opts->bulk_qlen = GZERO_SS_BULK_QLEN; 1259 ss_opts->iso_qlen = GZERO_SS_ISO_QLEN; 1260 1261 config_group_init_type_name(&ss_opts->func_inst.group, "", 1262 &ss_func_type); 1263 1264 return &ss_opts->func_inst; 1265 } > 1266 DECLARE_USB_FUNCTION_INIT(sourcesink, source_sink_alloc_inst,
diff --git a/drivers/usb/gadget/function/f_loopback.c b/drivers/usb/gadget/function/f_loopback.c index ae41f556eb75..45f542b5ff55 100644 --- a/drivers/usb/gadget/function/f_loopback.c +++ b/drivers/usb/gadget/function/f_loopback.c @@ -583,16 +583,5 @@ static struct usb_function_instance *loopback_alloc_instance(void) return &lb_opts->func_inst; } -DECLARE_USB_FUNCTION(Loopback, loopback_alloc_instance, loopback_alloc); - -int __init lb_modinit(void) -{ - return usb_function_register(&Loopbackusb_func); -} - -void __exit lb_modexit(void) -{ - usb_function_unregister(&Loopbackusb_func); -} - +DECLARE_USB_FUNCTION_INIT(loopback, loopback_alloc_instance, loopback_alloc); MODULE_LICENSE("GPL"); diff --git a/drivers/usb/gadget/function/f_sourcesink.c b/drivers/usb/gadget/function/f_sourcesink.c index 6803cd60cc6d..f6d1c095aa2c 100644 --- a/drivers/usb/gadget/function/f_sourcesink.c +++ b/drivers/usb/gadget/function/f_sourcesink.c @@ -858,7 +858,7 @@ static struct usb_function *source_sink_alloc_func( ss->bulk_qlen = ss_opts->bulk_qlen; ss->iso_qlen = ss_opts->iso_qlen; - ss->function.name = "source/sink"; + ss->function.name = "sourcesink"; ss->function.bind = sourcesink_bind; ss->function.set_alt = sourcesink_set_alt; ss->function.get_alt = sourcesink_get_alt; @@ -1263,27 +1263,6 @@ static struct usb_function_instance *source_sink_alloc_inst(void) return &ss_opts->func_inst; } -DECLARE_USB_FUNCTION(SourceSink, source_sink_alloc_inst, +DECLARE_USB_FUNCTION_INIT(sourcesink, source_sink_alloc_inst, source_sink_alloc_func); - -static int __init sslb_modinit(void) -{ - int ret; - - ret = usb_function_register(&SourceSinkusb_func); - if (ret) - return ret; - ret = lb_modinit(); - if (ret) - usb_function_unregister(&SourceSinkusb_func); - return ret; -} -static void __exit sslb_modexit(void) -{ - usb_function_unregister(&SourceSinkusb_func); - lb_modexit(); -} -module_init(sslb_modinit); -module_exit(sslb_modexit); - MODULE_LICENSE("GPL"); diff --git a/drivers/usb/gadget/function/g_zero.h b/drivers/usb/gadget/function/g_zero.h index 98b8462ad538..c1ea28526c73 100644 --- a/drivers/usb/gadget/function/g_zero.h +++ b/drivers/usb/gadget/function/g_zero.h @@ -62,9 +62,6 @@ struct f_lb_opts { int refcnt; }; -void lb_modexit(void); -int lb_modinit(void); - /* common utilities */ void disable_endpoints(struct usb_composite_dev *cdev, struct usb_ep *in, struct usb_ep *out,
Inconsistent function names can cause the USB config FS not work. Signed-off-by: zhouscd <zhouscd@gmail.com> --- drivers/usb/gadget/function/f_loopback.c | 13 +---------- drivers/usb/gadget/function/f_sourcesink.c | 25 ++-------------------- drivers/usb/gadget/function/g_zero.h | 3 --- 3 files changed, 3 insertions(+), 38 deletions(-)