> -----Original Message----- > From: Brian Norris [mailto:briannorris@xxxxxxxxxxxx] > > Hi, > > On Wed, Jan 30, 2019 at 12:02:15PM +0800, yhchuang@xxxxxxxxxxx wrote: > > From: Yan-Hsuan Chuang <yhchuang@xxxxxxxxxxx> > > > > debug files for Realtek 802.11ac wireless network chips > > > > Signed-off-by: Yan-Hsuan Chuang <yhchuang@xxxxxxxxxxx> > > --- > > 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. > So I think I can add a debug_mask and change dev_dbg to dev_printk. And leave rtw_[err|warn|info] unchanged. This way rtw_dbg will no more depend on DYNAMIC_DEBUG, also better for a wifi driver to select debug messages we want. Actually as more components added I think it's time to do it for developers to debug on various scenarios. :) It will take days for me to change the debug log and resend the patch set. But I think it's worth that we don't bother to have another patch to fix it. And I hope it won't have bad impact for reviewers. Yan-Hsuan
