Re: [PATCH 04/24] rtw89: add debug files

Re: [PATCH 04/24] rtw89: add debug files

[Oleksij Rempel]

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:
> > On Fri, Jun 18, 2021 at 02:46:05PM +0800, Ping-Ke Shih wrote:
> > > +#ifdef CONFIG_RTW89_DEBUGMSG
> > > +unsigned int rtw89_debug_mask;
> > > +EXPORT_SYMBOL(rtw89_debug_mask);
> > > +module_param_named(debug_mask, rtw89_debug_mask, uint, 0644);
> > > +MODULE_PARM_DESC(debug_mask, "Debugging mask");
> > > +#endif
> >
> >
> > 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.

> 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!" ? 

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

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 04/24] rtw89: add debug files

[Brian Norris]

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

Re: [PATCH 04/24] rtw89: add debug files

[Oleksij Rempel]

On Fri, Jul 02, 2021 at 11:38:26AM -0700, Brian Norris wrote:
> 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?

Ok, you right, ath mask would will set netif msg level on limit.

> > > 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.

It is technical discussion. There is no reason to get personal.

> 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.

Hm, my experience as patch submitter is rather different, but who knows,
every subsystem has diffent rules. Good to know, wireless is different.

> Otherwise, the bar for new contributions gets exceedingly high -- this
> one has already sat for more than 6 months with depressingly little
> useful feedback.

Ok, I compared this driver with different realtek drivers. Most of them have
own DHCP detection code? It is not hardware specific operation. There is no
need to review code duplicates just again. Is it so hard to share this code
at least for realtek drivers? Do reviewers have a lot of free time to
re-read code duplicates?

> 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.

Ok, i have nothing against unified realtek specif debugging code, as common
athers code do.

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

Well, i can provide a lot of "bikeshed". For example PCI related error
detection in this code. What level of review it will be?

> > 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.

There are two side of each patch: reviewer and submitter. Both side have
limited time budget. If reviewer is not allowed to request improve things to
save submitters timer, why is it OK to waste reviewers time by applying
code dups?

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
iAmtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 04/24] rtw89: add debug files

[Brian Norris]

On Fri, Jul 2, 2021 at 12:32 PM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> On Fri, Jul 02, 2021 at 11:38:26AM -0700, Brian Norris wrote:
> > Well mainly, I don't really like people dreaming up arbitrary rules
> > and enforcing them only on new submissions.
>
> It is technical discussion. There is no reason to get personal.

I'm not really intending to make this personal, so apologies if it
appeared that way.

What I'm trying to get at is that
(a) no other wireless driver does this, so why should this one? and
(b) the feature you claim this driver can use does not appear suited
to the task.

It's easier to make suggestions than to make them a reality.

> > 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.
>
> Hm, my experience as patch submitter is rather different, but who knows,
> every subsystem has diffent rules. Good to know, wireless is different.

I'm not an arbiter for "wireless" -- so my thoughts are purely my own
opinion. But I have noted some technical reasons why wireless drivers
may be different than ethernet drivers, and the suggested (again,
purely my own opinion) exercise might show you that your suggestion
won't really work out in practice.

Brian

Re: [PATCH 04/24] rtw89: add debug files

[Oleksij Rempel]

On Fri, Jul 02, 2021 at 01:00:27PM -0700, Brian Norris wrote:
> On Fri, Jul 2, 2021 at 12:32 PM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> > On Fri, Jul 02, 2021 at 11:38:26AM -0700, Brian Norris wrote:
> > > Well mainly, I don't really like people dreaming up arbitrary rules
> > > and enforcing them only on new submissions.
> >
> > It is technical discussion. There is no reason to get personal.
> 
> I'm not really intending to make this personal, so apologies if it
> appeared that way.
> 
> What I'm trying to get at is that
> (a) no other wireless driver does this, so why should this one? and
> (b) the feature you claim this driver can use does not appear suited
> to the task.
> 
> It's easier to make suggestions than to make them a reality.
> 
> > > 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.
> >
> > Hm, my experience as patch submitter is rather different, but who knows,
> > every subsystem has diffent rules. Good to know, wireless is different.
> 
> I'm not an arbiter for "wireless" -- so my thoughts are purely my own
> opinion. But I have noted some technical reasons why wireless drivers
> may be different than ethernet drivers, and the suggested (again,
> purely my own opinion) exercise might show you that your suggestion
> won't really work out in practice.

