[PATCH v2 00/11] lib/vsprintf: refactor and introduce %pl specifier

[PATCH v2 00/11] lib/vsprintf: refactor and introduce %pl specifier

[Andy Shevchenko]

This series refactors vsprintf.c, introduces %pl specifier to print unsigned
long long value in human-readable format, enhances EFI messages, and converts
existing users of such functionality.

The series has been tested on 32-bit and 64-bit Intel platforms with
test_printf.c suite.

In the future someone can extend %pl to cover the cases like string_get_size()
does.

Andy Shevchenko (10):
  lib/vsprintf: introduce put_one_char() for 3 line idiom
  lib/vsprintf: make default_dec_spec global
  lib/vsprintf: make default_str_spec global
  lib/string_helpers: export string_units_{2,10} for others
  lib/string_helpers: fix indentation in few places
  lib/vsprintf: introduce %pl to print in human-readable form
  lib/vsprintf: allow range of prefix for %pl[From[To]]
  lib/vsprintf: use precision field with %pl[From[To]]
  cxgb4: print value in human-readable form via %.0plKM
  pcmciamtd: print value in human-readable form via %.0plKM

Robert Elliott (1):
  x86/efi: print size and base in binary units in efi_print_memmap

 Documentation/printk-formats.txt                   |  22 ++
 arch/x86/platform/efi/efi.c                        |  11 +-
 drivers/mtd/maps/pcmciamtd.c                       |  12 +-
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c |  11 +-
 include/linux/string_helpers.h                     |   6 +
 lib/string_helpers.c                               |  26 +-
 lib/test_printf.c                                  |  84 +++++-
 lib/vsprintf.c                                     | 296 ++++++++++-----------
 8 files changed, 277 insertions(+), 191 deletions(-)

-- 
2.6.4

[PATCH v2 01/11] lib/vsprintf: introduce put_one_char() for 3 line idiom

[Andy Shevchenko]

There is an idiom used widely in the code

	if (buf < end)
		*buf = c;
	++buf;

Introduce put_one_char() helper as implementation of this idiom.

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

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 75eb342..fe6ffe3 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -325,6 +325,14 @@ char *put_dec(char *buf, unsigned long long n)
 
 #endif
 
+static
+char *put_one_char(char *buf, char *end, char c)
+{
+	if (buf < end)
+		*buf = c;
+	return ++buf;
+}
+
 /*
  * Convert passed number to decimal string.
  * Returns the length of string.  On buffer overflow, returns 0.
@@ -458,59 +466,35 @@ char *number(char *buf, char *end, unsigned long long num,
 	/* leading space padding */
 	field_width -= precision;
 	if (!(spec.flags & (ZEROPAD | LEFT))) {
-		while (--field_width >= 0) {
-			if (buf < end)
-				*buf = ' ';
-			++buf;
-		}
+		while (--field_width >= 0)
+			buf = put_one_char(buf, end, ' ');
 	}
 	/* sign */
-	if (sign) {
-		if (buf < end)
-			*buf = sign;
-		++buf;
-	}
+	if (sign)
+		buf = put_one_char(buf, end, sign);
 	/* "0x" / "0" prefix */
 	if (need_pfx) {
-		if (spec.base == 16 || !is_zero) {
-			if (buf < end)
-				*buf = '0';
-			++buf;
-		}
-		if (spec.base == 16) {
-			if (buf < end)
-				*buf = ('X' | locase);
-			++buf;
-		}
+		if (spec.base == 16 || !is_zero)
+			buf = put_one_char(buf, end, '0');
+		if (spec.base == 16)
+			buf = put_one_char(buf, end, 'X' | locase);
 	}
 	/* zero or space padding */
 	if (!(spec.flags & LEFT)) {
 		char c = ' ' + (spec.flags & ZEROPAD);
 		BUILD_BUG_ON(' ' + ZEROPAD != '0');
-		while (--field_width >= 0) {
-			if (buf < end)
-				*buf = c;
-			++buf;
-		}
+		while (--field_width >= 0)
+			buf = put_one_char(buf, end, c);
 	}
 	/* hmm even more zero padding? */
-	while (i <= --precision) {
-		if (buf < end)
-			*buf = '0';
-		++buf;
-	}
+	while (i <= --precision)
+		buf = put_one_char(buf, end, '0');
 	/* actual digits of result */
-	while (--i >= 0) {
-		if (buf < end)
-			*buf = tmp[i];
-		++buf;
-	}
+	while (--i >= 0)
+		buf = put_one_char(buf, end, tmp[i]);
 	/* trailing space padding */
-	while (--field_width >= 0) {
-		if (buf < end)
-			*buf = ' ';
-		++buf;
-	}
+	while (--field_width >= 0)
+		buf = put_one_char(buf, end, ' ');
 
 	return buf;
 }
@@ -571,11 +555,8 @@ char *widen_string(char *buf, int n, char *end, struct printf_spec spec)
 		move_right(buf - n, end, n, spaces);
 		return buf + spaces;
 	}
-	while (spaces--) {
-		if (buf < end)
-			*buf = ' ';
-		++buf;
-	}
+	while (spaces--)
+		buf = put_one_char(buf, end, ' ');
 	return buf;
 }
 
@@ -592,9 +573,7 @@ char *string(char *buf, char *end, const char *s, struct printf_spec spec)
 		char c = *s++;
 		if (!c)
 			break;
-		if (buf < end)
-			*buf = c;
-		++buf;
+		buf = put_one_char(buf, end, c);
 		++len;
 	}
 	return widen_string(buf, len, end, spec);
@@ -629,7 +608,7 @@ char *dentry_name(char *buf, char *end, const struct dentry *d, struct printf_sp
 		}
 	}
 	s = array[--i];
