DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 31D7F2077C
Message-ID: <a731948bf875867be4246b72a661ce71833620b8.camel@perches.com>
Subject: Re: [RFC v4 08/13] rtw88: debug files
From:   Joe Perches <joe@perches.com>
To:     yhchuang@realtek.com, kvalo@codeaurora.org
Cc:     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 07:19:31 -0700
In-Reply-To: <1539421245-11847-9-git-send-email-yhchuang@realtek.com>
References: <1539421245-11847-1-git-send-email-yhchuang@realtek.com>
         <1539421245-11847-9-git-send-email-yhchuang@realtek.com>
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 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.  Typical debugging would dynamic debugging on a
per-line instance and this uses a single dev_dbg for all debugging.

This seems unnecessarily complexity for a negative gain.

Also, many callers of the rtw_<level> logging function do not include
a terminating newline.

Please consistently use a newline for each format.