DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 51F7720693
Message-ID: <0a41d14b9a964b6b7825bf5c55c32f35166354cd.camel@perches.com>
Subject: Re: [RFC v4 08/13] rtw88: debug files
From:   Joe Perches <joe@perches.com>
To:     Kalle Valo <kvalo@codeaurora.org>
Cc:     yhchuang@realtek.com, Larry.Finger@lwfinger.net,
        pkshih@realtek.com, tehuang@realtek.com, sgruszka@redhat.com,
        johannes@sipsolutions.net, linux-wireless@vger.kernel.org
Date:   Sat, 13 Oct 2018 13:28:24 -0700
In-Reply-To: <87y3b1vpjk.fsf@codeaurora.org>
References: <1539421245-11847-1-git-send-email-yhchuang@realtek.com>
         <1539421245-11847-9-git-send-email-yhchuang@realtek.com>
         <a731948bf875867be4246b72a661ce71833620b8.camel@perches.com>
         <87y3b1vpjk.fsf@codeaurora.org>
Content-Type: text/plain; charset="ISO-8859-1"
User-Agent: Evolution 3.30.1-1build1 
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On Sat, 2018-10-13 at 18:23 +0300, Kalle Valo wrote:
> Joe Perches <joe@perches.com> writes:
> 
> > On Sat, 2018-10-13 at 17:00 +0800, yhchuang@realtek.com wrote:
> > > From: Yan-Hsuan Chuang <yhchuang@realtek.com>
> > []
> > > diff --git a/drivers/net/wireless/realtek/rtw88/debug.c
> > > b/drivers/net/wireless/realtek/rtw88/debug.c
> > []
> > > +#ifdef CONFIG_RTW88_DEBUG
> > > +
> > > +void __rtw_dbg(struct rtw_dev *rtwdev, const char *fmt, ...)
> > > +{
> > > +	struct va_format vaf = {
> > > +		.fmt = fmt,
> > > +	};
> > > +	va_list args;
> > > +
> > > +	va_start(args, fmt);
> > > +	vaf.va = &args;
> > > +
> > > +	if (net_ratelimit())
> > > +		dev_dbg(rtwdev->dev, "%pV", &vaf);
> > > +
> > > +	va_end(args);
> > > +}
> > > +EXPORT_SYMBOL(__rtw_dbg);
> > > +
> > > +#define __rtw_fn(fn)							\
> > > +void __rtw_ ##fn(struct rtw_dev *rtwdev, const char *fmt, ...)		\
> > > +{									\
> > > +	struct va_format vaf = {					\
> > > +		.fmt = fmt,						\
> > > +	};								\
> > > +	va_list args;							\
> > > +									\
> > > +	va_start(args, fmt);						\
> > > +	vaf.va = &args;							\
> > > +	dev_ ##fn(rtwdev->dev, "%pV", &vaf);				\
> > > +	va_end(args);							\
> > > +}									\
> > > +EXPORT_SYMBOL(__rtw_ ##fn);
> > > +
> > > +__rtw_fn(info)
> > > +__rtw_fn(warn)
> > > +__rtw_fn(err)
> > 
> > It's very unusual to have _all_ the logging under a CONFIG_<FOO>_DEBUG
> > config guard flag.
> 
> For wireless drivers that is actually quite typical.

No, it isn't.

> IIRC at least ath6kl, ath9k and ath10k do that, most likely also others.

No, they don't.  Check again.

> > Typical debugging would dynamic debugging on a per-line instance andl
> > this uses a single dev_dbg for all debugging.
> 
> I don't recall seeing anyone using per-line dynamic debugging with
> wireless drivers. The drivers are so complex that enabling one message
> at a time doesn't really get you anywhere, that's why we mostly group
> messages into similar groups (or levels) to make it easier to enable
> certain debug messages.

You should look harder.  

> > This seems unnecessarily complexity for a negative gain.
> 
> I haven't reviewed the driver yet but from a quick look I don't see this
> as a problem.
  
It is unnecessarily complex.
This saves one dereference per call, but is it really worth it?