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

PATCH Superseded zhouscd 3 participants 9 messages Aug 1, 2023 USB

lore

Share & tools

lore

zhouscd

author

2023-08-01 04:55 UTC #0 lore raw

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

Inconsistent function names can cause the USB config FS not work.

Signed-off-by zhouscd <[email protected]>

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

drivers/usb/gadget/function/f_loopback.c +1 ~~-12~~

index ae41f556eb75..45f542b5ff55 100644
@@ -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");

drivers/usb/gadget/function/f_sourcesink.c +2 ~~-23~~

index 6803cd60cc6d..f6d1c095aa2c 100644
@@ -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");

drivers/usb/gadget/function/g_zero.h +0 -3

index 98b8462ad538..c1ea28526c73 100644
@@ -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,

Signature · 1 line

2.25.1

Greg KH

maintainer

2023-08-01 05:06 UTC #1 lore raw

Quoted 1 line · zhouscd

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?

Quoted 2 lines

Signed-off-by zhouscd <[email protected]>

What commit caused the problem?

And please use your full name for patches.

Quoted 33 lines

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

drivers/usb/gadget/function/f_loopback.c +1 ~~-12~~

index ae41f556eb75..45f542b5ff55 100644
@@ -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");

drivers/usb/gadget/function/f_sourcesink.c +0 -0

index 6803cd60cc6d..f6d1c095aa2c 100644

drivers/usb/gadget/function/f_sourcesink.c static struct usb_function *source_sink_alloc_func( · line 861

Quoted 5 lines

drivers/usb/gadget/function/f_sourcesink.c +1 -1

@@ -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

周城东

author

2023-08-01 06:16 UTC #2 lore raw

Hi, Greg KH

Quoted 2 lines

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.

Quoted 1 line

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?

Quoted 2 lines

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.

Quoted 60 lines · Greg KH <[email protected]> 于2023年8月1日周二 13:06写道：

Quoted 1 line · zhouscd
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?
Quoted 2 lines
Signed-off-by zhouscd <[email protected]>
What commit caused the problem?
And please use your full name for patches.
Quoted 33 lines
---
 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(-)
drivers/usb/gadget/function/f_loopback.c +1 ~~-12~~
index ae41f556eb75..45f542b5ff55 100644
@@ -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");
drivers/usb/gadget/function/f_sourcesink.c +0 -0
index 6803cd60cc6d..f6d1c095aa2c 100644
drivers/usb/gadget/function/f_sourcesink.c static struct usb_function *source_sink_alloc_func( · line 861
Quoted 5 lines
drivers/usb/gadget/function/f_sourcesink.c +1 -1
@@ -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

Greg KH

maintainer

2023-08-01 06:24 UTC #3 lore raw

Quoted 9 lines · 周城东

Hi, Greg KH
Quoted 2 lines
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.

Quoted 5 lines

Quoted 1 line
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.

Quoted 9 lines

Quoted 2 lines
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:

Quoted 2 lines

Quoted 2 lines
drivers/usb/gadget/function/f_sourcesink.c static struct usb_function *source_sink_alloc_func( · line 861
Quoted 2 lines
drivers/usb/gadget/function/f_sourcesink.c +1 -1
-     ss->function.name = "source/sink";
+     ss->function.name = "sourcesink";

isn't that visable to userspace?

thanks,

greg k-h

chengdong zhou

author

2023-08-01 07:27 UTC #4 lore raw

Thank you for your patient response.

Quoted 3 lines · Greg KH

Please document this in the changelog text.

But I can't find the changelog text anywhere.

Quoted 8 lines

But you changed the name:
Quoted 2 lines
Quoted 2 lines
drivers/usb/gadget/function/f_sourcesink.c static struct usb_function *source_sink_alloc_func( · line 861
Quoted 2 lines
drivers/usb/gadget/function/f_sourcesink.c +1 -1
-     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.

Greg KH

maintainer

2023-08-01 07:35 UTC #5 lore raw

Quoted 8 lines · chengdong zhou

Thank you for your patient response.
Quoted 3 lines · Greg KH
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.

Quoted 5 lines

Quoted 5 lines
But you changed the name:
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
drivers/usb/gadget/function/f_sourcesink.c +1 -1
-     ss->function.name = "source/sink";
+     ss->function.name = "sourcesink";

Quoted 7 lines

Quoted 1 line
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

chengdong zhou

author

2023-08-01 08:00 UTC #6 lore raw

Quoted 9 lines · Greg KH

Quoted 9 lines
Quoted 8 lines
But you changed the name:
Quoted 2 lines
Quoted 2 lines
drivers/usb/gadget/function/f_sourcesink.c static struct usb_function *source_sink_alloc_func( · line 861
Quoted 2 lines
drivers/usb/gadget/function/f_sourcesink.c +1 -1
-     ss->function.name = "source/sink";
+     ss->function.name = "sourcesink";
isn't that visable to userspace?

Quoted 9 lines

Quoted 5 lines
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?

kernel test robot

build bot

2023-08-01 14:59 UTC #7 lore raw

In reply to zhouscd

kernel test robot noticed the following build errors:

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/[email protected]/config)
compiler: sparc64-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230801/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

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

Signature · 2 lines

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

kernel test robot

build bot

2023-08-01 21:39 UTC #8 lore raw

In reply to zhouscd

kernel test robot noticed the following build errors:

Hi zhouscd,

kernel test robot noticed the following build errors:

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/[email protected]/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230802/[email protected]/reproduce)

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,

Signature · 2 lines

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