| Message ID | 1559633647-29040-1-git-send-email-chunfeng.yun@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] usb: create usb_debug_root for gadget only | expand |
On Tue, Jun 04, 2019 at 03:34:07PM +0800, Chunfeng Yun wrote: > When CONFIG_USB is not set, and CONFIG_USB_GADGET is set, > there is an issue, e.g.: > > drivers/usb/mtu3/mtu3_debugfs.o: in function 'ssusb_debugfs_create_root': > mtu3_debugfs.c:(.text+0xba3): undefined reference to 'usb_debug_root' > > usb_debug_root is currently only built when host is supported > (CONFIG_USB is set), for convenience, we also want it created when > gadget only is enabled, this patch try to support it. > > Reported-by: Randy Dunlap <rdunlap@infradead.org> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> > --- > v4: > move common API into common.c suggested by Felipe > > v3: > 1. still create usb_debug_root for gadget only > 2. abandon mtu3's change > 3. drop acked-by Randy > > v2(resend): add acked-by Randy > > v1: fix mtu3's build error, replace usb_debug_root by NULL; > --- > drivers/usb/common/common.c | 26 ++++++++++++++++++++++++++ > drivers/usb/core/usb.c | 16 ++++------------ > drivers/usb/gadget/udc/core.c | 3 +++ > include/linux/usb.h | 2 ++ > 4 files changed, 35 insertions(+), 12 deletions(-) > > diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c > index 18f5dcf58b0d..c52e9c9f58ec 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", > @@ -139,6 +140,31 @@ enum usb_dr_mode usb_get_dr_mode(struct device *dev) > } > EXPORT_SYMBOL_GPL(usb_get_dr_mode); > > +struct dentry *usb_debug_root; > +EXPORT_SYMBOL_GPL(usb_debug_root); > + > +static atomic_t usb_debug_root_refcnt = ATOMIC_INIT(0); Ick, no. > + > +struct dentry *usb_debugfs_init(void) > +{ > + if (!usb_debug_root) > + usb_debug_root = debugfs_create_dir("usb", NULL); > + > + atomic_inc(&usb_debug_root_refcnt); > + > + return usb_debug_root; > +} > +EXPORT_SYMBOL_GPL(usb_debugfs_init); > + > +void usb_debugfs_cleanup(void) > +{ > + if (atomic_dec_and_test(&usb_debug_root_refcnt)) { > + debugfs_remove_recursive(usb_debug_root); > + usb_debug_root = NULL; > + } > +} > +EXPORT_SYMBOL_GPL(usb_debugfs_cleanup); Only remove the debugfs subdir if the usbcore module is removed. Create the debugfs subdir when the usbcore module is loaded. No need for any reference counting of any sort at all. No need to overthink this :) thanks, greg k-h
Hi, Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes: >> +struct dentry *usb_debugfs_init(void) >> +{ >> + if (!usb_debug_root) >> + usb_debug_root = debugfs_create_dir("usb", NULL); >> + >> + atomic_inc(&usb_debug_root_refcnt); >> + >> + return usb_debug_root; >> +} >> +EXPORT_SYMBOL_GPL(usb_debugfs_init); >> + >> +void usb_debugfs_cleanup(void) >> +{ >> + if (atomic_dec_and_test(&usb_debug_root_refcnt)) { >> + debugfs_remove_recursive(usb_debug_root); >> + usb_debug_root = NULL; >> + } >> +} >> +EXPORT_SYMBOL_GPL(usb_debugfs_cleanup); > > Only remove the debugfs subdir if the usbcore module is removed. Create > the debugfs subdir when the usbcore module is loaded. No need for any > reference counting of any sort at all. No need to overthink this :) There is a slight need to overthink. He wants to use the same directory for gadget-only builds too :-)
On Tue, Jun 04, 2019 at 10:47:55AM +0300, Felipe Balbi wrote: > > Hi, > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes: > >> +struct dentry *usb_debugfs_init(void) > >> +{ > >> + if (!usb_debug_root) > >> + usb_debug_root = debugfs_create_dir("usb", NULL); > >> + > >> + atomic_inc(&usb_debug_root_refcnt); > >> + > >> + return usb_debug_root; > >> +} > >> +EXPORT_SYMBOL_GPL(usb_debugfs_init); > >> + > >> +void usb_debugfs_cleanup(void) > >> +{ > >> + if (atomic_dec_and_test(&usb_debug_root_refcnt)) { > >> + debugfs_remove_recursive(usb_debug_root); > >> + usb_debug_root = NULL; > >> + } > >> +} > >> +EXPORT_SYMBOL_GPL(usb_debugfs_cleanup); > > > > Only remove the debugfs subdir if the usbcore module is removed. Create > > the debugfs subdir when the usbcore module is loaded. No need for any > > reference counting of any sort at all. No need to overthink this :) > > There is a slight need to overthink. He wants to use the same directory > for gadget-only builds too :-) Again, that's fine, this file will be loaded for those builds as well, right? Otherwise, how would this code even be present? :) thanks, greg k-h
On Tue, 2019-06-04 at 09:37 +0200, Greg Kroah-Hartman wrote: > On Tue, Jun 04, 2019 at 03:34:07PM +0800, Chunfeng Yun wrote: > > When CONFIG_USB is not set, and CONFIG_USB_GADGET is set, > > there is an issue, e.g.: > > > > drivers/usb/mtu3/mtu3_debugfs.o: in function 'ssusb_debugfs_create_root': > > mtu3_debugfs.c:(.text+0xba3): undefined reference to 'usb_debug_root' > > > > usb_debug_root is currently only built when host is supported > > (CONFIG_USB is set), for convenience, we also want it created when > > gadget only is enabled, this patch try to support it. > > > > Reported-by: Randy Dunlap <rdunlap@infradead.org> > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> > > --- > > v4: > > move common API into common.c suggested by Felipe > > > > v3: > > 1. still create usb_debug_root for gadget only > > 2. abandon mtu3's change > > 3. drop acked-by Randy > > > > v2(resend): add acked-by Randy > > > > v1: fix mtu3's build error, replace usb_debug_root by NULL; > > --- > > drivers/usb/common/common.c | 26 ++++++++++++++++++++++++++ > > drivers/usb/core/usb.c | 16 ++++------------ > > drivers/usb/gadget/udc/core.c | 3 +++ > > include/linux/usb.h | 2 ++ > > 4 files changed, 35 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c > > index 18f5dcf58b0d..c52e9c9f58ec 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", > > @@ -139,6 +140,31 @@ enum usb_dr_mode usb_get_dr_mode(struct device *dev) > > } > > EXPORT_SYMBOL_GPL(usb_get_dr_mode); > > > > +struct dentry *usb_debug_root; > > +EXPORT_SYMBOL_GPL(usb_debug_root); > > + > > +static atomic_t usb_debug_root_refcnt = ATOMIC_INIT(0); > > Ick, no. > > > + > > +struct dentry *usb_debugfs_init(void) > > +{ > > + if (!usb_debug_root) > > + usb_debug_root = debugfs_create_dir("usb", NULL); > > + > > + atomic_inc(&usb_debug_root_refcnt); > > + > > + return usb_debug_root; > > +} > > +EXPORT_SYMBOL_GPL(usb_debugfs_init); > > + > > +void usb_debugfs_cleanup(void) > > +{ > > + if (atomic_dec_and_test(&usb_debug_root_refcnt)) { > > + debugfs_remove_recursive(usb_debug_root); > > + usb_debug_root = NULL; > > + } > > +} > > +EXPORT_SYMBOL_GPL(usb_debugfs_cleanup); > > Only remove the debugfs subdir if the usbcore module is removed. Both usbcore module and gadget module will use this "usb" subdir now. Gadget module may still use it when remove usbcore module. > Create > the debugfs subdir when the usbcore module is loaded. No need for any > reference counting of any sort at all. No need to overthink this :) > > thanks, > > greg k-h
On Tue, 2019-06-04 at 10:24 +0200, Greg Kroah-Hartman wrote: > On Tue, Jun 04, 2019 at 10:47:55AM +0300, Felipe Balbi wrote: > > > > Hi, > > > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes: > > >> +struct dentry *usb_debugfs_init(void) > > >> +{ > > >> + if (!usb_debug_root) > > >> + usb_debug_root = debugfs_create_dir("usb", NULL); > > >> + > > >> + atomic_inc(&usb_debug_root_refcnt); > > >> + > > >> + return usb_debug_root; > > >> +} > > >> +EXPORT_SYMBOL_GPL(usb_debugfs_init); > > >> + > > >> +void usb_debugfs_cleanup(void) > > >> +{ > > >> + if (atomic_dec_and_test(&usb_debug_root_refcnt)) { > > >> + debugfs_remove_recursive(usb_debug_root); > > >> + usb_debug_root = NULL; > > >> + } > > >> +} > > >> +EXPORT_SYMBOL_GPL(usb_debugfs_cleanup); > > > > > > Only remove the debugfs subdir if the usbcore module is removed. Create > > > the debugfs subdir when the usbcore module is loaded. No need for any > > > reference counting of any sort at all. No need to overthink this :) > > > > There is a slight need to overthink. He wants to use the same directory > > for gadget-only builds too :-) > > Again, that's fine, this file will be loaded for those builds as well, > right? Yes, either usbcore or gadget will select this file. > Otherwise, how would this code even be present? :) > > thanks, > > greg k-h
On Tue, Jun 04, 2019 at 04:59:14PM +0800, Chunfeng Yun wrote: > On Tue, 2019-06-04 at 09:37 +0200, Greg Kroah-Hartman wrote: > > On Tue, Jun 04, 2019 at 03:34:07PM +0800, Chunfeng Yun wrote: > > > When CONFIG_USB is not set, and CONFIG_USB_GADGET is set, > > > there is an issue, e.g.: > > > > > > drivers/usb/mtu3/mtu3_debugfs.o: in function 'ssusb_debugfs_create_root': > > > mtu3_debugfs.c:(.text+0xba3): undefined reference to 'usb_debug_root' > > > > > > usb_debug_root is currently only built when host is supported > > > (CONFIG_USB is set), for convenience, we also want it created when > > > gadget only is enabled, this patch try to support it. > > > > > > Reported-by: Randy Dunlap <rdunlap@infradead.org> > > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> > > > --- > > > v4: > > > move common API into common.c suggested by Felipe > > > > > > v3: > > > 1. still create usb_debug_root for gadget only > > > 2. abandon mtu3's change > > > 3. drop acked-by Randy > > > > > > v2(resend): add acked-by Randy > > > > > > v1: fix mtu3's build error, replace usb_debug_root by NULL; > > > --- > > > drivers/usb/common/common.c | 26 ++++++++++++++++++++++++++ > > > drivers/usb/core/usb.c | 16 ++++------------ > > > drivers/usb/gadget/udc/core.c | 3 +++ > > > include/linux/usb.h | 2 ++ > > > 4 files changed, 35 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c > > > index 18f5dcf58b0d..c52e9c9f58ec 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", > > > @@ -139,6 +140,31 @@ enum usb_dr_mode usb_get_dr_mode(struct device *dev) > > > } > > > EXPORT_SYMBOL_GPL(usb_get_dr_mode); > > > > > > +struct dentry *usb_debug_root; > > > +EXPORT_SYMBOL_GPL(usb_debug_root); > > > + > > > +static atomic_t usb_debug_root_refcnt = ATOMIC_INIT(0); > > > > Ick, no. > > > > > + > > > +struct dentry *usb_debugfs_init(void) > > > +{ > > > + if (!usb_debug_root) > > > + usb_debug_root = debugfs_create_dir("usb", NULL); > > > + > > > + atomic_inc(&usb_debug_root_refcnt); > > > + > > > + return usb_debug_root; > > > +} > > > +EXPORT_SYMBOL_GPL(usb_debugfs_init); > > > + > > > +void usb_debugfs_cleanup(void) > > > +{ > > > + if (atomic_dec_and_test(&usb_debug_root_refcnt)) { > > > + debugfs_remove_recursive(usb_debug_root); > > > + usb_debug_root = NULL; > > > + } > > > +} > > > +EXPORT_SYMBOL_GPL(usb_debugfs_cleanup); > > > > Only remove the debugfs subdir if the usbcore module is removed. > Both usbcore module and gadget module will use this "usb" subdir now. > Gadget module may still use it when remove usbcore module. Did you try to remove the usb-common module with the udc gadget module remaining loaded with this change in place? I think you will find that it is impossible given that the udc gadget module now depends on a symbol in the usb-common code. So again, just use the module reference counting logic to drive this functionality, don't create an atomic variable that duplicates the logic we already have in place today. thanks, greg k-h
On Tue, Jun 04, 2019 at 10:24:07AM +0200, Greg Kroah-Hartman wrote: > On Tue, Jun 04, 2019 at 10:47:55AM +0300, Felipe Balbi wrote: > > > > Hi, > > > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes: > > >> +struct dentry *usb_debugfs_init(void) > > >> +{ > > >> + if (!usb_debug_root) > > >> + usb_debug_root = debugfs_create_dir("usb", NULL); > > >> + > > >> + atomic_inc(&usb_debug_root_refcnt); > > >> + > > >> + return usb_debug_root; > > >> +} > > >> +EXPORT_SYMBOL_GPL(usb_debugfs_init); > > >> + > > >> +void usb_debugfs_cleanup(void) > > >> +{ > > >> + if (atomic_dec_and_test(&usb_debug_root_refcnt)) { > > >> + debugfs_remove_recursive(usb_debug_root); > > >> + usb_debug_root = NULL; > > >> + } > > >> +} > > >> +EXPORT_SYMBOL_GPL(usb_debugfs_cleanup); > > > > > > Only remove the debugfs subdir if the usbcore module is removed. Create > > > the debugfs subdir when the usbcore module is loaded. No need for any > > > reference counting of any sort at all. No need to overthink this :) > > > > There is a slight need to overthink. He wants to use the same directory > > for gadget-only builds too :-) > > Again, that's fine, this file will be loaded for those builds as well, > right? Otherwise, how would this code even be present? :) As it seems to be easier to just write the patch instead of trying to describe it in email, and the patch is even simpler than the text, here's what I was thinking of: Note, it's not fully correct, now that I think of it, but you get the idea... From foo@baz Tue 04 Jun 2019 11:25:30 AM CEST Date: Tue, 04 Jun 2019 11:25:30 +0200 To: Greg KH <gregkh@linuxfoundation.org> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Subject: [PATCH] USB: move usb debugfs directory creation to the usb core 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> 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..2aa28445277d 100644 --- a/drivers/usb/core/usb.c +++ b/drivers/usb/core/usb.c @@ -1185,12 +1185,8 @@ static struct notifier_block usb_bus_nb = { .notifier_call = usb_bus_notify, }; -struct dentry *usb_debug_root; -EXPORT_SYMBOL_GPL(usb_debug_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); }
diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c index 18f5dcf58b0d..c52e9c9f58ec 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", @@ -139,6 +140,31 @@ enum usb_dr_mode usb_get_dr_mode(struct device *dev) } EXPORT_SYMBOL_GPL(usb_get_dr_mode); +struct dentry *usb_debug_root; +EXPORT_SYMBOL_GPL(usb_debug_root); + +static atomic_t usb_debug_root_refcnt = ATOMIC_INIT(0); + +struct dentry *usb_debugfs_init(void) +{ + if (!usb_debug_root) + usb_debug_root = debugfs_create_dir("usb", NULL); + + atomic_inc(&usb_debug_root_refcnt); + + return usb_debug_root; +} +EXPORT_SYMBOL_GPL(usb_debugfs_init); + +void usb_debugfs_cleanup(void) +{ + if (atomic_dec_and_test(&usb_debug_root_refcnt)) { + debugfs_remove_recursive(usb_debug_root); + usb_debug_root = NULL; + } +} +EXPORT_SYMBOL_GPL(usb_debugfs_cleanup); + #ifdef CONFIG_OF /** * of_usb_get_dr_mode_by_phy - Get dual role mode for the controller device diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c index 7fcb9f782931..9d8db7faf75f 100644 --- a/drivers/usb/core/usb.c +++ b/drivers/usb/core/usb.c @@ -1185,19 +1185,11 @@ static struct notifier_block usb_bus_nb = { .notifier_call = usb_bus_notify, }; -struct dentry *usb_debug_root; -EXPORT_SYMBOL_GPL(usb_debug_root); - -static void usb_debugfs_init(void) +static void usb_core_debugfs_init(void) { - usb_debug_root = debugfs_create_dir("usb", NULL); - debugfs_create_file("devices", 0444, usb_debug_root, NULL, - &usbfs_devices_fops); -} + struct dentry *root = usb_debugfs_init(); -static void usb_debugfs_cleanup(void) -{ - debugfs_remove_recursive(usb_debug_root); + debugfs_create_file("devices", 0444, root, NULL, &usbfs_devices_fops); } /* @@ -1212,7 +1204,7 @@ static int __init usb_init(void) } usb_init_pool_max(); - usb_debugfs_init(); + usb_core_debugfs_init(); usb_acpi_register(); retval = bus_register(&usb_bus_type); diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c index 7cf34beb50df..8ef000b9baa4 100644 --- a/drivers/usb/gadget/udc/core.c +++ b/drivers/usb/gadget/udc/core.c @@ -1597,12 +1597,15 @@ static int __init usb_udc_init(void) } udc_class->dev_uevent = usb_udc_uevent; + usb_debugfs_init(); + return 0; } subsys_initcall(usb_udc_init); static void __exit usb_udc_exit(void) { + usb_debugfs_cleanup(); class_destroy(udc_class); } module_exit(usb_udc_exit); diff --git a/include/linux/usb.h b/include/linux/usb.h index ae82d9d1112b..175e4a0b1847 100644 --- a/include/linux/usb.h +++ b/include/linux/usb.h @@ -1995,6 +1995,8 @@ extern void usb_unregister_notify(struct notifier_block *nb); /* debugfs stuff */ extern struct dentry *usb_debug_root; +extern struct dentry *usb_debugfs_init(void); +extern void usb_debugfs_cleanup(void); /* LED triggers */ enum usb_led_event {
When CONFIG_USB is not set, and CONFIG_USB_GADGET is set, there is an issue, e.g.: drivers/usb/mtu3/mtu3_debugfs.o: in function 'ssusb_debugfs_create_root': mtu3_debugfs.c:(.text+0xba3): undefined reference to 'usb_debug_root' usb_debug_root is currently only built when host is supported (CONFIG_USB is set), for convenience, we also want it created when gadget only is enabled, this patch try to support it. Reported-by: Randy Dunlap <rdunlap@infradead.org> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> --- v4: move common API into common.c suggested by Felipe v3: 1. still create usb_debug_root for gadget only 2. abandon mtu3's change 3. drop acked-by Randy v2(resend): add acked-by Randy v1: fix mtu3's build error, replace usb_debug_root by NULL; --- drivers/usb/common/common.c | 26 ++++++++++++++++++++++++++ drivers/usb/core/usb.c | 16 ++++------------ drivers/usb/gadget/udc/core.c | 3 +++ include/linux/usb.h | 2 ++ 4 files changed, 35 insertions(+), 12 deletions(-)