| Message ID | 20190604093258.GB30054@kroah.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | USB: move usb debugfs directory creation to the usb common core | expand |
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 <gregkh@linuxfoundation.org> > > --- > > 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 <gregkh@linuxfoundation.org> 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?
On Tue, Jun 04, 2019 at 03:25:14PM +0300, Felipe Balbi wrote: > > Hi, > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org> > > > > --- > > > > 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 Kroah-Hartman <gregkh@linuxfoundation.org> writes: >> Greg Kroah-Hartman <gregkh@linuxfoundation.org> 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
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 <gregkh@linuxfoundation.org> > > > > --- > > > > 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 <gregkh@linuxfoundation.org> writes: > >> Greg Kroah-Hartman <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org> > > > > > > --- > > > > > > 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
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); } /*
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 <gregkh@linuxfoundation.org> --- This should be the "correct" version of this, Chunfeng, can you test this to verify it works for you?