Ok, so we still need to find the way to go.
For example drivers/net/wireless/realtek/rtw89/debug.c is 2404 of potentially
removable code. Some one should review it or outoptimize it by using
existing frameworks.

As you noticed, not many people are willing to review this driver. IMO,
it is related to the RealTek reputation of making code drops with lots
of not not usable or duplicated code. So to say - offloading the dirty work to
the community. For example this patch set:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/drivers/staging/rts5139?h=v5.13&qt=author&q=rempel
This new rtw89 driver seems to confirm this reputation, but I cani't say it
for sure without spending a week on reverse engineering it.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 04/24] rtw89: add debug files

[Kalle Valo]

Oleksij Rempel <o.rempel@pengutronix.de> writes:

> 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:
>> > On Fri, Jun 18, 2021 at 02:46:05PM +0800, Ping-Ke Shih wrote:
>> > > +#ifdef CONFIG_RTW89_DEBUGMSG
>> > > +unsigned int rtw89_debug_mask;
>> > > +EXPORT_SYMBOL(rtw89_debug_mask);
>> > > +module_param_named(debug_mask, rtw89_debug_mask, uint, 0644);
>> > > +MODULE_PARM_DESC(debug_mask, "Debugging mask");
>> > > +#endif
>> >
>> >
>> > 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.
>
>> 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!" ? 
>
> Even dynamic printk provide even more granularity. So module parameter looks
> like stone age against all existing possibilities.

I'm all for improving wireless driver debugging features, but let's
please keep that as a separate thread from reviewing new drivers. I
think there are 4-5 new drivers in the queue at the moment so to keep
all this manageable let's have the review process as simple as possible,
please.

Using a module parameter for setting the debug mask is a standard
feature in wireless drivers so it shouldn't block rtw89. If we want a
generic debug framework for wireless drivers, an rfc patch for an
existing upstream wireless driver is a good way to get that discussion
forward.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

RE: [PATCH 04/24] rtw89: add debug files

[Pkshih]

> -----Original Message-----
> From: Brian Norris [mailto:briannorris@chromium.org]
> Sent: Saturday, July 03, 2021 2:38 AM
> To: Oleksij Rempel
> Cc: Pkshih; Kalle Valo; linux-wireless; <netdev@vger.kernel.org>; Sascha Hauer
> Subject: Re: [PATCH 04/24] rtw89: add debug files
> 
> 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.

I encounter the problems you mentioned mostly:
1. no netdev to be the parameter 'dev' of 'netif_dbg(priv, type, dev, format, args...)'
2. predefined 'type' isn't enough for wifi application. There're many wifi- or vendor-
   specific components, such as RF calibration, BT coexistence, DIG, CFO and so on.

So, I don't plan to change them for now.

--
Ping-Ke

Re: [PATCH 04/24] rtw89: add debug files

[Oleksij Rempel]

On Mon, Jul 05, 2021 at 08:58:51AM +0000, Pkshih wrote:
> 
> > -----Original Message-----
> > From: Brian Norris [mailto:briannorris@chromium.org]
> > Sent: Saturday, July 03, 2021 2:38 AM
> > To: Oleksij Rempel
> > Cc: Pkshih; Kalle Valo; linux-wireless; <netdev@vger.kernel.org>; Sascha Hauer
> > Subject: Re: [PATCH 04/24] rtw89: add debug files
> > 
> > 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.
> 
> I encounter the problems you mentioned mostly:
> 1. no netdev to be the parameter 'dev' of 'netif_dbg(priv, type, dev, format, args...)'
> 2. predefined 'type' isn't enough for wifi application. There're many wifi- or vendor-
>    specific components, such as RF calibration, BT coexistence, DIG, CFO and so on.
> 
> So, I don't plan to change them for now.

