New sparse warning in net/mac80211/debugfs_sta.c

New sparse warning in net/mac80211/debugfs_sta.c

[Harvey Harrison]

commit 0795af5729b18218767fab27c44b1384f72dc9ad
Author: Joe Perches <joe@perches.com>
Date:   Wed Oct 3 17:59:30 2007 -0700

    [NET]: Introduce and use print_mac() and DECLARE_MAC_BUF()
    
    This is nicer than the MAC_FMT stuff.
    
    Signed-off-by: Joe Perches <joe@perches.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

Introduced:
net/mac80211/debugfs_sta.c: In function ‘ieee80211_sta_debugfs_add’:
net/mac80211/debugfs_sta.c:211: warning: statement with no effect

Does print_mac modify the mac buffer in-place, or return a new buffer?

Harvey

Re: New sparse warning in net/mac80211/debugfs_sta.c

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1072 bytes --]


On Thu, 2008-02-21 at 01:34 -0800, Harvey Harrison wrote:
> commit 0795af5729b18218767fab27c44b1384f72dc9ad
> Author: Joe Perches <joe@perches.com>
> Date:   Wed Oct 3 17:59:30 2007 -0700
> 
>     [NET]: Introduce and use print_mac() and DECLARE_MAC_BUF()
>     
>     This is nicer than the MAC_FMT stuff.
>     
>     Signed-off-by: Joe Perches <joe@perches.com>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> Introduced:
> net/mac80211/debugfs_sta.c: In function ‘ieee80211_sta_debugfs_add’:
> net/mac80211/debugfs_sta.c:211: warning: statement with no effect
> 
> Does print_mac modify the mac buffer in-place, or return a new buffer?

It modifies the buffer, and I think it's more likely that the warning
was introduced by

commit 8f789c48448aed74fe1c07af76de8f04adacec7d
Author: David S. Miller <davem@davemloft.net>
Date:   Mon Feb 18 16:50:22 2008 -0800

    [NET]: Elminate spurious print_mac() calls.

since __pure indicates that the function has no side effects apart from
the return value (IIRC)

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: New sparse warning in net/mac80211/debugfs_sta.c

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Thu, 21 Feb 2008 10:54:59 +0100

> It modifies the buffer, and I think it's more likely that the warning
> was introduced by
 ...
>     [NET]: Elminate spurious print_mac() calls.

Right and that's what I just suggested in another reply.

Harvey how did you "narrow it down" to Joe's patch?  Did you really
bisect to figure it out, or did you just guess?  You said "commit X
introduced" and if you really didn't bisect it down to that
such a claim is very misleading.

Thanks.

Re: New sparse warning in net/mac80211/debugfs_sta.c

[Harvey Harrison]

On Thu, 2008-02-21 at 02:03 -0800, David Miller wrote:
> From: Johannes Berg <johannes@sipsolutions.net>
> Date: Thu, 21 Feb 2008 10:54:59 +0100
> 
> > It modifies the buffer, and I think it's more likely that the warning
> > was introduced by
>  ...
> >     [NET]: Elminate spurious print_mac() calls.
> 
> Right and that's what I just suggested in another reply.
> 
> Harvey how did you "narrow it down" to Joe's patch?  Did you really
> bisect to figure it out, or did you just guess?  You said "commit X
> introduced" and if you really didn't bisect it down to that
> such a claim is very misleading.

Sorry, that was misleading, it was a simple-minded git-log/git-blame
very short look, saw that line introduced here.

Really, that should have read:

Sparse warning introduced since -rc2:
..etc

Harvey's best guess is this commit...

I'll be more careful about this language going forward.

Cheers,

Harvey

Re: New sparse warning in net/mac80211/debugfs_sta.c

[David Miller]

From: Harvey Harrison <harvey.harrison@gmail.com>
Date: Thu, 21 Feb 2008 02:08:18 -0800

> Really, that should have read:
> 
> Sparse warning introduced since -rc2:
> ..etc
> 
> Harvey's best guess is this commit...

I think the commit you originally blamed got added in 2.6.24,
so it would be very difficult for it to cause a post-2.6.25-rc2
regression :-)

Re: New sparse warning in net/mac80211/debugfs_sta.c

[Harvey Harrison]

On Thu, 2008-02-21 at 02:12 -0800, David Miller wrote:
> From: Harvey Harrison <harvey.harrison@gmail.com>
> Date: Thu, 21 Feb 2008 02:08:18 -0800
> 
> > Really, that should have read:
> > 
> > Sparse warning introduced since -rc2:
> > ..etc
> > 
> > Harvey's best guess is this commit...
> 
> I think the commit you originally blamed got added in 2.6.24,
> so it would be very difficult for it to cause a post-2.6.25-rc2
> regression :-)
> 

I don't view it as a regression, just seeing if warning maintainers
incrementally when new sparse warnings get in ends up being well
received.

In addition to trying to lower the noise level of the existing
warnings, might as well try to stem the tide of new ones.

Harvey

[PATCH] mac80211: fix debugfs_sta print_mac() warning

[Johannes Berg]

When print_mac() was marked as __pure to avoid emitting a function
call in pr_debug() scenarios, a warning in this code surfaced since
it relies on the fact that the buffer is modified and doesn't use
the return value. This patch makes it use the return value instead.

Signed-off-by: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
Reported-by: Harvey Harrison <harvey.harrison-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 net/mac80211/debugfs_sta.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

--- everything.orig/net/mac80211/debugfs_sta.c	2008-02-21 11:17:48.000000000 +0100
+++ everything/net/mac80211/debugfs_sta.c	2008-02-21 11:17:59.000000000 +0100
@@ -297,12 +297,13 @@ STA_OPS_WR(agg_status);
 void ieee80211_sta_debugfs_add(struct sta_info *sta)
 {
 	struct dentry *stations_dir = sta->local->debugfs.stations;
-	DECLARE_MAC_BUF(mac);
+	DECLARE_MAC_BUF(mbuf);
+	u8 *mac;
 
 	if (!stations_dir)
 		return;
 
-	print_mac(mac, sta->addr);
+	mac = print_mac(mbuf, sta->addr);
 
 	sta->debugfs.dir = debugfs_create_dir(mac, stations_dir);
 	if (!sta->debugfs.dir)

Re: New sparse warning in net/mac80211/debugfs_sta.c

[Harvey Harrison]

On Thu, 2008-02-21 at 01:57 -0800, David Miller wrote:
> From: Harvey Harrison <harvey.harrison@gmail.com>
> Date: Thu, 21 Feb 2008 01:34:27 -0800
> 
> > commit 0795af5729b18218767fab27c44b1384f72dc9ad
> > Author: Joe Perches <joe@perches.com>
> > Date:   Wed Oct 3 17:59:30 2007 -0700
> > 
> >     [NET]: Introduce and use print_mac() and DECLARE_MAC_BUF()
> >     
> >     This is nicer than the MAC_FMT stuff.
> >     
> >     Signed-off-by: Joe Perches <joe@perches.com>
> >     Signed-off-by: David S. Miller <davem@davemloft.net>
> > 
> > Introduced:
> > net/mac80211/debugfs_sta.c: In function ‘ieee80211_sta_debugfs_add’:
> > net/mac80211/debugfs_sta.c:211: warning: statement with no effect
> > 
> > Does print_mac modify the mac buffer in-place, or return a new buffer?
> 
> It modifies the buffer in place.
> 
> The warning is likely not from Joe's patch, but more likely
> from the __pure attribute I recently added to print_mac()'s
> declaration.  If so, we'll need to perhaps rework things.

In this case, it's being passed to a debugfs create function, could it
instead use sysfs_format_mac?

Harvey

Re: New sparse warning in net/mac80211/debugfs_sta.c

[David Miller]

From: Harvey Harrison <harvey.harrison@gmail.com>
Date: Thu, 21 Feb 2008 02:01:19 -0800

> In this case, it's being passed to a debugfs create function, could it
> instead use sysfs_format_mac?

Just assigning print_mac() to a local variable then passing that to
debugfs_create_dir() will make the warning go away.

>From another perspective adding that __pure attribute to print_mac()
might not have been the best idea.  But I can't think of another
way to elimitate the "passing print_mac() args to pr_debug()
still generates calls to print_mac() even when DEBUG is not
defined" problem :-/

Re: New sparse warning in net/mac80211/debugfs_sta.c

[Harvey Harrison]

On Thu, 2008-02-21 at 02:05 -0800, David Miller wrote:
> From: Harvey Harrison <harvey.harrison@gmail.com>
> Date: Thu, 21 Feb 2008 02:01:19 -0800
> 
> > In this case, it's being passed to a debugfs create function, could it
> > instead use sysfs_format_mac?
> 
> Just assigning print_mac() to a local variable then passing that to
> debugfs_create_dir() will make the warning go away.

Well, in this case I'd suggest either reverting the __pure annotation
or just ignoring the warning.

Harvey

Re: New sparse warning in net/mac80211/debugfs_sta.c

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1191 bytes --]


