[PATCH] wireless: at76c50x: fix multithread access to hex2str

[PATCH] wireless: at76c50x: fix multithread access to hex2str

[Andy Shevchenko]

The original hex2str uses finite array of buffers to keep output data. It's a wrong approach, because we can't say at compile time how many threads will be used.

This patch introduces one buffer and global mutex to access this function. All
calls of it are wrapped by locking this mutex. It saves some memory as a side
effect.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: "John W. Linville" <linville@tuxdriver.com>
---
 drivers/net/wireless/at76c50x-usb.c |   23 +++++++++++++++++++----
 1 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/at76c50x-usb.c b/drivers/net/wireless/at76c50x-usb.c
index 39322d4..af043f6 100644
--- a/drivers/net/wireless/at76c50x-usb.c
+++ b/drivers/net/wireless/at76c50x-usb.c
@@ -498,15 +498,14 @@ exit:
 	return ret;
 }
 
-#define HEX2STR_BUFFERS 4
 #define HEX2STR_MAX_LEN 64
 
+static DEFINE_MUTEX(hex2str_mutex);
+
 /* Convert binary data into hex string */
 static char *hex2str(void *buf, size_t len)
 {
-	static atomic_t a = ATOMIC_INIT(0);
-	static char bufs[HEX2STR_BUFFERS][3 * HEX2STR_MAX_LEN + 1];
-	char *ret = bufs[atomic_inc_return(&a) & (HEX2STR_BUFFERS - 1)];
+	static char ret[3 * HEX2STR_MAX_LEN + 1];
 	char *obuf = ret;
 	u8 *ibuf = buf;
 
@@ -1003,10 +1002,13 @@ static void at76_dump_mib_mac_wep(struct at76_priv *priv)
 	key_len = (m->encryption_level == 1) ?
 	    WEP_SMALL_KEY_LEN : WEP_LARGE_KEY_LEN;
 
+	mutex_lock(&hex2str_mutex);
 	for (i = 0; i < WEP_KEYS; i++)
 		at76_dbg(DBG_MIB, "%s: MIB MAC_WEP: key %d: %s",
 			 wiphy_name(priv->hw->wiphy), i,
 			 hex2str(m->wep_default_keyvalue[i], key_len));
+	mutex_unlock(&hex2str_mutex);
+
 exit:
 	kfree(m);
 }
@@ -1028,6 +1030,7 @@ static void at76_dump_mib_mac_mgmt(struct at76_priv *priv)
 		goto exit;
 	}
 
+	mutex_lock(&hex2str_mutex);
 	at76_dbg(DBG_MIB, "%s: MIB MAC_MGMT: beacon_period %d CFP_max_duration "
 		 "%d medium_occupancy_limit %d station_id 0x%x ATIM_window %d "
 		 "CFP_mode %d privacy_opt_impl %d DTIM_period %d CFP_period %d "
@@ -1045,6 +1048,8 @@ static void at76_dump_mib_mac_mgmt(struct at76_priv *priv)
 		 m->current_bss_type, m->power_mgmt_mode, m->ibss_change,
 		 m->res, m->multi_domain_capability_implemented,
 		 m->multi_domain_capability_enabled, m->country_string);
+	mutex_unlock(&hex2str_mutex);
+
 exit:
 	kfree(m);
 }
@@ -1064,6 +1069,7 @@ static void at76_dump_mib_mac(struct at76_priv *priv)
 		goto exit;
 	}
 
+	mutex_lock(&hex2str_mutex);
 	at76_dbg(DBG_MIB, "%s: MIB MAC: max_tx_msdu_lifetime %d "
 		 "max_rx_lifetime %d frag_threshold %d rts_threshold %d "
 		 "cwmin %d cwmax %d short_retry_time %d long_retry_time %d "
@@ -1082,6 +1088,8 @@ static void at76_dump_mib_mac(struct at76_priv *priv)
 		 le16_to_cpu(m->listen_interval),
 		 hex2str(m->desired_ssid, IW_ESSID_MAX_SIZE),
 		 m->desired_bssid, m->desired_bsstype);
+	mutex_unlock(&hex2str_mutex);
+
 exit:
 	kfree(m);
 }
@@ -1160,6 +1168,8 @@ static void at76_dump_mib_mdomain(struct at76_priv *priv)
 		goto exit;
 	}
 
+	mutex_lock(&hex2str_mutex);
+
 	at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: channel_list %s",
 		 wiphy_name(priv->hw->wiphy),
 		 hex2str(m->channel_list, sizeof(m->channel_list)));
@@ -1167,6 +1177,8 @@ static void at76_dump_mib_mdomain(struct at76_priv *priv)
 	at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: tx_powerlevel %s",
 		 wiphy_name(priv->hw->wiphy),
 		 hex2str(m->tx_powerlevel, sizeof(m->tx_powerlevel)));
+
+	mutex_unlock(&hex2str_mutex);
 exit:
 	kfree(m);
 }
@@ -1368,6 +1380,7 @@ static int at76_startup_device(struct at76_priv *priv)
 	struct at76_card_config *ccfg = &priv->card_config;
 	int ret;
 
+	mutex_lock(&hex2str_mutex);
 	at76_dbg(DBG_PARAMS,
 		 "%s param: ssid %.*s (%s) mode %s ch %d wep %s key %d "
 		 "keylen %d", wiphy_name(priv->hw->wiphy), priv->essid_size,
@@ -1375,6 +1388,8 @@ static int at76_startup_device(struct at76_priv *priv)
 		 priv->iw_mode == IW_MODE_ADHOC ? "adhoc" : "infra",
 		 priv->channel, priv->wep_enabled ? "enabled" : "disabled",
 		 priv->wep_key_id, priv->wep_keys_len[priv->wep_key_id]);
+	mutex_unlock(&hex2str_mutex);
+
 	at76_dbg(DBG_PARAMS,
 		 "%s param: preamble %s rts %d retry %d frag %d "
 		 "txrate %s auth_mode %d", wiphy_name(priv->hw->wiphy),
-- 
1.7.6.3

Re: [PATCH] wireless: at76c50x: fix multithread access to hex2str

[Pavel Roskin]

On Wed, 28 Sep 2011 16:17:31 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> The original hex2str uses finite array of buffers to keep output
> data. It's a wrong approach, because we can't say at compile time how
> many threads will be used.
> 
> This patch introduces one buffer and global mutex to access this
> function. All calls of it are wrapped by locking this mutex. It saves
> some memory as a side effect.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: "John W. Linville" <linville@tuxdriver.com>

The reason for having 4 buffers is to allow using hex2str() more than
once in the same printk() statement.  As you can see, hex2str() is
never used in one statement more than once.  Your solution would make
it impossible to use hex2str() twice in the same printk(), as the
buffer would be reused.  Actually, wrong data would be printed with no
warning!  Such code doesn't exist in the driver, but it could be added
by somebody looking for a problem.  And that would hamper debugging
instead of helping it.

I think we can live with a debug function printing wrong data if it's
called by 5 callers at once.

And if you want a perfectly correct fix, I suggest that you use
buffers allocated by the callers on the stack.  That's less intrusive
than mutexes, and it would allow using more than one hex2str() in one
statement as long as the buffers are different.

Or maybe hex2str() should be replaced with calls to
print_hex_dump_bytes()?  That would add extra lines to the kernel
output, but it's no big deal.  Of course, if you are concerned about 5
callers printing bytes in the same time, the output may be hard to
understand, but I don't think it a problem that needs addressing.

Or maybe you could develop an extension for printk() format that would
dump strings of the given length?  Something like %pM, but with an
extra argument (and make sure it would not trigger a gcc warning).
This way, everybody would benefit.

Please rethink whether it's helpful to send such "fixes" for old and
little maintained drivers.  There are few people who can work on them,
and they have little time for the old drivers.  Their time is better
spent fixing real problems than reviewing untested or badly conceived
patches for imaginary problems and style issues.  Besides, there are few
users who could test the driver and report breakage.

-- 
Regards,
Pavel Roskin

Re: [PATCH] wireless: at76c50x: fix multithread access to hex2str

[Andy Shevchenko]

On Wed, 2011-09-28 at 17:19 -0400, Pavel Roskin wrote: 
> On Wed, 28 Sep 2011 16:17:31 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > The original hex2str uses finite array of buffers to keep output
> > data. It's a wrong approach, because we can't say at compile time how
> > many threads will be used.
> > 
> > This patch introduces one buffer and global mutex to access this
> > function. All calls of it are wrapped by locking this mutex. It saves
> > some memory as a side effect.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: "John W. Linville" <linville@tuxdriver.com>
> 
> The reason for having 4 buffers is to allow using hex2str() more than
> once in the same printk() statement.
And even in this case it has a weird limitation. Why 4? Why not 5 or 20?

> As you can see, hex2str() is never used in one statement more than once.
Yeah, that's why it works in both cases.

> Your solution would make
> it impossible to use hex2str() twice in the same printk(), as the
> buffer would be reused.  Actually, wrong data would be printed with no
> warning!  Such code doesn't exist in the driver, but it could be added
> by somebody looking for a problem.  And that would hamper debugging
> instead of helping it.
You know, there is no statement, how to use the function for the 3rd
party developer. We have the mailing list not only for bots. I'm glad to
hear some explanations which I didn't catch from the code itself.

> I think we can live with a debug function printing wrong data if it's
> called by 5 callers at once.
I disagree with such attitude.

> And if you want a perfectly correct fix, I suggest that you use
> buffers allocated by the callers on the stack.  That's less intrusive
> than mutexes, and it would allow using more than one hex2str() in one
> statement as long as the buffers are different.
> Or maybe hex2str() should be replaced with calls to print_hex_dump_bytes()?
I thought about it. Actually I must write RFC prefix to the patch, my
bad.

> Or maybe you could develop an extension for printk() format that would
> dump strings of the given length?  Something like %pM, but with an
> extra argument (and make sure it would not trigger a gcc warning).
> This way, everybody would benefit.
I don't think it would be a noticeable benefit. On the other hand
vsnprintf() is hugely overloaded by many "extensions" to the C99
variant.

> Please rethink whether it's helpful to send such "fixes" for old and
> little maintained drivers.
Last copyright is 2010, TODO list actually suggests to remove hex2str at
all.


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] wireless: at76c50x: fix multithread access to hex2str

[Pavel Roskin]

On Thu, 29 Sep 2011 13:46:55 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Wed, 2011-09-28 at 17:19 -0400, Pavel Roskin wrote: 
> > Or maybe you could develop an extension for printk() format that
> > would dump strings of the given length?  Something like %pM, but
> > with an extra argument (and make sure it would not trigger a gcc
> > warning). This way, everybody would benefit.
> I don't think it would be a noticeable benefit. On the other hand
> vsnprintf() is hugely overloaded by many "extensions" to the C99
> variant.

I looked at the code, and it seems very little is needed to support
"%*pM"

> > Please rethink whether it's helpful to send such "fixes" for old and
> > little maintained drivers.
> Last copyright is 2010, TODO list actually suggests to remove hex2str
> at all.

I don't know where it's written, but I think it's a good idea,
especially if there is a better alternative available for all drivers.

-- 
Regards,
Pavel Roskin

[RFC][PATCH 1/2] lib: printf: append support of '%*p[Mm][FR]'

[Andy Shevchenko]

There are many places in the kernel where the drivers print small buffers as a
hex string. This patch adds a support of the variable width buffer to print it
as a hex string with a delimiter. The idea came from Pavel Roskin here:
http://www.digipedia.pl/usenet/thread/18835/17449/

Sample output of
	pr_info("buf[%d:%d] %*pM\n", from, len, len, &buf[from]);
could be like this:
	[ 0.726130] buf[51:8] e8:16:b6:ef:e3:74:45:6e
	[ 0.750736] buf[59:15] 31:81:b8:3f:35:49:06:ae:df:32:06:05:4a:af:55
	[ 0.757602] buf[17:5] ac:16:d5:2c:ef

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/vsprintf.c |   22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index a0b5f15..1645d7e 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -655,11 +655,12 @@ char *resource_string(char *buf, char *end, struct resource *res,
 }
 
 static noinline_for_stack