OK, understand.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 04/24] rtw89: add debug files

[Oleksij Rempel]

On Mon, Jul 05, 2021 at 11:08:07AM +0300, Kalle Valo wrote:
> Oleksij Rempel <o.rempel@pengutronix.de> writes:
> 
> > 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:
> >> > On Fri, Jun 18, 2021 at 02:46:05PM +0800, Ping-Ke Shih wrote:
> >> > > +#ifdef CONFIG_RTW89_DEBUGMSG
> >> > > +unsigned int rtw89_debug_mask;
> >> > > +EXPORT_SYMBOL(rtw89_debug_mask);
> >> > > +module_param_named(debug_mask, rtw89_debug_mask, uint, 0644);
> >> > > +MODULE_PARM_DESC(debug_mask, "Debugging mask");
> >> > > +#endif
> >> >
> >> >
> >> > 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.
> >
> >> 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!" ? 
> >
> > Even dynamic printk provide even more granularity. So module parameter looks
> > like stone age against all existing possibilities.
> 
> I'm all for improving wireless driver debugging features, but let's
> please keep that as a separate thread from reviewing new drivers. I
> think there are 4-5 new drivers in the queue at the moment so to keep
> all this manageable let's have the review process as simple as possible,
> please.

Ok, there is enough work to do.

> Using a module parameter for setting the debug mask is a standard
> feature in wireless drivers so it shouldn't block rtw89. If we want a
> generic debug framework for wireless drivers, an rfc patch for an
> existing upstream wireless driver is a good way to get that discussion
> forward.

Ok, sounds good.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 04/24] rtw89: add debug files

[Brian Norris]

On Fri, Jul 2, 2021 at 9:15 PM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> For example drivers/net/wireless/realtek/rtw89/debug.c is 2404 of potentially
> removable code. Some one should review it or outoptimize it by using
> existing frameworks.

Only ~28 of those lines are for the debug logging you point out.
(There's a few more in the .h file, but still.) Most of that is
unrelated code for dumping other debug info about the Realtek
chip/firmware/driver state, or performing other debugging operations.

> As you noticed, not many people are willing to review this driver. IMO,
> it is related to the RealTek reputation of making code drops with lots
> of not not usable or duplicated code. So to say - offloading the dirty work to
> the community. For example this patch set:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/drivers/staging/rts5139?h=v5.13&qt=author&q=rempel

FWIW, that driver was introduced into mainline (staging) by somebody @
Realsil, not Realtek:

https://git.kernel.org/linus/1dac4186bcc663cb8c2bcc59481aea8fe9124a6c

Sure, Realtek previously developed plenty of copy/paste/modify Linux
drivers, and it's likely most of that code was based off their
downstream drivers (which suffered from duplication), but they rarely
(never?) tried to get them merged upstream. Can you really blame them
for having non-upstream-friendly code in their
not-intended-for-upstream drivers? Now they've been nudged into doing
the upstream work themselves (*cough* *cough*) with rtw88 and now
rtw89, and IMO, they aren't suffering nearly of the same kinds of
duplication problems you note. But agreed, the reputational problems
might still be lingering.

If we're opining on lack of review: I haven't watched so many other
wireless drivers' review and inclusion in mainline (I tend to bother
with them once they're already mostly upstream, and I mostly just need
to fix bugs), but my impression is that the biggest impediment is
Kalle's limited resources. Most other successful drivers have
dedicated submaintainers who do the review or else append their name
on submissions and do pull requests, whether or not they got much
mailing list review. Everyone else who isn't so lucky has to wait in
line for Kalle, often for quite some time. This is getting a bit off
topic though.

> This new rtw89 driver seems to confirm this reputation, but I cani't say it
> for sure without spending a week on reverse engineering it.

FWIW, I've looked through it lightly (and I looked through rtw88 quite
a bit), and I don't see much (or any, really) of those same kinds of
problems. It's not perfect code of course, but I don't think
duplication is the biggest sort of problem.

Anyway, thanks for reviewing, and thanks for any issues you do point out!

Brian