On Thu, 2008-02-21 at 02:05 -0800, David Miller wrote:
> From: Harvey Harrison <harvey.harrison@gmail.com>
> Date: Thu, 21 Feb 2008 02:01:19 -0800
> 
> > In this case, it's being passed to a debugfs create function, could it
> > instead use sysfs_format_mac?
> 
> Just assigning print_mac() to a local variable then passing that to
> debugfs_create_dir() will make the warning go away.

Indeed, either of those will work.

> From another perspective adding that __pure attribute to print_mac()
> might not have been the best idea.  But I can't think of another
> way to elimitate the "passing print_mac() args to pr_debug()
> still generates calls to print_mac() even when DEBUG is not
> defined" problem :-/

Yeah, I saw that discussion. I think it's fine, it's just something we
need to be aware of. In fact, I Joe had a patch (that seems to have
gotten lost?) to make DECLARE_MAC_BUF() declare a structure with the u8
pointer in it instead to get type checking for the args, which would
make our code there not even compile, and imho rightfully so. I'll send
in a patch to fix this (via John) and Joe can resend his patch to get
typechecking there.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: New sparse warning in net/mac80211/debugfs_sta.c

[Joe Perches]

On Thu, 2008-02-21 at 11:17 +0100, Johannes Berg wrote:
> Yeah, I saw that discussion. I think it's fine, it's just something we
> need to be aware of. In fact, I Joe had a patch (that seems to have
> gotten lost?) to make DECLARE_MAC_BUF() declare a structure with the u8
> pointer in it instead to get type checking for the args, which would
> make our code there not even compile, and imho rightfully so. I'll send
> in a patch to fix this (via John) and Joe can resend his patch to get
> typechecking there.

This removes the __pure from print_mac, so reject as appropriate...

Add some type safety to print_mac by using 
struct print_mac_buf * instead of char *.

Signed-off-by: Joe Perches <joe@perches.com>

diff --git a/include/linux/if_ether.h b/include/linux/if_ether.h
index 42dc6a3..2f8df76 100644
--- a/include/linux/if_ether.h
+++ b/include/linux/if_ether.h
@@ -129,9 +129,16 @@ extern ssize_t sysfs_format_mac(char *buf, const unsigned char *addr, int len);
 /*
  *	Display a 6 byte device address (MAC) in a readable format.
  */
-extern __pure char *print_mac(char *buf, const unsigned char *addr);
-#define MAC_BUF_SIZE	18
-#define DECLARE_MAC_BUF(var) char var[MAC_BUF_SIZE] __maybe_unused
+
+struct print_mac_buf {
+	char formatted_mac_addr[18];
+};
+
+#define DECLARE_MAC_BUF(var) \
+	struct print_mac_buf __maybe_unused _##var; \
+	struct print_mac_buf __maybe_unused *var = &_##var
+
+extern char *print_mac(struct print_mac_buf *buf, const unsigned char *addr);
 
 #endif
 
diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index a7b4175..ce607ab 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -384,9 +384,10 @@ ssize_t sysfs_format_mac(char *buf, const unsigned char *addr, int len)
 }
 EXPORT_SYMBOL(sysfs_format_mac);
 
-char *print_mac(char *buf, const unsigned char *addr)
+char *print_mac(struct print_mac_buf *buf, const unsigned char *addr)
 {
-	_format_mac_addr(buf, MAC_BUF_SIZE, addr, ETH_ALEN);
-	return buf;
+	_format_mac_addr(buf->formatted_mac_addr,
+			 sizeof(buf->formatted_mac_addr), addr, ETH_ALEN);
+	return buf->formatted_mac_addr;
 }
 EXPORT_SYMBOL(print_mac);

Re: New sparse warning in net/mac80211/debugfs_sta.c

[Patrick McHardy]

Joe Perches wrote:
> On Thu, 2008-02-21 at 11:17 +0100, Johannes Berg wrote:
>   
>> Yeah, I saw that discussion. I think it's fine, it's just something we
>> need to be aware of. In fact, I Joe had a patch (that seems to have
>> gotten lost?) to make DECLARE_MAC_BUF() declare a structure with the u8
>> pointer in it instead to get type checking for the args, which would
>> make our code there not even compile, and imho rightfully so. I'll send
>> in a patch to fix this (via John) and Joe can resend his patch to get
>> typechecking there.
>>     
>
> This removes the __pure from print_mac, so reject as appropriate...
>
> Add some type safety to print_mac by using 
> struct print_mac_buf * instead of char *.

And adds back the overhead of two completely unnecessary
function calls to the VLAN fastpath. How about just
stopping this idiocy and reverting the appropriate patches
to bring back MAC_FMT and use it where appropriate?

Re: New sparse warning in net/mac80211/debugfs_sta.c

[Patrick McHardy]

Patrick McHardy wrote:
> Joe Perches wrote:
>>
>> This removes the __pure from print_mac, so reject as appropriate...
>>
>> Add some type safety to print_mac by using struct print_mac_buf * 
>> instead of char *.
>
> And adds back the overhead of two completely unnecessary
> function calls to the VLAN fastpath. 

BTW, this also affects ATM, with 3 calls in hard_start_xmit,
3 calls in lec_xmit and 1 cakk in lec_atm_send.

> How about just
> stopping this idiocy and reverting the appropriate patches
> to bring back MAC_FMT and use it where appropriate? 

Re: New sparse warning in net/mac80211/debugfs_sta.c

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Thu, 21 Feb 2008 19:00:03 +0100

> And adds back the overhead of two completely unnecessary
> function calls to the VLAN fastpath. How about just
> stopping this idiocy and reverting the appropriate patches
> to bring back MAC_FMT and use it where appropriate?

Agreed, I'll do that.

Re: New sparse warning in net/mac80211/debugfs_sta.c

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 607 bytes --]


On Sat, 2008-02-23 at 20:02 -0800, David Miller wrote:
> From: Patrick McHardy <kaber@trash.net>
> Date: Thu, 21 Feb 2008 19:00:03 +0100
> 
> > And adds back the overhead of two completely unnecessary
> > function calls to the VLAN fastpath. How about just
> > stopping this idiocy and reverting the appropriate patches
> > to bring back MAC_FMT and use it where appropriate?
> 
> Agreed, I'll do that.

Maybe we should just add a new printf modifier like %M for MAC
addresses? Then we could use sprintf, snprintf, printk and whatever we
please without any of the macro stuff...

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: New sparse warning in net/mac80211/debugfs_sta.c

