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

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

[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

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

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

[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

[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

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.5] 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                   |   58 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 68 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..85707df 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -655,6 +655,53 @@ 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;
+	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;
+
+	if (ZERO_OR_NULL_PTR(addr))
+		/* NULL pointer */
+		return string(buf, end, NULL, spec);
+
+	if (spec.field_width > 0)
+		len = min_t(int, spec.field_width, 64);
+	else
+		/* if we pass '%ph[CDN]', field witdh remains negative value,
+		 * fallback to 1 */
+		len = 1;
+
+	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 +1021,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 +1061,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 +1354,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.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

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.

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