The USB gadget subsystem wants to use the USB debugfs root directory, so
move it to the common "core" USB code so that it is properly initialized
and removed as needed.
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
This should be the "correct" version of this, Chunfeng, can you test
this to verify it works for you?
diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
index 18f5dcf58b0d..3b5e4263ffef 100644
--- a/drivers/usb/common/common.c
+++ b/drivers/usb/common/common.c
@@ -15,6 +15,7 @@
#include <linux/usb/of.h>
#include <linux/usb/otg.h>
#include <linux/of_platform.h>
+#include <linux/debugfs.h>
static const char *const ep_type_names[] = {
[USB_ENDPOINT_XFER_CONTROL] = "ctrl",
@@ -291,4 +292,21 @@ struct device *usb_of_get_companion_dev(struct device *dev)
EXPORT_SYMBOL_GPL(usb_of_get_companion_dev);
#endif
+struct dentry *usb_debug_root;
+EXPORT_SYMBOL_GPL(usb_debug_root);
+
+static int usb_common_init(void)
+{
+ usb_debug_root = debugfs_create_dir("usb", NULL);
+ return 0;
+}
+
+static void usb_common_exit(void)
+{
+ debugfs_remove_recursive(usb_debug_root);
+}
+
+module_init(usb_common_init);
+module_exit(usb_common_exit);
+
MODULE_LICENSE("GPL");
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 7fcb9f782931..f3d6b1ab80cb 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -1185,19 +1185,17 @@ static struct notifier_block usb_bus_nb = {
.notifier_call = usb_bus_notify,
};
-struct dentry *usb_debug_root;
-EXPORT_SYMBOL_GPL(usb_debug_root);
+static struct dentry *usb_devices_root;
static void usb_debugfs_init(void)
{
- usb_debug_root = debugfs_create_dir("usb", NULL);
- debugfs_create_file("devices", 0444, usb_debug_root, NULL,
- &usbfs_devices_fops);
+ usb_devices_root = debugfs_create_file("devices", 0444, usb_debug_root,
+ NULL, &usbfs_devices_fops);
}
static void usb_debugfs_cleanup(void)
{
- debugfs_remove_recursive(usb_debug_root);
+ debugfs_remove_recursive(usb_devices_root);
}
/*
On Tue, Jun 04, 2019 at 11:32:58AM +0200, Greg Kroah-Hartman wrote:
> The USB gadget subsystem wants to use the USB debugfs root directory, so
> move it to the common "core" USB code so that it is properly initialized
> and removed as needed.
>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
>
> ---
>
> This should be the "correct" version of this, Chunfeng, can you test
> this to verify it works for you?
>
>
> diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
> index 18f5dcf58b0d..3b5e4263ffef 100644
> --- a/drivers/usb/common/common.c
> +++ b/drivers/usb/common/common.c
> @@ -15,6 +15,7 @@
> #include <linux/usb/of.h>
> #include <linux/usb/otg.h>
> #include <linux/of_platform.h>
> +#include <linux/debugfs.h>
>
> static const char *const ep_type_names[] = {
> [USB_ENDPOINT_XFER_CONTROL] = "ctrl",
> @@ -291,4 +292,21 @@ struct device *usb_of_get_companion_dev(struct device *dev)
> EXPORT_SYMBOL_GPL(usb_of_get_companion_dev);
> #endif
>
> +struct dentry *usb_debug_root;
> +EXPORT_SYMBOL_GPL(usb_debug_root);
> +
> +static int usb_common_init(void)
> +{
> + usb_debug_root = debugfs_create_dir("usb", NULL);
> + return 0;
> +}
> +
> +static void usb_common_exit(void)
> +{
> + debugfs_remove_recursive(usb_debug_root);
> +}
> +
> +module_init(usb_common_init);
> +module_exit(usb_common_exit);
> +
> MODULE_LICENSE("GPL");
> diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> index 7fcb9f782931..f3d6b1ab80cb 100644
> --- a/drivers/usb/core/usb.c
> +++ b/drivers/usb/core/usb.c
> @@ -1185,19 +1185,17 @@ static struct notifier_block usb_bus_nb = {
> .notifier_call = usb_bus_notify,
> };
>
> -struct dentry *usb_debug_root;
> -EXPORT_SYMBOL_GPL(usb_debug_root);
> +static struct dentry *usb_devices_root;
>
> static void usb_debugfs_init(void)
> {
> - usb_debug_root = debugfs_create_dir("usb", NULL);
> - debugfs_create_file("devices", 0444, usb_debug_root, NULL,
> - &usbfs_devices_fops);
> + usb_devices_root = debugfs_create_file("devices", 0444, usb_debug_root,
> + NULL, &usbfs_devices_fops);
> }
>
> static void usb_debugfs_cleanup(void)
> {
> - debugfs_remove_recursive(usb_debug_root);
> + debugfs_remove_recursive(usb_devices_root);
That should just be debugfs_remove();
I'll fix it up after someone tests this :)
thanks,
greg k-h
Hi,
Greg Kroah-Hartman <[email protected]> writes:
> diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> index 7fcb9f782931..f3d6b1ab80cb 100644
> --- a/drivers/usb/core/usb.c
> +++ b/drivers/usb/core/usb.c
> @@ -1185,19 +1185,17 @@ static struct notifier_block usb_bus_nb = {
> .notifier_call = usb_bus_notify,
> };
>
> -struct dentry *usb_debug_root;
> -EXPORT_SYMBOL_GPL(usb_debug_root);
> +static struct dentry *usb_devices_root;
>
> static void usb_debugfs_init(void)
> {
> - usb_debug_root = debugfs_create_dir("usb", NULL);
> - debugfs_create_file("devices", 0444, usb_debug_root, NULL,
> - &usbfs_devices_fops);
> + usb_devices_root = debugfs_create_file("devices", 0444, usb_debug_root,
don't we have a race now? Can usbcore ever probe before usb common?
--
balbi
On Tue, Jun 04, 2019 at 03:25:14PM +0300, Felipe Balbi wrote:
>
> Hi,
>
> Greg Kroah-Hartman <[email protected]> writes:
> > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> > index 7fcb9f782931..f3d6b1ab80cb 100644
> > --- a/drivers/usb/core/usb.c
> > +++ b/drivers/usb/core/usb.c
> > @@ -1185,19 +1185,17 @@ static struct notifier_block usb_bus_nb = {
> > .notifier_call = usb_bus_notify,
> > };
> >
> > -struct dentry *usb_debug_root;
> > -EXPORT_SYMBOL_GPL(usb_debug_root);
> > +static struct dentry *usb_devices_root;
> >
> > static void usb_debugfs_init(void)
> > {
> > - usb_debug_root = debugfs_create_dir("usb", NULL);
> > - debugfs_create_file("devices", 0444, usb_debug_root, NULL,
> > - &usbfs_devices_fops);
> > + usb_devices_root = debugfs_create_file("devices", 0444, usb_debug_root,
>
> don't we have a race now? Can usbcore ever probe before usb common?
How can that happen if usb_debug_root is in usb common? The module
loader will not let that happen. Or it shouldn't :)
thanks,
greg k-h
On Tue, 2019-06-04 at 13:59 +0200, Greg Kroah-Hartman wrote:
> On Tue, Jun 04, 2019 at 11:32:58AM +0200, Greg Kroah-Hartman wrote:
> > The USB gadget subsystem wants to use the USB debugfs root directory, so
> > move it to the common "core" USB code so that it is properly initialized
> > and removed as needed.
> >
> > Signed-off-by: Greg Kroah-Hartman <[email protected]>
> >
> > ---
> >
> > This should be the "correct" version of this, Chunfeng, can you test
> > this to verify it works for you?
I'll test it ASAP, thanks a lot
> >
> >
> > diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
> > index 18f5dcf58b0d..3b5e4263ffef 100644
> > --- a/drivers/usb/common/common.c
> > +++ b/drivers/usb/common/common.c
> > @@ -15,6 +15,7 @@
> > #include <linux/usb/of.h>
> > #include <linux/usb/otg.h>
> > #include <linux/of_platform.h>
> > +#include <linux/debugfs.h>
> >
> > static const char *const ep_type_names[] = {
> > [USB_ENDPOINT_XFER_CONTROL] = "ctrl",
> > @@ -291,4 +292,21 @@ struct device *usb_of_get_companion_dev(struct device *dev)
> > EXPORT_SYMBOL_GPL(usb_of_get_companion_dev);
> > #endif
> >
> > +struct dentry *usb_debug_root;
> > +EXPORT_SYMBOL_GPL(usb_debug_root);
> > +
> > +static int usb_common_init(void)
> > +{
> > + usb_debug_root = debugfs_create_dir("usb", NULL);
> > + return 0;
> > +}
> > +
> > +static void usb_common_exit(void)
> > +{
> > + debugfs_remove_recursive(usb_debug_root);
> > +}
> > +
> > +module_init(usb_common_init);
> > +module_exit(usb_common_exit);
> > +
> > MODULE_LICENSE("GPL");
> > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> > index 7fcb9f782931..f3d6b1ab80cb 100644
> > --- a/drivers/usb/core/usb.c
> > +++ b/drivers/usb/core/usb.c
> > @@ -1185,19 +1185,17 @@ static struct notifier_block usb_bus_nb = {
> > .notifier_call = usb_bus_notify,
> > };
> >
> > -struct dentry *usb_debug_root;
> > -EXPORT_SYMBOL_GPL(usb_debug_root);
> > +static struct dentry *usb_devices_root;
> >
> > static void usb_debugfs_init(void)
> > {
> > - usb_debug_root = debugfs_create_dir("usb", NULL);
> > - debugfs_create_file("devices", 0444, usb_debug_root, NULL,
> > - &usbfs_devices_fops);
> > + usb_devices_root = debugfs_create_file("devices", 0444, usb_debug_root,
> > + NULL, &usbfs_devices_fops);
> > }
> >
> > static void usb_debugfs_cleanup(void)
> > {
> > - debugfs_remove_recursive(usb_debug_root);
> > + debugfs_remove_recursive(usb_devices_root);
>
> That should just be debugfs_remove();
>
> I'll fix it up after someone tests this :)
>
> thanks,
>
> greg k-h
Hi Greg,
I love your patch! Yet something to improve:
[auto build test ERROR on usb/usb-testing]
[also build test ERROR on v5.2-rc3 next-20190604]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Greg-Kroah-Hartman/USB-move-usb-debugfs-directory-creation-to-the-usb-common-core/20190605-114326
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 7.4.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=sh
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>
All errors (new ones prefixed by >>):
drivers/usb/common/led.o: In function `ledtrig_usb_exit':
>> led.c:(.exit.text+0x0): multiple definition of `cleanup_module'
drivers/usb/common/common.o:common.c:(.text+0x518): first defined here
drivers/usb/common/led.o: In function `ledtrig_usb_init':
>> led.c:(.init.text+0x0): multiple definition of `init_module'
drivers/usb/common/common.o:common.c:(.text+0x4dc): first defined here
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi,
Greg Kroah-Hartman <[email protected]> writes:
>> Greg Kroah-Hartman <[email protected]> writes:
>> > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
>> > index 7fcb9f782931..f3d6b1ab80cb 100644
>> > --- a/drivers/usb/core/usb.c
>> > +++ b/drivers/usb/core/usb.c
>> > @@ -1185,19 +1185,17 @@ static struct notifier_block usb_bus_nb = {
>> > .notifier_call = usb_bus_notify,
>> > };
>> >
>> > -struct dentry *usb_debug_root;
>> > -EXPORT_SYMBOL_GPL(usb_debug_root);
>> > +static struct dentry *usb_devices_root;
>> >
>> > static void usb_debugfs_init(void)
>> > {
>> > - usb_debug_root = debugfs_create_dir("usb", NULL);
>> > - debugfs_create_file("devices", 0444, usb_debug_root, NULL,
>> > - &usbfs_devices_fops);
>> > + usb_devices_root = debugfs_create_file("devices", 0444, usb_debug_root,
>>
>> don't we have a race now? Can usbcore ever probe before usb common?
>
> How can that happen if usb_debug_root is in usb common? The module
> loader will not let that happen. Or it shouldn't :)
argh, indeed. The very fact that usbcore tries to resolve usb_debug_root
already forces a dependency :-p
--
balbi
On Tue, 2019-06-04 at 13:59 +0200, Greg Kroah-Hartman wrote:
> On Tue, Jun 04, 2019 at 11:32:58AM +0200, Greg Kroah-Hartman wrote:
> > The USB gadget subsystem wants to use the USB debugfs root directory, so
> > move it to the common "core" USB code so that it is properly initialized
> > and removed as needed.
> >
> > Signed-off-by: Greg Kroah-Hartman <[email protected]>
> >
> > ---
> >
> > This should be the "correct" version of this, Chunfeng, can you test
> > this to verify it works for you?
> >
> >
> > diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
> > index 18f5dcf58b0d..3b5e4263ffef 100644
> > --- a/drivers/usb/common/common.c
> > +++ b/drivers/usb/common/common.c
> > @@ -15,6 +15,7 @@
> > #include <linux/usb/of.h>
> > #include <linux/usb/otg.h>
> > #include <linux/of_platform.h>
> > +#include <linux/debugfs.h>
> >
> > static const char *const ep_type_names[] = {
> > [USB_ENDPOINT_XFER_CONTROL] = "ctrl",
> > @@ -291,4 +292,21 @@ struct device *usb_of_get_companion_dev(struct device *dev)
> > EXPORT_SYMBOL_GPL(usb_of_get_companion_dev);
> > #endif
> >
> > +struct dentry *usb_debug_root;
> > +EXPORT_SYMBOL_GPL(usb_debug_root);
> > +
> > +static int usb_common_init(void)
> > +{
> > + usb_debug_root = debugfs_create_dir("usb", NULL);
> > + return 0;
> > +}
> > +
> > +static void usb_common_exit(void)
> > +{
> > + debugfs_remove_recursive(usb_debug_root);
> > +}
> > +
> > +module_init(usb_common_init);
I tested this patch.
Here use module_init() indeed have a race as Felipe said before.
usbcore uses subsys_initcall(), and have a higher priority than
module_init(), so when usbcore tries to create "devices" file,
usb_debug_root is not created.
after I replace it by postcore_initcall() (debugfs uses
core_initcall()), test two cases:
1. buildin usbcore/udc-core
"usb" directory is created, and usb/devices file is also created by
usbcore
2. build both usbcore and gadget as ko
usbcore.ko, udc-core.ko and usb-common.ko are created.
2.1
insmod usb-common.ko // "usb" directory is created
insmod usb-core.ko // usb/devices file is created
2.2
rmmod usb-common.ko // failed, usb_common is in use by usb-core
2.3
rmmod usb-core.ko // usb/devices file is destroyed
rmmod usb-common.ko // usb directory is destroyed
2.4
insmod usb-common.ko // "usb" directory is created
insmod udc-core.ko
2.5
rmmod usb-common.ko // failed, usb_common is in use by udc-core
2.6
rmmod udc-core.ko
rmmod usb-common.ko // usb directory is destroyed
they are all in line with expectations
> > +module_exit(usb_common_exit);
> > +
> > MODULE_LICENSE("GPL");
> > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> > index 7fcb9f782931..f3d6b1ab80cb 100644
> > --- a/drivers/usb/core/usb.c
> > +++ b/drivers/usb/core/usb.c
> > @@ -1185,19 +1185,17 @@ static struct notifier_block usb_bus_nb = {
> > .notifier_call = usb_bus_notify,
> > };
> >
> > -struct dentry *usb_debug_root;
> > -EXPORT_SYMBOL_GPL(usb_debug_root);
> > +static struct dentry *usb_devices_root;
> >
> > static void usb_debugfs_init(void)
> > {
> > - usb_debug_root = debugfs_create_dir("usb", NULL);
> > - debugfs_create_file("devices", 0444, usb_debug_root, NULL,
> > - &usbfs_devices_fops);
> > + usb_devices_root = debugfs_create_file("devices", 0444, usb_debug_root,
> > + NULL, &usbfs_devices_fops);
> > }
> >
> > static void usb_debugfs_cleanup(void)
> > {
> > - debugfs_remove_recursive(usb_debug_root);
> > + debugfs_remove_recursive(usb_devices_root);
>
> That should just be debugfs_remove();
>
> I'll fix it up after someone tests this :)
>
> thanks,
>
> greg k-h
On Wed, 2019-06-05 at 10:28 +0300, Felipe Balbi wrote:
> Hi,
>
> Greg Kroah-Hartman <[email protected]> writes:
> >> Greg Kroah-Hartman <[email protected]> writes:
> >> > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> >> > index 7fcb9f782931..f3d6b1ab80cb 100644
> >> > --- a/drivers/usb/core/usb.c
> >> > +++ b/drivers/usb/core/usb.c
> >> > @@ -1185,19 +1185,17 @@ static struct notifier_block usb_bus_nb = {
> >> > .notifier_call = usb_bus_notify,
> >> > };
> >> >
> >> > -struct dentry *usb_debug_root;
> >> > -EXPORT_SYMBOL_GPL(usb_debug_root);
> >> > +static struct dentry *usb_devices_root;
> >> >
> >> > static void usb_debugfs_init(void)
> >> > {
> >> > - usb_debug_root = debugfs_create_dir("usb", NULL);
> >> > - debugfs_create_file("devices", 0444, usb_debug_root, NULL,
> >> > - &usbfs_devices_fops);
> >> > + usb_devices_root = debugfs_create_file("devices", 0444, usb_debug_root,
> >>
> >> don't we have a race now? Can usbcore ever probe before usb common?
> >
> > How can that happen if usb_debug_root is in usb common? The module
> > loader will not let that happen. Or it shouldn't :)
>
> argh, indeed. The very fact that usbcore tries to resolve usb_debug_root
> already forces a dependency :-p
When build as module, usbcore depend on usb-common, but when buildin,
usbcore init before usb-common (use module_init)
>
On Wed, Jun 05, 2019 at 03:50:31PM +0800, Chunfeng Yun wrote:
> On Tue, 2019-06-04 at 13:59 +0200, Greg Kroah-Hartman wrote:
> > On Tue, Jun 04, 2019 at 11:32:58AM +0200, Greg Kroah-Hartman wrote:
> > > The USB gadget subsystem wants to use the USB debugfs root directory, so
> > > move it to the common "core" USB code so that it is properly initialized
> > > and removed as needed.
> > >
> > > Signed-off-by: Greg Kroah-Hartman <[email protected]>
> > >
> > > ---
> > >
> > > This should be the "correct" version of this, Chunfeng, can you test
> > > this to verify it works for you?
> > >
> > >
> > > diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
> > > index 18f5dcf58b0d..3b5e4263ffef 100644
> > > --- a/drivers/usb/common/common.c
> > > +++ b/drivers/usb/common/common.c
> > > @@ -15,6 +15,7 @@
> > > #include <linux/usb/of.h>
> > > #include <linux/usb/otg.h>
> > > #include <linux/of_platform.h>
> > > +#include <linux/debugfs.h>
> > >
> > > static const char *const ep_type_names[] = {
> > > [USB_ENDPOINT_XFER_CONTROL] = "ctrl",
> > > @@ -291,4 +292,21 @@ struct device *usb_of_get_companion_dev(struct device *dev)
> > > EXPORT_SYMBOL_GPL(usb_of_get_companion_dev);
> > > #endif
> > >
> > > +struct dentry *usb_debug_root;
> > > +EXPORT_SYMBOL_GPL(usb_debug_root);
> > > +
> > > +static int usb_common_init(void)
> > > +{
> > > + usb_debug_root = debugfs_create_dir("usb", NULL);
> > > + return 0;
> > > +}
> > > +
> > > +static void usb_common_exit(void)
> > > +{
> > > + debugfs_remove_recursive(usb_debug_root);
> > > +}
> > > +
> > > +module_init(usb_common_init);
> I tested this patch.
>
> Here use module_init() indeed have a race as Felipe said before.
> usbcore uses subsys_initcall(), and have a higher priority than
> module_init(), so when usbcore tries to create "devices" file,
> usb_debug_root is not created.
Ah, let me fix that, it should have the same init level and I'll ensure
it comes first in the linking.
> after I replace it by postcore_initcall() (debugfs uses
> core_initcall()), test two cases:
>
> 1. buildin usbcore/udc-core
>
> "usb" directory is created, and usb/devices file is also created by
> usbcore
>
> 2. build both usbcore and gadget as ko
>
> usbcore.ko, udc-core.ko and usb-common.ko are created.
>
> 2.1
> insmod usb-common.ko // "usb" directory is created
> insmod usb-core.ko // usb/devices file is created
>
> 2.2
> rmmod usb-common.ko // failed, usb_common is in use by usb-core
>
> 2.3
> rmmod usb-core.ko // usb/devices file is destroyed
> rmmod usb-common.ko // usb directory is destroyed
>
> 2.4
> insmod usb-common.ko // "usb" directory is created
> insmod udc-core.ko
>
> 2.5
> rmmod usb-common.ko // failed, usb_common is in use by udc-core
>
> 2.6
> rmmod udc-core.ko
> rmmod usb-common.ko // usb directory is destroyed
>
> they are all in line with expectations
Wonderful!
Let me fix up the init level, and the build issue tha kbuild found, and
post a v2 patch.
thanks,
greg k-h