[PATCH] wcn36xx: Fix logging macro with unnecessary semicolon

[PATCH] wcn36xx: Fix logging macro with unnecessary semicolon

[Joe Perches]

The wcn36xx_err macro should not end in a semicolon as
there are 2 consecutive semicolons in the preprocessed
output.

Remove it.

Neaten the logging macros to use a more common style.
Convert to use pr_debug to enable dynamic debug.

Signed-off-by: Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org>
---
 drivers/net/wireless/ath/wcn36xx/wcn36xx.h | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
index 58b6383..d157bb7 100644
--- a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
+++ b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
@@ -53,22 +53,20 @@ enum wcn36xx_debug_mask {
 	WCN36XX_DBG_ANY		= 0xffffffff,
 };
 
-#define wcn36xx_err(fmt, arg...)				\
-	printk(KERN_ERR pr_fmt("ERROR " fmt), ##arg);
-
-#define wcn36xx_warn(fmt, arg...)				\
-	printk(KERN_WARNING pr_fmt("WARNING " fmt), ##arg)
-
-#define wcn36xx_info(fmt, arg...)		\
-	printk(KERN_INFO pr_fmt(fmt), ##arg)
-
-#define wcn36xx_dbg(mask, fmt, arg...) do {			\
-	if (wcn36xx_dbg_mask & mask)					\
-		printk(KERN_DEBUG pr_fmt(fmt), ##arg);	\
+#define wcn36xx_err(fmt, ...)					\
+	pr_err("ERROR " fmt, ##__VA_ARGS__)
+#define wcn36xx_warn(fmt, ...)					\
+	pr_warn("WARNING " fmt, ##__VA_ARGS__)
+#define wcn36xx_info(fmt, ...)					\
+	pr_info(fmt, ##__VA_ARGS__)
+#define wcn36xx_dbg(mask, fmt, ...)				\
+do {								\
+	if (wcn36xx_dbg_mask & mask)				\
+		pr_debug(fmt, ##__VA_ARGS__);			\
 } while (0)

Re: [PATCH] wcn36xx: Fix logging macro with unnecessary semicolon

[Eugene Krasnikov]

Hm... when it comes to semicolon the patch seems to be good. When it
comes to dynamic debugging i think we should have a separate
discussion about that.
I personally like the whole idea about dynamic debug but if you want
to change it i would suggest to have some kind of framework for all
ath drivers(or maybe all wireless drivers). Because obviously you can
find common code in every driver that defines it's own debug
functions/debug levels and so on. Why not to make a framework with
standard API/levels?

On Tue, Nov 5, 2013 at 8:40 PM, Joe Perches <joe@perches.com> wrote:
> The wcn36xx_err macro should not end in a semicolon as
> there are 2 consecutive semicolons in the preprocessed
> output.
>
> Remove it.
>
> Neaten the logging macros to use a more common style.
> Convert to use pr_debug to enable dynamic debug.
>
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  drivers/net/wireless/ath/wcn36xx/wcn36xx.h | 28 +++++++++++++---------------
>  1 file changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
> index 58b6383..d157bb7 100644
> --- a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
> +++ b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
> @@ -53,22 +53,20 @@ enum wcn36xx_debug_mask {
>         WCN36XX_DBG_ANY         = 0xffffffff,
>  };
>
> -#define wcn36xx_err(fmt, arg...)                               \
> -       printk(KERN_ERR pr_fmt("ERROR " fmt), ##arg);
> -
> -#define wcn36xx_warn(fmt, arg...)                              \
> -       printk(KERN_WARNING pr_fmt("WARNING " fmt), ##arg)
> -
> -#define wcn36xx_info(fmt, arg...)              \
> -       printk(KERN_INFO pr_fmt(fmt), ##arg)
> -
> -#define wcn36xx_dbg(mask, fmt, arg...) do {                    \
> -       if (wcn36xx_dbg_mask & mask)                                    \
> -               printk(KERN_DEBUG pr_fmt(fmt), ##arg);  \
> +#define wcn36xx_err(fmt, ...)                                  \
> +       pr_err("ERROR " fmt, ##__VA_ARGS__)
> +#define wcn36xx_warn(fmt, ...)                                 \
> +       pr_warn("WARNING " fmt, ##__VA_ARGS__)
> +#define wcn36xx_info(fmt, ...)                                 \
> +       pr_info(fmt, ##__VA_ARGS__)
> +#define wcn36xx_dbg(mask, fmt, ...)                            \
> +do {                                                           \
> +       if (wcn36xx_dbg_mask & mask)                            \
> +               pr_debug(fmt, ##__VA_ARGS__);                   \
>  } while (0)
> -
> -#define wcn36xx_dbg_dump(mask, prefix_str, buf, len) do {      \
> -       if (wcn36xx_dbg_mask & mask)                                    \
> +#define wcn36xx_dbg_dump(mask, prefix_str, buf, len)           \
> +do {                                                           \
> +       if (wcn36xx_dbg_mask & mask)                            \
>                 print_hex_dump(KERN_DEBUG, pr_fmt(prefix_str),  \
>                                DUMP_PREFIX_OFFSET, 32, 1,       \
>                                buf, len, false);                \
>
>



-- 
Best regards,
Eugene

Re: [PATCH] wcn36xx: Fix logging macro with unnecessary semicolon

[Joe Perches]

On Wed, 2013-11-06 at 07:49 +0000, Eugene Krasnikov wrote:
> Hm... when it comes to semicolon the patch seems to be good. When it
> comes to dynamic debugging i think we should have a separate
> discussion about that.
> I personally like the whole idea about dynamic debug but if you want
> to change it i would suggest to have some kind of framework for all
> ath drivers(or maybe all wireless drivers). Because obviously you can
> find common code in every driver that defines it's own debug
> functions/debug levels and so on. Why not to make a framework with
> standard API/levels?

You need to bring that up with the Atheros folk.
I've tried.  The view seemed to be it was more
trouble than it was worth.

Re: [PATCH] wcn36xx: Fix logging macro with unnecessary semicolon

[Eugene Krasnikov]

Hi Joe,

I personally like the idea of making some kind of framework on top of
printing because all ath drivers are using the same printing approach
and combining all that code in one place will reduce amount of code in
each particular driver. As a true engineer i like when it's less code
= less work to do = less bugs:)

Suggestion is to send a patch with semicolon fix only and have a
second round of convincing ath guys to change printing code. How does
that sound?

On Wed, Nov 6, 2013 at 5:55 PM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
> On Wed, 2013-11-06 at 07:49 +0000, Eugene Krasnikov wrote:
>> Hm... when it comes to semicolon the patch seems to be good. When it
>> comes to dynamic debugging i think we should have a separate
>> discussion about that.
>> I personally like the whole idea about dynamic debug but if you want
>> to change it i would suggest to have some kind of framework for all
>> ath drivers(or maybe all wireless drivers). Because obviously you can
>> find common code in every driver that defines it's own debug
>> functions/debug levels and so on. Why not to make a framework with
>> standard API/levels?
>
> You need to bring that up with the Atheros folk.
> I've tried.  The view seemed to be it was more
> trouble than it was worth.
>
>
>



-- 
Best regards,
Eugene
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] wcn36xx: Fix logging macro with unnecessary semicolon

[Joe Perches]

On Thu, 2013-11-07 at 07:32 +0000, Eugene Krasnikov wrote:
> Hi Joe,

Hi Eugene.

> I personally like the idea of making some kind of framework on top of
> printing because all ath drivers are using the same printing approach
> and combining all that code in one place will reduce amount of code in
> each particular driver. As a true engineer i like when it's less code
> = less work to do = less bugs:)
> 
> Suggestion is to send a patch with semicolon fix only

It's a one line fix, and pretty trivial to do yourself if you
want it.

I do think though the slightly odd wcn36xx_<level> uses of:

-#define wcn36xx_err(fmt, arg...)                               \
-       printk(KERN_ERR pr_fmt("ERROR " fmt), ##arg);
-
-#define wcn36xx_warn(fmt, arg...)                              \
-       printk(KERN_WARNING pr_fmt("WARNING " fmt), ##arg)
-
-#define wcn36xx_info(fmt, arg...)              \
-       printk(KERN_INFO pr_fmt(fmt), ##arg)
-
[]
+#define wcn36xx_err(fmt, ...)                                  \
+       pr_err("ERROR " fmt, ##__VA_ARGS__)
+#define wcn36xx_warn(fmt, ...)                                 \
+       pr_warn("WARNING " fmt, ##__VA_ARGS__)
+#define wcn36xx_info(fmt, ...)                                 \
+       pr_info(fmt, ##__VA_ARGS__)

should still be converted to more normal pr_<level> like I did.

I don't believe that prefixing of "ERROR " and "WARNING " to
pr_err and pr_warn actually gain anything as the same level
information can be got already with dmesg -r via the "<3>" and
"<4>" prefixes or dmesg -l3 -l4

> have a
> second round of convincing ath guys to change printing code. How does
> that sound?

Luis?  Kalle?  Other Qualcomm/Atheros folk?

One of the nominal benefits of separating the ath_<level>
macros by subsystem was perf/tracing.  I believe some of the
ath subsystems have integrated the dmesg and tracing code.

I'm not sure how separable those may be here or if it's even
desirable to separate them.  I'd guess not.

Re: [PATCH] wcn36xx: Fix logging macro with unnecessary semicolon

[Kalle Valo]

Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> writes:

>> have a
>> second round of convincing ath guys to change printing code. How does
>> that sound?
>
> Luis?  Kalle?  Other Qualcomm/Atheros folk?

I didn't quite get what you are asking from me. But for me most
important is that the current debugging facilities from user's point of
view don't change. I don't care if the code is in ath10k.ko or ath.ko,
we are talking about ~100 lines of code anyway.

> One of the nominal benefits of separating the ath_<level>
> macros by subsystem was perf/tracing.

Nominal?

-- 
Kalle Valo
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html