-	for (n = 0; n != spec.precision; n++, buf++) {
+	for (n = 0; n != spec.precision; n++) {
 		char c = *s++;
 		if (!c) {
 			if (!i)
@@ -637,8 +616,7 @@ char *dentry_name(char *buf, char *end, const struct dentry *d, struct printf_sp
 			c = '/';
 			s = array[--i];
 		}
-		if (buf < end)
-			*buf = c;
+		buf = put_one_char(buf, end, c);
 	}
 	rcu_read_unlock();
 	return widen_string(buf, n, end, spec);
@@ -653,11 +631,8 @@ char *bdev_name(char *buf, char *end, struct block_device *bdev,
 	
 	buf = string(buf, end, hd->disk_name, spec);
 	if (bdev->bd_part->partno) {
-		if (isdigit(hd->disk_name[strlen(hd->disk_name)-1])) {
-			if (buf < end)
-				*buf = 'p';
-			buf++;
-		}
+		if (isdigit(hd->disk_name[strlen(hd->disk_name) - 1]))
+			buf = put_one_char(buf, end, 'p');
 		buf = number(buf, end, bdev->bd_part->partno, spec);
 	}
 	return buf;
@@ -834,18 +809,11 @@ char *hex_string(char *buf, char *end, u8 *addr, struct printf_spec spec,
 		len = min_t(int, spec.field_width, 64);
 
 	for (i = 0; i < len; ++i) {
-		if (buf < end)
-			*buf = hex_asc_hi(addr[i]);
-		++buf;
-		if (buf < end)
-			*buf = hex_asc_lo(addr[i]);
-		++buf;
-
-		if (separator && i != len - 1) {
-			if (buf < end)
-				*buf = separator;
-			++buf;
-		}
+		buf = put_one_char(buf, end, hex_asc_hi(addr[i]));
+		buf = put_one_char(buf, end, hex_asc_lo(addr[i]));
+
+		if (separator && i != len - 1)
+			buf = put_one_char(buf, end, separator);
 	}
 
 	return buf;
@@ -877,11 +845,8 @@ char *bitmap_string(char *buf, char *end, unsigned long *bitmap,
 		bit = i % BITS_PER_LONG;
 		val = (bitmap[word] >> bit) & chunkmask;
 
-		if (!first) {
-			if (buf < end)
-				*buf = ',';
-			buf++;
-		}
+		if (!first)
+			buf = put_one_char(buf, end, ',');
 		first = false;
 
 		spec.field_width = DIV_ROUND_UP(chunksz, 4);
@@ -911,19 +876,13 @@ char *bitmap_list_string(char *buf, char *end, unsigned long *bitmap,
 		if (cur < nr_bits && cur <= rtop + 1)
 			continue;
 
-		if (!first) {
-			if (buf < end)
-				*buf = ',';
-			buf++;
-		}
+		if (!first)
+			buf = put_one_char(buf, end, ',');
 		first = false;
 
 		buf = number(buf, end, rbot, spec);
 		if (rbot < rtop) {
-			if (buf < end)
-				*buf = '-';
-			buf++;
-
+			buf = put_one_char(buf, end, '-');
 			buf = number(buf, end, rtop, spec);
 		}
 
@@ -1950,28 +1909,15 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 			set_precision(&spec, va_arg(args, int));
 			break;
 
-		case FORMAT_TYPE_CHAR: {
-			char c;
-
+		case FORMAT_TYPE_CHAR:
 			if (!(spec.flags & LEFT)) {
-				while (--spec.field_width > 0) {
-					if (str < end)
-						*str = ' ';
-					++str;
-
-				}
-			}
-			c = (unsigned char) va_arg(args, int);
-			if (str < end)
-				*str = c;
-			++str;
-			while (--spec.field_width > 0) {
-				if (str < end)
-					*str = ' ';
-				++str;
+				while (--spec.field_width > 0)
+					str = put_one_char(str, end, ' ');
 			}
+			str = put_one_char(str, end, (unsigned char)va_arg(args, int));
+			while (--spec.field_width > 0)
+				str = put_one_char(str, end, ' ');
 			break;
-		}
 
 		case FORMAT_TYPE_STR:
 			str = string(str, end, va_arg(args, char *), spec);
@@ -1985,9 +1931,7 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 			break;
 
 		case FORMAT_TYPE_PERCENT_CHAR:
-			if (str < end)
-				*str = '%';
-			++str;
+			str = put_one_char(str, end, '%');
 			break;
 
 		case FORMAT_TYPE_INVALID:
@@ -2394,27 +2338,15 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf)
 			set_precision(&spec, get_arg(int));
 			break;
 
-		case FORMAT_TYPE_CHAR: {
-			char c;
-
+		case FORMAT_TYPE_CHAR:
 			if (!(spec.flags & LEFT)) {
-				while (--spec.field_width > 0) {
-					if (str < end)
-						*str = ' ';
-					++str;
-				}
-			}
-			c = (unsigned char) get_arg(char);
-			if (str < end)
-				*str = c;
-			++str;
-			while (--spec.field_width > 0) {
-				if (str < end)
-					*str = ' ';
-				++str;
+				while (--spec.field_width > 0)
+					str = put_one_char(str, end, ' ');
 			}
+			str = put_one_char(str, end, (unsigned char)get_arg(char));
+			while (--spec.field_width > 0)
+				str = put_one_char(str, end, ' ');
 			break;
-		}
 
 		case FORMAT_TYPE_STR: {
 			const char *str_arg = args;
@@ -2430,9 +2362,7 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf)
 			break;
 
 		case FORMAT_TYPE_PERCENT_CHAR:
-			if (str < end)
-				*str = '%';
-			++str;
+			str = put_one_char(str, end, '%');
 			break;
 
 		case FORMAT_TYPE_INVALID:
-- 
2.6.4

[PATCH v2 02/11] lib/vsprintf: make default_dec_spec global

[Andy Shevchenko]

There are places where default specification to print decimal numbers is used.
Make it global and convert existing users.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/test_printf.c |  5 +++--
 lib/vsprintf.c    | 21 +++++++++------------
 2 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 4f6ae60..fd985f6 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -376,9 +376,10 @@ large_bitmap(void)
 	if (!bits)
 		return;
 
-	bitmap_set(bits, 1, 20);
+	bitmap_set(bits, 0, 1);
+	bitmap_set(bits, 2, 20);
 	bitmap_set(bits, 60000, 15);
-	test("1-20,60000-60014", "%*pbl", nbits, bits);
+	test("0,2-21,60000-60014", "%*pbl", nbits, bits);
 	kfree(bits);
 }
 
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index fe6ffe3..53a6e7c 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -666,6 +666,11 @@ char *symbol_string(char *buf, char *end, void *ptr,
 #endif
 }
 
+static const struct printf_spec default_dec_spec = {
+	.base = 10,
+	.precision = -1,
+};
+
 static noinline_for_stack
 char *resource_string(char *buf, char *end, struct resource *res,
 		      struct printf_spec spec, const char *fmt)
@@ -695,11 +700,6 @@ char *resource_string(char *buf, char *end, struct resource *res,
 		.precision = -1,
 		.flags = SMALL | ZEROPAD,
 	};
-	static const struct printf_spec dec_spec = {
-		.base = 10,
-		.precision = -1,
-		.flags = 0,
-	};
 	static const struct printf_spec str_spec = {
 		.field_width = -1,
 		.precision = 10,
@@ -733,10 +733,10 @@ char *resource_string(char *buf, char *end, struct resource *res,
 		specp = &mem_spec;
 	} else if (res->flags & IORESOURCE_IRQ) {
 		p = string(p, pend, "irq ", str_spec);
-		specp = &dec_spec;
+		specp = &default_dec_spec;
 	} else if (res->flags & IORESOURCE_DMA) {
 		p = string(p, pend, "dma ", str_spec);
-		specp = &dec_spec;
+		specp = &default_dec_spec;
 	} else if (res->flags & IORESOURCE_BUS) {
 		p = string(p, pend, "bus ", str_spec);
 		specp = &bus_spec;
@@ -866,9 +866,6 @@ char *bitmap_list_string(char *buf, char *end, unsigned long *bitmap,
 	int cur, rbot, rtop;
 	bool first = true;
 
-	/* reused to print numbers */
-	spec = (struct printf_spec){ .base = 10 };
-
 	rbot = cur = find_first_bit(bitmap, nr_bits);
 	while (cur < nr_bits) {
 		rtop = cur;
@@ -880,10 +877,10 @@ char *bitmap_list_string(char *buf, char *end, unsigned long *bitmap,
 			buf = put_one_char(buf, end, ',');
 		first = false;
 
-		buf = number(buf, end, rbot, spec);
+		buf = number(buf, end, rbot, default_dec_spec);
 		if (rbot < rtop) {
 			buf = put_one_char(buf, end, '-');
-			buf = number(buf, end, rtop, spec);
+			buf = number(buf, end, rtop, default_dec_spec);
 		}
 
 		rbot = cur;
-- 
2.6.4

[PATCH v2 03/11] lib/vsprintf: make default_str_spec global

[Andy Shevchenko]

There will be place where default specification to print string is used.
Make it global and convert existing user.

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

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 53a6e7c..f0789e3 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -671,6 +671,12 @@ static const struct printf_spec default_dec_spec = {
 	.precision = -1,
 };
 
+static const struct printf_spec default_str_spec = {
+	.field_width = -1,
+	.precision = 10,
+	.flags = LEFT,
+};
+
 static noinline_for_stack
 char *resource_string(char *buf, char *end, struct resource *res,
 		      struct printf_spec spec, const char *fmt)
@@ -700,11 +706,6 @@ char *resource_string(char *buf, char *end, struct resource *res,
 		.precision = -1,
 		.flags = SMALL | ZEROPAD,
 	};
-	static const struct printf_spec str_spec = {
-		.field_width = -1,
-		.precision = 10,
-		.flags = LEFT,
-	};
 	static const struct printf_spec flag_spec = {
 		.base = 16,
 		.precision = -1,
@@ -726,27 +727,27 @@ char *resource_string(char *buf, char *end, struct resource *res,
 
 	*p++ = '[';
 	if (res->flags & IORESOURCE_IO) {
-		p = string(p, pend, "io  ", str_spec);
+		p = string(p, pend, "io  ", default_str_spec);
 		specp = &io_spec;
 	} else if (res->flags & IORESOURCE_MEM) {
-		p = string(p, pend, "mem ", str_spec);
+		p = string(p, pend, "mem ", default_str_spec);
 		specp = &mem_spec;
 	} else if (res->flags & IORESOURCE_IRQ) {
-		p = string(p, pend, "irq ", str_spec);
+		p = string(p, pend, "irq ", default_str_spec);
 		specp = &default_dec_spec;
 	} else if (res->flags & IORESOURCE_DMA) {
-		p = string(p, pend, "dma ", str_spec);
+		p = string(p, pend, "dma ", default_str_spec);
 		specp = &default_dec_spec;
 	} else if (res->flags & IORESOURCE_BUS) {
-		p = string(p, pend, "bus ", str_spec);
+		p = string(p, pend, "bus ", default_str_spec);
 		specp = &bus_spec;
 	} else {
-		p = string(p, pend, "??? ", str_spec);
+		p = string(p, pend, "??? ", default_str_spec);
 		specp = &mem_spec;
 		decode = 0;
 	}
 	if (decode && res->flags & IORESOURCE_UNSET) {
-		p = string(p, pend, "size ", str_spec);
+		p = string(p, pend, "size ", default_str_spec);
 		p = number(p, pend, resource_size(res), *specp);
 	} else {
 		p = number(p, pend, res->start, *specp);
@@ -757,15 +758,15 @@ char *resource_string(char *buf, char *end, struct resource *res,
 	}
 	if (decode) {
 		if (res->flags & IORESOURCE_MEM_64)
-			p = string(p, pend, " 64bit", str_spec);
+			p = string(p, pend, " 64bit", default_str_spec);
 		if (res->flags & IORESOURCE_PREFETCH)
-			p = string(p, pend, " pref", str_spec);
+			p = string(p, pend, " pref", default_str_spec);
 		if (res->flags & IORESOURCE_WINDOW)
-			p = string(p, pend, " window", str_spec);
+			p = string(p, pend, " window", default_str_spec);
 		if (res->flags & IORESOURCE_DISABLED)
-			p = string(p, pend, " disabled", str_spec);
+			p = string(p, pend, " disabled", default_str_spec);
 	} else {
-		p = string(p, pend, " flags ", str_spec);
+		p = string(p, pend, " flags ", default_str_spec);
 		p = number(p, pend, res->flags, flag_spec);
 	}
 	*p++ = ']';
-- 
2.6.4

[PATCH v2 04/11] lib/string_helpers: export string_units_{2,10} for others

[Andy Shevchenko]

There is one user coming which would like to use those string arrays. It might
be useful for any other user in the future.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/string_helpers.h |  6 ++++++
 lib/string_helpers.c           | 21 ++++++++++++---------
 2 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
index dabe643..a55c9cc 100644
--- a/include/linux/string_helpers.h
+++ b/include/linux/string_helpers.h
@@ -10,6 +10,12 @@ enum string_size_units {
 	STRING_UNITS_2,		/* use binary powers of 2^10 */
 };
 
+#define STRING_UNITS_10_NUM	9
+#define STRING_UNITS_2_NUM	9
+
+extern const char *const string_units_10[STRING_UNITS_10_NUM];
+extern const char *const string_units_2[STRING_UNITS_2_NUM];
+
 void string_get_size(u64 size, u64 blk_size, enum string_size_units units,
 		     char *buf, int len);
 
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 5c88204..c62acc0 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -13,6 +13,15 @@
 #include <linux/string.h>
 #include <linux/string_helpers.h>
 
+const char * const string_units_10[STRING_UNITS_10_NUM] = {
+	"B", "kB", "MB", "GB", "TB", "PB", "EB", "ZB", "YB",
+};
+EXPORT_SYMBOL(string_units_10);
+const char * const string_units_2[STRING_UNITS_2_NUM] = {
+	"B", "KiB", "MiB", "GiB", "TiB", "PiB", "EiB", "ZiB", "YiB",
+};
+EXPORT_SYMBOL(string_units_2);
+
 /**
  * string_get_size - get the size in the specified units
  * @size:	The size to be converted in blocks
@@ -29,15 +38,9 @@
 void string_get_size(u64 size, u64 blk_size, const enum string_size_units units,
 		     char *buf, int len)
 {
-	static const char *const units_10[] = {
-		"B", "kB", "MB", "GB", "TB", "PB", "EB", "ZB", "YB"
-	};
-	static const char *const units_2[] = {
-		"B", "KiB", "MiB", "GiB", "TiB", "PiB", "EiB", "ZiB", "YiB"
-	};
 	static const char *const *const units_str[] = {
-		[STRING_UNITS_10] = units_10,
-		[STRING_UNITS_2] = units_2,
+		[STRING_UNITS_10] = string_units_10,
+		[STRING_UNITS_2] = string_units_2,
 	};
 	static const unsigned int divisor[] = {
 		[STRING_UNITS_10] = 1000,
@@ -115,7 +118,7 @@ void string_get_size(u64 size, u64 blk_size, const enum string_size_units units,
 	}
 
  out:
-	if (i >= ARRAY_SIZE(units_2))
+	if (i >= STRING_UNITS_2_NUM)
 		unit = "UNK";
 	else
 		unit = units_str[units][i];
-- 
2.6.4

[PATCH v2 05/11] lib/string_helpers: fix indentation in few places

[Andy Shevchenko]

Fix the indentation of label and put snprintf() to one line.

There is no functional change.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/string_helpers.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index c62acc0..6e91f31 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -117,14 +117,13 @@ void string_get_size(u64 size, u64 blk_size, const enum string_size_units units,
 		tmp[j+1] = '\0';
 	}
 
- out:
+out:
 	if (i >= STRING_UNITS_2_NUM)
 		unit = "UNK";
 	else
 		unit = units_str[units][i];
 
-	snprintf(buf, len, "%u%s %s", (u32)size,
-		 tmp, unit);
+	snprintf(buf, len, "%u%s %s", (u32)size, tmp, unit);
 }
 EXPORT_SYMBOL(string_get_size);
 
-- 
2.6.4

[PATCH v2 06/11] lib/vsprintf: introduce %pl to print in human-readable form

[Andy Shevchenko]

Introduce a new extension for printing given unsigned long long value in
human-readable form with automatically choosen IEC binary prefix. The value is
passed by reference.

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

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 5d1128b..8e8b4c0 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -287,6 +287,17 @@ struct clk:
 
 	Passed by reference.
 
+unsigned long long value in human-readable form (with IEC binary prefix):
+
+	%pl		1023 KiB	(1047552)
+	%pl		1048575 B	(1048575)
+	%pl		1 MiB		(1048576)
+
+	For printing given unsigned long long value in human-readable form with
+	automatically choosen IEC binary prefix.
+
+	Passed by reference.
+
 bitmap and its derivatives such as cpumask and nodemask:
 
 	%*pb	0779
diff --git a/lib/test_printf.c b/lib/test_printf.c
index fd985f6..1e078c2 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -407,6 +407,26 @@ bitmap(void)
 }
 
 static void __init
+human_readable(void)
+{
+	struct hr_test_data {
+		unsigned long long v;
+		const char *r;
+	};
+	struct hr_test_data htdpl[] = {
+		{ .v = 1097364144128ULL,	.r = "1022 GiB" },
+		{ .v = 1097364144125ULL,	.r = "1097364144125 B" },
+		{ .v = 1048576ULL,		.r = "1 MiB" },
+		{ .v = 1048575ULL,		.r = "1048575 B" },
+		{ .v = 1047552ULL,		.r = "1023 KiB" },
+	};
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(htdpl); i++)
+		test(htdpl[i].r, "%pl", &htdpl[i].v);
+}
+
+static void __init
 netdev_features(void)
 {
 }
@@ -428,6 +448,7 @@ test_pointer(void)
 	struct_va_format();
 	struct_clk();
 	bitmap();
+	human_readable();
 	netdev_features();
 }
 
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index f0789e3..8288470 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1306,6 +1306,28 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
 }
 
 static noinline_for_stack
+char *human_readable(char *buf, char *end, const void *addr,
+		     struct printf_spec spec, const char *fmt)
+{
+	unsigned long long value = *(unsigned long long *)addr;
+	unsigned long index;
+
+	if (value)
+		index = __ffs64(value) / 10;
+	else
+		index = 0;
+
+	/* Print numeric part */
+	buf = number(buf, end, value >> (index * 10), default_dec_spec);
+
+	/* Space between numeric part and units */
+	buf = put_one_char(buf, end, ' ');
+
+	/* Print units */
+	return string(buf, end, string_units_2[index], default_str_spec);
+}
+
+static noinline_for_stack
 char *netdev_bits(char *buf, char *end, const void *addr, const char *fmt)
 {
 	unsigned long long num;
@@ -1437,6 +1459,7 @@ int kptr_restrict __read_mostly;
  *       Do not use this feature without some mechanism to verify the
  *       correctness of the format string and va_list arguments.
  * - 'K' For a kernel pointer that should be hidden from unprivileged users
+ * - 'l' For a value in human-readable form (with IEC binary prefix)
  * - '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):
@@ -1590,7 +1613,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 			break;
 		}
 		break;
-
+	case 'l':
+		return human_readable(buf, end, ptr, spec, fmt);
 	case 'N':
 		return netdev_bits(buf, end, ptr, fmt);
 	case 'a':
-- 
2.6.4

[PATCH v2 07/11] x86/efi: print size and base in binary units in efi_print_memmap

[Andy Shevchenko]

From: Robert Elliott <elliott@hpe.com>

Print the base address for each range in decimal alongside the size.
Use a "(size @ base)" format similar to the fake_memmap kernel parameter.

Print the range and base in the best-fit B, KiB, MiB, etc. units rather than
always MiB. This avoids rounding, which can be misleading.

Use proper IEC binary units (KiB, MiB, etc.) rather than misuse SI
decimal units (KB, MB, etc.).

old:
    efi: mem61: [Persistent Memory  |   |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000880000000-0x0000000c7fffffff) (16384MB)

new:
    efi: mem61: [Persistent Memory  |   |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000880000000-0x0000000c7fffffff] (16 GiB @ 34 GiB)

Signed-off-by: Robert Elliott <elliott@hpe.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 arch/x86/platform/efi/efi.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index e0846b5..f219e9f 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -225,21 +225,20 @@ int __init efi_memblock_x86_reserve_range(void)
 void __init efi_print_memmap(void)
 {
 #ifdef EFI_DEBUG
-	efi_memory_desc_t *md;
 	void *p;
 	int i;
 
 	for (p = memmap.map, i = 0;
 	     p < memmap.map_end;
 	     p += memmap.desc_size, i++) {
+		efi_memory_desc_t *md = p;
+		u64 size = md->num_pages << EFI_PAGE_SHIFT;
 		char buf[64];
 
-		md = p;
-		pr_info("mem%02u: %s range=[0x%016llx-0x%016llx] (%lluMB)\n",
+		pr_info("mem%02u: %s range=[0x%016llx-0x%016llx] (%pl @ %pl)\n",
 			i, efi_md_typeattr_format(buf, sizeof(buf), md),
-			md->phys_addr,
-			md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT) - 1,
-			(md->num_pages >> (20 - EFI_PAGE_SHIFT)));
+			md->phys_addr, md->phys_addr + size - 1,
+			&size, &md->phys_addr);
 	}
 #endif  /*  EFI_DEBUG  */
 }
-- 
2.6.4

[PATCH v2 08/11] lib/vsprintf: allow range of prefix for %pl[From[To]]

[Andy Shevchenko]

The user may supply the range of desired prefixes in a format %pl[From[To]],
where 'From' is letter of minimum allowed prefix, and 'To' is a maximum
correspondently.

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

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 8e8b4c0..3b41899 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -289,9 +289,13 @@ struct clk:
 
 unsigned long long value in human-readable form (with IEC binary prefix):
 
+	%pl[From[To]]
+
 	%pl		1023 KiB	(1047552)
 	%pl		1048575 B	(1048575)
+	%plKM		1023 KiB	(1048575)
 	%pl		1 MiB		(1048576)
+	%plG		2 TiB		(2199023255552)
 
 	For printing given unsigned long long value in human-readable form with
 	automatically choosen IEC binary prefix.
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 1e078c2..ae35885 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -420,10 +420,39 @@ human_readable(void)
 		{ .v = 1048575ULL,		.r = "1048575 B" },
 		{ .v = 1047552ULL,		.r = "1023 KiB" },
 	};
-	unsigned int i;
+	struct hr_test_data htdplM[] = {
+		{ .v = 1097364144128ULL,	.r = "1022 GiB" },
+		{ .v = 1097364144125ULL,	.r = "1046527 MiB" },
+		{ .v = 1048576ULL,		.r = "1 MiB" },
+		{ .v = 1048575ULL,		.r = "0 MiB" },
+		{ .v = 1047552ULL,		.r = "0 MiB" },
+	};
+	struct hr_test_data htdplKM[] = {
+		{ .v = 1097364144128ULL,	.r = "1046528 MiB" },
+		{ .v = 1097364144125ULL,	.r = "1071644671 KiB" },
+		{ .v = 1048578ULL,		.r = "1024 KiB" },
+		{ .v = 1048576ULL,		.r = "1 MiB" },
+		{ .v = 1048575ULL,		.r = "1023 KiB" },
+		{ .v = 1047552ULL,		.r = "1023 KiB" },
+	};
+	struct hr_test_array {
+		const char *fmt;
+		struct hr_test_data *data;
+		size_t sz;
+	};
+	struct hr_test_array hta[] = {
+		{ "%pl",	htdpl,		ARRAY_SIZE(htdpl) },
+		{ "%plM",	htdplM,		ARRAY_SIZE(htdplM) },
+		{ "%plKM",	htdplKM,	ARRAY_SIZE(htdplKM) },
+		/* No reversed %pl[To[From]] order */
+		{ "%plMK",	htdplM,		ARRAY_SIZE(htdplM) },
+	};
+	unsigned int i, j;
 
-	for (i = 0; i < ARRAY_SIZE(htdpl); i++)
-		test(htdpl[i].r, "%pl", &htdpl[i].v);
+	for (j = 0; j < ARRAY_SIZE(hta); j++) {
+		for (i = 0; i < hta[j].sz; i++)
+			test(hta[j].data[i].r, hta[j].fmt, &hta[j].data[i].v);
+	}
 }
 
 static void __init
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 8288470..fbcb221 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1310,12 +1310,28 @@ char *human_readable(char *buf, char *end, const void *addr,
 		     struct printf_spec spec, const char *fmt)
 {
 	unsigned long long value = *(unsigned long long *)addr;
+	unsigned short from = 0, to = STRING_UNITS_2_NUM - 1;
 	unsigned long index;
+	unsigned int i;
+
+	/* Parse %pl[From[To]] */
+	for (i = 0; i < STRING_UNITS_2_NUM; i++)
+		if (fmt[1] == string_units_2[i][0])
+			break;
+	if (i < STRING_UNITS_2_NUM) {
+		from = i;
+		for (i = 0; i < STRING_UNITS_2_NUM; i++)
+			if (fmt[2] == string_units_2[i][0])
+				break;
+		if (i < STRING_UNITS_2_NUM && i >= from)
+			to = i;
+	}
 
 	if (value)
 		index = __ffs64(value) / 10;
 	else
 		index = 0;
+	index = clamp_t(unsigned long, index, from, to);
 
 	/* Print numeric part */
 	buf = number(buf, end, value >> (index * 10), default_dec_spec);
@@ -1459,7 +1475,7 @@ int kptr_restrict __read_mostly;
  *       Do not use this feature without some mechanism to verify the
  *       correctness of the format string and va_list arguments.
  * - 'K' For a kernel pointer that should be hidden from unprivileged users
- * - 'l' For a value in human-readable form (with IEC binary prefix)
+ * - 'l[from[to]]' For a value in human-readable form (with IEC binary prefix)
  * - '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):
-- 
2.6.4

[PATCH v2 09/11] lib/vsprintf: use precision field with %pl[From[To]]

[Andy Shevchenko]

The precision field defines how many digits after period ('.') will be printed.

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

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 3b41899..534407c 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -295,11 +295,18 @@ unsigned long long value in human-readable form (with IEC binary prefix):
 	%pl		1048575 B	(1048575)
 	%plKM		1023 KiB	(1048575)
 	%pl		1 MiB		(1048576)
+	%.1plKM		1046527.9 MiB	(1097364144125)
 	%plG		2 TiB		(2199023255552)
 
 	For printing given unsigned long long value in human-readable form with
 	automatically choosen IEC binary prefix.
 
+	Precision, if passed, defines how many digits will be printed after
+	period ('.'). In this case the biggest possible unit is used in the
+	given range [From[To]]. The printed value isn't rounded anyhow.
+
+	For other use cases consider to call string_get_size().
+
 	Passed by reference.
 
 bitmap and its derivatives such as cpumask and nodemask:
diff --git a/lib/test_printf.c b/lib/test_printf.c
index ae35885..4a2e30b 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -435,6 +435,30 @@ human_readable(void)
 		{ .v = 1048575ULL,		.r = "1023 KiB" },
 		{ .v = 1047552ULL,		.r = "1023 KiB" },
 	};
+	struct hr_test_data htdpl0KM[] = {
+		{ .v = 1097364144128ULL,	.r = "1046528 MiB" },
+		{ .v = 1097364144125ULL,	.r = "1046527 MiB" },
+		{ .v = 1048578ULL,		.r = "1 MiB" },
+		{ .v = 1048576ULL,		.r = "1 MiB" },
+		{ .v = 1048575ULL,		.r = "1023 KiB" },
+		{ .v = 1047552ULL,		.r = "1023 KiB" },
+	};
+	struct hr_test_data htdpl1KM[] = {
+		{ .v = 1097364144128ULL,	.r = "1046528.0 MiB" },
+		{ .v = 1097364144125ULL,	.r = "1046527.9 MiB" },
+		{ .v = 1048578ULL,		.r = "1.0 MiB" },
+		{ .v = 1048576ULL,		.r = "1.0 MiB" },
+		{ .v = 1048575ULL,		.r = "1023.9 KiB" },
+		{ .v = 1047552ULL,		.r = "1023.0 KiB" },
+	};
+	struct hr_test_data htdpl3KM[] = {
+		{ .v = 1097364144128ULL,	.r = "1046528.000 MiB" },
+		{ .v = 1097364144125ULL,	.r = "1046527.999 MiB" },
+		{ .v = 1048578ULL,		.r = "1.000 MiB" },
+		{ .v = 1048576ULL,		.r = "1.000 MiB" },
+		{ .v = 1048575ULL,		.r = "1023.999 KiB" },
+		{ .v = 1047552ULL,		.r = "1023.000 KiB" },
+	};
 	struct hr_test_array {
 		const char *fmt;
 		struct hr_test_data *data;
@@ -446,6 +470,11 @@ human_readable(void)
 		{ "%plKM",	htdplKM,	ARRAY_SIZE(htdplKM) },
 		/* No reversed %pl[To[From]] order */
 		{ "%plMK",	htdplM,		ARRAY_SIZE(htdplM) },
+		{ "%.0plKM",	htdpl0KM,	ARRAY_SIZE(htdpl0KM) },
+		{ "%.1plKM",	htdpl1KM,	ARRAY_SIZE(htdpl1KM) },
+		{ "%.3plKM",	htdpl3KM,	ARRAY_SIZE(htdpl3KM) },
+		/* For now precision is in range [0..3] */
+		{ "%.4plKM",	htdplKM,	ARRAY_SIZE(htdplKM) },
 	};
 	unsigned int i, j;
 
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index fbcb221..2dd86c9 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1327,7 +1327,9 @@ char *human_readable(char *buf, char *end, const void *addr,
 			to = i;
 	}
 
-	if (value)
+	if (spec.precision >= 0 && spec.precision <= 3 && value)
+		index = (fls64(value) - 1) / 10;
+	else if (value)
 		index = __ffs64(value) / 10;
 	else
 		index = 0;
@@ -1336,6 +1338,22 @@ char *human_readable(char *buf, char *end, const void *addr,
 	/* Print numeric part */
 	buf = number(buf, end, value >> (index * 10), default_dec_spec);
 
+	/* Print precision part if asked */
+	if (spec.precision > 0 && spec.precision <= 3 && index) {
+		unsigned int remainder;
+
+		remainder = (value >> (index - 1) * 10) % 1024;
+		for (i = 0; i < spec.precision; i++)
+			remainder *= 10;
+		remainder >>= 10;
+
+		/* Period... */
+		buf = put_one_char(buf, end, '.');
+
+		/* ...and remainder part */
+		buf = number(buf, end, remainder, spec);
+	}
+
 	/* Space between numeric part and units */
 	buf = put_one_char(buf, end, ' ');
 
-- 
2.6.4

[PATCH v2 10/11] cxgb4: print value in human-readable form via %.0plKM

[Andy Shevchenko]

Recently added %pl[From[To]] specifier is dedicated to print values in
human-readable format with IEC prefix. Convert the code to use it.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
index e6a4072..5185d8e 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
@@ -2820,6 +2820,7 @@ static int meminfo_show(struct seq_file *seq, void *v)
 	};
 
 	int i, n;
+	u64 size;
 	u32 lo, hi, used, alloc;
 	struct mem_desc avail[4];
 	struct mem_desc mem[ARRAY_SIZE(region) + 3];      /* up to 3 holes */
@@ -2936,10 +2937,10 @@ static int meminfo_show(struct seq_file *seq, void *v)
 	md->base = 0;
 	md->idx = ARRAY_SIZE(region);
 	if (!is_t4(adap->params.chip)) {
-		u32 size = 0;
 		u32 sge_ctrl = t4_read_reg(adap, SGE_CONTROL2_A);
 		u32 fifo_size = t4_read_reg(adap, SGE_DBVFIFO_SIZE_A);
 
+		size = 0;
 		if (is_t5(adap->params.chip)) {
 			if (sge_ctrl & VFIFO_ENABLE_F)
 				size = DBVFIFO_SIZE_G(fifo_size);
@@ -3010,11 +3011,9 @@ static int meminfo_show(struct seq_file *seq, void *v)
 		   (lo & PMRXNUMCHN_F) ? 2 : 1);
 
 	lo = t4_read_reg(adap, TP_PMM_TX_MAX_PAGE_A);
-	hi = t4_read_reg(adap, TP_PMM_TX_PAGE_SIZE_A);
-	seq_printf(seq, "%u Tx pages of size %u%ciB for %u channels\n",
-		   PMTXMAXPAGE_G(lo),
-		   hi >= (1 << 20) ? (hi >> 20) : (hi >> 10),
-		   hi >= (1 << 20) ? 'M' : 'K', 1 << PMTXNUMCHN_G(lo));
+	size = t4_read_reg(adap, TP_PMM_TX_PAGE_SIZE_A);
+	seq_printf(seq, "%u Tx pages of size %.0plKM for %u channels\n",
+		   PMTXMAXPAGE_G(lo), &size, 1 << PMTXNUMCHN_G(lo));
 	seq_printf(seq, "%u p-structs\n\n",
 		   t4_read_reg(adap, TP_CMM_MM_MAX_PSTRUCT_A));
 
-- 
2.6.4

[PATCH v2 11/11] pcmciamtd: print value in human-readable form via %.0plKM

[Andy Shevchenko]

Recently added %pl[From[To]] specifier is dedicated to print values in
human-readable format with IEC prefix. Convert the code to use it.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/mtd/maps/pcmciamtd.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/mtd/maps/pcmciamtd.c b/drivers/mtd/maps/pcmciamtd.c
index 70bb403..44c2fe9 100644
--- a/drivers/mtd/maps/pcmciamtd.c
+++ b/drivers/mtd/maps/pcmciamtd.c
@@ -589,19 +589,11 @@ static int pcmciamtd_config(struct pcmcia_device *link)
 	mtd->owner = THIS_MODULE;
 
 	if(new_name) {
-		int size = 0;
-		char unit = ' ';
 		/* Since we are using a default name, make it better by adding
 		 * in the size
 		 */
-		if(mtd->size < 1048576) { /* <1MiB in size, show size in KiB */
-			size = mtd->size >> 10;
-			unit = 'K';
-		} else {
-			size = mtd->size >> 20;
-			unit = 'M';
-		}
-		snprintf(dev->mtd_name, sizeof(dev->mtd_name), "%d%ciB %s", size, unit, "PCMCIA Memory card");
+		snprintf(dev->mtd_name, sizeof(dev->mtd_name), "%.0plKM %s",
+			 &mtd->size, "PCMCIA Memory card");
 	}
 
 	/* If the memory found is fits completely into the mapped PCMCIA window,
-- 
2.6.4

Re: [PATCH v2 06/11] lib/vsprintf: introduce %pl to print in human-readable form

[Joe Perches]

On Fri, 2016-01-15 at 00:23 +0200, Andy Shevchenko wrote:
> Introduce a new extension for printing given unsigned long long value in
> human-readable form with automatically choosen IEC binary prefix. The value is
> passed by reference.

Perhaps there should be some 1, 2, 4, 8 argument so that
u8, u16, u32, and u64 can all use this extension and not
just unsigned long long.

Re: [PATCH v2 01/11] lib/vsprintf: introduce put_one_char() for 3 line idiom

[Rasmus Villemoes]

On Thu, Jan 14 2016, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> There is an idiom used widely in the code
>
> 	if (buf < end)
> 		*buf = c;
> 	++buf;
>
> Introduce put_one_char() helper as implementation of this idiom.
>

I suppose it does make the code a little more readable, but the current
idiom isn't that bad. Does it affect the generated code, and if so, does
it become smaller/larger, faster/slower?

Rasmus

Re: [PATCH v2 02/11] lib/vsprintf: make default_dec_spec global

[Rasmus Villemoes]

On Thu, Jan 14 2016, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> There are places where default specification to print decimal numbers is used.
> Make it global and convert existing users.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  lib/test_printf.c |  5 +++--
>  lib/vsprintf.c    | 21 +++++++++------------
>  2 files changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 4f6ae60..fd985f6 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -376,9 +376,10 @@ large_bitmap(void)
>  	if (!bits)
>  		return;
>  
> -	bitmap_set(bits, 1, 20);
> +	bitmap_set(bits, 0, 1);
> +	bitmap_set(bits, 2, 20);
>  	bitmap_set(bits, 60000, 15);
> -	test("1-20,60000-60014", "%*pbl", nbits, bits);
> +	test("0,2-21,60000-60014", "%*pbl", nbits, bits);
>  	kfree(bits);
>  }

What does this have to do with what the commit message claims this patch
is about? (I don't mind the rest of the patch, just curious why this is
included.)

Rasmus

Re: [PATCH v2 06/11] lib/vsprintf: introduce %pl to print in human-readable form

[Rasmus Villemoes]

On Thu, Jan 14 2016, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Introduce a new extension for printing given unsigned long long value in
> human-readable form with automatically choosen IEC binary prefix. The value is
> passed by reference.
>

I don't like this, and don't find it particularly useful. There's also
the problem Joe mentions with types; inevitably, someone wants to pass a
size_t or an unsigned int or whatnot, and allowing that gets ugly, as is
forcing the caller to copy the value to a temp u64.

If you really want this, I think it would be much better to introduce a
helper which takes a user-supplied (typically small stack) buffer,
formats into that, and maybe for convenience returns that buffer (so it
can be passed directly to a %s). That's a lot more flexible, and avoids
the annoying type issue since the user's value just gets passed directly
(and thus auto-promoted to u64) to the helper.

The %pX space is also rapidly diminishing, so we should think twice
before introducing yet another rarely used extension.

In any case, I had to read the implementation to figure out that the
prefix chosen is the largest for which the printed value is exactly what
was passed in. Please make that more explicit. [It also means that the
printed string isn't necessarily human readable at all, as your test
value 1097364144125 shows].

Rasmus

Re: [PATCH v2 08/11] lib/vsprintf: allow range of prefix for %pl[From[To]]

[Rasmus Villemoes]

On Thu, Jan 14 2016, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> The user may supply the range of desired prefixes in a format %pl[From[To]],
> where 'From' is letter of minimum allowed prefix, and 'To' is a maximum
> correspondently.
>

Sorry, but yuck. I really don't think a %p extension is a good way to
achieve this pretty-printing (with or without the min/max unit thing
added). Also, this makes it too easy to lose information (since %plX
would round down to X), so we'd neither get something representing the
passed-in value nor something human readable (as your test case { .v =
1097364144125ULL, .r = "1071644671 KiB" } shows).

>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  Documentation/printk-formats.txt |  4 ++++
>  lib/test_printf.c                | 35 ++++++++++++++++++++++++++++++++---
>  lib/vsprintf.c                   | 18 +++++++++++++++++-
>  3 files changed, 53 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
> index 8e8b4c0..3b41899 100644
> --- a/Documentation/printk-formats.txt
> +++ b/Documentation/printk-formats.txt
> @@ -289,9 +289,13 @@ struct clk:
>  
>  unsigned long long value in human-readable form (with IEC binary prefix):
>  
> +	%pl[From[To]]
> +
>  	%pl		1023 KiB	(1047552)
>  	%pl		1048575 B	(1048575)
> +	%plKM		1023 KiB	(1048575)
>  	%pl		1 MiB		(1048576)
> +	%plG		2 TiB		(2199023255552)
>  
>  	For printing given unsigned long long value in human-readable form with
>  	automatically choosen IEC binary prefix.
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 1e078c2..ae35885 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -420,10 +420,39 @@ human_readable(void)
>  		{ .v = 1048575ULL,		.r = "1048575 B" },
>  		{ .v = 1047552ULL,		.r = "1023 KiB" },
>  	};
> -	unsigned int i;
> +	struct hr_test_data htdplM[] = {
> +		{ .v = 1097364144128ULL,	.r = "1022 GiB" },
> +		{ .v = 1097364144125ULL,	.r = "1046527 MiB" },
> +		{ .v = 1048576ULL,		.r = "1 MiB" },
> +		{ .v = 1048575ULL,		.r = "0 MiB" },
> +		{ .v = 1047552ULL,		.r = "0 MiB" },
> +	};
> +	struct hr_test_data htdplKM[] = {
> +		{ .v = 1097364144128ULL,	.r = "1046528 MiB" },
> +		{ .v = 1097364144125ULL,	.r = "1071644671 KiB" },
> +		{ .v = 1048578ULL,		.r = "1024 KiB" },
> +		{ .v = 1048576ULL,		.r = "1 MiB" },
> +		{ .v = 1048575ULL,		.r = "1023 KiB" },
> +		{ .v = 1047552ULL,		.r = "1023 KiB" },
> +	};
> +	struct hr_test_array {
> +		const char *fmt;
> +		struct hr_test_data *data;
> +		size_t sz;
> +	};
> +	struct hr_test_array hta[] = {
> +		{ "%pl",	htdpl,		ARRAY_SIZE(htdpl) },
> +		{ "%plM",	htdplM,		ARRAY_SIZE(htdplM) },
> +		{ "%plKM",	htdplKM,	ARRAY_SIZE(htdplKM) },
> +		/* No reversed %pl[To[From]] order */
> +		{ "%plMK",	htdplM,		ARRAY_SIZE(htdplM) },
> +	};
> +	unsigned int i, j;
>  
> -	for (i = 0; i < ARRAY_SIZE(htdpl); i++)
> -		test(htdpl[i].r, "%pl", &htdpl[i].v);
> +	for (j = 0; j < ARRAY_SIZE(hta); j++) {
> +		for (i = 0; i < hta[j].sz; i++)
> +			test(hta[j].data[i].r, hta[j].fmt, &hta[j].data[i].v);
> +	}
>  }
>  
>  static void __init
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 8288470..fbcb221 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1310,12 +1310,28 @@ char *human_readable(char *buf, char *end, const void *addr,
>  		     struct printf_spec spec, const char *fmt)
>  {
>  	unsigned long long value = *(unsigned long long *)addr;
> +	unsigned short from = 0, to = STRING_UNITS_2_NUM - 1;
>  	unsigned long index;
> +	unsigned int i;
> +

Is there any reason you try to use every integer rank? from, to, index
and i never take values other than [0..9], right? So unsigned int
(or u8 if you somehow think that would generate better code) should be
good for them all? 

> +	/* Parse %pl[From[To]] */
> +	for (i = 0; i < STRING_UNITS_2_NUM; i++)
> +		if (fmt[1] == string_units_2[i][0])
> +			break;
> +	if (i < STRING_UNITS_2_NUM) {
> +		from = i;
> +		for (i = 0; i < STRING_UNITS_2_NUM; i++)
> +			if (fmt[2] == string_units_2[i][0])
> +				break;
> +		if (i < STRING_UNITS_2_NUM && i >= from)
> +			to = i;
> +	}
>  
>  	if (value)
>  		index = __ffs64(value) / 10;
>  	else
>  		index = 0;
> +	index = clamp_t(unsigned long, index, from, to);

And then you wouldn't need to use the _t version here.

>  	/* Print numeric part */
>  	buf = number(buf, end, value >> (index * 10), default_dec_spec);
> @@ -1459,7 +1475,7 @@ int kptr_restrict __read_mostly;
>   *       Do not use this feature without some mechanism to verify the
>   *       correctness of the format string and va_list arguments.
>   * - 'K' For a kernel pointer that should be hidden from unprivileged users
> - * - 'l' For a value in human-readable form (with IEC binary prefix)
> + * - 'l[from[to]]' For a value in human-readable form (with IEC binary prefix)
>   * - '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):

Re: [PATCH v2 09/11] lib/vsprintf: use precision field with %pl[From[To]]

[Rasmus Villemoes]

On Thu, Jan 14 2016, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> The precision field defines how many digits after period ('.') will be
> printed.

Yuck again.

Didn't you notice gcc barking at you?

warning: precision used with ‘%p’ gnu_printf format [-Wformat=]

So I don't think this will ever be used. 

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  Documentation/printk-formats.txt |  7 +++++++
>  lib/test_printf.c                | 29 +++++++++++++++++++++++++++++
>  lib/vsprintf.c                   | 20 +++++++++++++++++++-
>  3 files changed, 55 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
> index 3b41899..534407c 100644
> --- a/Documentation/printk-formats.txt
> +++ b/Documentation/printk-formats.txt
> @@ -295,11 +295,18 @@ unsigned long long value in human-readable form (with IEC binary prefix):
>  	%pl		1048575 B	(1048575)
>  	%plKM		1023 KiB	(1048575)
>  	%pl		1 MiB		(1048576)
> +	%.1plKM		1046527.9 MiB	(1097364144125)
>  	%plG		2 TiB		(2199023255552)
>  
>  	For printing given unsigned long long value in human-readable form with
>  	automatically choosen IEC binary prefix.
>  
> +	Precision, if passed, defines how many digits will be printed after
> +	period ('.'). In this case the biggest possible unit is used in the
> +	given range [From[To]]. The printed value isn't rounded anyhow.
> +
> +	For other use cases consider to call string_get_size().
> +
>  	Passed by reference.
>  
>  bitmap and its derivatives such as cpumask and nodemask:
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index ae35885..4a2e30b 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -435,6 +435,30 @@ human_readable(void)
>  		{ .v = 1048575ULL,		.r = "1023 KiB" },
>  		{ .v = 1047552ULL,		.r = "1023 KiB" },
>  	};
> +	struct hr_test_data htdpl0KM[] = {
> +		{ .v = 1097364144128ULL,	.r = "1046528 MiB" },
> +		{ .v = 1097364144125ULL,	.r = "1046527 MiB" },
> +		{ .v = 1048578ULL,		.r = "1 MiB" },
> +		{ .v = 1048576ULL,		.r = "1 MiB" },
> +		{ .v = 1048575ULL,		.r = "1023 KiB" },
> +		{ .v = 1047552ULL,		.r = "1023 KiB" },
> +	};
> +	struct hr_test_data htdpl1KM[] = {
> +		{ .v = 1097364144128ULL,	.r = "1046528.0 MiB" },
> +		{ .v = 1097364144125ULL,	.r = "1046527.9 MiB" },
> +		{ .v = 1048578ULL,		.r = "1.0 MiB" },
> +		{ .v = 1048576ULL,		.r = "1.0 MiB" },
> +		{ .v = 1048575ULL,		.r = "1023.9 KiB" },
> +		{ .v = 1047552ULL,		.r = "1023.0 KiB" },
> +	};
> +	struct hr_test_data htdpl3KM[] = {
> +		{ .v = 1097364144128ULL,	.r = "1046528.000 MiB" },
> +		{ .v = 1097364144125ULL,	.r = "1046527.999 MiB" },
> +		{ .v = 1048578ULL,		.r = "1.000 MiB" },
> +		{ .v = 1048576ULL,		.r = "1.000 MiB" },
> +		{ .v = 1048575ULL,		.r = "1023.999 KiB" },
> +		{ .v = 1047552ULL,		.r = "1023.000 KiB" },
> +	};
>  	struct hr_test_array {
>  		const char *fmt;
>  		struct hr_test_data *data;
> @@ -446,6 +470,11 @@ human_readable(void)
>  		{ "%plKM",	htdplKM,	ARRAY_SIZE(htdplKM) },
>  		/* No reversed %pl[To[From]] order */
>  		{ "%plMK",	htdplM,		ARRAY_SIZE(htdplM) },
> +		{ "%.0plKM",	htdpl0KM,	ARRAY_SIZE(htdpl0KM) },
> +		{ "%.1plKM",	htdpl1KM,	ARRAY_SIZE(htdpl1KM) },
> +		{ "%.3plKM",	htdpl3KM,	ARRAY_SIZE(htdpl3KM) },
> +		/* For now precision is in range [0..3] */
> +		{ "%.4plKM",	htdplKM,	ARRAY_SIZE(htdplKM) },
>  	};
>  	unsigned int i, j;
>  
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index fbcb221..2dd86c9 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1327,7 +1327,9 @@ char *human_readable(char *buf, char *end, const void *addr,
>  			to = i;
>  	}
>  
> -	if (value)
> +	if (spec.precision >= 0 && spec.precision <= 3 && value)
> +		index = (fls64(value) - 1) / 10;
> +	else if (value)
>  		index = __ffs64(value) / 10;
>  	else
>  		index = 0;
> @@ -1336,6 +1338,22 @@ char *human_readable(char *buf, char *end, const void *addr,
>  	/* Print numeric part */
>  	buf = number(buf, end, value >> (index * 10), default_dec_spec);
>  
> +	/* Print precision part if asked */
> +	if (spec.precision > 0 && spec.precision <= 3 && index) {
> +		unsigned int remainder;
> +
> +		remainder = (value >> (index - 1) * 10) % 1024;
> +		for (i = 0; i < spec.precision; i++)
> +			remainder *= 10;
> +		remainder >>= 10;
> +
> +		/* Period... */
> +		buf = put_one_char(buf, end, '.');
> +
> +		/* ...and remainder part */
> +		buf = number(buf, end, remainder, spec);
> +	}
> +
>  	/* Space between numeric part and units */
>  	buf = put_one_char(buf, end, ' ');

Re: [PATCH v2 00/11] lib/vsprintf: refactor and introduce %pl specifier

[Andy Shevchenko]

On Fri, 2016-01-15 at 00:23 +0200, Andy Shevchenko wrote:
> This series refactors vsprintf.c, introduces %pl specifier to print
> unsigned
> long long value in human-readable format, enhances EFI messages, and
> converts
> existing users of such functionality.
> 
> The series has been tested on 32-bit and 64-bit Intel platforms with
> test_printf.c suite.
> 
> In the future someone can extend %pl to cover the cases like
> string_get_size()
> does.

Rasmus, thanks for review.

Andrew, please don't take this series anyhow, I'm also pretty sure
there will be no v3 of it anyway.

> 
> Andy Shevchenko (10):
>   lib/vsprintf: introduce put_one_char() for 3 line idiom
>   lib/vsprintf: make default_dec_spec global
>   lib/vsprintf: make default_str_spec global
>   lib/string_helpers: export string_units_{2,10} for others
>   lib/string_helpers: fix indentation in few places
>   lib/vsprintf: introduce %pl to print in human-readable form
>   lib/vsprintf: allow range of prefix for %pl[From[To]]
>   lib/vsprintf: use precision field with %pl[From[To]]
>   cxgb4: print value in human-readable form via %.0plKM
>   pcmciamtd: print value in human-readable form via %.0plKM
> 
> Robert Elliott (1):
>   x86/efi: print size and base in binary units in efi_print_memmap
> 
>  Documentation/printk-formats.txt                   |  22 ++
>  arch/x86/platform/efi/efi.c                        |  11 +-
>  drivers/mtd/maps/pcmciamtd.c                       |  12 +-
>  drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c |  11 +-
>  include/linux/string_helpers.h                     |   6 +
>  lib/string_helpers.c                               |  26 +-
>  lib/test_printf.c                                  |  84 +++++-
>  lib/vsprintf.c                                     | 296 ++++++++++-
> ----------
>  8 files changed, 277 insertions(+), 191 deletions(-)
> 

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

Re: [PATCH v2 06/11] lib/vsprintf: introduce %pl to print in human-readable form

[H. Peter Anvin]

On 01/14/16 14:23, Andy Shevchenko wrote:
> Introduce a new extension for printing given unsigned long long value in
> human-readable form with automatically choosen IEC binary prefix. The value is
> passed by reference.

Why by reference?  That seems to make little sense.

The prefix should not include the unit (e.g. B) as we may want to use
another thing.

It is also worth noting that what you have here is to generate *exact*
values, which aren't always what we want.  Another common thing is to
round/truncate to 3 significant figures in order to get the order of
magnitude clear; the prefix being so different depending on the low
order bits can be confusing when an exact representation isn't needed.

	-hpa