-char *mac_address_string(char *buf, char *end, u8 *addr,
-			 struct printf_spec spec, const char *fmt)
+char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
+		 const char *fmt)
 {
-	char mac_addr[sizeof("xx:xx:xx:xx:xx:xx")];
-	char *p = mac_addr;
+	char hex_str[64*3];	/* support up to 64 bytes to print */
+	int len = 6;		/* default length is 6 bytes */
+	char *p = hex_str;
 	int i;
 	char separator;
 	bool reversed = false;
@@ -678,18 +679,21 @@ char *mac_address_string(char *buf, char *end, u8 *addr,
 		break;
 	}
 
-	for (i = 0; i < 6; i++) {
+	if (spec.field_width > 0)
+		len = min_t(int, spec.field_width, 64);
+
+	for (i = 0; i < len; i++) {
 		if (reversed)
-			p = hex_byte_pack(p, addr[5 - i]);
+			p = hex_byte_pack(p, addr[len - 1 - i]);
 		else
 			p = hex_byte_pack(p, addr[i]);
 
-		if (fmt[0] == 'M' && i != 5)
+		if (fmt[0] == 'M' && i != len - 1)
 			*p++ = separator;
 	}
 	*p = '\0';
 
-	return string(buf, end, mac_addr, spec);
+	return string(buf, end, hex_str, spec);
 }
 
 static noinline_for_stack
@@ -1011,7 +1015,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	case 'm':			/* Contiguous: 000102030405 */
 					/* [mM]F (FDDI) */
 					/* [mM]R (Reverse order; Bluetooth) */
-		return mac_address_string(buf, end, ptr, spec, fmt);
+		return hex_string(buf, end, ptr, spec, fmt);
 	case 'I':			/* Formatted IP supported
 					 * 4:	1.2.3.4
 					 * 6:	0001:0203:...:0708
-- 
1.7.10

[RFC][PATCH 2/2] wireless: at76c50x: eliminate hex2str()

[Andy Shevchenko]

The hex2str() is substituted by '%*pMF' specificator.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/net/wireless/at76c50x-usb.c |   54 ++++++++---------------------------
 1 file changed, 12 insertions(+), 42 deletions(-)

diff --git a/drivers/net/wireless/at76c50x-usb.c b/drivers/net/wireless/at76c50x-usb.c
index efc162e..f82f76b 100644
--- a/drivers/net/wireless/at76c50x-usb.c
+++ b/drivers/net/wireless/at76c50x-usb.c
@@ -498,36 +498,6 @@ exit:
 	return ret;
 }
 
-#define HEX2STR_BUFFERS 4
-#define HEX2STR_MAX_LEN 64
-
-/* Convert binary data into hex string */
-static char *hex2str(void *buf, size_t len)
-{
-	static atomic_t a = ATOMIC_INIT(0);
-	static char bufs[HEX2STR_BUFFERS][3 * HEX2STR_MAX_LEN + 1];
-	char *ret = bufs[atomic_inc_return(&a) & (HEX2STR_BUFFERS - 1)];
-	char *obuf = ret;
-	u8 *ibuf = buf;
-
-	if (len > HEX2STR_MAX_LEN)
-		len = HEX2STR_MAX_LEN;
-
-	if (len == 0)
-		goto exit;
-
-	while (len--) {
-		obuf = hex_byte_pack(obuf, *ibuf++);
-		*obuf++ = '-';
-	}
-	obuf--;
-
-exit:
-	*obuf = '\0';
-
-	return ret;
-}
-
 /* LED trigger */
 static int tx_activity;
 static void at76_ledtrig_tx_timerfunc(unsigned long data);
@@ -1004,9 +974,9 @@ static void at76_dump_mib_mac_wep(struct at76_priv *priv)
 	    WEP_SMALL_KEY_LEN : WEP_LARGE_KEY_LEN;
 
 	for (i = 0; i < WEP_KEYS; i++)
-		at76_dbg(DBG_MIB, "%s: MIB MAC_WEP: key %d: %s",
+		at76_dbg(DBG_MIB, "%s: MIB MAC_WEP: key %d: %*pMF",
 			 wiphy_name(priv->hw->wiphy), i,
-			 hex2str(m->wep_default_keyvalue[i], key_len));
+			 key_len, m->wep_default_keyvalue[i]);
 exit:
 	kfree(m);
 }
@@ -1031,7 +1001,7 @@ static void at76_dump_mib_mac_mgmt(struct at76_priv *priv)
 	at76_dbg(DBG_MIB, "%s: MIB MAC_MGMT: beacon_period %d CFP_max_duration "
 		 "%d medium_occupancy_limit %d station_id 0x%x ATIM_window %d "
 		 "CFP_mode %d privacy_opt_impl %d DTIM_period %d CFP_period %d "
-		 "current_bssid %pM current_essid %s current_bss_type %d "
+		 "current_bssid %pM current_essid %*pMF current_bss_type %d "
 		 "pm_mode %d ibss_change %d res %d "
 		 "multi_domain_capability_implemented %d "
 		 "international_roaming %d country_string %.3s",
@@ -1041,7 +1011,7 @@ static void at76_dump_mib_mac_mgmt(struct at76_priv *priv)
 		 le16_to_cpu(m->station_id), le16_to_cpu(m->ATIM_window),
 		 m->CFP_mode, m->privacy_option_implemented, m->DTIM_period,
 		 m->CFP_period, m->current_bssid,
-		 hex2str(m->current_essid, IW_ESSID_MAX_SIZE),
+		 IW_ESSID_MAX_SIZE, m->current_essid,
 		 m->current_bss_type, m->power_mgmt_mode, m->ibss_change,
 		 m->res, m->multi_domain_capability_implemented,
 		 m->multi_domain_capability_enabled, m->country_string);
@@ -1069,7 +1039,7 @@ static void at76_dump_mib_mac(struct at76_priv *priv)
 		 "cwmin %d cwmax %d short_retry_time %d long_retry_time %d "
 		 "scan_type %d scan_channel %d probe_delay %u "
 		 "min_channel_time %d max_channel_time %d listen_int %d "
-		 "desired_ssid %s desired_bssid %pM desired_bsstype %d",
+		 "desired_ssid %*pMF desired_bssid %pM desired_bsstype %d",
 		 wiphy_name(priv->hw->wiphy),
 		 le32_to_cpu(m->max_tx_msdu_lifetime),
 		 le32_to_cpu(m->max_rx_lifetime),
@@ -1080,7 +1050,7 @@ static void at76_dump_mib_mac(struct at76_priv *priv)
 		 le16_to_cpu(m->min_channel_time),
 		 le16_to_cpu(m->max_channel_time),
 		 le16_to_cpu(m->listen_interval),
-		 hex2str(m->desired_ssid, IW_ESSID_MAX_SIZE),
+		 IW_ESSID_MAX_SIZE, m->desired_ssid,
 		 m->desired_bssid, m->desired_bsstype);
 exit:
 	kfree(m);
@@ -1160,13 +1130,13 @@ static void at76_dump_mib_mdomain(struct at76_priv *priv)
 		goto exit;
 	}
 
-	at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: channel_list %s",
+	at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: channel_list %*pMF",
 		 wiphy_name(priv->hw->wiphy),
-		 hex2str(m->channel_list, sizeof(m->channel_list)));
+		 sizeof(m->channel_list), m->channel_list);
 
-	at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: tx_powerlevel %s",
+	at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: tx_powerlevel %*pMF",
 		 wiphy_name(priv->hw->wiphy),
-		 hex2str(m->tx_powerlevel, sizeof(m->tx_powerlevel)));
+		 sizeof(m->tx_powerlevel), m->tx_powerlevel);
 exit:
 	kfree(m);
 }
@@ -1369,9 +1339,9 @@ static int at76_startup_device(struct at76_priv *priv)
 	int ret;
 
 	at76_dbg(DBG_PARAMS,
-		 "%s param: ssid %.*s (%s) mode %s ch %d wep %s key %d "
+		 "%s param: ssid %.*s (%*pMF) mode %s ch %d wep %s key %d "
 		 "keylen %d", wiphy_name(priv->hw->wiphy), priv->essid_size,
-		 priv->essid, hex2str(priv->essid, IW_ESSID_MAX_SIZE),
+		 priv->essid, IW_ESSID_MAX_SIZE, priv->essid,
 		 priv->iw_mode == IW_MODE_ADHOC ? "adhoc" : "infra",
 		 priv->channel, priv->wep_enabled ? "enabled" : "disabled",
 		 priv->wep_key_id, priv->wep_keys_len[priv->wep_key_id]);
-- 
1.7.10

Re: [RFC][PATCH 2/2] wireless: at76c50x: eliminate hex2str()

[Larry Finger]

On 06/29/2012 10:58 AM, Andy Shevchenko wrote:
> The hex2str() is substituted by '%*pMF' specificator.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   drivers/net/wireless/at76c50x-usb.c |   54 ++++++++---------------------------
>   1 file changed, 12 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/net/wireless/at76c50x-usb.c b/drivers/net/wireless/at76c50x-usb.c
> index efc162e..f82f76b 100644
> --- a/drivers/net/wireless/at76c50x-usb.c
> +++ b/drivers/net/wireless/at76c50x-usb.c
> @@ -498,36 +498,6 @@ exit:
>   	return ret;
>   }
>
> -#define HEX2STR_BUFFERS 4
> -#define HEX2STR_MAX_LEN 64
> -
> -/* Convert binary data into hex string */
> -static char *hex2str(void *buf, size_t len)
> -{
> -	static atomic_t a = ATOMIC_INIT(0);
> -	static char bufs[HEX2STR_BUFFERS][3 * HEX2STR_MAX_LEN + 1];
> -	char *ret = bufs[atomic_inc_return(&a) & (HEX2STR_BUFFERS - 1)];
> -	char *obuf = ret;
> -	u8 *ibuf = buf;
> -
> -	if (len > HEX2STR_MAX_LEN)
> -		len = HEX2STR_MAX_LEN;
> -
> -	if (len == 0)
> -		goto exit;
> -
> -	while (len--) {
> -		obuf = hex_byte_pack(obuf, *ibuf++);
> -		*obuf++ = '-';
> -	}
> -	obuf--;
> -
> -exit:
> -	*obuf = '\0';
> -
> -	return ret;
> -}
> -
>   /* LED trigger */
>   static int tx_activity;
>   static void at76_ledtrig_tx_timerfunc(unsigned long data);
> @@ -1004,9 +974,9 @@ static void at76_dump_mib_mac_wep(struct at76_priv *priv)
>   	    WEP_SMALL_KEY_LEN : WEP_LARGE_KEY_LEN;
>
>   	for (i = 0; i < WEP_KEYS; i++)
> -		at76_dbg(DBG_MIB, "%s: MIB MAC_WEP: key %d: %s",
> +		at76_dbg(DBG_MIB, "%s: MIB MAC_WEP: key %d: %*pMF",
>   			 wiphy_name(priv->hw->wiphy), i,
> -			 hex2str(m->wep_default_keyvalue[i], key_len));
> +			 key_len, m->wep_default_keyvalue[i]);
>   exit:
>   	kfree(m);
>   }
> @@ -1031,7 +1001,7 @@ static void at76_dump_mib_mac_mgmt(struct at76_priv *priv)
>   	at76_dbg(DBG_MIB, "%s: MIB MAC_MGMT: beacon_period %d CFP_max_duration "
>   		 "%d medium_occupancy_limit %d station_id 0x%x ATIM_window %d "
>   		 "CFP_mode %d privacy_opt_impl %d DTIM_period %d CFP_period %d "
> -		 "current_bssid %pM current_essid %s current_bss_type %d "
> +		 "current_bssid %pM current_essid %*pMF current_bss_type %d "
>   		 "pm_mode %d ibss_change %d res %d "
>   		 "multi_domain_capability_implemented %d "
>   		 "international_roaming %d country_string %.3s",
> @@ -1041,7 +1011,7 @@ static void at76_dump_mib_mac_mgmt(struct at76_priv *priv)
>   		 le16_to_cpu(m->station_id), le16_to_cpu(m->ATIM_window),
>   		 m->CFP_mode, m->privacy_option_implemented, m->DTIM_period,
>   		 m->CFP_period, m->current_bssid,
> -		 hex2str(m->current_essid, IW_ESSID_MAX_SIZE),
> +		 IW_ESSID_MAX_SIZE, m->current_essid,
>   		 m->current_bss_type, m->power_mgmt_mode, m->ibss_change,
>   		 m->res, m->multi_domain_capability_implemented,
>   		 m->multi_domain_capability_enabled, m->country_string);
> @@ -1069,7 +1039,7 @@ static void at76_dump_mib_mac(struct at76_priv *priv)
>   		 "cwmin %d cwmax %d short_retry_time %d long_retry_time %d "
>   		 "scan_type %d scan_channel %d probe_delay %u "
>   		 "min_channel_time %d max_channel_time %d listen_int %d "
> -		 "desired_ssid %s desired_bssid %pM desired_bsstype %d",
> +		 "desired_ssid %*pMF desired_bssid %pM desired_bsstype %d",
>   		 wiphy_name(priv->hw->wiphy),
>   		 le32_to_cpu(m->max_tx_msdu_lifetime),
>   		 le32_to_cpu(m->max_rx_lifetime),
> @@ -1080,7 +1050,7 @@ static void at76_dump_mib_mac(struct at76_priv *priv)
>   		 le16_to_cpu(m->min_channel_time),
>   		 le16_to_cpu(m->max_channel_time),
>   		 le16_to_cpu(m->listen_interval),
> -		 hex2str(m->desired_ssid, IW_ESSID_MAX_SIZE),
> +		 IW_ESSID_MAX_SIZE, m->desired_ssid,
>   		 m->desired_bssid, m->desired_bsstype);
>   exit:
>   	kfree(m);
> @@ -1160,13 +1130,13 @@ static void at76_dump_mib_mdomain(struct at76_priv *priv)
>   		goto exit;
>   	}
>
> -	at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: channel_list %s",
> +	at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: channel_list %*pMF",
>   		 wiphy_name(priv->hw->wiphy),
> -		 hex2str(m->channel_list, sizeof(m->channel_list)));
> +		 sizeof(m->channel_list), m->channel_list);
>
> -	at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: tx_powerlevel %s",
> +	at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: tx_powerlevel %*pMF",
>   		 wiphy_name(priv->hw->wiphy),
> -		 hex2str(m->tx_powerlevel, sizeof(m->tx_powerlevel)));
> +		 sizeof(m->tx_powerlevel), m->tx_powerlevel);
>   exit:
>   	kfree(m);
>   }
> @@ -1369,9 +1339,9 @@ static int at76_startup_device(struct at76_priv *priv)
>   	int ret;
>
>   	at76_dbg(DBG_PARAMS,
> -		 "%s param: ssid %.*s (%s) mode %s ch %d wep %s key %d "
> +		 "%s param: ssid %.*s (%*pMF) mode %s ch %d wep %s key %d "
>   		 "keylen %d", wiphy_name(priv->hw->wiphy), priv->essid_size,
> -		 priv->essid, hex2str(priv->essid, IW_ESSID_MAX_SIZE),
> +		 priv->essid, IW_ESSID_MAX_SIZE, priv->essid,
>   		 priv->iw_mode == IW_MODE_ADHOC ? "adhoc" : "infra",
>   		 priv->channel, priv->wep_enabled ? "enabled" : "disabled",
>   		 priv->wep_key_id, priv->wep_keys_len[priv->wep_key_id]);
>

