In this case, it's being passed to a debugfs create function, could it

instead use sysfs_format_mac?
Harvey
--

From: Harvey Harrison <harvey.harrison@gmail.com>
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.

--

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
 /*
--

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

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);
--

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?
--

BTW, this also affects ATM, with 3 calls in hard_start_xmit,
--

From: Patrick McHardy <kaber@trash.net>
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.
--

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
 ...
[ message continues ]

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

[]
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)

 #els...
[ message continues ]

Yes, I'm using pre-built debian packages though since

I couldn't get the source to work properly on debian
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.
--

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
--

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.
--

From: Joe Perches <joe@perches.com>
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?

--

Enjoy.
As far as I'm concerned, I fixed it February 21.

http://lkml.org/lkml/2008/2/21/244
--

From: Joe Perches <joe@perches.com>
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.

--

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

From: Johannes Berg <johannes@sipsolutions.net>
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

From: Johannes Berg <johannes@sipsolutions.net>
Ok, so it simply can't validate the thing.

--

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

From: Johannes Berg <johannes@sipsolutions.net>
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.

--

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 */...
[ message continues ]

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));
--

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

Well, in this case I'd suggest either reverting the __pure annotation

or just ignoring the warning.
Harvey
--