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 <[email protected]>
Signed-off-by: Chunfeng Yun <[email protected]>
---
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 */
--
2.21.0
Hi,
Chunfeng Yun <[email protected]> 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);
}
--
balbi
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 <[email protected]>
> Signed-off-by: Chunfeng Yun <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> writes:
> Hi Felipe,
> On Tue, 2019-05-28 at 11:11 +0300, Felipe Balbi wrote:
>> Hi,
>>
>> Chunfeng Yun <[email protected]> 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
--
balbi
On Fri, 2019-05-31 at 08:44 +0300, Felipe Balbi wrote:
> Hi,
>
> Chunfeng Yun <[email protected]> writes:
>
> > Hi Felipe,
> > On Tue, 2019-05-28 at 11:11 +0300, Felipe Balbi wrote:
> >> Hi,
> >>
> >> Chunfeng Yun <[email protected]> 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
>