I have not yet tested this patch, but it generates warnings on a 64-bit system 
as follows:

CC [M]  drivers/net/wireless/at76c50x-usb.o
drivers/net/wireless/at76c50x-usb.c: In function ‘at76_dump_mib_mdomain’:
drivers/net/wireless/at76c50x-usb.c:1133:2: warning: field width specifier ‘*’ 
expects argument of type ‘int’, but argument 3 has type ‘long unsigned int’ 
[-Wformat]
drivers/net/wireless/at76c50x-usb.c:1137:2: warning: field width specifier ‘*’ 
expects argument of type ‘int’, but argument 3 has type ‘long unsigned int’ 
[-Wformat]

Replacing "sizeof" by "(int)sizeof" removes the warning.

Larry

Re: [RFC][PATCH 1/2] lib: printf: append support of '%*p[Mm][FR]'

[Joe Perches]

On Fri, 2012-06-29 at 18:58 +0300, Andy Shevchenko wrote:
> There are many places in the kernel where the drivers print small buffers as a
> hex string. This patch adds a support of the variable width buffer to print it
> as a hex string with a delimiter. The idea came from Pavel Roskin here:
> http://www.digipedia.pl/usenet/thread/18835/17449/

Seems sensible, but one stack caveat below

> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
[]
> @@ -655,11 +655,12 @@ char *resource_string(char *buf, char *end, struct resource *res,
>  }
>  
>  static noinline_for_stack
> -char *mac_address_string(char *buf, char *end, u8 *addr,
> -			 struct printf_spec spec, const char *fmt)
> +char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
> +		 const char *fmt)
>  {
> -	char mac_addr[sizeof("xx:xx:xx:xx:xx:xx")];
> -	char *p = mac_addr;
> +	char hex_str[64*3];	/* support up to 64 bytes to print */

Might be too much stack though.

Re: [RFC][PATCH 1/2] lib: printf: append support of '%*p[Mm][FR]'

[Andrew Morton]

On Fri, 29 Jun 2012 09:08:06 -0700
Joe Perches <joe@perches.com> wrote:

> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> []
> > @@ -655,11 +655,12 @@ char *resource_string(char *buf, char *end, struct resource *res,
> >  }
> >  
> >  static noinline_for_stack
> > -char *mac_address_string(char *buf, char *end, u8 *addr,
> > -			 struct printf_spec spec, const char *fmt)
> > +char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
> > +		 const char *fmt)
> >  {
> > -	char mac_addr[sizeof("xx:xx:xx:xx:xx:xx")];
> > -	char *p = mac_addr;
> > +	char hex_str[64*3];	/* support up to 64 bytes to print */
> 
> Might be too much stack though.

Yes, it's a bit marginal, as this new capability might be used in debug
or crash situations where we're deep into the stack.  The average case
could be improved by using alloca()-style allocation.

Documentation/printk-formats.txt would need to be updated please.  Also
the big comment over vsnprintf().

Re: [RFC][PATCH 1/2] lib: printf: append support of '%*p[Mm][FR]'

[Joe Perches]

On Fri, 2012-06-29 at 16:26 -0700, Andrew Morton wrote:
> On Fri, 29 Jun 2012 09:08:06 -0700
> Joe Perches <joe@perches.com> wrote:
> 
> > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > []
> > > @@ -655,11 +655,12 @@ char *resource_string(char *buf, char *end, struct resource *res,
> > >  }
> > >  
> > >  static noinline_for_stack
> > > -char *mac_address_string(char *buf, char *end, u8 *addr,
> > > -			 struct printf_spec spec, const char *fmt)
> > > +char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
> > > +		 const char *fmt)
> > >  {
> > > -	char mac_addr[sizeof("xx:xx:xx:xx:xx:xx")];
> > > -	char *p = mac_addr;
> > > +	char hex_str[64*3];	/* support up to 64 bytes to print */
> > 
> > Might be too much stack though.
> 
> Yes, it's a bit marginal, as this new capability might be used in debug
> or crash situations where we're deep into the stack.  The average case
> could be improved by using alloca()-style allocation.

Or maybe support larger sizes with a smaller
stack buffer and a while loop.

Re: [RFC][PATCH 1/2] lib: printf: append support of '%*p[Mm][FR]'

[Andy Shevchenko]

On Sat, Jun 30, 2012 at 5:48 PM, Joe Perches <joe@perches.com> wrote:
> On Fri, 2012-06-29 at 16:26 -0700, Andrew Morton wrote:
>> On Fri, 29 Jun 2012 09:08:06 -0700
>> Joe Perches <joe@perches.com> wrote:
>>
>> > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>> > []
>> > > @@ -655,11 +655,12 @@ char *resource_string(char *buf, char *end, struct resource *res,
>> > >  }
>> > >
>> > >  static noinline_for_stack
>> > > -char *mac_address_string(char *buf, char *end, u8 *addr,
>> > > -                  struct printf_spec spec, const char *fmt)
>> > > +char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
>> > > +          const char *fmt)
>> > >  {
>> > > - char mac_addr[sizeof("xx:xx:xx:xx:xx:xx")];
>> > > - char *p = mac_addr;
>> > > + char hex_str[64*3];     /* support up to 64 bytes to print */
>> >
>> > Might be too much stack though.
>>
>> Yes, it's a bit marginal, as this new capability might be used in debug
>> or crash situations where we're deep into the stack.  The average case
>> could be improved by using alloca()-style allocation.
>
> Or maybe support larger sizes with a smaller
> stack buffer and a while loop.

What do you think about mixed approach? Let's say we would use buffer
on stack for 8 bytes or less, and allocated buffer in case of larger
input. It allows to keep implementation simple.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC][PATCH 1/2] lib: printf: append support of '%*p[Mm][FR]'

[Joe Perches]

On Mon, 2012-07-02 at 20:32 +0300, Andy Shevchenko wrote:
> On Sat, Jun 30, 2012 at 5:48 PM, Joe Perches <joe@perches.com> wrote:
> > On Fri, 2012-06-29 at 16:26 -0700, Andrew Morton wrote:
> >> On Fri, 29 Jun 2012 09:08:06 -0700
> >> Joe Perches <joe@perches.com> wrote:
> >>
> >> > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> >> > []
> >> > > @@ -655,11 +655,12 @@ char *resource_string(char *buf, char *end, struct resource *res,
> >> > >  }
> >> > >
> >> > >  static noinline_for_stack
> >> > > -char *mac_address_string(char *buf, char *end, u8 *addr,
> >> > > -                  struct printf_spec spec, const char *fmt)
> >> > > +char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
> >> > > +          const char *fmt)
> >> > >  {
> >> > > - char mac_addr[sizeof("xx:xx:xx:xx:xx:xx")];
> >> > > - char *p = mac_addr;
> >> > > + char hex_str[64*3];     /* support up to 64 bytes to print */
> >> >
> >> > Might be too much stack though.
> >>
> >> Yes, it's a bit marginal, as this new capability might be used in debug
> >> or crash situations where we're deep into the stack.  The average case
> >> could be improved by using alloca()-style allocation.
> >
> > Or maybe support larger sizes with a smaller
> > stack buffer and a while loop.
> 
> What do you think about mixed approach? Let's say we would use buffer
> on stack for 8 bytes or less, and allocated buffer in case of larger
> input. It allows to keep implementation simple.
> 

I think the while loop is simplest.
I'll code something up tomorrow unless
you get to it first.

[RFC][PATCHv2 1/3] lib: printf: update documentation to cover all of %p[Mm][FR]

[Andy Shevchenko]

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 Documentation/printk-formats.txt |    1 +
 lib/vsprintf.c                   |    3 +++
 2 files changed, 4 insertions(+)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index d8d168fa..90ff4d7 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -59,6 +59,7 @@ MAC/FDDI addresses:
 	%pMR	05:04:03:02:01:00
 	%pMF	00-01-02-03-04-05
 	%pm	000102030405
+	%pmR	050403020100
 
 	For printing 6-byte MAC/FDDI addresses in hex notation. The 'M' and 'm'
 	specifiers result in a printed address with ('M') or without ('m') byte
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index a0b5f15..c65f5d4 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1287,7 +1287,10 @@ qualifier:
  * %pR output the address range in a struct resource with decoded flags
  * %pr output the address range in a struct resource with raw flags
  * %pM output a 6-byte MAC address with colons
+ * %pMR output a 6-byte MAC address with colons in reversed order
+ * %pMF output a 6-byte MAC address with dashes
  * %pm output a 6-byte MAC address without colons
+ * %pmR output a 6-byte MAC address without colons in reversed order
  * %pI4 print an IPv4 address without leading zeros
  * %pi4 print an IPv4 address with leading zeros
  * %pI6 print an IPv6 address with colons
-- 
1.7.10

[RFC][PATCHv2 2/3] lib: printf: append support of '%*p[Mm][FR]'

[Andy Shevchenko]

There are many places in the kernel where the drivers print small buffers as a
hex string. This patch adds a support of the variable width buffer to print it
as a hex string with a delimiter. The idea came from Pavel Roskin here:
http://www.digipedia.pl/usenet/thread/18835/17449/

Sample output of
	pr_info("buf[%d:%d] %*pM\n", from, len, len, &buf[from]);
could be look like this:
	[ 0.726130] buf[51:8] e8:16:b6:ef:e3:74:45:6e
	[ 0.750736] buf[59:15] 31:81:b8:3f:35:49:06:ae:df:32:06:05:4a:af:55
	[ 0.757602] buf[17:5] ac:16:d5:2c:ef

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 Documentation/printk-formats.txt |    5 ++++
 lib/vsprintf.c                   |   49 ++++++++++++++++++++++++++------------
 2 files changed, 39 insertions(+), 15 deletions(-)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 90ff4d7..3ae3d32 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -73,6 +73,11 @@ MAC/FDDI addresses:
 	specifier to use reversed byte order suitable for visual interpretation
 	of Bluetooth addresses which are in the little endian order.
 
+	Optional usage of all of the above is to specify variable length via
+	putting '*' into the specificator ('%*p[Mm][FR]'). In this case it will
+	print up to 64 bytes of the input as a hex string with certain
+	separator. For larger buffers consider to use print_hex_dump().
+
 IPv4 addresses:
 
 	%pI4	1.2.3.4
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index c65f5d4..ef4bbd2 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -655,12 +655,13 @@ char *resource_string(char *buf, char *end, struct resource *res,
 }
 
 static noinline_for_stack
-char *mac_address_string(char *buf, char *end, u8 *addr,
-			 struct printf_spec spec, const char *fmt)
+char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
+		 const char *fmt)
 {
-	char mac_addr[sizeof("xx:xx:xx:xx:xx:xx")];
-	char *p = mac_addr;
-	int i;
+	char hex_str[8*3+1];	/* support up to 8 bytes to print */
+	int len = 6;		/* default length is 6 bytes */
+	char *p;
+	int i = 0, j;
 	char separator;
 	bool reversed = false;
 
@@ -678,18 +679,31 @@ char *mac_address_string(char *buf, char *end, u8 *addr,
 		break;
 	}
 
-	for (i = 0; i < 6; i++) {
-		if (reversed)
-			p = hex_byte_pack(p, addr[5 - i]);
-		else
-			p = hex_byte_pack(p, addr[i]);
+	if (spec.field_width > 0)
+		len = min_t(int, spec.field_width, 64);
+
+	while (i < len) {
+		p = hex_str;
+		for (j = 0; j < 8 && i < len; j++) {
+			if (reversed)
+				p = hex_byte_pack(p, addr[len - 1 - i]);
+			else
+				p = hex_byte_pack(p, addr[i]);
+
+			if (fmt[0] == 'M' && i != len - 1)
+				*p++ = separator;
+			i++;
+		}
+		*p = '\0';
 
-		if (fmt[0] == 'M' && i != 5)
-			*p++ = separator;
+		for (p = hex_str; *p != '\0'; p++) {
+			if (buf < end)
+				*buf = *p;
+			++buf;
+		}
 	}
-	*p = '\0';
 
-	return string(buf, end, mac_addr, spec);
+	return buf;
 }
 
 static noinline_for_stack
@@ -947,6 +961,9 @@ int kptr_restrict __read_mostly;
  * - 'MF' For a 6-byte MAC FDDI address, it prints the address
  *       with a dash-separated hex notation
  * - '[mM]R For a 6-byte MAC address, Reverse order (Bluetooth)
+ *       Optional usage is %*p[Mn][FR] with variable length to print. It
+ *       supports up to 64 bytes of the input. Consider to use print_hex_dump()
+ *       for the larger input.
  * - 'I' [46] for IPv4/IPv6 addresses printed in the usual way
  *       IPv4 uses dot-separated decimal without leading 0's (1.2.3.4)
  *       IPv6 uses colon separated network-order 16 bit hex with leading 0's
@@ -1011,7 +1028,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	case 'm':			/* Contiguous: 000102030405 */
 					/* [mM]F (FDDI) */
 					/* [mM]R (Reverse order; Bluetooth) */
-		return mac_address_string(buf, end, ptr, spec, fmt);
+		return hex_string(buf, end, ptr, spec, fmt);
 	case 'I':			/* Formatted IP supported
 					 * 4:	1.2.3.4
 					 * 6:	0001:0203:...:0708
@@ -1291,6 +1308,8 @@ qualifier:
  * %pMF output a 6-byte MAC address with dashes
  * %pm output a 6-byte MAC address without colons
  * %pmR output a 6-byte MAC address without colons in reversed order
+ * %*p[Mm][FR] a variable-length hex string with a separator (supports up to 64
+ *             bytes of the input)
  * %pI4 print an IPv4 address without leading zeros
  * %pi4 print an IPv4 address with leading zeros
  * %pI6 print an IPv6 address with colons
-- 
1.7.10

Re: [RFC][PATCHv2 2/3] lib: printf: append support of '%*p[Mm][FR]'

[Joe Perches]

On Tue, 2012-07-03 at 13:06 +0300, Andy Shevchenko wrote:
> There are many places in the kernel where the drivers print small buffers as a
> hex string. This patch adds a support of the variable width buffer to print it
> as a hex string with a delimiter. The idea came from Pavel Roskin here:
> http://www.digipedia.pl/usenet/thread/18835/17449/
> 
> Sample output of
> 	pr_info("buf[%d:%d] %*pM\n", from, len, len, &buf[from]);
> could be look like this:
> 	[ 0.726130] buf[51:8] e8:16:b6:ef:e3:74:45:6e
> 	[ 0.750736] buf[59:15] 31:81:b8:3f:35:49:06:ae:df:32:06:05:4a:af:55
> 	[ 0.757602] buf[17:5] ac:16:d5:2c:ef

Hi Andy.

> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
[]
> @@ -655,12 +655,13 @@ char *resource_string(char *buf, char *end, struct resource *res,
[]
> +char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
> +		 const char *fmt)
[]
> @@ -678,18 +679,31 @@ char *mac_address_string(char *buf, char *end, u8 *addr,
[]
> +	while (i < len) {

Oh good, a while loop, thanks.

[]

> @@ -947,6 +961,9 @@ int kptr_restrict __read_mostly;
>   * - 'MF' For a 6-byte MAC FDDI address, it prints the address
>   *       with a dash-separated hex notation
>   * - '[mM]R For a 6-byte MAC address, Reverse order (Bluetooth)
> + *       Optional usage is %*p[Mn][FR] with variable length to print. It
> + *       supports up to 64 bytes of the input. Consider to use print_hex_dump()
> + *       for the larger input.

It might be more sensible to use new, distinct
"%*pH" and "%*ph" functions and not touch the
mac address function at all.  Will anyone ever
really want to emit the buffer in reverse?
I don't think so.

Perhaps when using a hex_string_buffer func the
separator should be a space/no-space with %*pHh.

You could extend the max to 128 or larger now.

Re: [RFC][PATCHv2 2/3] lib: printf: append support of '%*p[Mm][FR]'

[Andy Shevchenko]

On Tue, Jul 3, 2012 at 6:33 PM, Joe Perches <joe@perches.com> wrote:
> On Tue, 2012-07-03 at 13:06 +0300, Andy Shevchenko wrote:
>> There are many places in the kernel where the drivers print small buffers as a
>> hex string. This patch adds a support of the variable width buffer to print it
>> as a hex string with a delimiter. The idea came from Pavel Roskin here:
>> http://www.digipedia.pl/usenet/thread/18835/17449/
>>
>> Sample output of
>>       pr_info("buf[%d:%d] %*pM\n", from, len, len, &buf[from]);
>> could be look like this:
>>       [ 0.726130] buf[51:8] e8:16:b6:ef:e3:74:45:6e
>>       [ 0.750736] buf[59:15] 31:81:b8:3f:35:49:06:ae:df:32:06:05:4a:af:55
>>       [ 0.757602] buf[17:5] ac:16:d5:2c:ef

> It might be more sensible to use new, distinct
> "%*pH" and "%*ph" functions and not touch the
> mac address function at all.  Will anyone ever
> really want to emit the buffer in reverse?
> I don't think so.
Yeah, probably it's only the case for the Bluetooth addresses.

> Perhaps when using a hex_string_buffer func the
> separator should be a space/no-space with %*pHh.
What I learned from today's linux-next is the most used separators are
' ' (space), '' (nothing), ':' and '-'. We have dozens of the cases
for first three. The '-' support could not be implemented
nevertheless.
So, might be %*pHh[CDS] C for 'colon', S for 'space', D for 'dash' looks better.
'Hh' for capital/small letters than?

> You could extend the max to 128 or larger now.
I don't think it is really needed. Most of the current cases usually
print not more than ~30bytes (in average) per time. And I couldn't
imagine good looking printing for long lines anyway.


-- 
With Best Regards,
Andy Shevchenko

Re: [RFC][PATCHv2 2/3] lib: printf: append support of '%*p[Mm][FR]'

[Joe Perches]

On Tue, 2012-07-03 at 21:32 +0300, Andy Shevchenko wrote:
> On Tue, Jul 3, 2012 at 6:33 PM, Joe Perches <joe@perches.com> wrote:
> > On Tue, 2012-07-03 at 13:06 +0300, Andy Shevchenko wrote:
> >> There are many places in the kernel where the drivers print small buffers as a
> >> hex string. This patch adds a support of the variable width buffer to print it
> >> as a hex string with a delimiter. The idea came from Pavel Roskin here:
> >> http://www.digipedia.pl/usenet/thread/18835/17449/
> >>
> >> Sample output of
> >>       pr_info("buf[%d:%d] %*pM\n", from, len, len, &buf[from]);
> >> could be look like this:
> >>       [ 0.726130] buf[51:8] e8:16:b6:ef:e3:74:45:6e
> >>       [ 0.750736] buf[59:15] 31:81:b8:3f:35:49:06:ae:df:32:06:05:4a:af:55
> >>       [ 0.757602] buf[17:5] ac:16:d5:2c:ef
> 
> > It might be more sensible to use new, distinct
> > "%*pH" and "%*ph" functions and not touch the
> > mac address function at all.  Will anyone ever
> > really want to emit the buffer in reverse?
> > I don't think so.
> Yeah, probably it's only the case for the Bluetooth addresses.
> 
> > Perhaps when using a hex_string_buffer func the
> > separator should be a space/no-space with %*pHh.
> What I learned from today's linux-next is the most used separators are
> ' ' (space), '' (nothing), ':' and '-'. We have dozens of the cases
> for first three. The '-' support could not be implemented
> nevertheless.
> So, might be %*pHh[CDS] C for 'colon', S for 'space', D for 'dash' looks better.

Maybe use a space default.

> 'Hh' for capital/small letters than?

If you want, though I'd hope nobody uses upper case.

> > You could extend the max to 128 or larger now.
> I don't think it is really needed.

I hope it's not, but I just don't see the need to limit it.

> Most of the current cases usually
> print not more than ~30bytes (in average) per time. And I couldn't
> imagine good looking printing for long lines anyway.

Yup, they'd be ugly.
print_hex_dump() should be favored anyway.

cheers, Joe

[PATCHv3 1/3] lib: printf: update documentation to cover all of %p[Mm][FR]

[Andy Shevchenko]

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 Documentation/printk-formats.txt |    1 +
 lib/vsprintf.c                   |    5 ++++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index d8d168fa..90ff4d7 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -59,6 +59,7 @@ MAC/FDDI addresses:
 	%pMR	05:04:03:02:01:00
 	%pMF	00-01-02-03-04-05
 	%pm	000102030405
+	%pmR	050403020100
 
 	For printing 6-byte MAC/FDDI addresses in hex notation. The 'M' and 'm'
 	specifiers result in a printed address with ('M') or without ('m') byte
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index a0b5f15..e500158 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -946,7 +946,7 @@ int kptr_restrict __read_mostly;
  * - 'm' For a 6-byte MAC address, it prints the hex address without colons
  * - 'MF' For a 6-byte MAC FDDI address, it prints the address
  *       with a dash-separated hex notation
- * - '[mM]R For a 6-byte MAC address, Reverse order (Bluetooth)
+ * - '[mM]R' For a 6-byte MAC address, Reverse order (Bluetooth)
  * - 'I' [46] for IPv4/IPv6 addresses printed in the usual way
  *       IPv4 uses dot-separated decimal without leading 0's (1.2.3.4)
  *       IPv6 uses colon separated network-order 16 bit hex with leading 0's
@@ -1287,7 +1287,10 @@ qualifier:
  * %pR output the address range in a struct resource with decoded flags
  * %pr output the address range in a struct resource with raw flags
  * %pM output a 6-byte MAC address with colons
+ * %pMR output a 6-byte MAC address with colons in reversed order
+ * %pMF output a 6-byte MAC address with dashes
  * %pm output a 6-byte MAC address without colons
+ * %pmR output a 6-byte MAC address without colons in reversed order
  * %pI4 print an IPv4 address without leading zeros
  * %pi4 print an IPv4 address with leading zeros
  * %pI6 print an IPv6 address with colons
-- 
1.7.10

[PATCHv3 2/3] lib: printf: append support of '%*ph[CDN]'

[Andy Shevchenko]

There are many places in the kernel where the drivers print small buffers as a
hex string. This patch adds a support of the variable width buffer to print it
as a hex string with a delimiter. The idea came from Pavel Roskin here:
http://www.digipedia.pl/usenet/thread/18835/17449/

Sample output of
	pr_info("buf[%d:%d] %*phC\n", from, len, len, &buf[from]);
could be look like this:
	[ 0.726130] buf[51:8] e8:16:b6:ef:e3:74:45:6e
	[ 0.750736] buf[59:15] 31:81:b8:3f:35:49:06:ae:df:32:06:05:4a:af:55
	[ 0.757602] buf[17:5] ac:16:d5:2c:ef

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 Documentation/printk-formats.txt |   10 ++++++
 lib/vsprintf.c                   |   62 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 90ff4d7..8ffb274 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -53,6 +53,16 @@ Struct Resources:
 	For printing struct resources. The 'R' and 'r' specifiers result in a
 	printed resource with ('R') or without ('r') a decoded flags member.
 
+Raw buffer as a hex string:
+	%*ph	00 01 02  ...  3f
+	%*phC	00:01:02: ... :3f
+	%*phD	00-01-02- ... -3f
+	%*phN	000102 ... 3f
+
+	For printing a small buffers (up to 64 bytes long) as a hex string with
+	certain separator. For the larger buffers consider to use
+	print_hex_dump().
+
 MAC/FDDI addresses:
 
 	%pM	00:01:02:03:04:05
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index e500158..86d6a12 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -655,6 +655,57 @@ char *resource_string(char *buf, char *end, struct resource *res,
 }
 
 static noinline_for_stack
+char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
+		 const char *fmt)
+{
+	char hex_str[8*3+1];	/* support up to 8 bytes to print */
+	int len;
+	char *p;
+	int i = 0, j;
+	char separator;
+
+	switch (fmt[1]) {
+	case 'C':
+		separator = ':';
+		break;
+	case 'D':
+		separator = '-';
+		break;
+	case 'N':
+		separator = 0;
+		break;
+	default:
+		separator = ' ';
+		break;
+	}
+
+	if (spec.field_width <= 0)
+		/* nothing to print */
+		return buf;
+
+	len = min_t(int, spec.field_width, 64);
+
+	while (i < len) {
+		p = hex_str;
+		for (j = 0; j < 8 && i < len; j++, i++) {
+			p = hex_byte_pack(p, addr[i]);
+
+			if (separator && i != len - 1)
+				*p++ = separator;
+		}
+		*p = '\0';
+
+		for (p = hex_str; *p != '\0'; p++) {
+			if (buf < end)
+				*buf = *p;
+			++buf;
+		}
+	}
+
+	return buf;
+}
+
+static noinline_for_stack
 char *mac_address_string(char *buf, char *end, u8 *addr,
 			 struct printf_spec spec, const char *fmt)
 {
@@ -974,6 +1025,13 @@ int kptr_restrict __read_mostly;
  *       correctness of the format string and va_list arguments.
  * - 'K' For a kernel pointer that should be hidden from unprivileged users
  * - 'NF' For a netdev_features_t
+ * - 'h[CDN]' For a variable-length buffer, it prints it as a hex string with
+ *            a certain separator (' ' by default):
+ *              C colon
+ *              D dash
+ *              N no separator
+ *            The maximum supported length is 64 bytes of the input. Consider
+ *            to use print_hex_dump() for the larger input.
  *
  * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
  * function pointers are really function descriptors, which contain a
@@ -1007,6 +1065,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	case 'R':
 	case 'r':
 		return resource_string(buf, end, ptr, spec, fmt);
+	case 'h':
+		return hex_string(buf, end, ptr, spec, fmt);
 	case 'M':			/* Colon separated: 00:01:02:03:04:05 */
 	case 'm':			/* Contiguous: 000102030405 */
 					/* [mM]F (FDDI) */
@@ -1298,6 +1358,8 @@ qualifier:
  * %pI6c print an IPv6 address as specified by RFC 5952
  * %pU[bBlL] print a UUID/GUID in big or little endian using lower or upper
  *   case.
+ * %*ph[CDN] a variable-length hex string with a separator (supports up to 64
+ *           bytes of the input)
  * %n is ignored
  *
  * ** Please update Documentation/printk-formats.txt when making changes **
-- 
1.7.10

Re: [PATCHv3 2/3] lib: printf: append support of '%*ph[CDN]'

[Joe Perches]

On Wed, 2012-07-04 at 11:45 +0300, Andy Shevchenko wrote:
> This patch adds a support of the variable width buffer to print it
> as a hex string with a delimiter.

Hi again Andy.

> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
[]
> @@ -655,6 +655,57 @@ char *resource_string(char *buf, char *end, struct resource *res,
>  }
>  
>  static noinline_for_stack
> +char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
> +		 const char *fmt)
> +{
> +	char hex_str[8*3+1];	/* support up to 8 bytes to print */

I think you don't need hex_str at all.

[]

> +	if (spec.field_width <= 0)
> +		/* nothing to print */
> +		return buf;

It may be better to default to a 1 and add

	if (addr == ZERO_OR_NULL_PTR)

to avoid dereferencing a NULL or a pointer
to a zero length object.

> +
> +	len = min_t(int, spec.field_width, 64);
> +
> +	while (i < len) {
> +		p = hex_str;
> +		for (j = 0; j < 8 && i < len; j++, i++) {
> +			p = hex_byte_pack(p, addr[i]);
> +
> +			if (separator && i != len - 1)
> +				*p++ = separator;
> +		}
> +		*p = '\0';
> +
> +		for (p = hex_str; *p != '\0'; p++) {
> +			if (buf < end)
> +				*buf = *p;
> +			++buf;
> +		}

why not just directly write to *buf as long as buf < end?

cheers, Joe

Re: [PATCHv3 2/3] lib: printf: append support of '%*ph[CDN]'

[Andy Shevchenko]

On Wed, Jul 4, 2012 at 6:09 PM, Joe Perches <joe@perches.com> wrote:
> On Wed, 2012-07-04 at 11:45 +0300, Andy Shevchenko wrote:
>> This patch adds a support of the variable width buffer to print it
>> as a hex string with a delimiter.

>> +     if (spec.field_width <= 0)
>> +             /* nothing to print */
>> +             return buf;
>
> It may be better to default to a 1 and add
>
>         if (addr == ZERO_OR_NULL_PTR)
>
> to avoid dereferencing a NULL or a pointer
> to a zero length object.
Good point.

>> +             for (p = hex_str; *p != '\0'; p++) {
>> +                     if (buf < end)
>> +                             *buf = *p;
>> +                     ++buf;
>> +             }
>
> why not just directly write to *buf as long as buf < end?
buf < end - 1, otherwise correct.
And yes, it eliminates hex_str array as well.

-- 
With Best Regards,
Andy Shevchenko

[PATCHv3.6] lib: printf: append support of '%*ph[CDN]'

[Andy Shevchenko]

There are many places in the kernel where the drivers print small buffers as a
hex string. This patch adds a support of the variable width buffer to print it
as a hex string with a delimiter. The idea came from Pavel Roskin here:
http://www.digipedia.pl/usenet/thread/18835/17449/

Sample output of
	pr_info("buf[%d:%d] %*phC\n", from, len, len, &buf[from]);
could be look like this:
	[ 0.726130] buf[51:8] e8:16:b6:ef:e3:74:45:6e
	[ 0.750736] buf[59:15] 31:81:b8:3f:35:49:06:ae:df:32:06:05:4a:af:55
	[ 0.757602] buf[17:5] ac:16:d5:2c:ef

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 Documentation/printk-formats.txt |   10 +++++++
 lib/vsprintf.c                   |   55 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 90ff4d7..8ffb274 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -53,6 +53,16 @@ Struct Resources:
 	For printing struct resources. The 'R' and 'r' specifiers result in a
 	printed resource with ('R') or without ('r') a decoded flags member.
 
+Raw buffer as a hex string:
+	%*ph	00 01 02  ...  3f
+	%*phC	00:01:02: ... :3f
+	%*phD	00-01-02- ... -3f
+	%*phN	000102 ... 3f
+
+	For printing a small buffers (up to 64 bytes long) as a hex string with
+	certain separator. For the larger buffers consider to use
+	print_hex_dump().
+
 MAC/FDDI addresses:
 
 	%pM	00:01:02:03:04:05
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index e500158..8afa954 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -655,6 +655,50 @@ char *resource_string(char *buf, char *end, struct resource *res,
 }
 
 static noinline_for_stack
+char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
+		 const char *fmt)
+{
+	int i, len = 1;		/* if we pass '%ph[CDN]', field witdh remains
+				   negative value, fallback to the default */
+	char separator;
+
+	if (spec.field_width == 0)
+		/* nothing to print */
+		return buf;
+
+	if (ZERO_OR_NULL_PTR(addr))
+		/* NULL pointer */
+		return string(buf, end, NULL, spec);
+
+	switch (fmt[1]) {
+	case 'C':
+		separator = ':';
+		break;
+	case 'D':
+		separator = '-';
+		break;
+	case 'N':
+		separator = 0;
+		break;
+	default:
+		separator = ' ';
+		break;
+	}
+
+	if (spec.field_width > 0)
+		len = min_t(int, spec.field_width, 64);
+
+	for (i = 0; i < len && buf < end - 1; i++) {
+		buf = hex_byte_pack(buf, addr[i]);
+
+		if (buf < end && separator && i != len - 1)
+			*buf++ = separator;
+	}
+
+	return buf;
+}
+
+static noinline_for_stack
 char *mac_address_string(char *buf, char *end, u8 *addr,
 			 struct printf_spec spec, const char *fmt)
 {
@@ -974,6 +1018,13 @@ int kptr_restrict __read_mostly;
  *       correctness of the format string and va_list arguments.
  * - 'K' For a kernel pointer that should be hidden from unprivileged users
  * - 'NF' For a netdev_features_t
+ * - 'h[CDN]' For a variable-length buffer, it prints it as a hex string with
+ *            a certain separator (' ' by default):
+ *              C colon
+ *              D dash
+ *              N no separator
+ *            The maximum supported length is 64 bytes of the input. Consider
+ *            to use print_hex_dump() for the larger input.
  *
  * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
  * function pointers are really function descriptors, which contain a
@@ -1007,6 +1058,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	case 'R':
 	case 'r':
 		return resource_string(buf, end, ptr, spec, fmt);
+	case 'h':
+		return hex_string(buf, end, ptr, spec, fmt);
 	case 'M':			/* Colon separated: 00:01:02:03:04:05 */
 	case 'm':			/* Contiguous: 000102030405 */
 					/* [mM]F (FDDI) */
@@ -1298,6 +1351,8 @@ qualifier:
  * %pI6c print an IPv6 address as specified by RFC 5952
  * %pU[bBlL] print a UUID/GUID in big or little endian using lower or upper
  *   case.
+ * %*ph[CDN] a variable-length hex string with a separator (supports up to 64
+ *           bytes of the input)
  * %n is ignored
  *
  * ** Please update Documentation/printk-formats.txt when making changes **
-- 
1.7.10

[PATCHv3 3/3] wireless: at76c50x: eliminate hex2str()

[Andy Shevchenko]

The hex2str() is substituted by '%*phD' specificator.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/net/wireless/at76c50x-usb.c |   54 ++++++++---------------------------
 1 file changed, 12 insertions(+), 42 deletions(-)

diff --git a/drivers/net/wireless/at76c50x-usb.c b/drivers/net/wireless/at76c50x-usb.c
index efc162e..e5bd2ad 100644
--- a/drivers/net/wireless/at76c50x-usb.c
+++ b/drivers/net/wireless/at76c50x-usb.c
@@ -498,36 +498,6 @@ exit:
 	return ret;
 }
 
-#define HEX2STR_BUFFERS 4
-#define HEX2STR_MAX_LEN 64
-
-/* Convert binary data into hex string */
-static char *hex2str(void *buf, size_t len)
-{
-	static atomic_t a = ATOMIC_INIT(0);
-	static char bufs[HEX2STR_BUFFERS][3 * HEX2STR_MAX_LEN + 1];
-	char *ret = bufs[atomic_inc_return(&a) & (HEX2STR_BUFFERS - 1)];
-	char *obuf = ret;
-	u8 *ibuf = buf;
-
-	if (len > HEX2STR_MAX_LEN)
-		len = HEX2STR_MAX_LEN;
-
-	if (len == 0)
-		goto exit;
-
-	while (len--) {
-		obuf = hex_byte_pack(obuf, *ibuf++);
-		*obuf++ = '-';
-	}
-	obuf--;
-
-exit:
-	*obuf = '\0';
-
-	return ret;
-}
-
 /* LED trigger */
 static int tx_activity;
 static void at76_ledtrig_tx_timerfunc(unsigned long data);
@@ -1004,9 +974,9 @@ static void at76_dump_mib_mac_wep(struct at76_priv *priv)
 	    WEP_SMALL_KEY_LEN : WEP_LARGE_KEY_LEN;
 
 	for (i = 0; i < WEP_KEYS; i++)
-		at76_dbg(DBG_MIB, "%s: MIB MAC_WEP: key %d: %s",
+		at76_dbg(DBG_MIB, "%s: MIB MAC_WEP: key %d: %*phD",
 			 wiphy_name(priv->hw->wiphy), i,
-			 hex2str(m->wep_default_keyvalue[i], key_len));
+			 key_len, m->wep_default_keyvalue[i]);
 exit:
 	kfree(m);
 }
@@ -1031,7 +1001,7 @@ static void at76_dump_mib_mac_mgmt(struct at76_priv *priv)
 	at76_dbg(DBG_MIB, "%s: MIB MAC_MGMT: beacon_period %d CFP_max_duration "
 		 "%d medium_occupancy_limit %d station_id 0x%x ATIM_window %d "
 		 "CFP_mode %d privacy_opt_impl %d DTIM_period %d CFP_period %d "
-		 "current_bssid %pM current_essid %s current_bss_type %d "
+		 "current_bssid %pM current_essid %*phD current_bss_type %d "
 		 "pm_mode %d ibss_change %d res %d "
 		 "multi_domain_capability_implemented %d "
 		 "international_roaming %d country_string %.3s",
@@ -1041,7 +1011,7 @@ static void at76_dump_mib_mac_mgmt(struct at76_priv *priv)
 		 le16_to_cpu(m->station_id), le16_to_cpu(m->ATIM_window),
 		 m->CFP_mode, m->privacy_option_implemented, m->DTIM_period,
 		 m->CFP_period, m->current_bssid,
-		 hex2str(m->current_essid, IW_ESSID_MAX_SIZE),
+		 IW_ESSID_MAX_SIZE, m->current_essid,
 		 m->current_bss_type, m->power_mgmt_mode, m->ibss_change,
 		 m->res, m->multi_domain_capability_implemented,
 		 m->multi_domain_capability_enabled, m->country_string);
@@ -1069,7 +1039,7 @@ static void at76_dump_mib_mac(struct at76_priv *priv)
 		 "cwmin %d cwmax %d short_retry_time %d long_retry_time %d "
 		 "scan_type %d scan_channel %d probe_delay %u "
 		 "min_channel_time %d max_channel_time %d listen_int %d "
-		 "desired_ssid %s desired_bssid %pM desired_bsstype %d",
+		 "desired_ssid %*phD desired_bssid %pM desired_bsstype %d",
 		 wiphy_name(priv->hw->wiphy),
 		 le32_to_cpu(m->max_tx_msdu_lifetime),
 		 le32_to_cpu(m->max_rx_lifetime),
@@ -1080,7 +1050,7 @@ static void at76_dump_mib_mac(struct at76_priv *priv)
 		 le16_to_cpu(m->min_channel_time),
 		 le16_to_cpu(m->max_channel_time),
 		 le16_to_cpu(m->listen_interval),
-		 hex2str(m->desired_ssid, IW_ESSID_MAX_SIZE),
+		 IW_ESSID_MAX_SIZE, m->desired_ssid,
 		 m->desired_bssid, m->desired_bsstype);
 exit:
 	kfree(m);
@@ -1160,13 +1130,13 @@ static void at76_dump_mib_mdomain(struct at76_priv *priv)
 		goto exit;
 	}
 
-	at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: channel_list %s",
+	at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: channel_list %*phD",
 		 wiphy_name(priv->hw->wiphy),
-		 hex2str(m->channel_list, sizeof(m->channel_list)));
+		 (int)sizeof(m->channel_list), m->channel_list);
 
-	at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: tx_powerlevel %s",
+	at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: tx_powerlevel %*phD",
 		 wiphy_name(priv->hw->wiphy),
-		 hex2str(m->tx_powerlevel, sizeof(m->tx_powerlevel)));
+		 (int)sizeof(m->tx_powerlevel), m->tx_powerlevel);
 exit:
 	kfree(m);
 }
@@ -1369,9 +1339,9 @@ static int at76_startup_device(struct at76_priv *priv)
 	int ret;
 
 	at76_dbg(DBG_PARAMS,
-		 "%s param: ssid %.*s (%s) mode %s ch %d wep %s key %d "
+		 "%s param: ssid %.*s (%*phD) mode %s ch %d wep %s key %d "
 		 "keylen %d", wiphy_name(priv->hw->wiphy), priv->essid_size,
-		 priv->essid, hex2str(priv->essid, IW_ESSID_MAX_SIZE),
+		 priv->essid, IW_ESSID_MAX_SIZE, priv->essid,
 		 priv->iw_mode == IW_MODE_ADHOC ? "adhoc" : "infra",
 		 priv->channel, priv->wep_enabled ? "enabled" : "disabled",
 		 priv->wep_key_id, priv->wep_keys_len[priv->wep_key_id]);
-- 
1.7.10

[resend][PATCH] wireless: at76c50x: eliminate hex2str()

[Andy Shevchenko]

The hex2str() is substituted by '%*phD' specificator.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Larry Finger <Larry.Finger@lwfinger.net>

---
 drivers/net/wireless/at76c50x-usb.c |   54 ++++++++---------------------------
 1 file changed, 12 insertions(+), 42 deletions(-)

diff --git a/drivers/net/wireless/at76c50x-usb.c b/drivers/net/wireless/at76c50x-usb.c
index e361afe..99b9ddf 100644
--- a/drivers/net/wireless/at76c50x-usb.c
+++ b/drivers/net/wireless/at76c50x-usb.c
@@ -498,36 +498,6 @@ exit:
 	return ret;
 }
 
