| Message ID | 1572575349-5596-1-git-send-email-chunfeng.yun@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] usb: common: change usb_debug_root as static variable | expand |
On Fri, Nov 01, 2019 at 10:29:09AM +0800, Chunfeng Yun wrote: > Try to avoid using extern global variable, and provide two > functions for the usage cases That is 3 different things all in one patch, not generally considered a good thing at all. Also, who is going to use these new functions? Why are they needed? > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> > --- > NOTE: > Prepared but not send out patches for drivers using usb_debug_root, > because I'm not sure whether this patch is needed, and many drivers > will be modified. > --- > drivers/usb/common/common.c | 16 ++++++++++++++-- > include/linux/usb.h | 5 ++++- > 2 files changed, 18 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c > index 1433260d99b4..639ee6d243a2 100644 > --- a/drivers/usb/common/common.c > +++ b/drivers/usb/common/common.c > @@ -293,8 +293,20 @@ 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 struct dentry *usb_debug_root; Doesn't this break things as-is? You can't do that in a single patch either :( > + > +struct dentry *usb_debugfs_create_dir(const char *name) > +{ > + return debugfs_create_dir(name, usb_debug_root); > +} > +EXPORT_SYMBOL_GPL(usb_debugfs_create_dir); > + > +struct dentry *usb_debugfs_create_file(const char *name, umode_t mode, > + void *data, const struct file_operations *fops) > +{ > + return debugfs_create_file(name, mode, usb_debug_root, data, fops); I doubt many people want to create a file in the usb "root" debugfs directory, right? They _should_ be just creating a new subdirectory in there instead. thanks, greg k-h
On Fri, 2019-11-01 at 10:02 +0100, Greg Kroah-Hartman wrote: > On Fri, Nov 01, 2019 at 10:29:09AM +0800, Chunfeng Yun wrote: > > Try to avoid using extern global variable, and provide two > > functions for the usage cases > > That is 3 different things all in one patch, not generally considered a > good thing at all. > > Also, who is going to use these new functions? Why are they needed? After remove global variable usb_debug_root, the drivers using usb_debug_root to create directory or files will use these new APIs instead. > > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> > > --- > > NOTE: > > Prepared but not send out patches for drivers using usb_debug_root, > > because I'm not sure whether this patch is needed, and many drivers > > will be modified. > > --- > > drivers/usb/common/common.c | 16 ++++++++++++++-- > > include/linux/usb.h | 5 ++++- > > 2 files changed, 18 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c > > index 1433260d99b4..639ee6d243a2 100644 > > --- a/drivers/usb/common/common.c > > +++ b/drivers/usb/common/common.c > > @@ -293,8 +293,20 @@ 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 struct dentry *usb_debug_root; > > Doesn't this break things as-is? Yes, it will, I didn't send out other patches for the drivers using usb_debug_root. > You can't do that in a single patch > either :( When I make usb_debug_root as static variable, two APIs need be added due to other driver use it to create directory or file. > > > + > > +struct dentry *usb_debugfs_create_dir(const char *name) > > +{ > > + return debugfs_create_dir(name, usb_debug_root); > > +} > > +EXPORT_SYMBOL_GPL(usb_debugfs_create_dir); > > + > > +struct dentry *usb_debugfs_create_file(const char *name, umode_t mode, > > + void *data, const struct file_operations *fops) > > +{ > > + return debugfs_create_file(name, mode, usb_debug_root, data, fops); > > I doubt many people want to create a file in the usb "root" debugfs > directory, right? They _should_ be just creating a new subdirectory in > there instead. Currently only three .c files creates a file under usb 'root' debugfs directory. > > thanks, > > greg k-h
diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c index 1433260d99b4..639ee6d243a2 100644 --- a/drivers/usb/common/common.c +++ b/drivers/usb/common/common.c @@ -293,8 +293,20 @@ 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 struct dentry *usb_debug_root; + +struct dentry *usb_debugfs_create_dir(const char *name) +{ + return debugfs_create_dir(name, usb_debug_root); +} +EXPORT_SYMBOL_GPL(usb_debugfs_create_dir); + +struct dentry *usb_debugfs_create_file(const char *name, umode_t mode, + void *data, const struct file_operations *fops) +{ + return debugfs_create_file(name, mode, usb_debug_root, data, fops); +} +EXPORT_SYMBOL_GPL(usb_debugfs_create_file); static int __init usb_common_init(void) { diff --git a/include/linux/usb.h b/include/linux/usb.h index e656e7b4b1e4..ad96e0aa0127 100644 --- a/include/linux/usb.h +++ b/include/linux/usb.h @@ -2001,7 +2001,10 @@ extern void usb_register_notify(struct notifier_block *nb); extern void usb_unregister_notify(struct notifier_block *nb); /* debugfs stuff */ -extern struct dentry *usb_debug_root; +extern struct dentry *usb_debugfs_create_dir(const char *name); +extern struct dentry * +usb_debugfs_create_file(const char *name, umode_t mode, void *data, + const struct file_operations *fops); /* LED triggers */ enum usb_led_event {
Try to avoid using extern global variable, and provide two functions for the usage cases Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> --- NOTE: Prepared but not send out patches for drivers using usb_debug_root, because I'm not sure whether this patch is needed, and many drivers will be modified. --- drivers/usb/common/common.c | 16 ++++++++++++++-- include/linux/usb.h | 5 ++++- 2 files changed, 18 insertions(+), 3 deletions(-)