[Joe Perches]

On Mon, 2008-02-25 at 10:53 +0100, Johannes Berg wrote:
> Maybe we should just add a new printf modifier like %M for MAC
> addresses? Then we could use sprintf, snprintf, printk and whatever we
> please without any of the macro stuff...

Could gcc validate the printf %M arguments?

Another possibility without changing printf argument validation
is to use a MAC_FMT macro in place of "%s"

#ifdef CONFIG_INLINE_MAC

#define MAC_FMT "%02x:%02x:%02x:%02x:%02x:%02x"
#define DECLARE_MAC_BUF(var)
#define print_mac(buf, addr) (addr)[0], (addr)[1], (addr)[2], (addr)[3], (addr)[4], (addr)[5]

#else

#define MAC_FMT "%s"
#define DECLARE_MAC_BUF(var) char var[18];
extern char *print_mac(char *buf, const unsigned char *addr);

#endif

use:

DECLARE_MAC_BUF(mac);
printk(KERN_INFO "Mac address is: " MAC_FMT "\n", print_mac(mac, addr));

Re: New sparse warning in net/mac80211/debugfs_sta.c

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 704 bytes --]


> > Maybe we should just add a new printf modifier like %M for MAC
> > addresses? Then we could use sprintf, snprintf, printk and whatever we
> > please without any of the macro stuff...
> 
> Could gcc validate the printf %M arguments?

No, but it won't barf on it either, it silently ignores unknown stuff.

> Another possibility without changing printf argument validation
> is to use a MAC_FMT macro in place of "%s"
> 
> #ifdef CONFIG_INLINE_MAC

Yes, but the argument was that in some debugging cases these things are
in the fast path even when debugging is turned off, so to not have  them
there would force you to always turn this config ON which defeats the
point.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: New sparse warning in net/mac80211/debugfs_sta.c

[Johannes Berg]

On Mon, 2008-02-25 at 10:53 +0100, Johannes Berg wrote:
> On Sat, 2008-02-23 at 20:02 -0800, David Miller wrote:
> > From: Patrick McHardy <kaber@trash.net>
> > Date: Thu, 21 Feb 2008 19:00:03 +0100
> > 
> > > And adds back the overhead of two completely unnecessary
> > > function calls to the VLAN fastpath. How about just
> > > stopping this idiocy and reverting the appropriate patches
> > > to bring back MAC_FMT and use it where appropriate?
> > 
> > Agreed, I'll do that.
> 
> Maybe we should just add a new printf modifier like %M for MAC
> addresses? Then we could use sprintf, snprintf, printk and whatever we
> please without any of the macro stuff...

I have something like this in mind. Might even be faster than using the
MAC_FMT/MAC_ARG stuff because it's done in a single loop rather than
invoking the hex digit printing 6 times.


From: Johannes Berg <johannes@sipsolutions.net>
Subject: MAC printing with %M/%m

We've been through two ways to print MAC addresses to the kernel
logs/buffers/whatever.

Until recently, we had

#define MAC_FMT		"%.2x:%.2x:%.2x:..."
#define MAC_ARG(m)	(m)[0], (m)[1], (m)[2], ...

printk("bla bla " MAC_FMT "\n", MAC_ARG(mac));

This is fairly ugly and was found to lead to quite long strings
embedded in the kernel, all the %.2x stuff.


Recently, we changed to using print_mac():

DECLARE_MAC_BUF(mbuf);
printk("bla bla %s\n", print_mac(mbuf, mac));

This was, however, found to force the compiler to emit print_mac()
function calls in fast paths even when debugging was turned off.


Here's my take on it, putting it right into the vsnprintf code.
It allows you to write

printk("bla bla %m\n", mac);

without any further function calls or local variables.

The only problem I see with using 'm' and 'M' is that 'm' is already
defined by glibc to print strerror(errno).

This patch doesn't contain any conversion yet but that could happen
gradually, starting with the fast paths.

I've tested this code in userspace with a number of MAC addresses
and various output buffer limits.

Field length specifiers are allowed and treated as if the already
printed MAC address was given to a %s specifier, ie.
	%-20m	is like %-20s
etc.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
 lib/vsprintf.c |   47 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 42 insertions(+), 5 deletions(-)

