Re: [PATCH 01/28] usb: common: change usb_debug_root as static variable - Greg Kroah-Hartman

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Chunfeng Yun <chunfeng.yun@mediatek.com>
Cc: Peter Chen <Peter.Chen@nxp.com>,
	Alexandre Belloni <alexandre.belloni@bootlin.com>,
	Heikki Krogerus <heikki.krogerus@linux.intel.com>,
	Wolfram Sang <wsa@the-dreams.de>,
	Yangtao Li <tiny.windzz@gmail.com>,
	Linus Walleij <linus.walleij@linaro.org>,
	linux-kernel@vger.kernel.org,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Mans Rullgard <mans@mansr.com>,
	Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Fabio Estevam <festevam@gmail.com>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Robert Jarzmik <robert.jarzmik@free.fr>,
	Kevin Cernekee <cernekee@gmail.com>,
	"Gustavo A. R. Silva" <gustavo@embeddedor.com>,
	Ludovic Desroches <ludovic.desroches@microchip.com>,
	Alan Stern <stern@rowland.harvard.edu>,
	NXP Linux Team <linux-imx@nxp.com>,
	bcm-kernel-feedback-list@broadcom.com,
	David Kershner <david.kershner@unisys.com>,
	Hans Ulli Kroll <ulli.kroll@googlemail.com>,
	Cristian Birsan <cristian.birsan@microchip.com>,
	linux-media@vger.kernel.org,
	Fabrizio Castro <fabrizio.castro@bp.renesas.com>,
	Mathias Payer <mathias.payer@nebelwelt.net>,
	Mathias Nyman <mathias.nyman@intel.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	Haojian Zhuang <haojian.zhuang@gmail.com>,
	Stephen Boyd <swboyd@chromium.org>,
	linux-mediatek@lists.infradead.org,
	Minas Harutyunyan <hminas@synopsys.com>,
	Simon Horman <horms+renesas@verge.net.au>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Bin Liu <b-liu@ti.com>,
	linux-arm-kernel@lists.infradead.org,
	Biju Das <biju.das@bp.renesas.com>,
	Felipe Balbi <balbi@kernel.org>,
	Laurentiu Tudor <laurentiu.tudor@nxp.com>,
	Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>,
	linux-usb@vger.kernel.org,
	Nicolas Ferre <nicolas.ferre@microchip.com>,
	Dmitry Torokhov <dtor@chromium.org>,
	Guenter Roeck <linux@roeck-us.net>,
	Pengutronix Kernel Team <kernel@pengutronix.de>,
	Colin Ian King <colin.king@canonical.com>,
	Shawn Guo <shawnguo@kernel.org>, Daniel Mack <daniel@zonque.org>
Subject: Re: [PATCH 01/28] usb: common: change usb_debug_root as static variable
Date: Thu, 7 Nov 2019 10:16:24 +0100	[thread overview]
Message-ID: <20191107091624.GB1328892@kroah.com> (raw)
In-Reply-To: <1573117594.18464.87.camel@mhfsdcap03>

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

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek