| Message ID | cffd6d75f69e4d908c8f39b8a60ddae27d6b7c88.1559028752.git.chunfeng.yun@mediatek.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] usb: create usb_debug_root for gadget only | expand |
Hi, Chunfeng Yun <chunfeng.yun@mediatek.com> writes: > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c > index 7fcb9f782931..88b3ee03a12d 100644 > --- a/drivers/usb/core/usb.c > +++ b/drivers/usb/core/usb.c > @@ -1190,7 +1190,7 @@ EXPORT_SYMBOL_GPL(usb_debug_root); > > static void usb_debugfs_init(void) > { > - usb_debug_root = debugfs_create_dir("usb", NULL); > + usb_debug_root = debugfs_create_dir(USB_DEBUG_ROOT_NAME, NULL); > debugfs_create_file("devices", 0444, usb_debug_root, NULL, > &usbfs_devices_fops); > } might be a better idea to move this to usb common. Then have a function which can be called by both host and gadget to maybe create the directory: static struct dentry *usb_debug_root; struct dentry *usb_debugfs_init(void) { if (!usb_debug_root) usb_debug_root = debugfs_create_dir("usb", NULL); return usb_debug_root; } Then usb core would be updated to something like: static void usb_core_debugfs_init(void) { struct dentry *root = usb_debugfs_init(); debugfs_create_file("devices", 0444, root, NULL, &usbfs_devices_fops); }
add Felipe, sorry On Tue, 2019-05-28 at 15:54 +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> > --- > 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/core/usb.c | 2 +- > drivers/usb/gadget/udc/core.c | 27 +++++++++++++++++++++++++++ > include/linux/usb.h | 1 + > 3 files changed, 29 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c > index 7fcb9f782931..88b3ee03a12d 100644 > --- a/drivers/usb/core/usb.c > +++ b/drivers/usb/core/usb.c > @@ -1190,7 +1190,7 @@ EXPORT_SYMBOL_GPL(usb_debug_root); > > static void usb_debugfs_init(void) > { > - usb_debug_root = debugfs_create_dir("usb", NULL); > + usb_debug_root = debugfs_create_dir(USB_DEBUG_ROOT_NAME, NULL); > debugfs_create_file("devices", 0444, usb_debug_root, NULL, > &usbfs_devices_fops); > } > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c > index 7cf34beb50df..ed45f9429e58 100644 > --- a/drivers/usb/gadget/udc/core.c > +++ b/drivers/usb/gadget/udc/core.c > @@ -8,6 +8,7 @@ > > #include <linux/kernel.h> > #include <linux/module.h> > +#include <linux/debugfs.h> > #include <linux/device.h> > #include <linux/list.h> > #include <linux/err.h> > @@ -1587,12 +1588,37 @@ static int usb_udc_uevent(struct device *dev, struct kobj_uevent_env *env) > return 0; > } > > +/* if CONFIG_USB is set, leave USB core to create usb_debug_root */ > +#ifndef CONFIG_USB > +struct dentry *usb_debug_root; > +EXPORT_SYMBOL_GPL(usb_debug_root); > + > +static void usb_debugfs_init(void) > +{ > + usb_debug_root = debugfs_create_dir(USB_DEBUG_ROOT_NAME, NULL); > +} > + > +static void usb_debugfs_cleanup(void) > +{ > + debugfs_remove_recursive(usb_debug_root); > +} > +#else > +static void usb_debugfs_init(void) > +{} > + > +static void usb_debugfs_cleanup(void) > +{} > +#endif > + > static int __init usb_udc_init(void) > { > + usb_debugfs_init(); > + > udc_class = class_create(THIS_MODULE, "udc"); > if (IS_ERR(udc_class)) { > pr_err("failed to create udc class --> %ld\n", > PTR_ERR(udc_class)); > + usb_debugfs_cleanup(); > return PTR_ERR(udc_class); > } > > @@ -1604,6 +1630,7 @@ subsys_initcall(usb_udc_init); > static void __exit usb_udc_exit(void) > { > class_destroy(udc_class); > + usb_debugfs_cleanup(); > } > module_exit(usb_udc_exit); > > diff --git a/include/linux/usb.h b/include/linux/usb.h > index ae82d9d1112b..9c6e7b3265af 100644 > --- a/include/linux/usb.h > +++ b/include/linux/usb.h > @@ -1994,6 +1994,7 @@ extern void usb_register_notify(struct notifier_block *nb); > extern void usb_unregister_notify(struct notifier_block *nb); > > /* debugfs stuff */ > +#define USB_DEBUG_ROOT_NAME "usb" > extern struct dentry *usb_debug_root; > > /* LED triggers */
Hi Felipe, On Tue, 2019-05-28 at 11:11 +0300, Felipe Balbi wrote: > Hi, > > Chunfeng Yun <chunfeng.yun@mediatek.com> writes: > > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c > > index 7fcb9f782931..88b3ee03a12d 100644 > > --- a/drivers/usb/core/usb.c > > +++ b/drivers/usb/core/usb.c > > @@ -1190,7 +1190,7 @@ EXPORT_SYMBOL_GPL(usb_debug_root); > > > > static void usb_debugfs_init(void) > > { > > - usb_debug_root = debugfs_create_dir("usb", NULL); > > + usb_debug_root = debugfs_create_dir(USB_DEBUG_ROOT_NAME, NULL); > > debugfs_create_file("devices", 0444, usb_debug_root, NULL, > > &usbfs_devices_fops); > > } > > might be a better idea to move this to usb common. Good idea, I forgot there is a common file. > Then have a function > which can be called by both host and gadget to maybe create the > directory: I'll try it. Thanks a lot > > static struct dentry *usb_debug_root; > > struct dentry *usb_debugfs_init(void) > { > if (!usb_debug_root) > usb_debug_root = debugfs_create_dir("usb", NULL); > > return usb_debug_root; > } > > > Then usb core would be updated to something like: > > static void usb_core_debugfs_init(void) > { > struct dentry *root = usb_debugfs_init(); > > debugfs_create_file("devices", 0444, root, NULL, &usbfs_devices_fops); > } >
Hi Felipe, On Tue, 2019-05-28 at 11:11 +0300, Felipe Balbi wrote: > Hi, > > Chunfeng Yun <chunfeng.yun@mediatek.com> writes: > > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c > > index 7fcb9f782931..88b3ee03a12d 100644 > > --- a/drivers/usb/core/usb.c > > +++ b/drivers/usb/core/usb.c > > @@ -1190,7 +1190,7 @@ EXPORT_SYMBOL_GPL(usb_debug_root); > > > > static void usb_debugfs_init(void) > > { > > - usb_debug_root = debugfs_create_dir("usb", NULL); > > + usb_debug_root = debugfs_create_dir(USB_DEBUG_ROOT_NAME, NULL); > > debugfs_create_file("devices", 0444, usb_debug_root, NULL, > > &usbfs_devices_fops); > > } > > might be a better idea to move this to usb common. Then have a function > which can be called by both host and gadget to maybe create the > directory: > > static struct dentry *usb_debug_root; > > struct dentry *usb_debugfs_init(void) > { > if (!usb_debug_root) > usb_debug_root = debugfs_create_dir("usb", NULL); > > return usb_debug_root; > } > > > Then usb core would be updated to something like: > > static void usb_core_debugfs_init(void) > { > struct dentry *root = usb_debugfs_init(); > > debugfs_create_file("devices", 0444, root, NULL, &usbfs_devices_fops); > } > I find a problem when move usb_debugfs_init() and usb_debugfs_cleanup() into usb common, it's easy to create "usb" directory, but difficult to cleanup it: common/common.c struct dentry *usb_debugfs_init(void) { if (!usb_debug_root) usb_debug_root = debugfs_create_dir("usb", NULL); return usb_debug_root; } void usb_debugfs_cleanup(void) { debugfs_remove_recursive(usb_debug_root); usb_debug_root = NULL; } core/usb.c static void usb_core_debugfs_init(void) { struct dentry *root = usb_debugfs_init(); debugfs_create_file("devices", 0444, root, NULL, &usbfs_devices_fops); } static int __init usb_init(void) { ... usb_core_debugfs_init(); ... } static void __exit usb_exit(void) { ... usb_debugfs_cleanup(); // will be error, gadget may use it. ... } gadget/udc/core.c static int __init usb_udc_init(void) { ... usb_debugfs_init(); ... } static void __exit usb_udc_exit(void) { ... usb_debugfs_cleanup(); // can't cleanup in fact, usb core may use it. } How to handle this case? introduce a reference count? do you have any suggestion? Thanks a lot
Hi, Chunfeng Yun <chunfeng.yun@mediatek.com> writes: > Hi Felipe, > On Tue, 2019-05-28 at 11:11 +0300, Felipe Balbi wrote: >> Hi, >> >> Chunfeng Yun <chunfeng.yun@mediatek.com> writes: >> > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c >> > index 7fcb9f782931..88b3ee03a12d 100644 >> > --- a/drivers/usb/core/usb.c >> > +++ b/drivers/usb/core/usb.c >> > @@ -1190,7 +1190,7 @@ EXPORT_SYMBOL_GPL(usb_debug_root); >> > >> > static void usb_debugfs_init(void) >> > { >> > - usb_debug_root = debugfs_create_dir("usb", NULL); >> > + usb_debug_root = debugfs_create_dir(USB_DEBUG_ROOT_NAME, NULL); >> > debugfs_create_file("devices", 0444, usb_debug_root, NULL, >> > &usbfs_devices_fops); >> > } >> >> might be a better idea to move this to usb common. Then have a function >> which can be called by both host and gadget to maybe create the >> directory: >> >> static struct dentry *usb_debug_root; >> >> struct dentry *usb_debugfs_init(void) >> { >> if (!usb_debug_root) >> usb_debug_root = debugfs_create_dir("usb", NULL); >> >> return usb_debug_root; >> } >> >> >> Then usb core would be updated to something like: >> >> static void usb_core_debugfs_init(void) >> { >> struct dentry *root = usb_debugfs_init(); >> >> debugfs_create_file("devices", 0444, root, NULL, &usbfs_devices_fops); >> } >> > I find a problem when move usb_debugfs_init() and usb_debugfs_cleanup() > into usb common, it's easy to create "usb" directory, but difficult to > cleanup it: > > common/common.c > > struct dentry *usb_debugfs_init(void) > { > if (!usb_debug_root) > usb_debug_root = debugfs_create_dir("usb", NULL); > > return usb_debug_root; > } > > void usb_debugfs_cleanup(void) > { > debugfs_remove_recursive(usb_debug_root); > usb_debug_root = NULL; > } > > core/usb.c > > static void usb_core_debugfs_init(void) > { > struct dentry *root = usb_debugfs_init(); > > debugfs_create_file("devices", 0444, root, NULL, > &usbfs_devices_fops); > } > > static int __init usb_init(void) > { > ... > usb_core_debugfs_init(); > ... > } > > static void __exit usb_exit(void) > { > ... > usb_debugfs_cleanup(); > // will be error, gadget may use it. > ... > } > > gadget/udc/core.c > > static int __init usb_udc_init(void) > { > ... > usb_debugfs_init(); > ... > } > > static void __exit usb_udc_exit(void) > { > ... > usb_debugfs_cleanup(); > // can't cleanup in fact, usb core may use it. > } > > How to handle this case? introduce a reference count? do you have any > suggestion? I guess a simple refcount is the way to go: struct dentry *usb_debugfs_init(void) { if (!usb_debug_root) usb_debug_root = debugfs_create_dir("usb", NULL); usb_debug_root_refcnt++; return usb_debug_root; } void usb_debugfs_cleanup(void) { if (!(--usb_debug_root_refcnt)) { debugfs_remove_recursive(usb_debug_root); usb_debug_root = NULL; } } Or something along those lines
On Fri, 2019-05-31 at 08:44 +0300, Felipe Balbi wrote: > Hi, > > Chunfeng Yun <chunfeng.yun@mediatek.com> writes: > > > Hi Felipe, > > On Tue, 2019-05-28 at 11:11 +0300, Felipe Balbi wrote: > >> Hi, > >> > >> Chunfeng Yun <chunfeng.yun@mediatek.com> writes: > >> > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c > >> > index 7fcb9f782931..88b3ee03a12d 100644 > >> > --- a/drivers/usb/core/usb.c > >> > +++ b/drivers/usb/core/usb.c > >> > @@ -1190,7 +1190,7 @@ EXPORT_SYMBOL_GPL(usb_debug_root); > >> > > >> > static void usb_debugfs_init(void) > >> > { > >> > - usb_debug_root = debugfs_create_dir("usb", NULL); > >> > + usb_debug_root = debugfs_create_dir(USB_DEBUG_ROOT_NAME, NULL); > >> > debugfs_create_file("devices", 0444, usb_debug_root, NULL, > >> > &usbfs_devices_fops); > >> > } > >> > >> might be a better idea to move this to usb common. Then have a function > >> which can be called by both host and gadget to maybe create the > >> directory: > >> > >> static struct dentry *usb_debug_root; > >> > >> struct dentry *usb_debugfs_init(void) > >> { > >> if (!usb_debug_root) > >> usb_debug_root = debugfs_create_dir("usb", NULL); > >> > >> return usb_debug_root; > >> } > >> > >> > >> Then usb core would be updated to something like: > >> > >> static void usb_core_debugfs_init(void) > >> { > >> struct dentry *root = usb_debugfs_init(); > >> > >> debugfs_create_file("devices", 0444, root, NULL, &usbfs_devices_fops); > >> } > >> > > I find a problem when move usb_debugfs_init() and usb_debugfs_cleanup() > > into usb common, it's easy to create "usb" directory, but difficult to > > cleanup it: > > > > common/common.c > > > > struct dentry *usb_debugfs_init(void) > > { > > if (!usb_debug_root) > > usb_debug_root = debugfs_create_dir("usb", NULL); > > > > return usb_debug_root; > > } > > > > void usb_debugfs_cleanup(void) > > { > > debugfs_remove_recursive(usb_debug_root); > > usb_debug_root = NULL; > > } > > > > core/usb.c > > > > static void usb_core_debugfs_init(void) > > { > > struct dentry *root = usb_debugfs_init(); > > > > debugfs_create_file("devices", 0444, root, NULL, > > &usbfs_devices_fops); > > } > > > > static int __init usb_init(void) > > { > > ... > > usb_core_debugfs_init(); > > ... > > } > > > > static void __exit usb_exit(void) > > { > > ... > > usb_debugfs_cleanup(); > > // will be error, gadget may use it. > > ... > > } > > > > gadget/udc/core.c > > > > static int __init usb_udc_init(void) > > { > > ... > > usb_debugfs_init(); > > ... > > } > > > > static void __exit usb_udc_exit(void) > > { > > ... > > usb_debugfs_cleanup(); > > // can't cleanup in fact, usb core may use it. > > } > > > > How to handle this case? introduce a reference count? do you have any > > suggestion? > > I guess a simple refcount is the way to go: > > struct dentry *usb_debugfs_init(void) > { > if (!usb_debug_root) > usb_debug_root = debugfs_create_dir("usb", NULL); > > usb_debug_root_refcnt++; > return usb_debug_root; > } > > void usb_debugfs_cleanup(void) > { > if (!(--usb_debug_root_refcnt)) { > debugfs_remove_recursive(usb_debug_root); > usb_debug_root = NULL; > } > } I'll try it, thanks > > Or something along those lines >
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c index 7fcb9f782931..88b3ee03a12d 100644 --- a/drivers/usb/core/usb.c +++ b/drivers/usb/core/usb.c @@ -1190,7 +1190,7 @@ EXPORT_SYMBOL_GPL(usb_debug_root); static void usb_debugfs_init(void) { - usb_debug_root = debugfs_create_dir("usb", NULL); + usb_debug_root = debugfs_create_dir(USB_DEBUG_ROOT_NAME, NULL); debugfs_create_file("devices", 0444, usb_debug_root, NULL, &usbfs_devices_fops); } diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c index 7cf34beb50df..ed45f9429e58 100644 --- a/drivers/usb/gadget/udc/core.c +++ b/drivers/usb/gadget/udc/core.c @@ -8,6 +8,7 @@ #include <linux/kernel.h> #include <linux/module.h> +#include <linux/debugfs.h> #include <linux/device.h> #include <linux/list.h> #include <linux/err.h> @@ -1587,12 +1588,37 @@ static int usb_udc_uevent(struct device *dev, struct kobj_uevent_env *env) return 0; } +/* if CONFIG_USB is set, leave USB core to create usb_debug_root */ +#ifndef CONFIG_USB +struct dentry *usb_debug_root; +EXPORT_SYMBOL_GPL(usb_debug_root); + +static void usb_debugfs_init(void) +{ + usb_debug_root = debugfs_create_dir(USB_DEBUG_ROOT_NAME, NULL); +} + +static void usb_debugfs_cleanup(void) +{ + debugfs_remove_recursive(usb_debug_root); +} +#else +static void usb_debugfs_init(void) +{} + +static void usb_debugfs_cleanup(void) +{} +#endif + static int __init usb_udc_init(void) { + usb_debugfs_init(); + udc_class = class_create(THIS_MODULE, "udc"); if (IS_ERR(udc_class)) { pr_err("failed to create udc class --> %ld\n", PTR_ERR(udc_class)); + usb_debugfs_cleanup(); return PTR_ERR(udc_class); } @@ -1604,6 +1630,7 @@ subsys_initcall(usb_udc_init); static void __exit usb_udc_exit(void) { class_destroy(udc_class); + usb_debugfs_cleanup(); } module_exit(usb_udc_exit); diff --git a/include/linux/usb.h b/include/linux/usb.h index ae82d9d1112b..9c6e7b3265af 100644 --- a/include/linux/usb.h +++ b/include/linux/usb.h @@ -1994,6 +1994,7 @@ extern void usb_register_notify(struct notifier_block *nb); extern void usb_unregister_notify(struct notifier_block *nb); /* debugfs stuff */ +#define USB_DEBUG_ROOT_NAME "usb" extern struct dentry *usb_debug_root; /* LED triggers */
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> --- 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/core/usb.c | 2 +- drivers/usb/gadget/udc/core.c | 27 +++++++++++++++++++++++++++ include/linux/usb.h | 1 + 3 files changed, 29 insertions(+), 1 deletion(-)