--- everything.orig/lib/vsprintf.c	2008-02-25 17:31:56.000000000 +0100
+++ everything/lib/vsprintf.c	2008-02-25 17:57:34.000000000 +0100
@@ -366,11 +366,11 @@ static noinline char* put_dec(char *buf,
 #define SMALL	32		/* Must be 32 == 0x20 */
 #define SPECIAL	64		/* 0x */
 
+/* we are called with base 8, 10 or 16, only, thus don't need "G..."  */
+static const char hexdigits[16] = "0123456789ABCDEF"; /* "GHIJKLMNOPQRSTUVWXYZ"; */
+
 static char *number(char *buf, char *end, unsigned long long num, int base, int size, int precision, int type)
 {
-	/* we are called with base 8, 10 or 16, only, thus don't need "G..."  */
-	static const char digits[16] = "0123456789ABCDEF"; /* "GHIJKLMNOPQRSTUVWXYZ"; */
-
 	char tmp[66];
 	char sign;
 	char locase;
@@ -408,7 +408,7 @@ static char *number(char *buf, char *end
 		tmp[i++] = '0';
 	/* Generic code, for any base:
 	else do {
-		tmp[i++] = (digits[do_div(num,base)] | locase);
+		tmp[i++] = (hexdigits[do_div(num,base)] | locase);
 	} while (num != 0);
 	*/
 	else if (base != 10) { /* 8 or 16 */
@@ -416,7 +416,7 @@ static char *number(char *buf, char *end
 		int shift = 3;
 		if (base == 16) shift = 4;
 		do {
-			tmp[i++] = (digits[((unsigned char)num) & mask] | locase);
+			tmp[i++] = (hexdigits[((unsigned char)num) & mask] | locase);
 			num >>= shift;
 		} while (num);
 	} else { /* base 10 */
@@ -621,6 +621,43 @@ int vsnprintf(char *buf, size_t size, co
 				}
 				continue;
 
+
+			case 'm':
+				flags |= SMALL;
+			case 'M':
+				s = va_arg(args, unsigned char *);
+				len = 17;
+
+				if (!(flags & LEFT)) {
+					while (len < field_width--) {
+						if (str < end)
+							*str = ' ';
+						++str;
+					}
+				}
+				for (i = 0; i < len; i++) {
+					if (str < end) {
+						if ((i % 3) == 0)
+							*str = hexdigits[(*(unsigned char *)s) >> 4];
+						else if ((i % 3) == 1)
+							*str = hexdigits[(*s) & 0xF];
+						else {
+							*str = ':';
+							s++;
+						}
+						if (flags & SMALL)
+							*str = TOLOWER(*str);
+					}
+					++str;
+				}
+				while (len < field_width--) {
+					if (str < end)
+						*str = ' ';
+					++str;
+				}
+				continue;
+
+
 			case 's':
 				s = va_arg(args, char *);
 				if ((unsigned long)s < PAGE_SIZE)

Re: New sparse warning in net/mac80211/debugfs_sta.c

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Mon, 25 Feb 2008 10:53:48 +0100

> Maybe we should just add a new printf modifier like %M for MAC
> addresses? Then we could use sprintf, snprintf, printk and whatever we
> please without any of the macro stuff...

But GCC has no idea what the heck it is and will warn.

Re: New sparse warning in net/mac80211/debugfs_sta.c

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 538 bytes --]


On Mon, 2008-02-25 at 11:52 -0800, David Miller wrote:
> From: Johannes Berg <johannes@sipsolutions.net>
> Date: Mon, 25 Feb 2008 10:53:48 +0100
> 
> > Maybe we should just add a new printf modifier like %M for MAC
> > addresses? Then we could use sprintf, snprintf, printk and whatever we
> > please without any of the macro stuff...
> 
> But GCC has no idea what the heck it is and will warn.

No, I actually wondered about that too and finally just tried, it simply
ignores it when doing the printf warnings.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: New sparse warning in net/mac80211/debugfs_sta.c

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 687 bytes --]


On Mon, 2008-02-25 at 20:56 +0100, Johannes Berg wrote:
> On Mon, 2008-02-25 at 11:52 -0800, David Miller wrote:
> > From: Johannes Berg <johannes@sipsolutions.net>
> > Date: Mon, 25 Feb 2008 10:53:48 +0100
> > 
> > > Maybe we should just add a new printf modifier like %M for MAC
> > > addresses? Then we could use sprintf, snprintf, printk and whatever we
> > > please without any of the macro stuff...
> > 
> > But GCC has no idea what the heck it is and will warn.
> 
> No, I actually wondered about that too and finally just tried, it simply
> ignores it when doing the printf warnings.

Wait, no, you're right, I had the wrong warning flags enabled :(

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: New sparse warning in net/mac80211/debugfs_sta.c

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Mon, 25 Feb 2008 21:05:42 +0100

> 
> On Mon, 2008-02-25 at 20:56 +0100, Johannes Berg wrote:
> > On Mon, 2008-02-25 at 11:52 -0800, David Miller wrote:
> > > From: Johannes Berg <johannes@sipsolutions.net>
> > > Date: Mon, 25 Feb 2008 10:53:48 +0100
> > > 
> > > > Maybe we should just add a new printf modifier like %M for MAC
> > > > addresses? Then we could use sprintf, snprintf, printk and whatever we
> > > > please without any of the macro stuff...
> > > 
> > > But GCC has no idea what the heck it is and will warn.
> > 
> > No, I actually wondered about that too and finally just tried, it simply
> > ignores it when doing the printf warnings.
> 
> Wait, no, you're right, I had the wrong warning flags enabled :(

Oh well :-/

I really think it's not worth dorking around with this print_mac()
stuff so much like this, let's just take care of the cases that
Patrick mentioned (which need to go back to MAC_FMT because they
are in fast paths) and then leave this alone for a while.

We've already wasted too much time on this.

Thanks.

Re: New sparse warning in net/mac80211/debugfs_sta.c

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Mon, 25 Feb 2008 20:56:43 +0100

> 
> On Mon, 2008-02-25 at 11:52 -0800, David Miller wrote:
> > From: Johannes Berg <johannes@sipsolutions.net>
> > Date: Mon, 25 Feb 2008 10:53:48 +0100
> > 
> > > Maybe we should just add a new printf modifier like %M for MAC
> > > addresses? Then we could use sprintf, snprintf, printk and whatever we
> > > please without any of the macro stuff...
> > 
> > But GCC has no idea what the heck it is and will warn.
> 
> No, I actually wondered about that too and finally just tried, it simply
> ignores it when doing the printf warnings.

Ok, so it simply can't validate the thing.

Re: New sparse warning in net/mac80211/debugfs_sta.c

[Patrick McHardy]

David Miller wrote:
> From: Patrick McHardy <kaber@trash.net>
> Date: Thu, 21 Feb 2008 19:00:03 +0100
>
>   
>> And adds back the overhead of two completely unnecessary
>> function calls to the VLAN fastpath. How about just
>> stopping this idiocy and reverting the appropriate patches
>> to bring back MAC_FMT and use it where appropriate?
>>     
>
> Agreed, I'll do that.
>   

It would be good if Joe could go through the remaining print_mac users
and convert the remaining unintended function calls in fastpaths back
to MAC_FMT. Grepping for "start_xmit" in commit 0795af5729b shows that
at least 10 hard_start_xmit functions are affected and I expect that
some of the changes in the wireless code affect fastpaths as well.

Re: New sparse warning in net/mac80211/debugfs_sta.c

[Joe Perches]

On Mon, 2008-02-25 at 12:47 +0100, Patrick McHardy wrote: 
> It would be good if Joe could go through the remaining print_mac users
> and convert the remaining unintended function calls in fastpaths back
> to MAC_FMT. Grepping for "start_xmit" in commit 0795af5729b shows that
> at least 10 hard_start_xmit functions are affected and I expect that
> some of the changes in the wireless code affect fastpaths as well.

I don't mind doing that, as calling print_mac in these fastpaths in
unintentional and undesirable.  But wouldn't it be better to find a
solution that removes all debug printk function calls that should
be optimized away?

I have not seen any response to a suggestion to convert debug printk
macros (dprintk, pr_debug, dev_dbg, etc) to:

#define pr_debug(fmt, arg...) \
do { if (0) printk(KERN_DEBUG fmt, ##arg); } while (0)

This preserves argument verification and gives the compiler the
capability to optimize out the printk and any functions the printk
might call.

diff --git a/include/linux/kernel.h b/include/linux/kernel.h 
index 2df44e7..cd24112 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -293,10 +293,8 @@ extern void print_hex_dump_bytes(const char *prefix_str, int prefix_type,
 #define pr_debug(fmt, arg...) \
 	printk(KERN_DEBUG fmt, ##arg)
 #else
-static inline int __attribute__ ((format (printf, 1, 2))) pr_debug(const char * fmt, ...)
-{
-	return 0;
-}
+#define pr_debug(fmt, arg...) \
+	do { if (0) printk(KERN_DEBUG fmt, ##arg); } while (0)
 #endif
 
 /*
diff --git a/include/linux/device.h b/include/linux/device.h
index 2258d89..79601b1 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -608,21 +608,15 @@ extern const char *dev_driver_string(struct device *dev);
 #define dev_dbg(dev, format, arg...)		\
 	dev_printk(KERN_DEBUG , dev , format , ## arg)
 #else
-static inline int __attribute__ ((format (printf, 2, 3)))
-dev_dbg(struct device *dev, const char *fmt, ...)
-{
-	return 0;
-}
+#define dev_dbg(dev, format, arg...) \
+	do { if (0) dev_printk(KERN_DEBUG , dev , format, ## arg); } while (0)
 #endif
 
 #ifdef VERBOSE_DEBUG
 #define dev_vdbg	dev_dbg
 #else
-static inline int __attribute__ ((format (printf, 2, 3)))
-dev_vdbg(struct device *dev, const char *fmt, ...)
-{
-	return 0;
-}
+#define dev_vdbg(dev, format, arg...) \
+	do { if (0) dev_printk(KERN_DEBUG , dev , format, ## arg); } while (0)
 #endif
 
 /* Create alias, so I can be autoloaded. */

Re: New sparse warning in net/mac80211/debugfs_sta.c

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 1890 bytes --]

Joe Perches wrote:
> On Mon, 2008-02-25 at 12:47 +0100, Patrick McHardy wrote: 
>> It would be good if Joe could go through the remaining print_mac users
>> and convert the remaining unintended function calls in fastpaths back
>> to MAC_FMT. Grepping for "start_xmit" in commit 0795af5729b shows that
>> at least 10 hard_start_xmit functions are affected and I expect that
>> some of the changes in the wireless code affect fastpaths as well.
> 
> I don't mind doing that, as calling print_mac in these fastpaths in
> unintentional and undesirable.


Unfortunately the current tree still includes all the fallout,
are you planning on cleaning this up again any time soon?

I've attached a codiff of a tree with and without this change
(might not include all drivers, but I think I enabled all that
build on x86_64). The _probe and _init_one functions should
be harmless, but there are lots of functions that look like
they would prefer to avoid useless overhead. A small sample:

drivers/net/starfire.c:
   netdev_poll       |  +50

drivers/net/tokenring/olympic.c:
   olympic_interrupt |  +10

drivers/net/wan/pc300_drv.c:
   cpc_net_rx |  +11

drivers/net/wireless/ipw2200.c:
   ipw_net_hard_start_xmit |  +32

drivers/net/wireless/hostap/hostap_80211_rx.c:
   hostap_rx_frame_decrypt |  +27
   hostap_80211_rx         |  +96

drivers/net/wireless/hostap/hostap_80211_tx.c:
   hostap_master_start_xmit |  +18

drivers/net/wireless/hostap/hostap_ap.c:
   hostap_handle_sta_tx_exc |  +13
   hostap_ap_tx_cb_poll     |   +9
   hostap_rx                | +254
   hostap_ap_tx_cb_auth     |  +62
   hostap_ap_tx_cb_assoc    |  +26
   hostap_handle_sta_tx     |  +90
   hostap_handle_sta_rx     |  +63

<lots more wireless>

drivers/net/virtio_net.c:
   start_xmit |  +19

net/atm/lec.c:
   lec_start_xmit | +118
   lec_atm_send   |   +2

net/ieee80211/ieee80211_rx.c:
   ieee80211_rx |  +32



[-- Attachment #2: diff --]
[-- Type: text/plain, Size: 13221 bytes --]

drivers/net/e1000/e1000_main.c:
  e1000_probe |  +32
 1 function changed, 32 bytes added

drivers/net/bonding/bond_main.c:
  bond_info_seq_show |  +14
  bond_release       |  +12
 2 functions changed, 26 bytes added

drivers/net/bonding/bond_sysfs.c:
  bonding_show_ad_partner_mac |  +29
 1 function changed, 29 bytes added

drivers/net/rrunner.c:
  rr_init_one |  +30
 1 function changed, 30 bytes added

drivers/net/sunhme.c:
  happy_meal_pci_probe |  +16
 1 function changed, 16 bytes added

drivers/net/sungem.c:
  gem_init_one |  +16
 1 function changed, 16 bytes added

drivers/net/cassini.c:
  cas_init_one |   +3
 1 function changed, 3 bytes added

drivers/net/3c59x.c:
  vortex_probe1 |  +16
 1 function changed, 16 bytes added

drivers/net/typhoon.c:
  typhoon_init_one |  +51
 1 function changed, 51 bytes added

drivers/net/ne2k-pci.c:
  ne2k_pci_init_one |  +42
 1 function changed, 42 bytes added

drivers/net/pcnet32.c:
  pcnet32_probe1 |  +32
 1 function changed, 32 bytes added

drivers/net/eepro100.c:
  eepro100_init_one |  +21
 1 function changed, 21 bytes added

drivers/net/e100.c:
  e100_probe |  +12
 1 function changed, 12 bytes added

drivers/net/epic100.c:
  epic_init_one |  +16
 1 function changed, 16 bytes added

drivers/net/sis190.c:
  sis190_init_one |  +31
 1 function changed, 31 bytes added

drivers/net/sis900.c:
  sis900_probe |  +44
 1 function changed, 44 bytes added

drivers/net/yellowfin.c:
  yellowfin_init_one |  +76
 1 function changed, 76 bytes added

drivers/net/acenic.c:
  acenic_probe_one |  +17
 1 function changed, 17 bytes added

drivers/net/natsemi.c:
  natsemi_probe1 |  +31
 1 function changed, 31 bytes added

drivers/net/ns83820.c:
  ns83820_init_one |  +30
 1 function changed, 30 bytes added

drivers/net/fealnx.c:
  fealnx_init_one |  +45
 1 function changed, 45 bytes added

drivers/net/tg3.c:
  tg3_init_one | +112
 1 function changed, 112 bytes added

drivers/net/bnx2.c:
  bnx2_init_one |  +81
 1 function changed, 81 bytes added

drivers/net/bnx2x.c:
  bnx2x_init_one |  +16
 1 function changed, 16 bytes added

drivers/net/skge.c:
  skge_show_addr |  +22
 1 function changed, 22 bytes added

drivers/net/sky2.c:
  sky2_show_addr |  +22
 1 function changed, 22 bytes added

drivers/net/via-rhine.c:
  rhine_init_one |  +23
 1 function changed, 23 bytes added

drivers/net/starfire.c:
  netdev_poll       |  +50
  starfire_init_one |  +16
 2 functions changed, 66 bytes added

drivers/net/sundance.c:
  sundance_probe1 |  +34
 1 function changed, 34 bytes added

drivers/net/hamachi.c:
  hamachi_init_one |  +13
 1 function changed, 13 bytes added

drivers/net/b44.c:
  b44_init_one |  +18
 1 function changed, 18 bytes added

drivers/net/forcedeth.c:
  nv_probe |  +48
 1 function changed, 48 bytes added

drivers/net/qla3xxx.c:
  ql3xxx_probe |   +4
 1 function changed, 4 bytes added

drivers/net/pppoe.c:
  pppoe_seq_show |  +39
 1 function changed, 39 bytes added

drivers/net/de600.c:
  de600_init |  +26
 1 function changed, 26 bytes added

drivers/net/de620.c:
  de620_probe |  +16
 1 function changed, 16 bytes added

drivers/net/8139cp.c:
  cp_init_one |  +32
 1 function changed, 32 bytes added

drivers/net/8139too.c:
  rtl8139_init_one |  +74
 1 function changed, 74 bytes added

drivers/net/atp.c:
  atp_probe1 |   +8
 1 function changed, 8 bytes added

drivers/net/dl2k.c:
  rio_probe1 |  +16
 1 function changed, 16 bytes added

drivers/net/amd8111e.c:
  amd8111e_probe_one |  +16
 1 function changed, 16 bytes added

drivers/net/s2io.c:
  s2io_init_nic |   +3
 1 function changed, 3 bytes added

drivers/net/myri10ge/myri10ge.c:
  myri10ge_set_multicast_list |  +16
 1 function changed, 16 bytes added

drivers/net/tokenring/olympic.c:
  olympic_proc_info | +310
  olympic_open      |  +76
  olympic_interrupt |  +10
 3 functions changed, 396 bytes added

drivers/net/tokenring/abyss.c:
  abyss_attach |  +15
 1 function changed, 15 bytes added

drivers/net/tokenring/tmspci.c:
  tms_pci_attach |  +16
 1 function changed, 16 bytes added

drivers/net/wan/pc300_drv.c:
  cpc_net_rx |  +11
 1 function changed, 11 bytes added

drivers/net/wan/wanxl.c:
  wanxl_pci_init_one |   -3
 1 function changed, 3 bytes removed

drivers/net/usb/pegasus.c:
  pegasus_probe |  +69
 1 function changed, 69 bytes added

drivers/net/usb/usbnet.c:
  usbnet_probe |  +13
 1 function changed, 13 bytes added

drivers/net/wireless/ipw2100.c:
  show_bssinfo |  +21
 1 function changed, 21 bytes added

drivers/net/wireless/ipw2200.c:
  ipw_send_disassociate   |  +46
  ipw_associate_network   |  +18
  ipw_debug_config        |  +32
  ipw_best_network        | +241
  ipw_up                  |   +9
  ipw_add_station         |  +15
  ipw_rx_notification     | +139
  ipw_wx_set_wap          |   +8
  ipw_wx_get_wap          |  +20
  ipw_net_hard_start_xmit |  +32
  ipw_net_set_mac_address |   +9
  ipw_irq_tasklet         |  +29
 12 functions changed, 598 bytes added

drivers/net/wireless/orinoco.c:
  orinoco_init |  +80
 1 function changed, 80 bytes added

drivers/net/wireless/airo.c:
  reset_airo_card   |  +14
  _init_airo_card   |  +17
  proc_APList_open  |  +14
  proc_BSSList_open |  +46
 4 functions changed, 91 bytes added

drivers/net/wireless/atmel.c:
  init_atmel_card |   +5
 1 function changed, 5 bytes added

drivers/net/wireless/prism54/isl_ioctl.c:
  send_formatted_event |  +25
  prism54_process_trap |  +41
 2 functions changed, 66 bytes added

drivers/net/wireless/hostap/hostap_80211_rx.c:
  hostap_dump_rx_80211    |  +34
  hostap_rx_frame_decrypt |  +27
  hostap_80211_rx         |  +96
 3 functions changed, 157 bytes added

drivers/net/wireless/hostap/hostap_80211_tx.c:
  hostap_dump_tx_80211     |  +29
  hostap_master_start_xmit |  +18
 2 functions changed, 47 bytes added

drivers/net/wireless/hostap/hostap_ap.c:
  ap_sta_hash_del          |  +29
  hostap_handle_sta_tx_exc |  +13
  ap_free_sta              |   -5
  prism2_ap_proc_read      |  +28
  ap_control_proc_read     |  +26
  handle_assoc             |  +48
  hostap_ap_tx_cb_poll     |   +9
  hostap_rx                | +254
  handle_wds_oper_queue    |  +28
  hostap_ap_tx_cb_auth     |  +62
  hostap_ap_tx_cb_assoc    |  +26
  handle_add_proc_queue    |  -11
  hostap_handle_sta_tx     |  +90
  hostap_handle_sta_rx     |  +63
  prism2_sta_proc_read     | +150
  ap_handle_timer          |  +36
 16 functions changed, 862 bytes added, 16 bytes removed, diff: +846

drivers/net/wireless/hostap/hostap_info.c:
  prism2_host_roaming |  +37
  handle_info_queue   |  -16
 2 functions changed, 37 bytes added, 16 bytes removed, diff: +21

drivers/net/wireless/hostap/hostap_ioctl.c:
  hostap_ioctl       |  +17
  prism2_ioctl_siwap |  +29
 2 functions changed, 46 bytes added

drivers/net/wireless/hostap/hostap_main.c:
  hostap_dump_tx_header | +201
  hostap_dump_rx_header | +125
 2 functions changed, 326 bytes added

drivers/net/wireless/hostap/hostap_proc.c:
  prism2_scan_results_proc_read |   -9
  prism2_bss_list_proc_read     |  +36
  prism2_wds_proc_read          |  +12
 3 functions changed, 48 bytes added, 9 bytes removed, diff: +39

drivers/net/wireless/hostap/hostap_plx.c:
  hostap_bap_tasklet |  +19
 1 function changed, 19 bytes added

drivers/net/wireless/hostap/hostap_pci.c:
  hostap_bap_tasklet |  +32
 1 function changed, 32 bytes added

drivers/net/wireless/libertas/cmd.c:
  lbs_update_hw_spec |  +81
 1 function changed, 81 bytes added

drivers/net/wireless/libertas/debugfs.c:
  lbs_getscantable |   +9
 1 function changed, 9 bytes added

drivers/net/wireless/rtl8180_dev.c:
  rtl8180_probe |  +13
 1 function changed, 13 bytes added

drivers/net/wireless/rtl8187_dev.c:
  rtl8187_probe |   +7
 1 function changed, 7 bytes added

drivers/net/wireless/adm8211.c:
  adm8211_probe |  +28
 1 function changed, 28 bytes added

drivers/net/wireless/iwlwifi/iwl3945-base.c:
  iwl3945_add_station          |   +7
  iwl3945_commit_rxon          |  +33
  iwl3945_mac_tx               |  +69
  iwl3945_mac_set_key          |  +22
  iwl3945_pci_probe            |  +17
  iwl3945_bg_post_associate    |  -24
  iwl3945_mac_add_interface    |   +4
  iwl3945_mac_config_interface |  +31
 8 functions changed, 183 bytes added, 24 bytes removed, diff: +159

drivers/net/wireless/iwlwifi/iwl-3945.c:
  iwl3945_rx_reply_rx     | +161
  iwl3945_hw_find_station |  +24
 2 functions changed, 185 bytes added

drivers/net/wireless/iwlwifi/iwl-3945-rs.c:
  rs_get_rate |  -16
 1 function changed, 16 bytes removed

drivers/net/wireless/iwlwifi/iwl4965-base.c:
  iwl4965_add_station_flags    |  +12
  iwl4965_commit_rxon          |  +16
  iwl4965_mac_tx               |  +31
  iwl4965_mac_set_key          |  +22
  iwl4965_pci_probe            |  +16
  iwl4965_bg_post_associate    |  -19
  iwl4965_mac_add_interface    |   +4
  iwl4965_mac_config_interface |  +31
 8 functions changed, 132 bytes added, 19 bytes removed, diff: +113

drivers/net/wireless/iwlwifi/iwl-4965.c:
  iwl4965_hw_find_station |  +17
  iwl4965_rx_reply_rx     | +167
 2 functions changed, 184 bytes added

drivers/net/wireless/iwlwifi/iwl-4965-rs.c:
  rs_rate_init |  +30
  rs_get_rate  |  +10
 2 functions changed, 40 bytes added

drivers/net/wireless/p54usb.c:
  p54u_probe |  +41
 1 function changed, 41 bytes added

drivers/net/wireless/p54pci.c:
  p54p_probe |  +44
 1 function changed, 44 bytes added

drivers/net/tulip/dmfe.c:
  dmfe_init_one |  +25
 1 function changed, 25 bytes added

drivers/net/tulip/winbond-840.c:
  w840_probe1  |  +16
  intr_handler | +112
 2 functions changed, 128 bytes added

drivers/net/tulip/de2104x.c:
  de_init_one |  +23
 1 function changed, 23 bytes added

drivers/net/tulip/tulip_core.c:
  set_rx_mode    |  +16
  tulip_init_one |  +60
 2 functions changed, 76 bytes added

drivers/net/tulip/de4x5.c:
  DevicePresent   |  +11
  de4x5_interrupt |   -2
  de4x5_pci_probe |  +32
 3 functions changed, 43 bytes added, 2 bytes removed, diff: +41

drivers/net/tulip/uli526x.c:
  uli526x_init_one |   +9
 1 function changed, 9 bytes added

drivers/net/netconsole.c:
  show_remote_mac |  +16
  show_local_mac  |  +13
 2 functions changed, 29 bytes added

drivers/net/netxen/netxen_nic_main.c:
  netxen_nic_probe |  +45
 1 function changed, 45 bytes added

drivers/net/netxen/netxen_nic_init.c:
  netxen_rom_se |   +3
 1 function changed, 3 bytes added

drivers/net/netxen/netxen_nic_niu.c:
  netxen_niu_macaddr_set |  +21
 1 function changed, 21 bytes added

drivers/net/niu.c:
  niu_pci_init_one |  +35
 1 function changed, 35 bytes added

drivers/net/virtio_net.c:
  start_xmit |  +19
 1 function changed, 19 bytes added

net/core/pktgen.c:
  pktgen_if_show |  +88
 1 function changed, 88 bytes added

net/core/netpoll.c:
  netpoll_print_options |  +17
 1 function changed, 17 bytes added

net/llc/llc_proc.c:
  llc_ui_format_mac |  +21
 1 function changed, 21 bytes added

net/ethernet/eth.c:
  sysfs_format_mac | -115
 1 function changed, 115 bytes removed

net/802/tr.c:
  rif_seq_show |   +6
 1 function changed, 6 bytes added

net/appletalk/aarp.c:
  aarp_seq_show |  +15
 1 function changed, 15 bytes added

net/atm/br2684.c:
  br2684_seq_show |  +17
 1 function changed, 17 bytes added

net/atm/lec.c:
  lec_start_xmit | +118
  lec_atm_send   |   +2
 2 functions changed, 120 bytes added

net/mac80211/ieee80211.c:
  ieee80211_if_update_wds |  +14
 1 function changed, 14 bytes added

net/mac80211/sta_info.c:
  sta_info_cleanup |  +24
 1 function changed, 24 bytes added

net/mac80211/wpa.c:
  ieee80211_rx_h_michael_mic_verify |  +11
 1 function changed, 11 bytes added

net/mac80211/ieee80211_sta.c:
  ieee80211_handle_erp_ie      |  +28
  ieee80211_associate          |  +15
  ieee80211_sta_find_ibss      |  +20
  ieee80211_rx_bss_info        |  +16
  ieee80211_associated         |  +16
  ieee80211_rx_mgmt_assoc_resp |  +96
  ieee80211_ibss_add_sta       |  +23
  ieee80211_sta_work           | -3608
  ieee80211_sta_scan_results   |   -4
 9 functions changed, 214 bytes added, 3612 bytes removed, diff: -3398

net/mac80211/rx.c:
  ieee80211_data_to_8023       | +262
  __ieee80211_rx_handle_packet |  +48
  ieee80211_rx_h_defragment    |  +82
 3 functions changed, 392 bytes added

net/mac80211/tx.c:
  ieee80211_tx_h_ps_buf |   +8
 1 function changed, 8 bytes added

net/mac80211/key.c:
  ieee80211_key_disable_hw_accel |  +24
  ieee80211_key_enable_hw_accel  |  +15
 2 functions changed, 39 bytes added

net/mac80211/event.c:
  mac80211_ev_michael_mic_failure |  +14
 1 function changed, 14 bytes added

net/ieee80211/ieee80211_rx.c:
  ieee80211_rx |  +32
 1 function changed, 32 bytes added

net/ieee80211/ieee80211_crypt_ccmp.c:
  ieee80211_ccmp_decrypt | +114
 1 function changed, 114 bytes added

net/ieee80211/ieee80211_crypt_tkip.c:
  ieee80211_michael_mic_verify |  +37
  ieee80211_tkip_decrypt       |  +95
  ieee80211_tkip_encrypt       |  +43
 3 functions changed, 175 bytes added

net/ieee80211/softmac/ieee80211softmac_auth.c:
  ieee80211softmac_auth_queue |  -53
  ieee80211softmac_auth_resp  | +111
 2 functions changed, 111 bytes added, 53 bytes removed, diff: +58

net/ethernet/eth.c:
  print_mac |  +27
 1 function changed, 27 bytes added

vmlinux.with:
 190 functions changed, 7138 bytes added, 3885 bytes removed, diff: +3253

Re: New sparse warning in net/mac80211/debugfs_sta.c

[Joe Perches]

On Tue, 2008-04-08 at 22:18 +0200, Patrick McHardy wrote:
> Joe Perches wrote:
> > On Mon, 2008-02-25 at 12:47 +0100, Patrick McHardy wrote: 
> >> It would be good if Joe could go through the remaining print_mac users
> >> and convert the remaining unintended function calls in fastpaths back
> >> to MAC_FMT. Grepping for "start_xmit" in commit 0795af5729b shows that
> >> at least 10 hard_start_xmit functions are affected and I expect that
> >> some of the changes in the wireless code affect fastpaths as well.
> > I don't mind doing that, as calling print_mac in these fastpaths in
> > unintentional and undesirable.
> Unfortunately the current tree still includes all the fallout,
> are you planning on cleaning this up again any time soon?
> I've attached a codiff of a tree with and without this change
> (might not include all drivers, but I think I enabled all that
> build on x86_64). The _probe and _init_one functions should
> be harmless, but there are lots of functions that look like
> they would prefer to avoid useless overhead. A small sample:
> 
> drivers/net/starfire.c:
>    netdev_poll       |  +50
> 
> drivers/net/tokenring/olympic.c:
>    olympic_interrupt |  +10

[]

I also think this should be cleaned up before 2.6.25 is released.

I think that the changes to pr_debug, dev_dbg, and dev_vdbg
to use an "if (0) printk" macro rather than an empty inline
I posted a few times without any reply or comment should work
for most all cases.

These changes should allow gcc to eliminate unused functions
called as arguments to those debugging logging functions
while maintaining the printf argument validation.

I'll check out codiff as I haven't used it.
Is this the latest codiff tool?
http://git.kernel.org/?p=linux/kernel/git/acme/pahole.git;a=summary

cheers, Joe

Here is the patch again:

Signed-off-by: Joe Perches <joe@perches.com>

diff --git a/include/linux/device.h b/include/linux/device.h
index 2258d89..12bb248 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -608,21 +608,21 @@ extern const char *dev_driver_string(struct device *dev);
 #define dev_dbg(dev, format, arg...)		\
 	dev_printk(KERN_DEBUG , dev , format , ## arg)
 #else
-static inline int __attribute__ ((format (printf, 2, 3)))
-dev_dbg(struct device *dev, const char *fmt, ...)
-{
-	return 0;
-}
+#define dev_dbg(dev, format, arg...)				\
+do {								\
+	if (0)							\
+		dev_printk(KERN_DEBUG , dev , format , ## arg);	\
+} while (0)
 #endif
 
 #ifdef VERBOSE_DEBUG
 #define dev_vdbg	dev_dbg
 #else
-static inline int __attribute__ ((format (printf, 2, 3)))
-dev_vdbg(struct device *dev, const char *fmt, ...)
-{
-	return 0;
-}
+#define dev_vdbg(dev, format, arg...)				\
+do {								\
+	if (0)							\
+		dev_printk(KERN_DEBUG , dev , format , ## arg);	\
+} while (0)
 #endif
 
 /* Create alias, so I can be autoloaded. */
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 2df44e7..80be070 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -293,10 +293,11 @@ extern void print_hex_dump_bytes(const char *prefix_str, int prefix_type,
 #define pr_debug(fmt, arg...) \
 	printk(KERN_DEBUG fmt, ##arg)
 #else
-static inline int __attribute__ ((format (printf, 1, 2))) pr_debug(const char * fmt, ...)
-{
-	return 0;
-}
+#define pr_debug(fmt, arg...)			\
+do {						\
+	if (0)					\
+		printk(KERN_DEBUG fmt, ##arg);	\
+} while (0)
 #endif
 
 /*

Re: New sparse warning in net/mac80211/debugfs_sta.c

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Tue, 08 Apr 2008 14:02:29 -0700

> I think that the changes to pr_debug, dev_dbg, and dev_vdbg
> to use an "if (0) printk" macro rather than an empty inline
> I posted a few times without any reply or comment should work
> for most all cases.

Joe, fix this directly like we did for VLAN, by changing
these spots away from print_mac().

We can do work on this other idea of yours independantly.
All you do by bringing it up is distract things and defer
the fix even further.

You've already wasted more than a month not fixing this
like you said you would, and you added this regression
so it is your responsibility to fix this in a timerly
manner.

Please remove print_mac() usage from these spots in the
code as requested by Patrick.

Thank you.

Re: New sparse warning in net/mac80211/debugfs_sta.c

[Patrick McHardy]

Joe Perches wrote:
> On Tue, 2008-04-08 at 22:18 +0200, Patrick McHardy wrote:
>
>> I've attached a codiff of a tree with and without this change
>> (might not include all drivers, but I think I enabled all that
>> build on x86_64). The _probe and _init_one functions should
>> be harmless, but there are lots of functions that look like
>> they would prefer to avoid useless overhead. A small sample:
>>
>> drivers/net/starfire.c:
>>    netdev_poll       |  +50
>>
>> drivers/net/tokenring/olympic.c:
>>    olympic_interrupt |  +10
> 
> []
> 
> I also think this should be cleaned up before 2.6.25 is released.

Great :)

> I think that the changes to pr_debug, dev_dbg, and dev_vdbg
> to use an "if (0) printk" macro rather than an empty inline
> I posted a few times without any reply or comment should work
> for most all cases.
> 
> These changes should allow gcc to eliminate unused functions
> called as arguments to those debugging logging functions
> while maintaining the printf argument validation.
> 
> I'll check out codiff as I haven't used it.
> Is this the latest codiff tool?
> http://git.kernel.org/?p=linux/kernel/git/acme/pahole.git;a=summary

Yes, I'm using pre-built debian packages though since
I couldn't get the source to work properly on debian
(deb ftp://ftp.nerim.net/debian/ sid main).

> Here is the patch again:

This might work (codiff should be able to tell). But I
just see that Dave also favours to use MAC_FMT directly,
so please just do this. Thanks.

Re: New sparse warning in net/mac80211/debugfs_sta.c

[Joe Perches]

On Tue, 2008-04-08 at 23:19 +0200, Patrick McHardy wrote:
> This might work (codiff should be able to tell). But I
> just see that Dave also favours to use MAC_FMT directly,
> so please just do this.

Precisely what are you objecting to Patrick?

Are you objecting to the existence of an additional
function call in a printk or the unnecessary call to
print_mac in functions like pr_debug that the compiler
can not optimize away?

Your long list seems to be mostly a list of functions
that are converted from direct use of MAC_FMT with
6 arguments on stack to a call to print_mac.

I think the only things that should be changed are those
functions that add a useless call to print_mac because 
pr_debug is optimized away.

I do _not_ think that merely because print_mac is used
as a printk argument it should be reverted.

cheers, Joe

Re: New sparse warning in net/mac80211/debugfs_sta.c

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Tue, 08 Apr 2008 15:09:33 -0700

> I do _not_ think that merely because print_mac is used
> as a printk argument it should be reverted.

This is getting rediculious Joe.

I'm going to fix this myself.

Why don't you go make yourself useful and spam lkml with
a hundred or so whitespace patches?

Re: New sparse warning in net/mac80211/debugfs_sta.c

[Joe Perches]

On Tue, 2008-04-08 at 15:30 -0700, David Miller wrote:
> I'm going to fix this myself.

Enjoy.

As far as I'm concerned, I fixed it February 21.
http://lkml.org/lkml/2008/2/21/244

Re: New sparse warning in net/mac80211/debugfs_sta.c

[Patrick McHardy]

Joe Perches wrote:
> On Tue, 2008-04-08 at 23:19 +0200, Patrick McHardy wrote:
>> This might work (codiff should be able to tell). But I
>> just see that Dave also favours to use MAC_FMT directly,
>> so please just do this.
> 
> Precisely what are you objecting to Patrick?
> 
> Are you objecting to the existence of an additional
> function call in a printk or the unnecessary call to
> print_mac in functions like pr_debug that the compiler
> can not optimize away?
> 
> Your long list seems to be mostly a list of functions
> that are converted from direct use of MAC_FMT with
> 6 arguments on stack to a call to print_mac.
> 
> I think the only things that should be changed are those
> functions that add a useless call to print_mac because 
> pr_debug is optimized away.
> 
> I do _not_ think that merely because print_mac is used
> as a printk argument it should be reverted.


No, those cases probably don't matter, if there was a printk
before it can't matter that much. What should be removed is
cases like virtio_net:

static int start_xmit(struct sk_buff *skb, struct net_device *dev) 

{
...
         pr_debug("%s: xmit %p %s\n", dev->name, skb, print_mac(mac, dest));

which adds a unconditional and useless function call in a
performance critical path.

You could probably generate a better starting point than
my codiff somehow, I just redefined print_mac to
"00:00:00:00:00:00", which means it doesn't care whether
its in a printk or a pr_debug() call. Might be quicker
to go through the list manually though.

Re: New sparse warning in net/mac80211/debugfs_sta.c

[Joe Perches]

On Thu, 2008-02-21 at 02:05 -0800, David Miller wrote:
> >From another perspective adding that __pure attribute to print_mac()
> might not have been the best idea.  But I can't think of another
> way to eliminate the "passing print_mac() args to pr_debug()
> still generates calls to print_mac() even when DEBUG is not
> defined" problem :-/

Philip Craig made a suggestion:
http://marc.info/?l=linux-wireless&m=120338413108120&w=2

to use this:

If not debugging, guard #define wrapper_printks with if (0).
The gcc optimizer removes the printk but gcc still does
printf argument verification.

There are many other debug printk wrappers

	#ifdef DEBUG
	#define printk_wrapper 
	#else
	#define printk_wrapper printk
	#endif

that should be converted as well.

Signed-off-by: Joe Perches <joe@perches.com>

diff --git a/include/linux/device.h b/include/linux/device.h
index 2258d89..79601b1 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -608,21 +608,15 @@ extern const char *dev_driver_string(struct device *dev);
 #define dev_dbg(dev, format, arg...)		\
 	dev_printk(KERN_DEBUG , dev , format , ## arg)
 #else
-static inline int __attribute__ ((format (printf, 2, 3)))
-dev_dbg(struct device *dev, const char *fmt, ...)
-{
-	return 0;
-}
+#define dev_dbg(dev, format, arg...) \
+	do { if (0) dev_printk(KERN_DEBUG , dev , format, ## arg); } while (0)
 #endif
 
 #ifdef VERBOSE_DEBUG
 #define dev_vdbg	dev_dbg
 #else
-static inline int __attribute__ ((format (printf, 2, 3)))
-dev_vdbg(struct device *dev, const char *fmt, ...)
-{
-	return 0;
-}
+#define dev_vdbg(dev, format, arg...) \
+	do { if (0) dev_printk(KERN_DEBUG , dev , format, ## arg); } while (0)
 #endif
 
 /* Create alias, so I can be autoloaded. */
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 2df44e7..cd24112 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -293,10 +293,8 @@ extern void print_hex_dump_bytes(const char *prefix_str, int prefix_type,
 #define pr_debug(fmt, arg...) \
 	printk(KERN_DEBUG fmt, ##arg)
 #else
-static inline int __attribute__ ((format (printf, 1, 2))) pr_debug(const char * fmt, ...)
-{
-	return 0;
-}
+#define pr_debug(fmt, arg...) \
+	do { if (0) printk(KERN_DEBUG fmt, ##arg); } while (0)
 #endif
 
 /*

Re: New sparse warning in net/mac80211/debugfs_sta.c

[Patrick McHardy]

David Miller wrote:
> From: Harvey Harrison <harvey.harrison@gmail.com>
> Date: Thu, 21 Feb 2008 02:01:19 -0800
> 
>> In this case, it's being passed to a debugfs create function, could it
>> instead use sysfs_format_mac?
> 
> Just assigning print_mac() to a local variable then passing that to
> debugfs_create_dir() will make the warning go away.
> 
>>From another perspective adding that __pure attribute to print_mac()
> might not have been the best idea.  But I can't think of another
> way to elimitate the "passing print_mac() args to pr_debug()
> still generates calls to print_mac() even when DEBUG is not
> defined" problem :-/


Frankly, I think the main problem is that MAC_FMT got removed
for no real reason and is forcing us to come up with lots of
workarounds. We have NIPQUAD_FMT, NIP6_FMT, so I don't see
whats wrong with MAC_FMT. In fact I think a simple

printk(MAC_FMT, addr);

is much nicer than all this temporary buffer and function
call stuff.