-#define HEX2STR_BUFFERS 4
-#define HEX2STR_MAX_LEN 64
-
-/* Convert binary data into hex string */
-static char *hex2str(void *buf, size_t len)
-{
-	static atomic_t a = ATOMIC_INIT(0);
-	static char bufs[HEX2STR_BUFFERS][3 * HEX2STR_MAX_LEN + 1];
-	char *ret = bufs[atomic_inc_return(&a) & (HEX2STR_BUFFERS - 1)];
-	char *obuf = ret;
-	u8 *ibuf = buf;
-
-	if (len > HEX2STR_MAX_LEN)
-		len = HEX2STR_MAX_LEN;
-
-	if (len == 0)
-		goto exit;
-
-	while (len--) {
-		obuf = hex_byte_pack(obuf, *ibuf++);
-		*obuf++ = '-';
-	}
-	obuf--;
-
-exit:
-	*obuf = '\0';
-
-	return ret;
-}
-
 /* LED trigger */
 static int tx_activity;
 static void at76_ledtrig_tx_timerfunc(unsigned long data);
@@ -1004,9 +974,9 @@ static void at76_dump_mib_mac_wep(struct at76_priv *priv)
 	    WEP_SMALL_KEY_LEN : WEP_LARGE_KEY_LEN;
 
 	for (i = 0; i < WEP_KEYS; i++)
-		at76_dbg(DBG_MIB, "%s: MIB MAC_WEP: key %d: %s",
+		at76_dbg(DBG_MIB, "%s: MIB MAC_WEP: key %d: %*phD",
 			 wiphy_name(priv->hw->wiphy), i,
-			 hex2str(m->wep_default_keyvalue[i], key_len));
+			 key_len, m->wep_default_keyvalue[i]);
 exit:
 	kfree(m);
 }
@@ -1031,7 +1001,7 @@ static void at76_dump_mib_mac_mgmt(struct at76_priv *priv)
 	at76_dbg(DBG_MIB, "%s: MIB MAC_MGMT: beacon_period %d CFP_max_duration "
 		 "%d medium_occupancy_limit %d station_id 0x%x ATIM_window %d "
 		 "CFP_mode %d privacy_opt_impl %d DTIM_period %d CFP_period %d "
-		 "current_bssid %pM current_essid %s current_bss_type %d "
+		 "current_bssid %pM current_essid %*phD current_bss_type %d "
 		 "pm_mode %d ibss_change %d res %d "
 		 "multi_domain_capability_implemented %d "
 		 "international_roaming %d country_string %.3s",
@@ -1041,7 +1011,7 @@ static void at76_dump_mib_mac_mgmt(struct at76_priv *priv)
 		 le16_to_cpu(m->station_id), le16_to_cpu(m->ATIM_window),
 		 m->CFP_mode, m->privacy_option_implemented, m->DTIM_period,
 		 m->CFP_period, m->current_bssid,
-		 hex2str(m->current_essid, IW_ESSID_MAX_SIZE),
+		 IW_ESSID_MAX_SIZE, m->current_essid,
 		 m->current_bss_type, m->power_mgmt_mode, m->ibss_change,
 		 m->res, m->multi_domain_capability_implemented,
 		 m->multi_domain_capability_enabled, m->country_string);
@@ -1069,7 +1039,7 @@ static void at76_dump_mib_mac(struct at76_priv *priv)
 		 "cwmin %d cwmax %d short_retry_time %d long_retry_time %d "
 		 "scan_type %d scan_channel %d probe_delay %u "
 		 "min_channel_time %d max_channel_time %d listen_int %d "
-		 "desired_ssid %s desired_bssid %pM desired_bsstype %d",
+		 "desired_ssid %*phD desired_bssid %pM desired_bsstype %d",
 		 wiphy_name(priv->hw->wiphy),
 		 le32_to_cpu(m->max_tx_msdu_lifetime),
 		 le32_to_cpu(m->max_rx_lifetime),
@@ -1080,7 +1050,7 @@ static void at76_dump_mib_mac(struct at76_priv *priv)
 		 le16_to_cpu(m->min_channel_time),
 		 le16_to_cpu(m->max_channel_time),
 		 le16_to_cpu(m->listen_interval),
-		 hex2str(m->desired_ssid, IW_ESSID_MAX_SIZE),
+		 IW_ESSID_MAX_SIZE, m->desired_ssid,
 		 m->desired_bssid, m->desired_bsstype);
 exit:
 	kfree(m);
@@ -1160,13 +1130,13 @@ static void at76_dump_mib_mdomain(struct at76_priv *priv)
 		goto exit;
 	}
 
-	at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: channel_list %s",
+	at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: channel_list %*phD",
 		 wiphy_name(priv->hw->wiphy),
-		 hex2str(m->channel_list, sizeof(m->channel_list)));
+		 (int)sizeof(m->channel_list), m->channel_list);
 
-	at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: tx_powerlevel %s",
+	at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: tx_powerlevel %*phD",
 		 wiphy_name(priv->hw->wiphy),
-		 hex2str(m->tx_powerlevel, sizeof(m->tx_powerlevel)));
+		 (int)sizeof(m->tx_powerlevel), m->tx_powerlevel);
 exit:
 	kfree(m);
 }
@@ -1369,9 +1339,9 @@ static int at76_startup_device(struct at76_priv *priv)
 	int ret;
 
 	at76_dbg(DBG_PARAMS,
-		 "%s param: ssid %.*s (%s) mode %s ch %d wep %s key %d "
+		 "%s param: ssid %.*s (%*phD) mode %s ch %d wep %s key %d "
 		 "keylen %d", wiphy_name(priv->hw->wiphy), priv->essid_size,
-		 priv->essid, hex2str(priv->essid, IW_ESSID_MAX_SIZE),
+		 priv->essid, IW_ESSID_MAX_SIZE, priv->essid,
 		 priv->iw_mode == IW_MODE_ADHOC ? "adhoc" : "infra",
 		 priv->channel, priv->wep_enabled ? "enabled" : "disabled",
 		 priv->wep_key_id, priv->wep_keys_len[priv->wep_key_id]);
-- 
1.7.10.4

Re: [resend][PATCH] wireless: at76c50x: eliminate hex2str()

[John W. Linville]

Could you point me at the commit that adds the 'phD' specification?

On Wed, Sep 05, 2012 at 11:52:32AM +0300, Andy Shevchenko wrote:
> The hex2str() is substituted by '%*phD' specificator.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Tested-by: Larry Finger <Larry.Finger@lwfinger.net>
> 
> ---
>  drivers/net/wireless/at76c50x-usb.c |   54 ++++++++---------------------------
>  1 file changed, 12 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/net/wireless/at76c50x-usb.c b/drivers/net/wireless/at76c50x-usb.c
> index e361afe..99b9ddf 100644
> --- a/drivers/net/wireless/at76c50x-usb.c
> +++ b/drivers/net/wireless/at76c50x-usb.c
> @@ -498,36 +498,6 @@ exit:
>  	return ret;
>  }
>  
> -#define HEX2STR_BUFFERS 4
> -#define HEX2STR_MAX_LEN 64
> -
> -/* Convert binary data into hex string */
> -static char *hex2str(void *buf, size_t len)
> -{
> -	static atomic_t a = ATOMIC_INIT(0);
> -	static char bufs[HEX2STR_BUFFERS][3 * HEX2STR_MAX_LEN + 1];
> -	char *ret = bufs[atomic_inc_return(&a) & (HEX2STR_BUFFERS - 1)];
> -	char *obuf = ret;
> -	u8 *ibuf = buf;
> -
> -	if (len > HEX2STR_MAX_LEN)
> -		len = HEX2STR_MAX_LEN;
> -
> -	if (len == 0)
> -		goto exit;
> -
> -	while (len--) {
> -		obuf = hex_byte_pack(obuf, *ibuf++);
> -		*obuf++ = '-';
> -	}
> -	obuf--;
> -
> -exit:
> -	*obuf = '\0';
> -
> -	return ret;
> -}
> -
>  /* LED trigger */
>  static int tx_activity;
>  static void at76_ledtrig_tx_timerfunc(unsigned long data);
> @@ -1004,9 +974,9 @@ static void at76_dump_mib_mac_wep(struct at76_priv *priv)
>  	    WEP_SMALL_KEY_LEN : WEP_LARGE_KEY_LEN;
>  
>  	for (i = 0; i < WEP_KEYS; i++)
> -		at76_dbg(DBG_MIB, "%s: MIB MAC_WEP: key %d: %s",
> +		at76_dbg(DBG_MIB, "%s: MIB MAC_WEP: key %d: %*phD",
>  			 wiphy_name(priv->hw->wiphy), i,
> -			 hex2str(m->wep_default_keyvalue[i], key_len));
> +			 key_len, m->wep_default_keyvalue[i]);
>  exit:
>  	kfree(m);
>  }
> @@ -1031,7 +1001,7 @@ static void at76_dump_mib_mac_mgmt(struct at76_priv *priv)
>  	at76_dbg(DBG_MIB, "%s: MIB MAC_MGMT: beacon_period %d CFP_max_duration "
>  		 "%d medium_occupancy_limit %d station_id 0x%x ATIM_window %d "
>  		 "CFP_mode %d privacy_opt_impl %d DTIM_period %d CFP_period %d "
> -		 "current_bssid %pM current_essid %s current_bss_type %d "
> +		 "current_bssid %pM current_essid %*phD current_bss_type %d "
>  		 "pm_mode %d ibss_change %d res %d "
>  		 "multi_domain_capability_implemented %d "
>  		 "international_roaming %d country_string %.3s",
> @@ -1041,7 +1011,7 @@ static void at76_dump_mib_mac_mgmt(struct at76_priv *priv)
>  		 le16_to_cpu(m->station_id), le16_to_cpu(m->ATIM_window),
>  		 m->CFP_mode, m->privacy_option_implemented, m->DTIM_period,
>  		 m->CFP_period, m->current_bssid,
> -		 hex2str(m->current_essid, IW_ESSID_MAX_SIZE),
> +		 IW_ESSID_MAX_SIZE, m->current_essid,
>  		 m->current_bss_type, m->power_mgmt_mode, m->ibss_change,
>  		 m->res, m->multi_domain_capability_implemented,
>  		 m->multi_domain_capability_enabled, m->country_string);
> @@ -1069,7 +1039,7 @@ static void at76_dump_mib_mac(struct at76_priv *priv)
>  		 "cwmin %d cwmax %d short_retry_time %d long_retry_time %d "
>  		 "scan_type %d scan_channel %d probe_delay %u "
>  		 "min_channel_time %d max_channel_time %d listen_int %d "
> -		 "desired_ssid %s desired_bssid %pM desired_bsstype %d",
> +		 "desired_ssid %*phD desired_bssid %pM desired_bsstype %d",
>  		 wiphy_name(priv->hw->wiphy),
>  		 le32_to_cpu(m->max_tx_msdu_lifetime),
>  		 le32_to_cpu(m->max_rx_lifetime),
> @@ -1080,7 +1050,7 @@ static void at76_dump_mib_mac(struct at76_priv *priv)
>  		 le16_to_cpu(m->min_channel_time),
>  		 le16_to_cpu(m->max_channel_time),
>  		 le16_to_cpu(m->listen_interval),
> -		 hex2str(m->desired_ssid, IW_ESSID_MAX_SIZE),
> +		 IW_ESSID_MAX_SIZE, m->desired_ssid,
>  		 m->desired_bssid, m->desired_bsstype);
>  exit:
>  	kfree(m);
> @@ -1160,13 +1130,13 @@ static void at76_dump_mib_mdomain(struct at76_priv *priv)
>  		goto exit;
>  	}
>  
> -	at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: channel_list %s",
> +	at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: channel_list %*phD",
>  		 wiphy_name(priv->hw->wiphy),
> -		 hex2str(m->channel_list, sizeof(m->channel_list)));
> +		 (int)sizeof(m->channel_list), m->channel_list);
>  
> -	at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: tx_powerlevel %s",
> +	at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: tx_powerlevel %*phD",
>  		 wiphy_name(priv->hw->wiphy),
> -		 hex2str(m->tx_powerlevel, sizeof(m->tx_powerlevel)));
> +		 (int)sizeof(m->tx_powerlevel), m->tx_powerlevel);
>  exit:
>  	kfree(m);
>  }
> @@ -1369,9 +1339,9 @@ static int at76_startup_device(struct at76_priv *priv)
>  	int ret;
>  
>  	at76_dbg(DBG_PARAMS,
> -		 "%s param: ssid %.*s (%s) mode %s ch %d wep %s key %d "
> +		 "%s param: ssid %.*s (%*phD) mode %s ch %d wep %s key %d "
>  		 "keylen %d", wiphy_name(priv->hw->wiphy), priv->essid_size,
> -		 priv->essid, hex2str(priv->essid, IW_ESSID_MAX_SIZE),
> +		 priv->essid, IW_ESSID_MAX_SIZE, priv->essid,
>  		 priv->iw_mode == IW_MODE_ADHOC ? "adhoc" : "infra",
>  		 priv->channel, priv->wep_enabled ? "enabled" : "disabled",
>  		 priv->wep_key_id, priv->wep_keys_len[priv->wep_key_id]);
> -- 
> 1.7.10.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [resend][PATCH] wireless: at76c50x: eliminate hex2str()

[Andy Shevchenko]

On Mon, 2012-09-10 at 14:34 -0400, John W. Linville wrote: 
> Could you point me at the commit that adds the 'phD' specification?
There it is:
http://www.spinics.net/lists/linux-wireless/msg93694.html

commit 31550a16a5d2af859e8a11839e8c6c6c9c92dfa7

> On Wed, Sep 05, 2012 at 11:52:32AM +0300, Andy Shevchenko wrote:
> > The hex2str() is substituted by '%*phD' specificator.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Tested-by: Larry Finger <Larry.Finger@lwfinger.net>
> > 
> > ---
> >  drivers/net/wireless/at76c50x-usb.c |   54 ++++++++---------------------------
> >  1 file changed, 12 insertions(+), 42 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/at76c50x-usb.c b/drivers/net/wireless/at76c50x-usb.c
> > index e361afe..99b9ddf 100644
> > --- a/drivers/net/wireless/at76c50x-usb.c
> > +++ b/drivers/net/wireless/at76c50x-usb.c
> > @@ -498,36 +498,6 @@ exit:
> >  	return ret;
> >  }
> >  
> > -#define HEX2STR_BUFFERS 4
> > -#define HEX2STR_MAX_LEN 64
> > -
> > -/* Convert binary data into hex string */
> > -static char *hex2str(void *buf, size_t len)
> > -{
> > -	static atomic_t a = ATOMIC_INIT(0);
> > -	static char bufs[HEX2STR_BUFFERS][3 * HEX2STR_MAX_LEN + 1];
> > -	char *ret = bufs[atomic_inc_return(&a) & (HEX2STR_BUFFERS - 1)];
> > -	char *obuf = ret;
> > -	u8 *ibuf = buf;
> > -
> > -	if (len > HEX2STR_MAX_LEN)
> > -		len = HEX2STR_MAX_LEN;
> > -
> > -	if (len == 0)
> > -		goto exit;
> > -
> > -	while (len--) {
> > -		obuf = hex_byte_pack(obuf, *ibuf++);
> > -		*obuf++ = '-';
> > -	}
> > -	obuf--;
> > -
> > -exit:
> > -	*obuf = '\0';
> > -
> > -	return ret;
> > -}
> > -
> >  /* LED trigger */
> >  static int tx_activity;
> >  static void at76_ledtrig_tx_timerfunc(unsigned long data);
> > @@ -1004,9 +974,9 @@ static void at76_dump_mib_mac_wep(struct at76_priv *priv)
> >  	    WEP_SMALL_KEY_LEN : WEP_LARGE_KEY_LEN;
> >  
> >  	for (i = 0; i < WEP_KEYS; i++)
> > -		at76_dbg(DBG_MIB, "%s: MIB MAC_WEP: key %d: %s",
> > +		at76_dbg(DBG_MIB, "%s: MIB MAC_WEP: key %d: %*phD",
> >  			 wiphy_name(priv->hw->wiphy), i,
> > -			 hex2str(m->wep_default_keyvalue[i], key_len));
> > +			 key_len, m->wep_default_keyvalue[i]);
> >  exit:
> >  	kfree(m);
> >  }
> > @@ -1031,7 +1001,7 @@ static void at76_dump_mib_mac_mgmt(struct at76_priv *priv)
> >  	at76_dbg(DBG_MIB, "%s: MIB MAC_MGMT: beacon_period %d CFP_max_duration "
> >  		 "%d medium_occupancy_limit %d station_id 0x%x ATIM_window %d "
> >  		 "CFP_mode %d privacy_opt_impl %d DTIM_period %d CFP_period %d "
> > -		 "current_bssid %pM current_essid %s current_bss_type %d "
> > +		 "current_bssid %pM current_essid %*phD current_bss_type %d "
> >  		 "pm_mode %d ibss_change %d res %d "
> >  		 "multi_domain_capability_implemented %d "
> >  		 "international_roaming %d country_string %.3s",
> > @@ -1041,7 +1011,7 @@ static void at76_dump_mib_mac_mgmt(struct at76_priv *priv)
> >  		 le16_to_cpu(m->station_id), le16_to_cpu(m->ATIM_window),
> >  		 m->CFP_mode, m->privacy_option_implemented, m->DTIM_period,
> >  		 m->CFP_period, m->current_bssid,
> > -		 hex2str(m->current_essid, IW_ESSID_MAX_SIZE),
> > +		 IW_ESSID_MAX_SIZE, m->current_essid,
> >  		 m->current_bss_type, m->power_mgmt_mode, m->ibss_change,
> >  		 m->res, m->multi_domain_capability_implemented,
> >  		 m->multi_domain_capability_enabled, m->country_string);
> > @@ -1069,7 +1039,7 @@ static void at76_dump_mib_mac(struct at76_priv *priv)
> >  		 "cwmin %d cwmax %d short_retry_time %d long_retry_time %d "
> >  		 "scan_type %d scan_channel %d probe_delay %u "
> >  		 "min_channel_time %d max_channel_time %d listen_int %d "
> > -		 "desired_ssid %s desired_bssid %pM desired_bsstype %d",
> > +		 "desired_ssid %*phD desired_bssid %pM desired_bsstype %d",
> >  		 wiphy_name(priv->hw->wiphy),
> >  		 le32_to_cpu(m->max_tx_msdu_lifetime),
> >  		 le32_to_cpu(m->max_rx_lifetime),
> > @@ -1080,7 +1050,7 @@ static void at76_dump_mib_mac(struct at76_priv *priv)
> >  		 le16_to_cpu(m->min_channel_time),
> >  		 le16_to_cpu(m->max_channel_time),
> >  		 le16_to_cpu(m->listen_interval),
> > -		 hex2str(m->desired_ssid, IW_ESSID_MAX_SIZE),
> > +		 IW_ESSID_MAX_SIZE, m->desired_ssid,
> >  		 m->desired_bssid, m->desired_bsstype);
> >  exit:
> >  	kfree(m);
> > @@ -1160,13 +1130,13 @@ static void at76_dump_mib_mdomain(struct at76_priv *priv)
> >  		goto exit;
> >  	}
> >  
> > -	at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: channel_list %s",
> > +	at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: channel_list %*phD",
> >  		 wiphy_name(priv->hw->wiphy),
> > -		 hex2str(m->channel_list, sizeof(m->channel_list)));
> > +		 (int)sizeof(m->channel_list), m->channel_list);
> >  
> > -	at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: tx_powerlevel %s",
> > +	at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: tx_powerlevel %*phD",
> >  		 wiphy_name(priv->hw->wiphy),
> > -		 hex2str(m->tx_powerlevel, sizeof(m->tx_powerlevel)));
> > +		 (int)sizeof(m->tx_powerlevel), m->tx_powerlevel);
> >  exit:
> >  	kfree(m);
> >  }
> > @@ -1369,9 +1339,9 @@ static int at76_startup_device(struct at76_priv *priv)
> >  	int ret;
> >  
> >  	at76_dbg(DBG_PARAMS,
> > -		 "%s param: ssid %.*s (%s) mode %s ch %d wep %s key %d "
> > +		 "%s param: ssid %.*s (%*phD) mode %s ch %d wep %s key %d "
> >  		 "keylen %d", wiphy_name(priv->hw->wiphy), priv->essid_size,
> > -		 priv->essid, hex2str(priv->essid, IW_ESSID_MAX_SIZE),
> > +		 priv->essid, IW_ESSID_MAX_SIZE, priv->essid,
> >  		 priv->iw_mode == IW_MODE_ADHOC ? "adhoc" : "infra",
> >  		 priv->channel, priv->wep_enabled ? "enabled" : "disabled",
> >  		 priv->wep_key_id, priv->wep_keys_len[priv->wep_key_id]);
> > -- 
> > 1.7.10.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCHv3 1/3] lib: printf: update documentation to cover all of %p[Mm][FR]

[andrei.emeltchenko.news]

On Wed, Jul 04, 2012 at 11:45:50AM +0300, Andy Shevchenko wrote:
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Acked-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com> 

> ---
>  Documentation/printk-formats.txt |    1 +
>  lib/vsprintf.c                   |    5 ++++-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
> index d8d168fa..90ff4d7 100644
> --- a/Documentation/printk-formats.txt
> +++ b/Documentation/printk-formats.txt
> @@ -59,6 +59,7 @@ MAC/FDDI addresses:
>  	%pMR	05:04:03:02:01:00
>  	%pMF	00-01-02-03-04-05
>  	%pm	000102030405
> +	%pmR	050403020100
>  
>  	For printing 6-byte MAC/FDDI addresses in hex notation. The 'M' and 'm'
>  	specifiers result in a printed address with ('M') or without ('m') byte
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index a0b5f15..e500158 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -946,7 +946,7 @@ int kptr_restrict __read_mostly;
>   * - 'm' For a 6-byte MAC address, it prints the hex address without colons
>   * - 'MF' For a 6-byte MAC FDDI address, it prints the address
>   *       with a dash-separated hex notation
> - * - '[mM]R For a 6-byte MAC address, Reverse order (Bluetooth)
> + * - '[mM]R' For a 6-byte MAC address, Reverse order (Bluetooth)
>   * - 'I' [46] for IPv4/IPv6 addresses printed in the usual way
>   *       IPv4 uses dot-separated decimal without leading 0's (1.2.3.4)
>   *       IPv6 uses colon separated network-order 16 bit hex with leading 0's
> @@ -1287,7 +1287,10 @@ qualifier:
>   * %pR output the address range in a struct resource with decoded flags
>   * %pr output the address range in a struct resource with raw flags
>   * %pM output a 6-byte MAC address with colons
> + * %pMR output a 6-byte MAC address with colons in reversed order
> + * %pMF output a 6-byte MAC address with dashes
>   * %pm output a 6-byte MAC address without colons
> + * %pmR output a 6-byte MAC address without colons in reversed order
>   * %pI4 print an IPv4 address without leading zeros
>   * %pi4 print an IPv4 address with leading zeros
>   * %pI6 print an IPv6 address with colons
> -- 
> 1.7.10
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC][PATCHv2 2/3] lib: printf: append support of '%*p[Mm][FR]'

[Andrei Emeltchenko]

Hi,

On Tue, Jul 03, 2012 at 11:48:00AM -0700, Joe Perches wrote:
> On Tue, 2012-07-03 at 21:32 +0300, Andy Shevchenko wrote:
> > On Tue, Jul 3, 2012 at 6:33 PM, Joe Perches <joe@perches.com> wrote:
> > > On Tue, 2012-07-03 at 13:06 +0300, Andy Shevchenko wrote:
> > >> There are many places in the kernel where the drivers print small buffers as a
> > >> hex string. This patch adds a support of the variable width buffer to print it
> > >> as a hex string with a delimiter. The idea came from Pavel Roskin here:
> > >> http://www.digipedia.pl/usenet/thread/18835/17449/
> > >>
> > >> Sample output of
> > >>       pr_info("buf[%d:%d] %*pM\n", from, len, len, &buf[from]);
> > >> could be look like this:
> > >>       [ 0.726130] buf[51:8] e8:16:b6:ef:e3:74:45:6e
> > >>       [ 0.750736] buf[59:15] 31:81:b8:3f:35:49:06:ae:df:32:06:05:4a:af:55
> > >>       [ 0.757602] buf[17:5] ac:16:d5:2c:ef

The idea is really good and can make redundant lots of print_hex
functions.

> > 
> > > It might be more sensible to use new, distinct
> > > "%*pH" and "%*ph" functions and not touch the
> > > mac address function at all.  Will anyone ever
> > > really want to emit the buffer in reverse?
> > > I don't think so.
> > Yeah, probably it's only the case for the Bluetooth addresses.
> > 
> > > Perhaps when using a hex_string_buffer func the
> > > separator should be a space/no-space with %*pHh.
> > What I learned from today's linux-next is the most used separators are
> > ' ' (space), '' (nothing), ':' and '-'. We have dozens of the cases
> > for first three. The '-' support could not be implemented
> > nevertheless.
> > So, might be %*pHh[CDS] C for 'colon', S for 'space', D for 'dash' looks better.
> 
> Maybe use a space default.

I do not think we need other specifier then space at all.

> 
> > 'Hh' for capital/small letters than?
> 
> If you want, though I'd hope nobody uses upper case.
> 
> > > You could extend the max to 128 or larger now.
> > I don't think it is really needed.
> 
> I hope it's not, but I just don't see the need to limit it.
> 
> > Most of the current cases usually
> > print not more than ~30bytes (in average) per time. And I couldn't
> > imagine good looking printing for long lines anyway.

Maybe insert '\n' after 16 numbers?

> 
> Yup, they'd be ugly.
> print_hex_dump() should be favored anyway.

For print_hex_dump we would need to invent unreadable constructions with
defines like you mentioned in other thread:

#ifdef SOME_BLUETOOTH_DEBUG_FLAG
#define bt_hex_dump_dbg(...)    \
        print_hex_dump(...)
#else
#define bt_hex_dump_dbg(...)    \
        do { } while (0)
#endif

Best regards 
Andrei Emeltchenko 

---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

[RFC][PATCHv2 3/3] wireless: at76c50x: eliminate hex2str()

[Andy Shevchenko]

The hex2str() is substituted by '%*pMF' specificator.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/net/wireless/at76c50x-usb.c |   54 ++++++++---------------------------
 1 file changed, 12 insertions(+), 42 deletions(-)

diff --git a/drivers/net/wireless/at76c50x-usb.c b/drivers/net/wireless/at76c50x-usb.c
index efc162e..f82f76b 100644
--- a/drivers/net/wireless/at76c50x-usb.c
+++ b/drivers/net/wireless/at76c50x-usb.c
@@ -498,36 +498,6 @@ exit:
 	return ret;
 }
 
-#define HEX2STR_BUFFERS 4
-#define HEX2STR_MAX_LEN 64
-
-/* Convert binary data into hex string */
-static char *hex2str(void *buf, size_t len)
-{
-	static atomic_t a = ATOMIC_INIT(0);
-	static char bufs[HEX2STR_BUFFERS][3 * HEX2STR_MAX_LEN + 1];
-	char *ret = bufs[atomic_inc_return(&a) & (HEX2STR_BUFFERS - 1)];
-	char *obuf = ret;
-	u8 *ibuf = buf;
-
-	if (len > HEX2STR_MAX_LEN)
-		len = HEX2STR_MAX_LEN;
-
-	if (len == 0)
-		goto exit;
-
-	while (len--) {
-		obuf = hex_byte_pack(obuf, *ibuf++);
-		*obuf++ = '-';
-	}
-	obuf--;
-
-exit:
-	*obuf = '\0';
-
-	return ret;
-}
-
 /* LED trigger */
 static int tx_activity;
 static void at76_ledtrig_tx_timerfunc(unsigned long data);
@@ -1004,9 +974,9 @@ static void at76_dump_mib_mac_wep(struct at76_priv *priv)
 	    WEP_SMALL_KEY_LEN : WEP_LARGE_KEY_LEN;
 
 	for (i = 0; i < WEP_KEYS; i++)
-		at76_dbg(DBG_MIB, "%s: MIB MAC_WEP: key %d: %s",
+		at76_dbg(DBG_MIB, "%s: MIB MAC_WEP: key %d: %*pMF",
 			 wiphy_name(priv->hw->wiphy), i,
-			 hex2str(m->wep_default_keyvalue[i], key_len));
+			 key_len, m->wep_default_keyvalue[i]);
 exit:
 	kfree(m);
 }
@@ -1031,7 +1001,7 @@ static void at76_dump_mib_mac_mgmt(struct at76_priv *priv)
 	at76_dbg(DBG_MIB, "%s: MIB MAC_MGMT: beacon_period %d CFP_max_duration "
 		 "%d medium_occupancy_limit %d station_id 0x%x ATIM_window %d "
 		 "CFP_mode %d privacy_opt_impl %d DTIM_period %d CFP_period %d "
-		 "current_bssid %pM current_essid %s current_bss_type %d "
+		 "current_bssid %pM current_essid %*pMF current_bss_type %d "
 		 "pm_mode %d ibss_change %d res %d "
 		 "multi_domain_capability_implemented %d "
 		 "international_roaming %d country_string %.3s",
@@ -1041,7 +1011,7 @@ static void at76_dump_mib_mac_mgmt(struct at76_priv *priv)
 		 le16_to_cpu(m->station_id), le16_to_cpu(m->ATIM_window),
 		 m->CFP_mode, m->privacy_option_implemented, m->DTIM_period,
 		 m->CFP_period, m->current_bssid,
-		 hex2str(m->current_essid, IW_ESSID_MAX_SIZE),
+		 IW_ESSID_MAX_SIZE, m->current_essid,
 		 m->current_bss_type, m->power_mgmt_mode, m->ibss_change,
 		 m->res, m->multi_domain_capability_implemented,
 		 m->multi_domain_capability_enabled, m->country_string);
@@ -1069,7 +1039,7 @@ static void at76_dump_mib_mac(struct at76_priv *priv)
 		 "cwmin %d cwmax %d short_retry_time %d long_retry_time %d "
 		 "scan_type %d scan_channel %d probe_delay %u "
 		 "min_channel_time %d max_channel_time %d listen_int %d "
-		 "desired_ssid %s desired_bssid %pM desired_bsstype %d",
+		 "desired_ssid %*pMF desired_bssid %pM desired_bsstype %d",
 		 wiphy_name(priv->hw->wiphy),
 		 le32_to_cpu(m->max_tx_msdu_lifetime),
 		 le32_to_cpu(m->max_rx_lifetime),
@@ -1080,7 +1050,7 @@ static void at76_dump_mib_mac(struct at76_priv *priv)
 		 le16_to_cpu(m->min_channel_time),
 		 le16_to_cpu(m->max_channel_time),
 		 le16_to_cpu(m->listen_interval),
-		 hex2str(m->desired_ssid, IW_ESSID_MAX_SIZE),
+		 IW_ESSID_MAX_SIZE, m->desired_ssid,
 		 m->desired_bssid, m->desired_bsstype);
 exit:
 	kfree(m);
@@ -1160,13 +1130,13 @@ static void at76_dump_mib_mdomain(struct at76_priv *priv)
 		goto exit;
 	}
 
-	at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: channel_list %s",
+	at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: channel_list %*pMF",
 		 wiphy_name(priv->hw->wiphy),
-		 hex2str(m->channel_list, sizeof(m->channel_list)));
+		 sizeof(m->channel_list), m->channel_list);
 
-	at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: tx_powerlevel %s",
+	at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: tx_powerlevel %*pMF",
 		 wiphy_name(priv->hw->wiphy),
-		 hex2str(m->tx_powerlevel, sizeof(m->tx_powerlevel)));
+		 sizeof(m->tx_powerlevel), m->tx_powerlevel);
 exit:
 	kfree(m);
 }
@@ -1369,9 +1339,9 @@ static int at76_startup_device(struct at76_priv *priv)
 	int ret;
 
 	at76_dbg(DBG_PARAMS,
-		 "%s param: ssid %.*s (%s) mode %s ch %d wep %s key %d "
+		 "%s param: ssid %.*s (%*pMF) mode %s ch %d wep %s key %d "
 		 "keylen %d", wiphy_name(priv->hw->wiphy), priv->essid_size,
-		 priv->essid, hex2str(priv->essid, IW_ESSID_MAX_SIZE),
+		 priv->essid, IW_ESSID_MAX_SIZE, priv->essid,
 		 priv->iw_mode == IW_MODE_ADHOC ? "adhoc" : "infra",
 		 priv->channel, priv->wep_enabled ? "enabled" : "disabled",
 		 priv->wep_key_id, priv->wep_keys_len[priv->wep_key_id]);
-- 
1.7.10

Re: [RFC][PATCHv2 3/3] wireless: at76c50x: eliminate hex2str()

[Larry Finger]

On 07/03/2012 05:06 AM, Andy Shevchenko wrote:
> The hex2str() is substituted by '%*pMF' specificator.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   drivers/net/wireless/at76c50x-usb.c |   54 ++++++++---------------------------
>   1 file changed, 12 insertions(+), 42 deletions(-)

Tested-by: Larry Finger <Larry.Finger@lwfinger.net>

>
> diff --git a/drivers/net/wireless/at76c50x-usb.c b/drivers/net/wireless/at76c50x-usb.c
> index efc162e..f82f76b 100644
> --- a/drivers/net/wireless/at76c50x-usb.c
> +++ b/drivers/net/wireless/at76c50x-usb.c
> @@ -498,36 +498,6 @@ exit:
>   	return ret;
>   }
>
> -#define HEX2STR_BUFFERS 4
> -#define HEX2STR_MAX_LEN 64
> -
> -/* Convert binary data into hex string */
> -static char *hex2str(void *buf, size_t len)
> -{
> -	static atomic_t a = ATOMIC_INIT(0);
> -	static char bufs[HEX2STR_BUFFERS][3 * HEX2STR_MAX_LEN + 1];
> -	char *ret = bufs[atomic_inc_return(&a) & (HEX2STR_BUFFERS - 1)];
> -	char *obuf = ret;
> -	u8 *ibuf = buf;
> -
> -	if (len > HEX2STR_MAX_LEN)
> -		len = HEX2STR_MAX_LEN;
> -
> -	if (len == 0)
> -		goto exit;
> -
> -	while (len--) {
> -		obuf = hex_byte_pack(obuf, *ibuf++);
> -		*obuf++ = '-';
> -	}
> -	obuf--;
> -
> -exit:
> -	*obuf = '\0';
> -
> -	return ret;
> -}
> -
>   /* LED trigger */
>   static int tx_activity;
>   static void at76_ledtrig_tx_timerfunc(unsigned long data);
> @@ -1004,9 +974,9 @@ static void at76_dump_mib_mac_wep(struct at76_priv *priv)
>   	    WEP_SMALL_KEY_LEN : WEP_LARGE_KEY_LEN;
>
>   	for (i = 0; i < WEP_KEYS; i++)
> -		at76_dbg(DBG_MIB, "%s: MIB MAC_WEP: key %d: %s",
> +		at76_dbg(DBG_MIB, "%s: MIB MAC_WEP: key %d: %*pMF",
>   			 wiphy_name(priv->hw->wiphy), i,
> -			 hex2str(m->wep_default_keyvalue[i], key_len));
> +			 key_len, m->wep_default_keyvalue[i]);
>   exit:
>   	kfree(m);
>   }
> @@ -1031,7 +1001,7 @@ static void at76_dump_mib_mac_mgmt(struct at76_priv *priv)
>   	at76_dbg(DBG_MIB, "%s: MIB MAC_MGMT: beacon_period %d CFP_max_duration "
>   		 "%d medium_occupancy_limit %d station_id 0x%x ATIM_window %d "
>   		 "CFP_mode %d privacy_opt_impl %d DTIM_period %d CFP_period %d "
> -		 "current_bssid %pM current_essid %s current_bss_type %d "
> +		 "current_bssid %pM current_essid %*pMF current_bss_type %d "
>   		 "pm_mode %d ibss_change %d res %d "
>   		 "multi_domain_capability_implemented %d "
>   		 "international_roaming %d country_string %.3s",
> @@ -1041,7 +1011,7 @@ static void at76_dump_mib_mac_mgmt(struct at76_priv *priv)
>   		 le16_to_cpu(m->station_id), le16_to_cpu(m->ATIM_window),
>   		 m->CFP_mode, m->privacy_option_implemented, m->DTIM_period,
>   		 m->CFP_period, m->current_bssid,
> -		 hex2str(m->current_essid, IW_ESSID_MAX_SIZE),
> +		 IW_ESSID_MAX_SIZE, m->current_essid,
>   		 m->current_bss_type, m->power_mgmt_mode, m->ibss_change,
>   		 m->res, m->multi_domain_capability_implemented,
>   		 m->multi_domain_capability_enabled, m->country_string);
> @@ -1069,7 +1039,7 @@ static void at76_dump_mib_mac(struct at76_priv *priv)
>   		 "cwmin %d cwmax %d short_retry_time %d long_retry_time %d "
>   		 "scan_type %d scan_channel %d probe_delay %u "
>   		 "min_channel_time %d max_channel_time %d listen_int %d "
> -		 "desired_ssid %s desired_bssid %pM desired_bsstype %d",
> +		 "desired_ssid %*pMF desired_bssid %pM desired_bsstype %d",
>   		 wiphy_name(priv->hw->wiphy),
>   		 le32_to_cpu(m->max_tx_msdu_lifetime),
>   		 le32_to_cpu(m->max_rx_lifetime),
> @@ -1080,7 +1050,7 @@ static void at76_dump_mib_mac(struct at76_priv *priv)
>   		 le16_to_cpu(m->min_channel_time),
>   		 le16_to_cpu(m->max_channel_time),
>   		 le16_to_cpu(m->listen_interval),
> -		 hex2str(m->desired_ssid, IW_ESSID_MAX_SIZE),
> +		 IW_ESSID_MAX_SIZE, m->desired_ssid,
>   		 m->desired_bssid, m->desired_bsstype);
>   exit:
>   	kfree(m);
> @@ -1160,13 +1130,13 @@ static void at76_dump_mib_mdomain(struct at76_priv *priv)
>   		goto exit;
>   	}
>
> -	at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: channel_list %s",
> +	at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: channel_list %*pMF",
>   		 wiphy_name(priv->hw->wiphy),
> -		 hex2str(m->channel_list, sizeof(m->channel_list)));
> +		 sizeof(m->channel_list), m->channel_list);
>
> -	at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: tx_powerlevel %s",
> +	at76_dbg(DBG_MIB, "%s: MIB MDOMAIN: tx_powerlevel %*pMF",
>   		 wiphy_name(priv->hw->wiphy),
> -		 hex2str(m->tx_powerlevel, sizeof(m->tx_powerlevel)));
> +		 sizeof(m->tx_powerlevel), m->tx_powerlevel);
>   exit:
>   	kfree(m);
>   }
> @@ -1369,9 +1339,9 @@ static int at76_startup_device(struct at76_priv *priv)
>   	int ret;
>
>   	at76_dbg(DBG_PARAMS,
> -		 "%s param: ssid %.*s (%s) mode %s ch %d wep %s key %d "
> +		 "%s param: ssid %.*s (%*pMF) mode %s ch %d wep %s key %d "
>   		 "keylen %d", wiphy_name(priv->hw->wiphy), priv->essid_size,
> -		 priv->essid, hex2str(priv->essid, IW_ESSID_MAX_SIZE),
> +		 priv->essid, IW_ESSID_MAX_SIZE, priv->essid,
>   		 priv->iw_mode == IW_MODE_ADHOC ? "adhoc" : "infra",
>   		 priv->channel, priv->wep_enabled ? "enabled" : "disabled",
>   		 priv->wep_key_id, priv->wep_keys_len[priv->wep_key_id]);
>

Re: [RFC][PATCHv2 3/3] wireless: at76c50x: eliminate hex2str()

[Andy Shevchenko]

On Tue, Jul 3, 2012 at 4:18 PM, Larry Finger <Larry.Finger@lwfinger.net> wrote:
> On 07/03/2012 05:06 AM, Andy Shevchenko wrote:
>>
>> The hex2str() is substituted by '%*pMF' specificator.
>>
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> ---
>>   drivers/net/wireless/at76c50x-usb.c |   54
>> ++++++++---------------------------
>>   1 file changed, 12 insertions(+), 42 deletions(-)
>
>
> Tested-by: Larry Finger <Larry.Finger@lwfinger.net>
Thanks for testing. Today I found in my mailbox (not gmail) your
previous message about compilation warnings. Are they still present?
(I guess so) Probably I need to update the patch (anyway it probably
has to be updated because we discussed with Joe about different letter
for the hex strings).


-- 
With Best Regards,
Andy Shevchenko