On Sat, 2018-10-13 at 18:23 +0300, Kalle Valo wrote:
> 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.
No, it isn't.
> IIRC at least ath6kl, ath9k and ath10k do that, most likely also others.
No, they don't. Check again.
> > Typical debugging would dynamic debugging on a per-line instance andl
> > 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.
You should look harder.
> > 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.
It is unnecessarily complex.
This saves one dereference per call, but is it really worth it?
