Joe Perches <joe@xxxxxxxxxxx> writes:
> On Sat, 2018-10-13 at 17:00 +0800, yhchuang@xxxxxxxxxxx wrote:
>> From: Yan-Hsuan Chuang <yhchuang@xxxxxxxxxxx>
> []
>> 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
