Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210618064625.14131-1-pkshih@realtek.com> <20210618064625.14131-5-pkshih@realtek.com>
 <20210702072308.GA4184@pengutronix.de> <CA+ASDXNjHJoXgRAM4E7TcLuz9zBmQkaBMuhK2DEVy3dnE-3XcA@mail.gmail.com>
 <20210702175740.5cdhmfp4ldiv6tn7@pengutronix.de>
In-Reply-To: <20210702175740.5cdhmfp4ldiv6tn7@pengutronix.de>
From:   Brian Norris <briannorris@chromium.org>
Date:   Fri, 2 Jul 2021 11:38:26 -0700
Message-ID: <CA+ASDXP0_Y1x_1OixJFWDCeZX3txV+xbwXcXfTbw1ZiGjSFiCQ@mail.gmail.com>
Subject: Re: [PATCH 04/24] rtw89: add debug files
To:     Oleksij Rempel <o.rempel@pengutronix.de>
Cc:     Ping-Ke Shih <pkshih@realtek.com>,
        Kalle Valo <kvalo@codeaurora.org>,
        linux-wireless <linux-wireless@vger.kernel.org>,
        "<netdev@vger.kernel.org>" <netdev@vger.kernel.org>,
        Sascha Hauer <kernel@pengutronix.de>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Jul 2, 2021 at 10:57 AM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> On Fri, Jul 02, 2021 at 10:08:53AM -0700, Brian Norris wrote:
> > On Fri, Jul 2, 2021 at 12:23 AM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> > > For dynamic debugging we usually use ethtool msglvl.
> > > Please, convert all dev_err/warn/inf.... to netif_ counterparts
> >
> > Have you ever looked at a WiFi driver?
>
> Yes. You can parse the kernel log for my commits.

OK! So I see you've touched a lot of ath9k, >3 years ago. You might
notice that it is one such example -- it supports exactly the same
kind of debugfs file, with a set of ath_dbg() log types. Why doesn't
it use this netif debug logging?

> > I haven't seen a single one that uses netif_*() for logging.
> > On the other hand, almost every
> > single one has a similar module parameter or debugfs knob for enabling
> > different types of debug messages.
> >
> > As it stands, the NETIF_* categories don't really align at all with
> > the kinds of message categories most WiFi drivers support. Do you
> > propose adding a bunch of new options to the netif debug feature?
>
> Why not? It make no sense or it is just "it is tradition, we never do
> it!" ?

Well mainly, I don't really like people dreaming up arbitrary rules
and enforcing them only on new submissions. If such a change was
Recommended, it seems like a better first step would be to prove that
existing drivers (where there are numerous examples) can be converted
nicely, instead of pushing the work to new contributors arbitrarily.
Otherwise, the bar for new contributions gets exceedingly high -- this
one has already sat for more than 6 months with depressingly little
useful feedback.

I also know very little about this netif log level feature, but if it
really depends on ethtool (seems like it does?) -- I don't even bother
installing ethtool on most of my systems. It's much easier to poke at
debugfs, sysfs, etc., than to construct the relevant ethtool ioctl()s
or netlink messages. It also seems that these debug knobs can't be set
before the driver finishes coming up, so one would still need a module
parameter to mirror some of the same features. Additionally, a WiFi
driver doesn't even have a netdev to speak of until after a lot of the
interesting stuff comes up (much of the mac80211 framework focuses on
a |struct ieee80211_hw| and a |struct wiphy|), so I'm not sure your
suggestion really fits these sorts of drivers (and the things they
might like to support debug-logging for) at all.

Anyway, if Ping-Ke wants to paint this bikeshed for you, I won't stop him.

> Even dynamic printk provide even more granularity. So module parameter looks
> like stone age against all existing possibilities.

Dynamic printk seems a bit beside the point (it's pretty different
than either of the methods we're talking about), but I'll bite: many
distributors disable it. It's easier to get targeted debugging for a
few modules you care about, than the entire dynamic debug feature for
~every print in the kernel.

Brian