Re: [PATCH 10/14] rtlwifi: Add BTC_TRACE_STRING to new btcoex

[Joe Perches]

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.

Re: [PATCH 10/14] rtlwifi: Add BTC_TRACE_STRING to new btcoex

[Markus Böhme]

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

Re: [PATCH 10/14] rtlwifi: Add BTC_TRACE_STRING to new btcoex

[Kalle Valo]

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

Re: [PATCH 10/14] rtlwifi: Add BTC_TRACE_STRING to new btcoex

[Kalle Valo]

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

Re: [PATCH 10/14] rtlwifi: Add BTC_TRACE_STRING to new btcoex

[Dan Carpenter]

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

Re: [PATCH 10/14] rtlwifi: Add BTC_TRACE_STRING to new btcoex

[Dan Carpenter]

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

Re: [PATCH 10/14] rtlwifi: Add BTC_TRACE_STRING to new btcoex

[Kalle Valo]

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

Re: [PATCH 10/14] rtlwifi: Add BTC_TRACE_STRING to new btcoex

[Larry Finger]

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

Re: [PATCH 10/14] rtlwifi: Add BTC_TRACE_STRING to new btcoex

[Dan Carpenter]

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

Re: [PATCH 10/14] rtlwifi: Add BTC_TRACE_STRING to new btcoex

[Kalle Valo]

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

Re: [PATCH 10/14] rtlwifi: Add BTC_TRACE_STRING to new btcoex

[Greg KH]

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

Re: [PATCH 10/14] rtlwifi: Add BTC_TRACE_STRING to new btcoex

[Larry Finger]

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

Re: [PATCH 10/14] rtlwifi: Add BTC_TRACE_STRING to new btcoex

[Dan Carpenter]

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

[PATCH 10/14] rtlwifi: Add BTC_TRACE_STRING to new btcoex

[Larry Finger]

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,