| Message ID | 8cb137d5376b4e317dc22dcb9e81a1125b781f8f.1573008519.git.chunfeng.yun@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/28] usb: common: change usb_debug_root as static variable | expand |
On 11/5/19 7:15 PM, Chunfeng Yun wrote: > Try to avoid using extern global variable, and provide two > functions for the usage cases > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> > --- > 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; > + I don't think it is a good idea to declare this variable static before all its uses are removed. This patch will leave the system in a non-compileable state until all patches of the series have been applied. I am curious. What is the rationale for this change ? Did exported variables run out of favor ? Sorry if there was a patch 0 of the series explaining the rationale and I missed it. Guenter
> Did exported variables run out of favor ? Sorry if there was a > patch 0 of the series explaining the rationale and I missed it. I neither got it if there was one. If there wasn't, I agree a cover letter makes a lot of sense here.
On Wed, Nov 06, 2019 at 08:33:10AM +0100, Wolfram Sang wrote: > > > Did exported variables run out of favor ? Sorry if there was a > > patch 0 of the series explaining the rationale and I missed it. > > I neither got it if there was one. If there wasn't, I agree a cover > letter makes a lot of sense here. I don't understand why this whole series is needed either. What is wrong with the original code? Also, like I said before, you broke the build here with the first patch, which is not ok. greg k-h
On Wed, Nov 06, 2019 at 11:15:02AM +0800, Chunfeng Yun wrote: > Try to avoid using extern global variable, and provide two > functions for the usage cases > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> > --- > 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; You just broke the kernel build, which is not acceptable at all :( greg k-h
On Tue, 2019-11-05 at 20:03 -0800, Guenter Roeck wrote: > On 11/5/19 7:15 PM, Chunfeng Yun wrote: > > Try to avoid using extern global variable, and provide two > > functions for the usage cases > > > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> > > --- > > 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; > > + > > I don't think it is a good idea to declare this variable static > before all its uses are removed. This patch will leave the system > in a non-compileable state until all patches of the series have > been applied. Yes, you are right, I'll fix it, may be split into two patch > > I am curious. What is the rationale for this change ? > Did exported variables run out of favor ? Personally, I think it's good practice to avoid global varibale, it may not a good reason. another one is that want to create all debug directory/file in usb root for host/device controller drivers. > Sorry if there was a > patch 0 of the series explaining the rationale and I missed it. Sorry, I do not prepare cover latter, will add it if send out next version. > > Guenter
Hi Greg, On Wed, 2019-11-06 at 09:51 +0100, Greg Kroah-Hartman wrote: > On Wed, Nov 06, 2019 at 11:15:02AM +0800, Chunfeng Yun wrote: > > Try to avoid using extern global variable, and provide two > > functions for the usage cases > > > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> > > --- > > 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; > > You just broke the kernel build, which is not acceptable at all :( Vey sorry, I didn't get what you mean before, now get it. > > greg k-h
On Wed, 2019-11-06 at 09:51 +0100, Greg Kroah-Hartman wrote: > On Wed, Nov 06, 2019 at 08:33:10AM +0100, Wolfram Sang wrote: > > > > > Did exported variables run out of favor ? Sorry if there was a > > > patch 0 of the series explaining the rationale and I missed it. > > > > I neither got it if there was one. If there wasn't, I agree a cover > > letter makes a lot of sense here. > > I don't understand why this whole series is needed either. > > What is wrong with the original code? No wrong at all, just think it's good practice to avoid global variable, more and more controller drivers use it now. And it's also clear enough for global variable usb_debug_root > Also, like I said before, you > broke the build here with the first patch, which is not ok. > > greg k-h
On 11/6/19 1:11 AM, Chunfeng Yun wrote: > On Tue, 2019-11-05 at 20:03 -0800, Guenter Roeck wrote: >> On 11/5/19 7:15 PM, Chunfeng Yun wrote: >>> Try to avoid using extern global variable, and provide two >>> functions for the usage cases >>> >>> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> >>> --- >>> 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; >>> + >> >> I don't think it is a good idea to declare this variable static >> before all its uses are removed. This patch will leave the system >> in a non-compileable state until all patches of the series have >> been applied. > Yes, you are right, I'll fix it, may be split into two patch > >> >> I am curious. What is the rationale for this change ? >> Did exported variables run out of favor ? > Personally, I think it's good practice to avoid global varibale, > it may not a good reason. Personally I don't think it is a good reason at all. When you are done, someone else may come in and declare the opposite, re-introduce the global variable, and drop the new functions as unnecessary. This is ok for new code, but I think we should leave existing code alone unless it is broken and needs to get fixed. This is not the case here. Thanks, Guenter
Hi Greg, On Wed, 2019-11-06 at 06:00 -0800, Guenter Roeck wrote: > On 11/6/19 1:11 AM, Chunfeng Yun wrote: > > On Tue, 2019-11-05 at 20:03 -0800, Guenter Roeck wrote: > >> On 11/5/19 7:15 PM, Chunfeng Yun wrote: > >>> Try to avoid using extern global variable, and provide two > >>> functions for the usage cases > >>> > >>> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> > >>> --- > >>> 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; > >>> + > >> > >> I don't think it is a good idea to declare this variable static > >> before all its uses are removed. This patch will leave the system > >> in a non-compileable state until all patches of the series have > >> been applied. > > Yes, you are right, I'll fix it, may be split into two patch > > > >> > >> I am curious. What is the rationale for this change ? > >> Did exported variables run out of favor ? > > Personally, I think it's good practice to avoid global varibale, > > it may not a good reason. > > Personally I don't think it is a good reason at all. When you are done, > someone else may come in and declare the opposite, re-introduce the global > variable, and drop the new functions as unnecessary. > > This is ok for new code, but I think we should leave existing code alone > unless it is broken and needs to get fixed. This is not the case here. What do you think about this, if you also agree with Guenter's opinion, I'll abandon this patch. BTW, how about moving udc's debug directory into usb root? Thanks > > Thanks, > Guenter
On Thu, Nov 07, 2019 at 05:06:34PM +0800, Chunfeng Yun wrote: > Hi Greg, > > On Wed, 2019-11-06 at 06:00 -0800, Guenter Roeck wrote: > > On 11/6/19 1:11 AM, Chunfeng Yun wrote: > > > On Tue, 2019-11-05 at 20:03 -0800, Guenter Roeck wrote: > > >> On 11/5/19 7:15 PM, Chunfeng Yun wrote: > > >>> Try to avoid using extern global variable, and provide two > > >>> functions for the usage cases > > >>> > > >>> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> > > >>> --- > > >>> 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; > > >>> + > > >> > > >> I don't think it is a good idea to declare this variable static > > >> before all its uses are removed. This patch will leave the system > > >> in a non-compileable state until all patches of the series have > > >> been applied. > > > Yes, you are right, I'll fix it, may be split into two patch > > > > > >> > > >> I am curious. What is the rationale for this change ? > > >> Did exported variables run out of favor ? > > > Personally, I think it's good practice to avoid global varibale, > > > it may not a good reason. > > > > Personally I don't think it is a good reason at all. When you are done, > > someone else may come in and declare the opposite, re-introduce the global > > variable, and drop the new functions as unnecessary. > > > > This is ok for new code, but I think we should leave existing code alone > > unless it is broken and needs to get fixed. This is not the case here. > What do you think about this, if you also agree with Guenter's opinion, I do. > I'll abandon this patch. Please do. > BTW, how about moving udc's debug directory into usb root? Sure, feel free to send a patch for that. 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> --- drivers/usb/common/common.c | 16 ++++++++++++++-- include/linux/usb.h | 5 ++++- 2 files changed, 18 insertions(+), 3 deletions(-)