[01/28] usb: common: change usb_debug_root as static variable
diff mbox series

Message ID 8cb137d5376b4e317dc22dcb9e81a1125b781f8f.1573008519.git.chunfeng.yun@mediatek.com
State New
Headers show
Series
  • [01/28] usb: common: change usb_debug_root as static variable
Related show

Commit Message

Chunfeng Yun Nov. 6, 2019, 3:15 a.m. UTC
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(-)

Comments

Guenter Roeck Nov. 6, 2019, 4:03 a.m. UTC | #1
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
Wolfram Sang Nov. 6, 2019, 7:33 a.m. UTC | #2
> 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.
Greg Kroah-Hartman Nov. 6, 2019, 8:51 a.m. UTC | #3
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
Greg Kroah-Hartman Nov. 6, 2019, 8:51 a.m. UTC | #4
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
Chunfeng Yun Nov. 6, 2019, 9:11 a.m. UTC | #5
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
Chunfeng Yun Nov. 6, 2019, 9:15 a.m. UTC | #6
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
Chunfeng Yun Nov. 6, 2019, 9:25 a.m. UTC | #7
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
Guenter Roeck Nov. 6, 2019, 2 p.m. UTC | #8
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
Chunfeng Yun Nov. 7, 2019, 9:06 a.m. UTC | #9
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
Greg Kroah-Hartman Nov. 7, 2019, 9:16 a.m. UTC | #10
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

Patch
diff mbox series

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 {