* Re: [PATCH 04/24] rtw89: add debug files [not found] ` <CA+ASDXNjHJoXgRAM4E7TcLuz9zBmQkaBMuhK2DEVy3dnE-3XcA@mail.gmail.com> @ 2021-07-02 17:57 ` Oleksij Rempel 2021-07-02 18:38 ` Brian Norris 2021-07-05 8:08 ` Kalle Valo 0 siblings, 2 replies; 10+ messages in thread From: Oleksij Rempel @ 2021-07-02 17:57 UTC (permalink / raw) To: Brian Norris; +Cc: Ping-Ke Shih, Kalle Valo, linux-wireless, netdev, kernel 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 | ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 04/24] rtw89: add debug files 2021-07-02 17:57 ` [PATCH 04/24] rtw89: add debug files Oleksij Rempel @ 2021-07-02 18:38 ` Brian Norris 2021-07-02 19:32 ` Oleksij Rempel 2021-07-05 8:58 ` Pkshih 2021-07-05 8:08 ` Kalle Valo 1 sibling, 2 replies; 10+ messages in thread From: Brian Norris @ 2021-07-02 18:38 UTC (permalink / raw) To: Oleksij Rempel Cc: Ping-Ke Shih, Kalle Valo, linux-wireless, <netdev@vger.kernel.org>, Sascha Hauer 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 04/24] rtw89: add debug files 2021-07-02 18:38 ` Brian Norris @ 2021-07-02 19:32 ` Oleksij Rempel 2021-07-02 20:00 ` Brian Norris 2021-07-05 8:58 ` Pkshih 1 sibling, 1 reply; 10+ messages in thread From: Oleksij Rempel @ 2021-07-02 19:32 UTC (permalink / raw) To: Brian Norris Cc: Ping-Ke Shih, Kalle Valo, linux-wireless, <netdev@vger.kernel.org>, Sascha Hauer 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 | ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 04/24] rtw89: add debug files 2021-07-02 19:32 ` Oleksij Rempel @ 2021-07-02 20:00 ` Brian Norris 2021-07-03 4:15 ` Oleksij Rempel 0 siblings, 1 reply; 10+ messages in thread From: Brian Norris @ 2021-07-02 20:00 UTC (permalink / raw) To: Oleksij Rempel Cc: Ping-Ke Shih, Kalle Valo, linux-wireless, <netdev@vger.kernel.org>, Sascha Hauer 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 04/24] rtw89: add debug files 2021-07-02 20:00 ` Brian Norris @ 2021-07-03 4:15 ` Oleksij Rempel 2021-07-13 1:40 ` Brian Norris 0 siblings, 1 reply; 10+ messages in thread From: Oleksij Rempel @ 2021-07-03 4:15 UTC (permalink / raw) To: Brian Norris Cc: <netdev@vger.kernel.org>, Ping-Ke Shih, linux-wireless, Sascha Hauer, Kalle Valo 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 | ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 04/24] rtw89: add debug files 2021-07-03 4:15 ` Oleksij Rempel @ 2021-07-13 1:40 ` Brian Norris 0 siblings, 0 replies; 10+ messages in thread From: Brian Norris @ 2021-07-13 1:40 UTC (permalink / raw) To: Oleksij Rempel Cc: <netdev@vger.kernel.org>, Ping-Ke Shih, linux-wireless, Sascha Hauer, Kalle Valo 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH 04/24] rtw89: add debug files 2021-07-02 18:38 ` Brian Norris 2021-07-02 19:32 ` Oleksij Rempel @ 2021-07-05 8:58 ` Pkshih 2021-07-05 9:09 ` Oleksij Rempel 1 sibling, 1 reply; 10+ messages in thread From: Pkshih @ 2021-07-05 8:58 UTC (permalink / raw) To: Brian Norris, Oleksij Rempel Cc: Kalle Valo, linux-wireless, <netdev@vger.kernel.org>, Sascha Hauer > -----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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 04/24] rtw89: add debug files 2021-07-05 8:58 ` Pkshih @ 2021-07-05 9:09 ` Oleksij Rempel 0 siblings, 0 replies; 10+ messages in thread From: Oleksij Rempel @ 2021-07-05 9:09 UTC (permalink / raw) To: Pkshih Cc: Brian Norris, <netdev@vger.kernel.org>, linux-wireless, Sascha Hauer, Kalle Valo 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 | ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 04/24] rtw89: add debug files 2021-07-02 17:57 ` [PATCH 04/24] rtw89: add debug files Oleksij Rempel 2021-07-02 18:38 ` Brian Norris @ 2021-07-05 8:08 ` Kalle Valo 2021-07-05 9:23 ` Oleksij Rempel 1 sibling, 1 reply; 10+ messages in thread From: Kalle Valo @ 2021-07-05 8:08 UTC (permalink / raw) To: Oleksij Rempel; +Cc: Brian Norris, Ping-Ke Shih, linux-wireless, netdev, kernel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 04/24] rtw89: add debug files 2021-07-05 8:08 ` Kalle Valo @ 2021-07-05 9:23 ` Oleksij Rempel 0 siblings, 0 replies; 10+ messages in thread From: Oleksij Rempel @ 2021-07-05 9:23 UTC (permalink / raw) To: Kalle Valo; +Cc: Brian Norris, Ping-Ke Shih, linux-wireless, netdev, kernel 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 | ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-07-13 1:40 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20210618064625.14131-1-pkshih@realtek.com>
[not found] ` <20210618064625.14131-5-pkshih@realtek.com>
[not found] ` <20210702072308.GA4184@pengutronix.de>
[not found] ` <CA+ASDXNjHJoXgRAM4E7TcLuz9zBmQkaBMuhK2DEVy3dnE-3XcA@mail.gmail.com>
2021-07-02 17:57 ` [PATCH 04/24] rtw89: add debug files Oleksij Rempel
2021-07-02 18:38 ` Brian Norris
2021-07-02 19:32 ` Oleksij Rempel
2021-07-02 20:00 ` Brian Norris
2021-07-03 4:15 ` Oleksij Rempel
2021-07-13 1:40 ` Brian Norris
2021-07-05 8:58 ` Pkshih
2021-07-05 9:09 ` Oleksij Rempel
2021-07-05 8:08 ` Kalle Valo
2021-07-05 9:23 ` Oleksij Rempel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).