Re: [PATCH 10/14] rtlwifi: Add BTC_TRACE_STRING to new btcoex
On Sat, 2016-12-10 at 00:56 +0100, Markus Böhme wrote: > On 12/09/2016 09:50 AM, Kalle Valo wrote: > > Dan Carpenter writes: > > > > > On Thu, Dec 08, 2016 at 02:50:49PM +0300, Dan Carpenter wrote: > > > > On Thu, Dec 08, 2016 at 01:43:42PM +0200, Kalle Valo wrote: > > > > > But it would make me very happy if someone would add a similar > > > > > grouping > > > > > functionality to dyndbg to make it easy to enable a set of debug > > > > > messages in a driver. > > > > > > > > Thats seems like a reasonable thing as well. > > > > > > I actually like the ath code... We could easily change it to be more > > > generic and make it a top level function for everyone to use. > > > > That would be great. And maybe add a sysfs file with a description > > different levels, like module parameters have. Too bad that I can't work > > on that, too much stuff on my plate right now. > > > > In this case I would like to step in and try to implement grouping of > messages in the dynamic debug code. This seems to be an interesting > learning opportunity. Use bitmaps and levels. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 10/14] rtlwifi: Add BTC_TRACE_STRING to new btcoex
On 12/09/2016 09:50 AM, Kalle Valo wrote: > Dan Carpenter writes: > >> On Thu, Dec 08, 2016 at 02:50:49PM +0300, Dan Carpenter wrote: >>> On Thu, Dec 08, 2016 at 01:43:42PM +0200, Kalle Valo wrote: But it would make me very happy if someone would add a similar grouping functionality to dyndbg to make it easy to enable a set of debug messages in a driver. >>> >>> Thats seems like a reasonable thing as well. >> >> I actually like the ath code... We could easily change it to be more >> generic and make it a top level function for everyone to use. > > That would be great. And maybe add a sysfs file with a description > different levels, like module parameters have. Too bad that I can't work > on that, too much stuff on my plate right now. > In this case I would like to step in and try to implement grouping of messages in the dynamic debug code. This seems to be an interesting learning opportunity. Regards, Markus ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 10/14] rtlwifi: Add BTC_TRACE_STRING to new btcoex
Dan Carpenter writes: > On Thu, Dec 08, 2016 at 02:50:49PM +0300, Dan Carpenter wrote: >> On Thu, Dec 08, 2016 at 01:43:42PM +0200, Kalle Valo wrote: >> > But it would make me very happy if someone would add a similar grouping >> > functionality to dyndbg to make it easy to enable a set of debug >> > messages in a driver. >> >> Thats seems like a reasonable thing as well. > > I actually like the ath code... We could easily change it to be more > generic and make it a top level function for everyone to use. That would be great. And maybe add a sysfs file with a description different levels, like module parameters have. Too bad that I can't work on that, too much stuff on my plate right now. -- Kalle Valo ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 10/14] rtlwifi: Add BTC_TRACE_STRING to new btcoex
Dan Carpenter writes: > I don't have a problem with the ath debug printks. Larry asked me for > examples of better debug functions and the ath code is an example. > Literally, any existing debug functions are better than the > BTC_TRACE_STRING() stuff. Sure, I agree with that. My point was just that sometimes it's ok to have own logging wrappers and there's no hard rule for this. -- Kalle Valo ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 10/14] rtlwifi: Add BTC_TRACE_STRING to new btcoex
On Thu, Dec 08, 2016 at 02:50:49PM +0300, Dan Carpenter wrote: > On Thu, Dec 08, 2016 at 01:43:42PM +0200, Kalle Valo wrote: > > But it would make me very happy if someone would add a similar grouping > > functionality to dyndbg to make it easy to enable a set of debug > > messages in a driver. > > Thats seems like a reasonable thing as well. I actually like the ath code... We could easily change it to be more generic and make it a top level function for everyone to use. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 10/14] rtlwifi: Add BTC_TRACE_STRING to new btcoex
I don't have a problem with the ath debug printks. Larry asked me for examples of better debug functions and the ath code is an example. Literally, any existing debug functions are better than the BTC_TRACE_STRING() stuff. On Thu, Dec 08, 2016 at 01:43:42PM +0200, Kalle Valo wrote: > But it would make me very happy if someone would add a similar grouping > functionality to dyndbg to make it easy to enable a set of debug > messages in a driver. Thats seems like a reasonable thing as well. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 10/14] rtlwifi: Add BTC_TRACE_STRING to new btcoex
Dan Carpenter writes: > On Wed, Dec 07, 2016 at 02:16:06PM +0200, Kalle Valo wrote: >> We have disccused this before, but for wireless it's not really that >> simple. AFAIK with dyndbg you can only control the messages per line >> (painful to enable group of messages) or per file (enables too many >> messages). In wireless we have cases when we need to enable group of >> messages, but not all. > > You can turn them on by the function or a range of lines, then disable > the spammy lines. With these new debug macros you can't do that so this > is a step backwards. >From my point of view dyndbg is step backwards. Line numbers change all the time and you can't have simple instructions like this for users and developers working with the driver: https://wireless.wiki.kernel.org/en/users/drivers/ath10k/debug#debug_log_messages I don't want waste time with this anymore as we seem to just disagree. My point here was that in wireless we have used log wrappers before and will continue to use them in certain drivers as dyndbg is not enough. But it would make me very happy if someone would add a similar grouping functionality to dyndbg to make it easy to enable a set of debug messages in a driver. -- Kalle Valo ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 10/14] rtlwifi: Add BTC_TRACE_STRING to new btcoex
On 12/07/2016 07:32 AM, Dan Carpenter wrote: On Wed, Dec 07, 2016 at 02:16:06PM +0200, Kalle Valo wrote: We have disccused this before, but for wireless it's not really that simple. AFAIK with dyndbg you can only control the messages per line (painful to enable group of messages) or per file (enables too many messages). In wireless we have cases when we need to enable group of messages, but not all. You can turn them on by the function or a range of lines, then disable the spammy lines. With these new debug macros you can't do that so this is a step backwards. If I'm totally honest, I've never seen uglier macros than these. I work in staging and I've spent a lot of time in ancient code but these ones really take the cake. They will be coming out. The Realtek engineer and I are learning more about the various options to see what to use. The dynamic debugging options seem to be the best at the moment. Larry ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 10/14] rtlwifi: Add BTC_TRACE_STRING to new btcoex
On Wed, Dec 07, 2016 at 02:16:06PM +0200, Kalle Valo wrote: > We have disccused this before, but for wireless it's not really that > simple. AFAIK with dyndbg you can only control the messages per line > (painful to enable group of messages) or per file (enables too many > messages). In wireless we have cases when we need to enable group of > messages, but not all. You can turn them on by the function or a range of lines, then disable the spammy lines. With these new debug macros you can't do that so this is a step backwards. If I'm totally honest, I've never seen uglier macros than these. I work in staging and I've spent a lot of time in ancient code but these ones really take the cake. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 10/14] rtlwifi: Add BTC_TRACE_STRING to new btcoex
Greg KH writes: > On Mon, Dec 05, 2016 at 04:34:08PM -0600, Larry Finger wrote: >> On 12/05/2016 03:34 PM, Dan Carpenter wrote: >> > On Thu, Dec 01, 2016 at 07:48:29PM -0600, Larry Finger wrote: >> > > --- >> > > wireless-drivers-next.orig/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtcoutsrc.h >> > > +++ >> > > wireless-drivers-next/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtcoutsrc.h >> > > @@ -27,6 +27,29 @@ >> > > >> > > #include"../wifi.h" >> > > >> > > +#ifdef CONFIG_RTLWIFI_DEBUG >> > > + >> > > +#define BTC_SPRINTF(ptr, ...) snprintf(ptr, ##__VA_ARGS__) >> > > +#define BTC_TRACE(fmt) \ >> > > +do {\ >> > > +struct rtl_priv *rtlpriv = gl_bt_coexist.adapter; \ >> > > +if (!rtlpriv) \ >> > > +break; \ >> > > +RT_TRACE_STRING(rtlpriv, COMP_COEX, DBG_LOUD, fmt); \ >> > > +} while (0) >> > > + >> > >> > This sort of macro is exactly when the rtl drivers spent so long in >> > staging... Subsystems should not invent their own tracing but in >> > particular these macros are so very very ugly. >> > >> > It's just super frustrating to even look at this... >> > >> > There are a lot of staging drivers I feel good about when they leave. >> > The HyperV drivers. The IIO stuff. A lot of the media stuff and >> > generally the media tree is getting better and better. I like comedi >> > and unisys, those are in staging, but they are great and could move out >> > any time as far as I'm concerned. >> > >> > But this patch just makes me super discouraged. What are we doing??? >> >> Dan, >> >> It would not matter to me if these drivers got moved to staging, but there >> are a lot of users whose distros do not build staging drivers that would be >> very unhappy. >> >> Can you point me to a driver with a better way to conditionally dump a >> debugging string to the logs? > > Just use 'dev_dbg()', or 'pr_debug()' if you don't have a device pointer > (hint, all drivers should have that pointer). That can be turned on or > off by a user dynamically as the kernel runs. No need to invent fancy > custom macros for things we have already for many many years. We have disccused this before, but for wireless it's not really that simple. AFAIK with dyndbg you can only control the messages per line (painful to enable group of messages) or per file (enables too many messages). In wireless we have cases when we need to enable group of messages, but not all. For example, some of the messages can slow down the system so much that the bug is not reproducable anymore. So unless dyndbg gets some sort of grouping support logging wrappers are needed with wireless code. (I'm talking in general here, I haven't looked at rtlwifi patches in detail yet.) -- Kalle Valo ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 10/14] rtlwifi: Add BTC_TRACE_STRING to new btcoex
On Mon, Dec 05, 2016 at 04:34:08PM -0600, Larry Finger wrote: > Can you point me to a driver with a better way to conditionally dump > a debugging string to the logs? People should look at using ftrace and kprobes. They really are very powerful and useful. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 10/14] rtlwifi: Add BTC_TRACE_STRING to new btcoex
On Mon, Dec 05, 2016 at 04:34:08PM -0600, Larry Finger wrote: > On 12/05/2016 03:34 PM, Dan Carpenter wrote: > > On Thu, Dec 01, 2016 at 07:48:29PM -0600, Larry Finger wrote: > > > --- > > > wireless-drivers-next.orig/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtcoutsrc.h > > > +++ > > > wireless-drivers-next/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtcoutsrc.h > > > @@ -27,6 +27,29 @@ > > > > > > #include "../wifi.h" > > > > > > +#ifdef CONFIG_RTLWIFI_DEBUG > > > + > > > +#define BTC_SPRINTF(ptr, ...)snprintf(ptr, ##__VA_ARGS__) > > > +#define BTC_TRACE(fmt) \ > > > +do { \ > > > + struct rtl_priv *rtlpriv = gl_bt_coexist.adapter; \ > > > + if (!rtlpriv) \ > > > + break; \ > > > + RT_TRACE_STRING(rtlpriv, COMP_COEX, DBG_LOUD, fmt); \ > > > +} while (0) > > > + > > > > This sort of macro is exactly when the rtl drivers spent so long in > > staging... Subsystems should not invent their own tracing but in > > particular these macros are so very very ugly. > > > > It's just super frustrating to even look at this... > > > > There are a lot of staging drivers I feel good about when they leave. > > The HyperV drivers. The IIO stuff. A lot of the media stuff and > > generally the media tree is getting better and better. I like comedi > > and unisys, those are in staging, but they are great and could move out > > any time as far as I'm concerned. > > > > But this patch just makes me super discouraged. What are we doing??? > > Dan, > > It would not matter to me if these drivers got moved to staging, but there > are a lot of users whose distros do not build staging drivers that would be > very unhappy. > > Can you point me to a driver with a better way to conditionally dump a > debugging string to the logs? Just use 'dev_dbg()', or 'pr_debug()' if you don't have a device pointer (hint, all drivers should have that pointer). That can be turned on or off by a user dynamically as the kernel runs. No need to invent fancy custom macros for things we have already for many many years. thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 10/14] rtlwifi: Add BTC_TRACE_STRING to new btcoex
On 12/05/2016 03:34 PM, Dan Carpenter wrote: On Thu, Dec 01, 2016 at 07:48:29PM -0600, Larry Finger wrote: --- wireless-drivers-next.orig/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtcoutsrc.h +++ wireless-drivers-next/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtcoutsrc.h @@ -27,6 +27,29 @@ #include "../wifi.h" +#ifdef CONFIG_RTLWIFI_DEBUG + +#define BTC_SPRINTF(ptr, ...) snprintf(ptr, ##__VA_ARGS__) +#define BTC_TRACE(fmt) \ +do { \ + struct rtl_priv *rtlpriv = gl_bt_coexist.adapter; \ + if (!rtlpriv) \ + break; \ + RT_TRACE_STRING(rtlpriv, COMP_COEX, DBG_LOUD, fmt); \ +} while (0) + This sort of macro is exactly when the rtl drivers spent so long in staging... Subsystems should not invent their own tracing but in particular these macros are so very very ugly. It's just super frustrating to even look at this... There are a lot of staging drivers I feel good about when they leave. The HyperV drivers. The IIO stuff. A lot of the media stuff and generally the media tree is getting better and better. I like comedi and unisys, those are in staging, but they are great and could move out any time as far as I'm concerned. But this patch just makes me super discouraged. What are we doing??? Dan, It would not matter to me if these drivers got moved to staging, but there are a lot of users whose distros do not build staging drivers that would be very unhappy. Can you point me to a driver with a better way to conditionally dump a debugging string to the logs? Larry ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 10/14] rtlwifi: Add BTC_TRACE_STRING to new btcoex
On Thu, Dec 01, 2016 at 07:48:29PM -0600, Larry Finger wrote: > --- > wireless-drivers-next.orig/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtcoutsrc.h > +++ > wireless-drivers-next/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtcoutsrc.h > @@ -27,6 +27,29 @@ > > #include "../wifi.h" > > +#ifdef CONFIG_RTLWIFI_DEBUG > + > +#define BTC_SPRINTF(ptr, ...)snprintf(ptr, ##__VA_ARGS__) > +#define BTC_TRACE(fmt) \ > +do { \ > + struct rtl_priv *rtlpriv = gl_bt_coexist.adapter; \ > + if (!rtlpriv) \ > + break; \ > + RT_TRACE_STRING(rtlpriv, COMP_COEX, DBG_LOUD, fmt); \ > +} while (0) > + This sort of macro is exactly when the rtl drivers spent so long in staging... Subsystems should not invent their own tracing but in particular these macros are so very very ugly. It's just super frustrating to even look at this... There are a lot of staging drivers I feel good about when they leave. The HyperV drivers. The IIO stuff. A lot of the media stuff and generally the media tree is getting better and better. I like comedi and unisys, those are in staging, but they are great and could move out any time as far as I'm concerned. But this patch just makes me super discouraged. What are we doing??? regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 10/14] rtlwifi: Add BTC_TRACE_STRING to new btcoex
From: Ping-Ke Shih Add a new debugging function. Signed-off-by: Ping-Ke Shih Signed-off-by: Larry Finger --- .../realtek/rtlwifi/btcoexist/halbtcoutsrc.h | 23 ++ drivers/net/wireless/realtek/rtlwifi/debug.h | 14 + 2 files changed, 37 insertions(+) Index: wireless-drivers-next/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtcoutsrc.h === --- wireless-drivers-next.orig/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtcoutsrc.h +++ wireless-drivers-next/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtcoutsrc.h @@ -27,6 +27,29 @@ #include "../wifi.h" +#ifdef CONFIG_RTLWIFI_DEBUG + +#define BTC_SPRINTF(ptr, ...) snprintf(ptr, ##__VA_ARGS__) +#define BTC_TRACE(fmt) \ +do { \ + struct rtl_priv *rtlpriv = gl_bt_coexist.adapter; \ + if (!rtlpriv) \ + break; \ + RT_TRACE_STRING(rtlpriv, COMP_COEX, DBG_LOUD, fmt); \ +} while (0) + +#else + +static inline void BTC_SPRINTF(char *ptr, ...) +{ +} + +static inline void BTC_TRACE(const char *ptr) +{ +} + +#endif + #defineNORMAL_EXEC false #defineFORCE_EXEC true Index: wireless-drivers-next/drivers/net/wireless/realtek/rtlwifi/debug.h === --- wireless-drivers-next.orig/drivers/net/wireless/realtek/rtlwifi/debug.h +++ wireless-drivers-next/drivers/net/wireless/realtek/rtlwifi/debug.h @@ -194,6 +194,15 @@ do { \ } \ } while (0) +#define RT_TRACE_STRING(__priv, comp, level, string) \ +do { \ + if (unlikely(((comp) & __priv->dbg.global_debugcomponents) && \ +((level) <= __priv->dbg.global_debuglevel))) { \ + printk(KBUILD_MODNAME ":%s():<%lx> %s", \ + __func__, in_interrupt(), string); \ + } \ +} while (0) + #define RT_PRINT_DATA(rtlpriv, _comp, _level, _titlestring, _hexdata, \ _hexdatalen) \ do { \ @@ -230,6 +239,11 @@ static inline void RTPRINT(struct rtl_pr { } +static inline void RT_TRACE_STRING(struct rtl_priv *rtlpriv, + u64 comp, int level, const char *string) +{ +} + static inline void RT_PRINT_DATA(struct rtl_priv *rtlpriv, u64 comp, int level, const char *titlestring, ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel