Re: [PATCH 01/28] usb: common: change usb_debug_root as static variable

Greg Kroah-Hartman
2019-11-07T09:16:42Z

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