[v1] USB: gadget: Fix the function name error in sourcesink/loopback.

[PATCH] USB: gadget: Fix the function name error in sourcesink/loopback.

Posted by zhouscd 2 years, 6 months ago

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(-)

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,
-- 
2.25.1

Re: [PATCH] USB: gadget: Fix the function name error in sourcesink/loopback.

Posted by kernel test robot 2 years, 6 months ago

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,

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] USB: gadget: Fix the function name error in sourcesink/loopback.

Posted by kernel test robot 2 years, 6 months ago

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

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] USB: gadget: Fix the function name error in sourcesink/loopback.

Posted by Greg KH 2 years, 6 months ago

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

Re: [PATCH] USB: gadget: Fix the function name error in sourcesink/loopback.

Posted by 周城东 2 years, 6 months ago

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

Re: [PATCH] USB: gadget: Fix the function name error in sourcesink/loopback.

Posted by Greg KH 2 years, 6 months ago

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

Re: [PATCH] USB: gadget: Fix the function name error in sourcesink/loopback.

Posted by chengdong zhou 2 years, 6 months ago

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.

Re: [PATCH] USB: gadget: Fix the function name error in sourcesink/loopback.

Posted by Greg KH 2 years, 6 months ago

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

Re: [PATCH] USB: gadget: Fix the function name error in sourcesink/loopback.

Posted by chengdong zhou 2 years, 6 months ago

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?