DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2312B20877
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 08EFE60452
From:   Kalle Valo <kvalo@codeaurora.org>
To:     Joe Perches <joe@perches.com>
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
Subject: Re: [RFC v4 08/13] rtw88: debug files
References: <1539421245-11847-1-git-send-email-yhchuang@realtek.com>
        <1539421245-11847-9-git-send-email-yhchuang@realtek.com>
        <a731948bf875867be4246b72a661ce71833620b8.camel@perches.com>
Date:   Sat, 13 Oct 2018 18:23:27 +0300
In-Reply-To: <a731948bf875867be4246b72a661ce71833620b8.camel@perches.com> (Joe
        Perches's message of "Sat, 13 Oct 2018 07:19:31 -0700")
Message-ID: <87y3b1vpjk.fsf@codeaurora.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

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. IIRC at least
ath6kl, ath9k and ath10k do that, most likely also others.

> Typical debugging would dynamic debugging on a per-line instance and
> 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.

> 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.

> Also, many callers of the rtw_<level> logging function do not include
> a terminating newline.
>
> Please consistently use a newline for each format.

But with this I do agree.

-- 
Kalle Valo