Re: [PATCH v4 08/13] rtw88: debug files

[Brian Norris <briannorris@chromium.org>]

Hi,

On Wed, Jan 30, 2019 at 12:02:15PM +0800, yhchuang@realtek.com wrote:
> From: Yan-Hsuan Chuang <yhchuang@realtek.com>
> 
> debug files for Realtek 802.11ac wireless network chips
> 
> Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
> ---
>  drivers/net/wireless/realtek/rtw88/debug.c | 631 +++++++++++++++++++++++++++++
>  drivers/net/wireless/realtek/rtw88/debug.h |  35 ++
>  2 files changed, 666 insertions(+)
>  create mode 100644 drivers/net/wireless/realtek/rtw88/debug.c
>  create mode 100644 drivers/net/wireless/realtek/rtw88/debug.h
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/debug.c b/drivers/net/wireless/realtek/rtw88/debug.c
> new file mode 100644
> index 0000000..d0cb9d3
> --- /dev/null
> +++ b/drivers/net/wireless/realtek/rtw88/debug.c
> @@ -0,0 +1,631 @@

...

> +#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);

I understand some questions came up about this dbg() interface
previously, without the most constructive result, but...

...I do find one particular aspect of this interface a little weird: it
has its own separate Kconfig flag, and yet it's still implicitly
dependent on CONFIG_DYNAMIC_DEBUG. Note how dev_dbg() gets completely
stubbed out if !defined(CONFIG_DYNAMIC_DEBUG) && !defined(DEBUG).

I think some other similar loggers end up just using
dev_printk(KERN_DEBUG, ...) for this piece of the equation, so that if
somebody has bothered to enable CONFIG_RTW88_DEBUG, they can be sure
their log messages are at least compiled in.

But then, other drivers that have used dev_printk() *also* have dynamic
methods to enable/disable their dbg-level messages (e.g., with a 'mask'
module parameter, for classifying different types of messages). That
also gives us the option of compiling in the messages while leaving them
disabled for printing by default. IOW, they basically implement a
categorized version of CONFIG_DYNAMIC_DEBUG.

(Also note: my systems generally have DYNAMIC_DEBUG disabled, but we
*do* like to have a few driver-specific debug options enabled for WiFi,
so we can debug problems in the field via runtime enable/disable.)

Altogether, I think this means you should either:

 (a) alias RTW88_DEBUG with DYNAMIC_DEBUG (e.g., RTW88_DEBUG selects or
     depends on DYNAMIC_DEBUG?) or, remove your own special Kconfig
     entirely; or
 (b) implement runtime controls to enable/disable your dbg() messages,
     and do not depend on DYNAMIC_DEBUG

I kinda lean toward (b), since that's how many other WiFi drivers work,
and it prevents me from having to enable all of DYNAMIC_DEBUG (although,
it may be time to reevaluate why Chrome OS has it disabled...probably
just for mild savings on size). It also gives you the option of
classifying your debug messages even further.

Regards,
Brian

> +
> +	va_end(args);
> +}
> +EXPORT_SYMBOL(__rtw_dbg);
> +
> +#endif /* CONFIG_RTW88_DEBUG */
> diff --git a/drivers/net/wireless/realtek/rtw88/debug.h b/drivers/net/wireless/realtek/rtw88/debug.h
> new file mode 100644
> index 0000000..231e4e8
> --- /dev/null
> +++ b/drivers/net/wireless/realtek/rtw88/debug.h
> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright(c) 2018  Realtek Corporation.
> + */
> +
> +#ifndef __RTW_DEBUG_H
> +#define __RTW_DEBUG_H
> +
> +#ifdef CONFIG_RTW88_DEBUGFS
> +
> +void rtw_debugfs_init(struct rtw_dev *rtwdev);
> +
> +#else
> +
> +static inline void rtw_debugfs_init(struct rtw_dev *rtwdev) {}
> +
> +#endif /* CONFIG_RTW88_DEBUGFS */
> +
> +#ifdef CONFIG_RTW88_DEBUG
> +
> +__printf(2, 3)
> +void __rtw_dbg(struct rtw_dev *rtwdev, const char *fmt, ...);
> +
> +#define rtw_dbg(rtwdev, a...) __rtw_dbg(rtwdev, ##a)
> +
> +#else
> +
> +static inline void rtw_dbg(struct rtw_dev *rtwdev, const char *fmt, ...) {}
> +
> +#endif /* CONFIG_RTW88_DEBUG */
> +
> +#define rtw_info(rtwdev, a...) dev_info(rtwdev->dev, ##a)
> +#define rtw_warn(rtwdev, a...) dev_warn(rtwdev->dev, ##a)
> +#define rtw_err(rtwdev, a...) dev_err(rtwdev->dev, ##a)
> +
> +#endif
> -- 
> 2.7.4
>