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

Chunfeng Yun
2019-11-07T09:06:58Z

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