[PATCH] of: Custom printk format specifier for device node

[PATCH] of: Custom printk format specifier for device node

[Pantelis Antoniou]

90% of the usage of device node's full_name is printing it out
in a kernel message. Preparing for the eventual delayed allocation
introduce a custom printk format specifier that is both more
compact and more pleasant to the eye.

For instance typical use is:
	pr_info("Frobbing node %s\n", node->full_name);

Which can be written now as:
	pr_info("Frobbing node %pO\n", node);

More fine-grained control of formatting includes printing the name,
flag, path-spec name, reference count and others, explained in the
documentation entry.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>

dt-print
---
 Documentation/printk-formats.txt |  29 ++++++++
 lib/vsprintf.c                   | 151 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 180 insertions(+)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 5a615c1..2d42c04 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -231,6 +231,35 @@ struct va_format:
 	Do not use this feature without some mechanism to verify the
 	correctness of the format string and va_list arguments.
 
+Device tree nodes:
+
+	%pO[fnpPcCFr]
+
+	For printing device tree nodes. The optional arguments are:
+            f device node full_name
+            n device node name
+            p device node phandle
+            P device node path spec (name + @unit)
+            F device node flags
+            c major compatible string
+            C full compatible string
+            r node reference count
+	Without any arguments prints full_name (same as %pOf)
+	The separator when using multiple arguments is '|'
+
+	Examples:
+
+	%pO	/foo/bar@0			- Node full name
+	%pOf	/foo/bar@0			- Same as above
+	%pOfp	/foo/bar@0|10			- Node full name + phandle
+	%pOfcF	/foo/bar@0|foo,device|--P-	- Node full name +
+	                                          major compatible string +
+						  node flags
+							D - dynamic
+							d - detached
+							P - Populated
+							B - Populated bus
+
 u64 SHOULD be printed with %llu/%llx:
 
 	printk("%llu", u64_var);
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index ec337f6..f3e49b9 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -29,6 +29,7 @@
 #include <linux/dcache.h>
 #include <linux/cred.h>
 #include <net/addrconf.h>
+#include <linux/of.h>
 
 #include <asm/page.h>		/* for PAGE_SIZE */
 #include <asm/sections.h>	/* for dereference_function_descriptor() */
@@ -1240,6 +1241,144 @@ char *address_val(char *buf, char *end, const void *addr,
 	return number(buf, end, num, spec);
 }
 
+static noinline_for_stack
+char *device_node_string(char *buf, char *end, struct device_node *dn,
+			 struct printf_spec spec, const char *fmt)
+{
+	char tbuf[sizeof("xxxxxxxxxx") + 1];
+	const char *fmtp, *p;
+	int len, ret, i, j, pass;
+	char c;
+
+	if (!IS_ENABLED(CONFIG_OF))
+		return string(buf, end, "(!OF)", spec);
+
+	if ((unsigned long)dn < PAGE_SIZE)
+		return string(buf, end, "(null)", spec);
+
+	/* simple case without anything any more format specifiers */
+	if (fmt[1] == '\0' || isspace(fmt[1]))
+		fmt = "Of";
+
+	len = 0;
+
+	/* two passes; the first calculates length, the second fills in */
+	for (pass = 1; pass <= 2; pass++) {
+		if (pass == 2 && !(spec.flags & LEFT)) {
+			/* padding */
+			while (len < spec.field_width--) {
+				if (buf < end)
+					*buf = ' ';
+				++buf;
+			}
+		}
+#undef _HANDLE_CH
+#define _HANDLE_CH(_ch) \
+	do { \
+		if (pass == 1) \
+			len++; \
+		else \
+			if (buf < end) \
+				*buf++ = (_ch); \
+	} while (0)
+#undef _HANDLE_STR
+#define _HANDLE_STR(_str) \
+	do { \
+		const char *str = (_str); \
+		if (pass == 1) \
+			len += strlen(str); \
+		else \
+			while (*str && buf < end) \
+				*buf++ = *str++; \
+	} while (0)
+
+		for (fmtp = fmt + 1, j = 0; (c = *fmtp++) != '\0'; ) {
+			switch (c) {
+			case 'f':	/* full_name */
+				if (j++ > 0)
+					_HANDLE_CH(':');
+				_HANDLE_STR(of_node_full_name(dn));
+				break;
+			case 'n':	/* name */
+				if (j++ > 0)
+					_HANDLE_CH('|');
+				_HANDLE_STR(dn->name);
+				break;
+			case 'p':	/* phandle */
+				if (j++ > 0)
+					_HANDLE_CH('|');
+				snprintf(tbuf, sizeof(tbuf), "%u",
+						(unsigned int)dn->phandle);
+				_HANDLE_STR(tbuf);
+				break;
+			case 'P':	/* path-spec */
+				if (j++ > 0)
+					_HANDLE_CH('|');
+				_HANDLE_STR(dn->name);
+				/* need to tack on the @ postfix */
+				p = strchr(of_node_full_name(dn), '@');
+				if (p)
+					_HANDLE_STR(p);
+				break;
+			case 'F':	/* flags */
+				if (j++ > 0)
+					_HANDLE_CH('|');
+				snprintf(tbuf, sizeof(tbuf), "%c%c%c%c",
+					of_node_check_flag(dn, OF_DYNAMIC) ?
+						'D' : '-',
+					of_node_check_flag(dn, OF_DETACHED) ?
+						'd' : '-',
+					of_node_check_flag(dn, OF_POPULATED) ?
+						'P' : '-',
+					of_node_check_flag(dn,
+						OF_POPULATED_BUS) ?  'B' : '-');
+				_HANDLE_STR(tbuf);
+				break;
+			case 'c':	/* major compatible string */
+				if (j++ > 0)
+					_HANDLE_CH('|');
+				ret = of_property_read_string(dn, "compatible",
+						&p);
+				if (ret == 0)
+					_HANDLE_STR(p);
+				break;
+			case 'C':	/* full compatible string */
+				if (j++ > 0)
+					_HANDLE_CH('|');
+				i = 0;
+				while (of_property_read_string_index(dn,
+						"compatible", i, &p) == 0) {
+					if (i == 0)
+						_HANDLE_STR("\"");
+					else
+						_HANDLE_STR("\",\"");
+					_HANDLE_STR(p);
+					i++;
+				}
+				if (i > 0)
+					_HANDLE_STR("\"");
+				break;
+			case 'r':	/* node reference count */
+				if (j++ > 0)
+					_HANDLE_CH('|');
+				snprintf(tbuf, sizeof(tbuf), "%u",
+					atomic_read(&dn->kobj.kref.refcount));
+				_HANDLE_STR(tbuf);
+				break;
+			default:
+				break;
+			}
+		}
+	}
+	/* finish up */
+	while (buf < end && len < spec.field_width--)
+		*buf++ = ' ';
+#undef _HANDLE_CH
+#undef _HANDLE_STR
+
+	return buf;
+}
+
 int kptr_restrict __read_mostly;
 
 /*
@@ -1318,6 +1457,16 @@ int kptr_restrict __read_mostly;
  *           (default assumed to be phys_addr_t, passed by reference)
  * - 'd[234]' For a dentry name (optionally 2-4 last components)
  * - 'D[234]' Same as 'd' but for a struct file
+ * - 'O[fnpPcCFr]' For an DT device node
+ *                Without any optional arguments prints the full_name
+ *                f device node full_name
+ *                n device node name
+ *                p device node phandle
+ *                P device node path spec (name + @unit)
+ *                F device node flags
+ *                c major compatible string
+ *                C full compatible string
+ *                r node reference count
  *
  * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
  * function pointers are really function descriptors, which contain a
@@ -1459,6 +1608,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		return dentry_name(buf, end,
 				   ((const struct file *)ptr)->f_path.dentry,
 				   spec, fmt);
+	case 'O':
+		return device_node_string(buf, end, ptr, spec, fmt);
 	}
 	spec.flags |= SMALL;
 	if (spec.field_width == -1) {
-- 
1.7.12

Re: [PATCH] of: Custom printk format specifier for device node

[Joe Perches]

On Wed, 2015-01-21 at 19:06 +0200, Pantelis Antoniou wrote:
> 90% of the usage of device node's full_name is printing it out
> in a kernel message. Preparing for the eventual delayed allocation
> introduce a custom printk format specifier that is both more
> compact and more pleasant to the eye.
> 
> For instance typical use is:
> 	pr_info("Frobbing node %s\n", node->full_name);
> 
> Which can be written now as:
> 	pr_info("Frobbing node %pO\n", node);

Still disliking use of %p0.

> More fine-grained control of formatting includes printing the name,
> flag, path-spec name, reference count and others, explained in the
> documentation entry.
> 
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> 
> dt-print
> ---
>  Documentation/printk-formats.txt |  29 ++++++++
>  lib/vsprintf.c                   | 151 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 180 insertions(+)
> 
> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
> index 5a615c1..2d42c04 100644
> --- a/Documentation/printk-formats.txt
> +++ b/Documentation/printk-formats.txt
> @@ -231,6 +231,35 @@ struct va_format:
>  	Do not use this feature without some mechanism to verify the
>  	correctness of the format string and va_list arguments.
>  
> +Device tree nodes:
> +
> +	%pO[fnpPcCFr]
> +
> +	For printing device tree nodes. The optional arguments are:
> +            f device node full_name
> +            n device node name
> +            p device node phandle
> +            P device node path spec (name + @unit)
> +            F device node flags
> +            c major compatible string
> +            C full compatible string
> +            r node reference count
> +	Without any arguments prints full_name (same as %pOf)
> +	The separator when using multiple arguments is '|'
> +
> +	Examples:
> +
> +	%pO	/foo/bar@0			- Node full name
> +	%pOf	/foo/bar@0			- Same as above
> +	%pOfp	/foo/bar@0|10			- Node full name + phandle
> +	%pOfcF	/foo/bar@0|foo,device|--P-	- Node full name +
> +	                                          major compatible string +
> +						  node flags
> +							D - dynamic
> +							d - detached
> +							P - Populated
> +							B - Populated bus
> +
>  u64 SHOULD be printed with %llu/%llx:
>  
>  	printk("%llu", u64_var);
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
[]

Add #ifdef back ?

> +static noinline_for_stack
> +char *device_node_string(char *buf, char *end, struct device_node *dn,
> +			 struct printf_spec spec, const char *fmt)
> +{
> +	char tbuf[sizeof("xxxxxxxxxx") + 1];
> +	const char *fmtp, *p;
> +	int len, ret, i, j, pass;
> +	char c;
> +
> +	if (!IS_ENABLED(CONFIG_OF))
> +		return string(buf, end, "(!OF)", spec);

Not very descriptive output, maybe the address would be better.

> +
> +	if ((unsigned long)dn < PAGE_SIZE)
> +		return string(buf, end, "(null)", spec);
> +
> +	/* simple case without anything any more format specifiers */
> +	if (fmt[1] == '\0' || isspace(fmt[1]))
> +		fmt = "Of";

why lower case here but upper case above?

> +
> +	len = 0;
> +
> +	/* two passes; the first calculates length, the second fills in */
> +	for (pass = 1; pass <= 2; pass++) {
> +		if (pass == 2 && !(spec.flags & LEFT)) {
> +			/* padding */
> +			while (len < spec.field_width--) {
> +				if (buf < end)
> +					*buf = ' ';
> +				++buf;
> +			}
> +		}
> +#undef _HANDLE_CH
> +#define _HANDLE_CH(_ch) \
> +	do { \
> +		if (pass == 1) \
> +			len++; \
> +		else \
> +			if (buf < end) \
> +				*buf++ = (_ch); \
> +	} while (0)
> +#undef _HANDLE_STR
> +#define _HANDLE_STR(_str) \
> +	do { \
> +		const char *str = (_str); \
> +		if (pass == 1) \
> +			len += strlen(str); \
> +		else \
> +			while (*str && buf < end) \
> +				*buf++ = *str++; \
> +	} while (0)

This isn't pretty.  Perhaps there's a better way?

> +
> +		for (fmtp = fmt + 1, j = 0; (c = *fmtp++) != '\0'; ) {
> +			switch (c) {
> +			case 'f':	/* full_name */
> +				if (j++ > 0)
> +					_HANDLE_CH(':');
> +				_HANDLE_STR(of_node_full_name(dn));
> +				break;
> +			case 'n':	/* name */
> +				if (j++ > 0)
> +					_HANDLE_CH('|');
> +				_HANDLE_STR(dn->name);
> +				break;
> +			case 'p':	/* phandle */
> +				if (j++ > 0)
> +					_HANDLE_CH('|');
> +				snprintf(tbuf, sizeof(tbuf), "%u",
> +						(unsigned int)dn->phandle);
> +				_HANDLE_STR(tbuf);
> +				break;
> +			case 'P':	/* path-spec */
> +				if (j++ > 0)
> +					_HANDLE_CH('|');
> +				_HANDLE_STR(dn->name);
> +				/* need to tack on the @ postfix */
> +				p = strchr(of_node_full_name(dn), '@');
> +				if (p)
> +					_HANDLE_STR(p);
> +				break;
> +			case 'F':	/* flags */
> +				if (j++ > 0)
> +					_HANDLE_CH('|');
> +				snprintf(tbuf, sizeof(tbuf), "%c%c%c%c",
> +					of_node_check_flag(dn, OF_DYNAMIC) ?
> +						'D' : '-',
> +					of_node_check_flag(dn, OF_DETACHED) ?
> +						'd' : '-',
> +					of_node_check_flag(dn, OF_POPULATED) ?
> +						'P' : '-',
> +					of_node_check_flag(dn,
> +						OF_POPULATED_BUS) ?  'B' : '-');
> +				_HANDLE_STR(tbuf);
> +				break;
> +			case 'c':	/* major compatible string */
> +				if (j++ > 0)
> +					_HANDLE_CH('|');
> +				ret = of_property_read_string(dn, "compatible",
> +						&p);
> +				if (ret == 0)
> +					_HANDLE_STR(p);
> +				break;
> +			case 'C':	/* full compatible string */
> +				if (j++ > 0)
> +					_HANDLE_CH('|');
> +				i = 0;
> +				while (of_property_read_string_index(dn,
> +						"compatible", i, &p) == 0) {
> +					if (i == 0)
> +						_HANDLE_STR("\"");
> +					else
> +						_HANDLE_STR("\",\"");
> +					_HANDLE_STR(p);
> +					i++;
> +				}
> +				if (i > 0)
> +					_HANDLE_STR("\"");
> +				break;
> +			case 'r':	/* node reference count */
> +				if (j++ > 0)
> +					_HANDLE_CH('|');
> +				snprintf(tbuf, sizeof(tbuf), "%u",
> +					atomic_read(&dn->kobj.kref.refcount));
> +				_HANDLE_STR(tbuf);
> +				break;
> +			default:
> +				break;
> +			}
> +		}
> +	}
> +	/* finish up */
> +	while (buf < end && len < spec.field_width--)
> +		*buf++ = ' ';
> +#undef _HANDLE_CH
> +#undef _HANDLE_STR

Re: [PATCH] of: Custom printk format specifier for device node

[Pantelis Antoniou]

Hi Joe,

> On Jan 21, 2015, at 19:37 , Joe Perches <joe@perches.com> wrote:
> 
> On Wed, 2015-01-21 at 19:06 +0200, Pantelis Antoniou wrote:
>> 90% of the usage of device node's full_name is printing it out
>> in a kernel message. Preparing for the eventual delayed allocation
>> introduce a custom printk format specifier that is both more
>> compact and more pleasant to the eye.
>> 
>> For instance typical use is:
>> 	pr_info("Frobbing node %s\n", node->full_name);
>> 
>> Which can be written now as:
>> 	pr_info("Frobbing node %pO\n", node);
> 
> Still disliking use of %p0.
> 

Choices are limited. And it’s pO not p0.

>> More fine-grained control of formatting includes printing the name,
>> flag, path-spec name, reference count and others, explained in the
>> documentation entry.
>> 
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>> 
>> dt-print
>> ---
>> Documentation/printk-formats.txt |  29 ++++++++
>> lib/vsprintf.c                   | 151 +++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 180 insertions(+)
>> 
>> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
>> index 5a615c1..2d42c04 100644
>> --- a/Documentation/printk-formats.txt
>> +++ b/Documentation/printk-formats.txt
>> @@ -231,6 +231,35 @@ struct va_format:
>> 	Do not use this feature without some mechanism to verify the
>> 	correctness of the format string and va_list arguments.
>> 
>> +Device tree nodes:
>> +
>> +	%pO[fnpPcCFr]
>> +
>> +	For printing device tree nodes. The optional arguments are:
>> +            f device node full_name
>> +            n device node name
>> +            p device node phandle
>> +            P device node path spec (name + @unit)
>> +            F device node flags
>> +            c major compatible string
>> +            C full compatible string
>> +            r node reference count
>> +	Without any arguments prints full_name (same as %pOf)
>> +	The separator when using multiple arguments is '|'
>> +
>> +	Examples:
>> +
>> +	%pO	/foo/bar@0			- Node full name
>> +	%pOf	/foo/bar@0			- Same as above
>> +	%pOfp	/foo/bar@0|10			- Node full name + phandle
>> +	%pOfcF	/foo/bar@0|foo,device|--P-	- Node full name +
>> +	                                          major compatible string +
>> +						  node flags
>> +							D - dynamic
>> +							d - detached
>> +							P - Populated
>> +							B - Populated bus
>> +
>> u64 SHOULD be printed with %llu/%llx:
>> 
>> 	printk("%llu", u64_var);
>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> []
> 
> Add #ifdef back ?
> 
>> +static noinline_for_stack
>> +char *device_node_string(char *buf, char *end, struct device_node *dn,
>> +			 struct printf_spec spec, const char *fmt)
>> +{
>> +	char tbuf[sizeof("xxxxxxxxxx") + 1];
>> +	const char *fmtp, *p;
>> +	int len, ret, i, j, pass;
>> +	char c;
>> +
>> +	if (!IS_ENABLED(CONFIG_OF))
>> +		return string(buf, end, "(!OF)", spec);
> 
> Not very descriptive output, maybe the address would be better.
> 
>> +
>> +	if ((unsigned long)dn < PAGE_SIZE)
>> +		return string(buf, end, "(null)", spec);
>> +
>> +	/* simple case without anything any more format specifiers */
>> +	if (fmt[1] == '\0' || isspace(fmt[1]))
>> +		fmt = "Of";
> 
> why lower case here but upper case above?
> 
>> +
>> +	len = 0;
>> +
>> +	/* two passes; the first calculates length, the second fills in */
>> +	for (pass = 1; pass <= 2; pass++) {
>> +		if (pass == 2 && !(spec.flags & LEFT)) {
>> +			/* padding */
>> +			while (len < spec.field_width--) {
>> +				if (buf < end)
>> +					*buf = ' ';
>> +				++buf;
>> +			}
>> +		}
>> +#undef _HANDLE_CH
>> +#define _HANDLE_CH(_ch) \
>> +	do { \
>> +		if (pass == 1) \
>> +			len++; \
>> +		else \
>> +			if (buf < end) \
>> +				*buf++ = (_ch); \
>> +	} while (0)
>> +#undef _HANDLE_STR
>> +#define _HANDLE_STR(_str) \
>> +	do { \
>> +		const char *str = (_str); \
>> +		if (pass == 1) \
>> +			len += strlen(str); \
>> +		else \
>> +			while (*str && buf < end) \
>> +				*buf++ = *str++; \
>> +	} while (0)
> 
> This isn't pretty.  Perhaps there's a better way?
> 

No there isn’t.

>> +
>> +		for (fmtp = fmt + 1, j = 0; (c = *fmtp++) != '\0'; ) {
>> +			switch (c) {
>> +			case 'f':	/* full_name */
>> +				if (j++ > 0)
>> +					_HANDLE_CH(':');
>> +				_HANDLE_STR(of_node_full_name(dn));
>> +				break;
>> +			case 'n':	/* name */
>> +				if (j++ > 0)
>> +					_HANDLE_CH('|');
>> +				_HANDLE_STR(dn->name);
>> +				break;
>> +			case 'p':	/* phandle */
>> +				if (j++ > 0)
>> +					_HANDLE_CH('|');
>> +				snprintf(tbuf, sizeof(tbuf), "%u",
>> +						(unsigned int)dn->phandle);
>> +				_HANDLE_STR(tbuf);
>> +				break;
>> +			case 'P':	/* path-spec */
>> +				if (j++ > 0)
>> +					_HANDLE_CH('|');
>> +				_HANDLE_STR(dn->name);
>> +				/* need to tack on the @ postfix */
>> +				p = strchr(of_node_full_name(dn), '@');
>> +				if (p)
>> +					_HANDLE_STR(p);
>> +				break;
>> +			case 'F':	/* flags */
>> +				if (j++ > 0)
>> +					_HANDLE_CH('|');
>> +				snprintf(tbuf, sizeof(tbuf), "%c%c%c%c",
>> +					of_node_check_flag(dn, OF_DYNAMIC) ?
>> +						'D' : '-',
>> +					of_node_check_flag(dn, OF_DETACHED) ?
>> +						'd' : '-',
>> +					of_node_check_flag(dn, OF_POPULATED) ?
>> +						'P' : '-',
>> +					of_node_check_flag(dn,
>> +						OF_POPULATED_BUS) ?  'B' : '-');
>> +				_HANDLE_STR(tbuf);
>> +				break;
>> +			case 'c':	/* major compatible string */
>> +				if (j++ > 0)
>> +					_HANDLE_CH('|');
>> +				ret = of_property_read_string(dn, "compatible",
>> +						&p);
>> +				if (ret == 0)
>> +					_HANDLE_STR(p);
>> +				break;
>> +			case 'C':	/* full compatible string */
>> +				if (j++ > 0)
>> +					_HANDLE_CH('|');
>> +				i = 0;
>> +				while (of_property_read_string_index(dn,
>> +						"compatible", i, &p) == 0) {
>> +					if (i == 0)
>> +						_HANDLE_STR("\"");
>> +					else
>> +						_HANDLE_STR("\",\"");
>> +					_HANDLE_STR(p);
>> +					i++;
>> +				}
>> +				if (i > 0)
>> +					_HANDLE_STR("\"");
>> +				break;
>> +			case 'r':	/* node reference count */
>> +				if (j++ > 0)
>> +					_HANDLE_CH('|');
>> +				snprintf(tbuf, sizeof(tbuf), "%u",
>> +					atomic_read(&dn->kobj.kref.refcount));
>> +				_HANDLE_STR(tbuf);
>> +				break;
>> +			default:
>> +				break;
>> +			}
>> +		}
>> +	}
>> +	/* finish up */
>> +	while (buf < end && len < spec.field_width--)
>> +		*buf++ = ' ';
>> +#undef _HANDLE_CH
>> +#undef _HANDLE_STR

> 

Regards

— Pantelis

Re: [PATCH] of: Custom printk format specifier for device node

[Joe Perches]

On Wed, 2015-01-21 at 19:39 +0200, Pantelis Antoniou wrote:
> Hi Joe,
> 
> > On Jan 21, 2015, at 19:37 , Joe Perches <joe@perches.com> wrote:
> > 
> > On Wed, 2015-01-21 at 19:06 +0200, Pantelis Antoniou wrote:
> >> 90% of the usage of device node's full_name is printing it out
> >> in a kernel message. Preparing for the eventual delayed allocation
> >> introduce a custom printk format specifier that is both more
> >> compact and more pleasant to the eye.
> >> 
> >> For instance typical use is:
> >> 	pr_info("Frobbing node %s\n", node->full_name);
> >> 
> >> Which can be written now as:
> >> 	pr_info("Frobbing node %pO\n", node);
> > 
> > Still disliking use of %p0.
> > 
> 
> Choices are limited. And it’s pO not p0.

Yet another reason to dislike it.

> > This isn't pretty.  Perhaps there's a better way?
> > 
> 
> No there isn’t.

There always is.

Re: [PATCH] of: Custom printk format specifier for device node

[Måns Rullgård]

Pantelis Antoniou <panto@antoniou-consulting.com> writes:

> Hi Joe,
>
>> On Jan 21, 2015, at 19:37 , Joe Perches <joe@perches.com> wrote:
>> 
>> On Wed, 2015-01-21 at 19:06 +0200, Pantelis Antoniou wrote:
>>> 90% of the usage of device node's full_name is printing it out
>>> in a kernel message. Preparing for the eventual delayed allocation
>>> introduce a custom printk format specifier that is both more
>>> compact and more pleasant to the eye.
>>> 
>>> For instance typical use is:
>>> 	pr_info("Frobbing node %s\n", node->full_name);
>>> 
>>> Which can be written now as:
>>> 	pr_info("Frobbing node %pO\n", node);
>> 
>> Still disliking use of %p0.
>> 
>
> Choices are limited. And it’s pO not p0.

O as in OF.  Makes sense.

-- 
Måns Rullgård
mans@mansr.com

Re: [PATCH] of: Custom printk format specifier for device node

[Måns Rullgård]

Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org> writes:

> Hi Joe,
>
>> On Jan 21, 2015, at 19:37 , Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
>> 
>> On Wed, 2015-01-21 at 19:06 +0200, Pantelis Antoniou wrote:
>>> 90% of the usage of device node's full_name is printing it out
>>> in a kernel message. Preparing for the eventual delayed allocation
>>> introduce a custom printk format specifier that is both more
>>> compact and more pleasant to the eye.
>>> 
>>> For instance typical use is:
>>> 	pr_info("Frobbing node %s\n", node->full_name);
>>> 
>>> Which can be written now as:
>>> 	pr_info("Frobbing node %pO\n", node);
>> 
>> Still disliking use of %p0.
>> 
>
> Choices are limited. And it’s pO not p0.

O as in OF.  Makes sense.

-- 
Måns Rullgård
mans-2StjZFpD7GcAvxtiuMwx3w@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] of: Custom printk format specifier for device node

[Pantelis Antoniou]

Hi Joe,

> On Jan 21, 2015, at 19:37 , Joe Perches <joe@perches.com> wrote:
> 
> On Wed, 2015-01-21 at 19:06 +0200, Pantelis Antoniou wrote:
>> 90% of the usage of device node's full_name is printing it out
>> in a kernel message. Preparing for the eventual delayed allocation
>> introduce a custom printk format specifier that is both more
>> compact and more pleasant to the eye.
>> 
>> For instance typical use is:
>> 	pr_info("Frobbing node %s\n", node->full_name);
>> 
>> Which can be written now as:
>> 	pr_info("Frobbing node %pO\n", node);

> Still disliking use of %p0.
> 

pO - Open Firmware

pT for tree is bad, cause we plan to use a tree type in the future in OF.

>> More fine-grained control of formatting includes printing the name,
>> flag, path-spec name, reference count and others, explained in the
>> documentation entry.
>> 
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>> 
>> dt-print
>> ---
>> Documentation/printk-formats.txt |  29 ++++++++
>> lib/vsprintf.c                   | 151 +++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 180 insertions(+)
>> 
>> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
>> index 5a615c1..2d42c04 100644
>> --- a/Documentation/printk-formats.txt
>> +++ b/Documentation/printk-formats.txt
>> @@ -231,6 +231,35 @@ struct va_format:
>> 	Do not use this feature without some mechanism to verify the
>> 	correctness of the format string and va_list arguments.
>> 
>> +Device tree nodes:
>> +
>> +	%pO[fnpPcCFr]
>> +
>> +	For printing device tree nodes. The optional arguments are:
>> +            f device node full_name
>> +            n device node name
>> +            p device node phandle
>> +            P device node path spec (name + @unit)
>> +            F device node flags
>> +            c major compatible string
>> +            C full compatible string
>> +            r node reference count
>> +	Without any arguments prints full_name (same as %pOf)
>> +	The separator when using multiple arguments is '|'
>> +
>> +	Examples:
>> +
>> +	%pO	/foo/bar@0			- Node full name
>> +	%pOf	/foo/bar@0			- Same as above
>> +	%pOfp	/foo/bar@0|10			- Node full name + phandle
>> +	%pOfcF	/foo/bar@0|foo,device|--P-	- Node full name +
>> +	                                          major compatible string +
>> +						  node flags
>> +							D - dynamic
>> +							d - detached
>> +							P - Populated
>> +							B - Populated bus
>> +
>> u64 SHOULD be printed with %llu/%llx:
>> 
>> 	printk("%llu", u64_var);
>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> []
> 
> Add #ifdef back ?
> 

The whole thing is optimized away when CONFIG_OF is not defined, leaving only
the return statement.

>> +static noinline_for_stack
>> +char *device_node_string(char *buf, char *end, struct device_node *dn,
>> +			 struct printf_spec spec, const char *fmt)
>> +{
>> +	char tbuf[sizeof("xxxxxxxxxx") + 1];
>> +	const char *fmtp, *p;
>> +	int len, ret, i, j, pass;
>> +	char c;
>> +
>> +	if (!IS_ENABLED(CONFIG_OF))
>> +		return string(buf, end, "(!OF)", spec);
> 
> Not very descriptive output, maybe the address would be better.
> 

OK

>> +
>> +	if ((unsigned long)dn < PAGE_SIZE)
>> +		return string(buf, end, "(null)", spec);
>> +
>> +	/* simple case without anything any more format specifiers */
>> +	if (fmt[1] == '\0' || isspace(fmt[1]))
>> +		fmt = "Of";
> 
> why lower case here but upper case above?
> 

Cause '(null)' is what’s printed in string() when null is passed as a pointer.

>> +
>> +	len = 0;
>> +
>> +	/* two passes; the first calculates length, the second fills in */
>> +	for (pass = 1; pass <= 2; pass++) {
>> +		if (pass == 2 && !(spec.flags & LEFT)) {
>> +			/* padding */
>> +			while (len < spec.field_width--) {
>> +				if (buf < end)
>> +					*buf = ' ';
>> +				++buf;
>> +			}
>> +		}
>> +#undef _HANDLE_CH
>> +#define _HANDLE_CH(_ch) \
>> +	do { \
>> +		if (pass == 1) \
>> +			len++; \
>> +		else \
>> +			if (buf < end) \
>> +				*buf++ = (_ch); \
>> +	} while (0)
>> +#undef _HANDLE_STR
>> +#define _HANDLE_STR(_str) \
>> +	do { \
>> +		const char *str = (_str); \
>> +		if (pass == 1) \
>> +			len += strlen(str); \
>> +		else \
>> +			while (*str && buf < end) \
>> +				*buf++ = *str++; \
>> +	} while (0)
> 
> This isn't pretty.  Perhaps there's a better way?
> 

It’s the simplest way to do the different operations for the two passes, without
bloating the code or adding superfluous methods.

We don’t want to allocate memory, we don’t want to use stack space. We’re probably
printing in atomic context too since device nodes don’t usually printed out
during normal operation.
  
>> +
>> +		for (fmtp = fmt + 1, j = 0; (c = *fmtp++) != '\0'; ) {
>> +			switch (c) {
>> +			case 'f':	/* full_name */
>> +				if (j++ > 0)
>> +					_HANDLE_CH(':');
>> +				_HANDLE_STR(of_node_full_name(dn));
>> +				break;
>> +			case 'n':	/* name */
>> +				if (j++ > 0)
>> +					_HANDLE_CH('|');
>> +				_HANDLE_STR(dn->name);
>> +				break;
>> +			case 'p':	/* phandle */
>> +				if (j++ > 0)
>> +					_HANDLE_CH('|');
>> +				snprintf(tbuf, sizeof(tbuf), "%u",
>> +						(unsigned int)dn->phandle);
>> +				_HANDLE_STR(tbuf);
>> +				break;
>> +			case 'P':	/* path-spec */
>> +				if (j++ > 0)
>> +					_HANDLE_CH('|');
>> +				_HANDLE_STR(dn->name);
>> +				/* need to tack on the @ postfix */
>> +				p = strchr(of_node_full_name(dn), '@');
>> +				if (p)
>> +					_HANDLE_STR(p);
>> +				break;
>> +			case 'F':	/* flags */
>> +				if (j++ > 0)
>> +					_HANDLE_CH('|');
>> +				snprintf(tbuf, sizeof(tbuf), "%c%c%c%c",
>> +					of_node_check_flag(dn, OF_DYNAMIC) ?
>> +						'D' : '-',
>> +					of_node_check_flag(dn, OF_DETACHED) ?
>> +						'd' : '-',
>> +					of_node_check_flag(dn, OF_POPULATED) ?
>> +						'P' : '-',
>> +					of_node_check_flag(dn,
>> +						OF_POPULATED_BUS) ?  'B' : '-');
>> +				_HANDLE_STR(tbuf);
>> +				break;
>> +			case 'c':	/* major compatible string */
>> +				if (j++ > 0)
>> +					_HANDLE_CH('|');
>> +				ret = of_property_read_string(dn, "compatible",
>> +						&p);
>> +				if (ret == 0)
>> +					_HANDLE_STR(p);
>> +				break;
>> +			case 'C':	/* full compatible string */
>> +				if (j++ > 0)
>> +					_HANDLE_CH('|');
>> +				i = 0;
>> +				while (of_property_read_string_index(dn,
>> +						"compatible", i, &p) == 0) {
>> +					if (i == 0)
>> +						_HANDLE_STR("\"");
>> +					else
>> +						_HANDLE_STR("\",\"");
>> +					_HANDLE_STR(p);
>> +					i++;
>> +				}
>> +				if (i > 0)
>> +					_HANDLE_STR("\"");
>> +				break;
>> +			case 'r':	/* node reference count */
>> +				if (j++ > 0)
>> +					_HANDLE_CH('|');
>> +				snprintf(tbuf, sizeof(tbuf), "%u",
>> +					atomic_read(&dn->kobj.kref.refcount));
>> +				_HANDLE_STR(tbuf);
>> +				break;
>> +			default:
>> +				break;
>> +			}
>> +		}
>> +	}
>> +	/* finish up */
>> +	while (buf < end && len < spec.field_width--)
>> +		*buf++ = ' ';
>> +#undef _HANDLE_CH
>> +#undef _HANDLE_STR
> 

Regards

— Pantelis

Re: [PATCH] of: Custom printk format specifier for device node

[Grant Likely]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 16443 bytes --]

On Thu, 22 Jan 2015 22:31:46 +0200
, Pantelis Antoniou <panto@antoniou-consulting.com>
 wrote:
> Hi Joe,
> 
> > On Jan 21, 2015, at 19:37 , Joe Perches <joe@perches.com> wrote:
> > 
> > On Wed, 2015-01-21 at 19:06 +0200, Pantelis Antoniou wrote:
> >> 90% of the usage of device node's full_name is printing it out
> >> in a kernel message. Preparing for the eventual delayed allocation
> >> introduce a custom printk format specifier that is both more
> >> compact and more pleasant to the eye.
> >> 
> >> For instance typical use is:
> >> 	pr_info("Frobbing node %s\n", node->full_name);
> >> 
> >> Which can be written now as:
> >> 	pr_info("Frobbing node %pO\n", node);
> 
> > Still disliking use of %p0.
> > 
> 
> pO - Open Firmware
> 
> pT for tree is bad, cause we plan to use a tree type in the future in OF.

So, here's a radical thought. How about we reserve '%pO' for objects, as
in kobjects.  We'll use extra flags to narrow down specifically to
device tree nodes, but we could teach vsprintf() to treat a plain '%pO'
as plain kobject pointer, and if it is able to recognize the kobj_type,
then run a specific decoder to format it.

This also gives us a namespace for various kinds of firmware data
output. %Od could be a struct device, %On for device tree node, %Oa for
an ACPI node, etc.

> 
> >> More fine-grained control of formatting includes printing the name,
> >> flag, path-spec name, reference count and others, explained in the
> >> documentation entry.
> >> 
> >> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> >> 
> >> dt-print
> >> ---
> >> Documentation/printk-formats.txt |  29 ++++++++
> >> lib/vsprintf.c                   | 151 +++++++++++++++++++++++++++++++++++++++
> >> 2 files changed, 180 insertions(+)
> >> 
> >> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
> >> index 5a615c1..2d42c04 100644
> >> --- a/Documentation/printk-formats.txt
> >> +++ b/Documentation/printk-formats.txt
> >> @@ -231,6 +231,35 @@ struct va_format:
> >> 	Do not use this feature without some mechanism to verify the
> >> 	correctness of the format string and va_list arguments.
> >> 
> >> +Device tree nodes:
> >> +
> >> +	%pO[fnpPcCFr]
> >> +
> >> +	For printing device tree nodes. The optional arguments are:
> >> +            f device node full_name
> >> +            n device node name
> >> +            p device node phandle
> >> +            P device node path spec (name + @unit)
> >> +            F device node flags
> >> +            c major compatible string
> >> +            C full compatible string
> >> +            r node reference count
> >> +	Without any arguments prints full_name (same as %pOf)
> >> +	The separator when using multiple arguments is '|'
> >> +
> >> +	Examples:
> >> +
> >> +	%pO	/foo/bar@0			- Node full name
> >> +	%pOf	/foo/bar@0			- Same as above
> >> +	%pOfp	/foo/bar@0|10			- Node full name + phandle
> >> +	%pOfcF	/foo/bar@0|foo,device|--P-	- Node full name +
> >> +	                                          major compatible string +
> >> +						  node flags
> >> +							D - dynamic
> >> +							d - detached
> >> +							P - Populated
> >> +							B - Populated bus
> >> +
> >> u64 SHOULD be printed with %llu/%llx:
> >> 
> >> 	printk("%llu", u64_var);
> >> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > []
> > 
> > Add #ifdef back ?
> > 
> 
> The whole thing is optimized away when CONFIG_OF is not defined, leaving only
> the return statement.
> 
> >> +static noinline_for_stack
> >> +char *device_node_string(char *buf, char *end, struct device_node *dn,
> >> +			 struct printf_spec spec, const char *fmt)
> >> +{
> >> +	char tbuf[sizeof("xxxxxxxxxx") + 1];
> >> +	const char *fmtp, *p;
> >> +	int len, ret, i, j, pass;
> >> +	char c;
> >> +
> >> +	if (!IS_ENABLED(CONFIG_OF))
> >> +		return string(buf, end, "(!OF)", spec);
> > 
> > Not very descriptive output, maybe the address would be better.
> > 
> 
> OK
> 
> >> +
> >> +	if ((unsigned long)dn < PAGE_SIZE)
> >> +		return string(buf, end, "(null)", spec);
> >> +
> >> +	/* simple case without anything any more format specifiers */
> >> +	if (fmt[1] == '\0' || isspace(fmt[1]))
> >> +		fmt = "Of";

s/isspace()/!isalnum() to match with what the pointer() function does
when finding the end of a format string.

> > 
> > why lower case here but upper case above?
> > 
> 
> Cause '(null)' is what’s printed in string() when null is passed as a pointer.
> 
> >> +
> >> +	len = 0;
> >> +
> >> +	/* two passes; the first calculates length, the second fills in */
> >> +	for (pass = 1; pass <= 2; pass++) {
> >> +		if (pass == 2 && !(spec.flags & LEFT)) {
> >> +			/* padding */
> >> +			while (len < spec.field_width--) {
> >> +				if (buf < end)
> >> +					*buf = ' ';
> >> +				++buf;
> >> +			}
> >> +		}
> >> +#undef _HANDLE_CH
> >> +#define _HANDLE_CH(_ch) \
> >> +	do { \
> >> +		if (pass == 1) \
> >> +			len++; \
> >> +		else \
> >> +			if (buf < end) \
> >> +				*buf++ = (_ch); \
> >> +	} while (0)
> >> +#undef _HANDLE_STR
> >> +#define _HANDLE_STR(_str) \
> >> +	do { \
> >> +		const char *str = (_str); \
> >> +		if (pass == 1) \
> >> +			len += strlen(str); \
> >> +		else \
> >> +			while (*str && buf < end) \
> >> +				*buf++ = *str++; \
> >> +	} while (0)
> > 
> > This isn't pretty.  Perhaps there's a better way?
> > 
> 
> It’s the simplest way to do the different operations for the two passes, without
> bloating the code or adding superfluous methods.
> 
> We don’t want to allocate memory, we don’t want to use stack space. We’re probably
> printing in atomic context too since device nodes don’t usually printed out
> during normal operation.

As far as I can tell from the code, the only thing that 2 passes is used
for is when the LEFT spec.flags value is not set. Instead of doing two,
what if the code generated the string right-aligned, and then if the
LEFT flag is set, shift it over the required amount, ' ' padding as
necessary? In fact, that's exactly what the dentry_name() helper does.

This is a complicated enough block of code, that I'd like to see
unittests for it added at the same time.

...

So, I'm keen enough on this feature that I've just gone ahead and played
with it this morning. The following is what I've come up with. I got rid
of the two passes, fixed up the boundary conditions, and added
unittests. I've also switched the format key to %pOn, and the separator
to ':'. ':' is never a valid character in a node name, so it should be
safe to use as a delimiter.

I've dropped the refcount decoder. I know it is useful for debugging the
core DT code, but it isn't something that will be used generally. Plus
the returned value cannot be relied upon to be stable because there may
be other code currently iterating over the tree.

---

of: Custom printk format specifier for device node

90% of the usage of device node's full_name is printing it out
in a kernel message. Preparing for the eventual delayed allocation
introduce a custom printk format specifier that is both more
compact and more pleasant to the eye.

For instance typical use is:
	pr_info("Frobbing node %s\n", node->full_name);

Which can be written now as:
	pr_info("Frobbing node %pOn\n", node);

More fine-grained control of formatting includes printing the name,
flag, path-spec name, reference count and others, explained in the
documentation entry.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
[grant.likely: Rework to be simpler]
Signed-off-by: Grant Likely <grant.likely@linaro.org>
---
 Documentation/printk-formats.txt             |  28 ++++++++
 drivers/of/unittest-data/tests-platform.dtsi |   4 +-
 drivers/of/unittest.c                        |  58 +++++++++++++++
 lib/vsprintf.c                               | 101 +++++++++++++++++++++++++++
 4 files changed, 190 insertions(+), 1 deletion(-)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 5a615c14f75d..f0dc2fd229e4 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -192,6 +192,34 @@ IPv4/IPv6 addresses (generic, with port, flowinfo, scope):
 	%pISsc		1.2.3.4		or [1:2:3:4:5:6:7:8]%1234567890
 	%pISpfc		1.2.3.4:12345	or [1:2:3:4:5:6:7:8]:12345/123456789
 
+Device tree nodes:
+
+	%pOn[fnpPcCFr]
+
+	For printing device tree nodes. The optional arguments are:
+	    f device node full_name
+	    n device node name
+	    p device node phandle
+	    P device node path spec (name + @unit)
+	    F device node flags
+	    c major compatible string
+	    C full compatible string
+	Without any arguments prints full_name (same as %pOnf)
+	The separator when using multiple arguments is ':'
+
+	Examples:
+
+	%pOn	/foo/bar@0			- Node full name
+	%pOnf	/foo/bar@0			- Same as above
+	%pOnfp	/foo/bar@0:10			- Node full name + phandle
+	%pOnfcF	/foo/bar@0:foo,device:--P-	- Node full name +
+	                                          major compatible string +
+						  node flags
+							D - dynamic
+							d - detached
+							P - Populated
+							B - Populated bus
+
 UUID/GUID addresses:
 
 	%pUb	00010203-0405-0607-0809-0a0b0c0d0e0f
diff --git a/drivers/of/unittest-data/tests-platform.dtsi b/drivers/of/unittest-data/tests-platform.dtsi
index eb20eeb2b062..a0c93822aee3 100644
--- a/drivers/of/unittest-data/tests-platform.dtsi
+++ b/drivers/of/unittest-data/tests-platform.dtsi
@@ -26,7 +26,9 @@
 				#size-cells = <0>;
 
 				dev@100 {
-					compatible = "test-sub-device";
+					compatible = "test-sub-device",
+						     "test-compat2",
+						     "test-compat3";
 					reg = <0x100>;
 				};
 			};
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index e844907c9efa..dc43209f2064 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -234,6 +234,63 @@ static void __init of_unittest_check_tree_linkage(void)
 	pr_debug("allnodes list size (%i); sibling lists size (%i)\n", allnode_count, child_count);
 }
 
+static void __init of_unittest_printf_one(struct device_node *np, const char *fmt,
+					  const char *expected)
+{
+	char buf[strlen(expected)+10];
+	int size, i;
+
+	/* Baseline; check conversion with a large size limit */
+	memset(buf, 0xff, sizeof(buf));
+	size = snprintf(buf, sizeof(buf) - 2, fmt, np);
+
+	/* use strcmp() instead of strncmp() here to be absolutely sure strings match */
+	unittest((strcmp(buf, expected) == 0) && (buf[size+1] == 0xff),
+		"sprintf failed; fmt='%s' expected='%s' rslt='%s'\n",
+		fmt, expected, buf);
+
+	/* Make sure length limits work */
+	size++;
+	for (i = 0; i < 2; i++, size--) {
+		/* Clear the buffer, and make sure it works correctly still */
+		memset(buf, 0xff, sizeof(buf));
+		snprintf(buf, size+1, fmt, np);
+		unittest(strncmp(buf, expected, size) == 0 && (buf[size+1] == 0xff),
+			"snprintf failed; size=%i fmt='%s' expected='%s' rslt='%s'\n",
+			size, fmt, expected, buf);
+	}
+}
+
+static void __init of_unittest_printf(void)
+{
+	struct device_node *np;
+	const char *full_name = "/testcase-data/platform-tests/test-device@1/dev@100";
+	char phandle_str[16] = "";
+
+	np = of_find_node_by_path(full_name);
+	if (!np) {
+		unittest(np, "testcase data missing\n");
+		return;
+	}
+
+	num_to_str(phandle_str, sizeof(phandle_str), np->phandle);
+
+	of_unittest_printf_one(np, "%pOn",  full_name);
+	of_unittest_printf_one(np, "%pOnf", full_name);
+	of_unittest_printf_one(np, "%pOnp", phandle_str);
+	of_unittest_printf_one(np, "%pOnP", "dev@100");
+	of_unittest_printf_one(np, "ABC %pOnP ABC", "ABC dev@100 ABC");
+	of_unittest_printf_one(np, "%10pOnP", "   dev@100");
+	of_unittest_printf_one(np, "%-10pOnP", "dev@100   ");
+	of_unittest_printf_one(of_root, "%pOnP", "/");
+	of_unittest_printf_one(np, "%pOnF", "----");
+	of_unittest_printf_one(np, "%pOnPF", "dev@100:----");
+	of_unittest_printf_one(np, "%pOnPFPc", "dev@100:----:dev@100:test-sub-device");
+	of_unittest_printf_one(np, "%pOnc", "test-sub-device");
+	of_unittest_printf_one(np, "%pOnC",
+			"\"test-sub-device\",\"test-compat2\",\"test-compat3\"");
+}
+
 struct node_hash {
 	struct hlist_node node;
 	struct device_node *np;
@@ -1888,6 +1945,7 @@ static int __init of_unittest(void)
 	of_unittest_find_node_by_name();
 	of_unittest_dynamic();
 	of_unittest_parse_phandle_with_args();
+	of_unittest_printf();
 	of_unittest_property_string();
 	of_unittest_property_copy();
 	of_unittest_changeset();
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index b235c96167d3..8a793a4560c2 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -29,6 +29,7 @@
 #include <linux/dcache.h>
 #include <linux/cred.h>
 #include <net/addrconf.h>
+#include <linux/of.h>
 
 #include <asm/page.h>		/* for PAGE_SIZE */
 #include <asm/sections.h>	/* for dereference_function_descriptor() */
@@ -1322,6 +1323,91 @@ char *address_val(char *buf, char *end, const void *addr,
 	return number(buf, end, num, spec);
 }
 
+static noinline_for_stack
+char *device_node_string(char *buf, char *end, struct device_node *dn,
+			 struct printf_spec spec, const char *fmt)
+{
+	char tbuf[] = "----";
+	struct property *prop;
+	const char *p;
+	int ret, i, j, n;
+	char c, *start = buf;
+	struct printf_spec str_spec =
+		{ .precision = -1, .field_width = 0, .base = 10, .flags = LEFT };
+
+	if (!IS_ENABLED(CONFIG_OF))
+		return string(buf, end, "(!OF)", spec);
+
+	if ((unsigned long)dn < PAGE_SIZE)
+		return string(buf, end, "(null)", spec);
+
+	/* simple case without anything any more format specifiers */
+	if (!isalnum(fmt[2]))
+		fmt = "Onf";
+
+	fmt += 2;
+	for (j = 0; isalnum((c = *fmt)); j++, fmt++) {
+		if (j)
+			buf = string(buf, end, ":", str_spec);
+
+		switch (c) {
+		case 'f':	/* full_name */
+			buf = string(buf, end, of_node_full_name(dn), str_spec);
+			break;
+		case 'n':	/* name */
+			buf = string(buf, end, dn->name, str_spec);
+			break;
+		case 'p':	/* phandle */
+			buf = number(buf, end, dn->phandle, str_spec);
+			break;
+		case 'P':	/* path-spec */
+			p = strrchr(of_node_full_name(dn), '/');
+			if (p[1]) /* Root node name is just "/" */
+				p++;
+			buf = string(buf, end, p, str_spec);
+			break;
+		case 'F':	/* flags */
+			snprintf(tbuf, sizeof(tbuf), "%c%c%c%c",
+				of_node_check_flag(dn, OF_DYNAMIC) ?  'D' : '-',
+				of_node_check_flag(dn, OF_DETACHED) ?  'd' : '-',
+				of_node_check_flag(dn, OF_POPULATED) ?  'P' : '-',
+				of_node_check_flag(dn, OF_POPULATED_BUS) ?  'B' : '-');
+			buf = string(buf, end, tbuf, str_spec);
+			break;
+		case 'c':	/* first compatible string */
+			ret = of_property_read_string(dn, "compatible", &p);
+			if (ret == 0)
+				buf = string(buf, end, p, str_spec);
+			break;
+		case 'C':	/* full compatible string list */
+			i = 0;
+			of_property_for_each_string(dn, "compatible", prop, p) {
+				buf = string(buf, end, i ? "\",\"" : "\"", str_spec);
+				buf = string(buf, end, p, str_spec);
+				i++;
+			}
+			buf = string(buf, end, "\"", str_spec);
+			break;
+		}
+	}
+
+	/* Pad out at the front or back if field_width is specified */
+	n = buf - start;
+	if (n < spec.field_width) {
+		unsigned spaces = spec.field_width - n;
+		if (!(spec.flags & LEFT)) {
+			widen(start, end, n, spaces);
+			return buf + spaces;
+		}
+		while (spaces-- && (buf < end)) {
+			if (buf < end)
+				*buf = ' ';
+			++buf;
+		}
+	}
+	return buf;
+}
+
 int kptr_restrict __read_mostly;
 
 /*
@@ -1393,6 +1479,15 @@ 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
+ * - 'On[fnpPcCF]' For a device tree object
+ *                Without any optional arguments prints the full_name
+ *                f device node full_name
+ *                n device node name
+ *                p device node phandle
+ *                P device node path spec (name + @unit)
+ *                F device node flags
+ *                c major compatible string
+ *                C full compatible string
  * - 'h[CDN]' For a variable-length buffer, it prints it as a hex string with
  *            a certain separator (' ' by default):
  *              C colon
@@ -1544,6 +1639,12 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 			return netdev_feature_string(buf, end, ptr, spec);
 		}
 		break;
+	case 'O':
+		switch (fmt[1]) {
+		case 'n':
+			return device_node_string(buf, end, ptr, spec, fmt);
+		}
+		break;
 	case 'a':
 		return address_val(buf, end, ptr, spec, fmt);
 	case 'd':
-- 
2.1.0

Re: [PATCH] of: Custom printk format specifier for device node

[Grant Likely]

On Thu, 22 Jan 2015 22:31:46 +0200
, Pantelis Antoniou <panto@antoniou-consulting.com>
 wrote:
> Hi Joe,
> 
> > On Jan 21, 2015, at 19:37 , Joe Perches <joe@perches.com> wrote:
> > 
> > On Wed, 2015-01-21 at 19:06 +0200, Pantelis Antoniou wrote:
> >> 90% of the usage of device node's full_name is printing it out
> >> in a kernel message. Preparing for the eventual delayed allocation
> >> introduce a custom printk format specifier that is both more
> >> compact and more pleasant to the eye.
> >> 
> >> For instance typical use is:
> >> 	pr_info("Frobbing node %s\n", node->full_name);
> >> 
> >> Which can be written now as:
> >> 	pr_info("Frobbing node %pO\n", node);
> 
> > Still disliking use of %p0.
> > 
> 
> pO - Open Firmware
> 
> pT for tree is bad, cause we plan to use a tree type in the future in OF.

So, here's a radical thought. How about we reserve '%pO' for objects, as
in kobjects.  We'll use extra flags to narrow down specifically to
device tree nodes, but we could teach vsprintf() to treat a plain '%pO'
as plain kobject pointer, and if it is able to recognize the kobj_type,
then run a specific decoder to format it.

This also gives us a namespace for various kinds of firmware data
output. %Od could be a struct device, %On for device tree node, %Oa for
an ACPI node, etc.

> 
> >> More fine-grained control of formatting includes printing the name,
> >> flag, path-spec name, reference count and others, explained in the
> >> documentation entry.
> >> 
> >> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> >> 
> >> dt-print
> >> ---
> >> Documentation/printk-formats.txt |  29 ++++++++
> >> lib/vsprintf.c                   | 151 +++++++++++++++++++++++++++++++++++++++
> >> 2 files changed, 180 insertions(+)
> >> 
> >> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
> >> index 5a615c1..2d42c04 100644
> >> --- a/Documentation/printk-formats.txt
> >> +++ b/Documentation/printk-formats.txt
> >> @@ -231,6 +231,35 @@ struct va_format:
> >> 	Do not use this feature without some mechanism to verify the
> >> 	correctness of the format string and va_list arguments.
> >> 
> >> +Device tree nodes:
> >> +
> >> +	%pO[fnpPcCFr]
> >> +
> >> +	For printing device tree nodes. The optional arguments are:
> >> +            f device node full_name
> >> +            n device node name
> >> +            p device node phandle
> >> +            P device node path spec (name + @unit)
> >> +            F device node flags
> >> +            c major compatible string
> >> +            C full compatible string
> >> +            r node reference count
> >> +	Without any arguments prints full_name (same as %pOf)
> >> +	The separator when using multiple arguments is '|'
> >> +
> >> +	Examples:
> >> +
> >> +	%pO	/foo/bar@0			- Node full name
> >> +	%pOf	/foo/bar@0			- Same as above
> >> +	%pOfp	/foo/bar@0|10			- Node full name + phandle
> >> +	%pOfcF	/foo/bar@0|foo,device|--P-	- Node full name +
> >> +	                                          major compatible string +
> >> +						  node flags
> >> +							D - dynamic
> >> +							d - detached
> >> +							P - Populated
> >> +							B - Populated bus
> >> +
> >> u64 SHOULD be printed with %llu/%llx:
> >> 
> >> 	printk("%llu", u64_var);
> >> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > []
> > 
> > Add #ifdef back ?
> > 
> 
> The whole thing is optimized away when CONFIG_OF is not defined, leaving only
> the return statement.
> 
> >> +static noinline_for_stack
> >> +char *device_node_string(char *buf, char *end, struct device_node *dn,
> >> +			 struct printf_spec spec, const char *fmt)
> >> +{
> >> +	char tbuf[sizeof("xxxxxxxxxx") + 1];
> >> +	const char *fmtp, *p;
> >> +	int len, ret, i, j, pass;
> >> +	char c;
> >> +
> >> +	if (!IS_ENABLED(CONFIG_OF))
> >> +		return string(buf, end, "(!OF)", spec);
> > 
> > Not very descriptive output, maybe the address would be better.
> > 
> 
> OK
> 
> >> +
> >> +	if ((unsigned long)dn < PAGE_SIZE)
> >> +		return string(buf, end, "(null)", spec);
> >> +
> >> +	/* simple case without anything any more format specifiers */
> >> +	if (fmt[1] == '\0' || isspace(fmt[1]))
> >> +		fmt = "Of";

s/isspace()/!isalnum() to match with what the pointer() function does
when finding the end of a format string.

> > 
> > why lower case here but upper case above?
> > 
> 
> Cause '(null)' is what’s printed in string() when null is passed as a pointer.
> 
> >> +
> >> +	len = 0;
> >> +
> >> +	/* two passes; the first calculates length, the second fills in */
> >> +	for (pass = 1; pass <= 2; pass++) {
> >> +		if (pass == 2 && !(spec.flags & LEFT)) {
> >> +			/* padding */
> >> +			while (len < spec.field_width--) {
> >> +				if (buf < end)
> >> +					*buf = ' ';
> >> +				++buf;
> >> +			}
> >> +		}
> >> +#undef _HANDLE_CH
> >> +#define _HANDLE_CH(_ch) \
> >> +	do { \
> >> +		if (pass == 1) \
> >> +			len++; \
> >> +		else \
> >> +			if (buf < end) \
> >> +				*buf++ = (_ch); \
> >> +	} while (0)
> >> +#undef _HANDLE_STR
> >> +#define _HANDLE_STR(_str) \
> >> +	do { \
> >> +		const char *str = (_str); \
> >> +		if (pass == 1) \
> >> +			len += strlen(str); \
> >> +		else \
> >> +			while (*str && buf < end) \
> >> +				*buf++ = *str++; \
> >> +	} while (0)
> > 
> > This isn't pretty.  Perhaps there's a better way?
> > 
> 
> It’s the simplest way to do the different operations for the two passes, without
> bloating the code or adding superfluous methods.
> 
> We don’t want to allocate memory, we don’t want to use stack space. We’re probably
> printing in atomic context too since device nodes don’t usually printed out
> during normal operation.

As far as I can tell from the code, the only thing that 2 passes is used
for is when the LEFT spec.flags value is not set. Instead of doing two,
what if the code generated the string right-aligned, and then if the
LEFT flag is set, shift it over the required amount, ' ' padding as
necessary? In fact, that's exactly what the dentry_name() helper does.

This is a complicated enough block of code, that I'd like to see
unittests for it added at the same time.

...

So, I'm keen enough on this feature that I've just gone ahead and played
with it this morning. The following is what I've come up with. I got rid
of the two passes, fixed up the boundary conditions, and added
unittests. I've also switched the format key to %pOn, and the separator
to ':'. ':' is never a valid character in a node name, so it should be
safe to use as a delimiter.

I've dropped the refcount decoder. I know it is useful for debugging the
core DT code, but it isn't something that will be used generally. Plus
the returned value cannot be relied upon to be stable because there may
be other code currently iterating over the tree.

---

of: Custom printk format specifier for device node

90% of the usage of device node's full_name is printing it out
in a kernel message. Preparing for the eventual delayed allocation
introduce a custom printk format specifier that is both more
compact and more pleasant to the eye.

For instance typical use is:
	pr_info("Frobbing node %s\n", node->full_name);

Which can be written now as:
	pr_info("Frobbing node %pOn\n", node);

More fine-grained control of formatting includes printing the name,
flag, path-spec name, reference count and others, explained in the
documentation entry.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
[grant.likely: Rework to be simpler]
Signed-off-by: Grant Likely <grant.likely@linaro.org>
---
 Documentation/printk-formats.txt             |  28 ++++++++
 drivers/of/unittest-data/tests-platform.dtsi |   4 +-
 drivers/of/unittest.c                        |  58 +++++++++++++++
 lib/vsprintf.c                               | 101 +++++++++++++++++++++++++++
 4 files changed, 190 insertions(+), 1 deletion(-)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 5a615c14f75d..f0dc2fd229e4 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -192,6 +192,34 @@ IPv4/IPv6 addresses (generic, with port, flowinfo, scope):
 	%pISsc		1.2.3.4		or [1:2:3:4:5:6:7:8]%1234567890
 	%pISpfc		1.2.3.4:12345	or [1:2:3:4:5:6:7:8]:12345/123456789
 
+Device tree nodes:
+
+	%pOn[fnpPcCFr]
+
+	For printing device tree nodes. The optional arguments are:
+	    f device node full_name
+	    n device node name
+	    p device node phandle
+	    P device node path spec (name + @unit)
+	    F device node flags
+	    c major compatible string
+	    C full compatible string
+	Without any arguments prints full_name (same as %pOnf)
+	The separator when using multiple arguments is ':'
+
+	Examples:
+
+	%pOn	/foo/bar@0			- Node full name
+	%pOnf	/foo/bar@0			- Same as above
+	%pOnfp	/foo/bar@0:10			- Node full name + phandle
+	%pOnfcF	/foo/bar@0:foo,device:--P-	- Node full name +
+	                                          major compatible string +
+						  node flags
+							D - dynamic
+							d - detached
+							P - Populated
+							B - Populated bus
+
 UUID/GUID addresses:
 
 	%pUb	00010203-0405-0607-0809-0a0b0c0d0e0f
diff --git a/drivers/of/unittest-data/tests-platform.dtsi b/drivers/of/unittest-data/tests-platform.dtsi
index eb20eeb2b062..a0c93822aee3 100644
--- a/drivers/of/unittest-data/tests-platform.dtsi
+++ b/drivers/of/unittest-data/tests-platform.dtsi
@@ -26,7 +26,9 @@
 				#size-cells = <0>;
 
 				dev@100 {
-					compatible = "test-sub-device";
+					compatible = "test-sub-device",
+						     "test-compat2",
+						     "test-compat3";
 					reg = <0x100>;
 				};
 			};
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index e844907c9efa..dc43209f2064 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -234,6 +234,63 @@ static void __init of_unittest_check_tree_linkage(void)
 	pr_debug("allnodes list size (%i); sibling lists size (%i)\n", allnode_count, child_count);
 }
 
+static void __init of_unittest_printf_one(struct device_node *np, const char *fmt,
+					  const char *expected)
+{
+	char buf[strlen(expected)+10];
+	int size, i;
+
+	/* Baseline; check conversion with a large size limit */
+	memset(buf, 0xff, sizeof(buf));
+	size = snprintf(buf, sizeof(buf) - 2, fmt, np);
+
+	/* use strcmp() instead of strncmp() here to be absolutely sure strings match */
+	unittest((strcmp(buf, expected) == 0) && (buf[size+1] == 0xff),
+		"sprintf failed; fmt='%s' expected='%s' rslt='%s'\n",
+		fmt, expected, buf);
+
+	/* Make sure length limits work */
+	size++;
+	for (i = 0; i < 2; i++, size--) {
+		/* Clear the buffer, and make sure it works correctly still */
+		memset(buf, 0xff, sizeof(buf));
+		snprintf(buf, size+1, fmt, np);
+		unittest(strncmp(buf, expected, size) == 0 && (buf[size+1] == 0xff),
+			"snprintf failed; size=%i fmt='%s' expected='%s' rslt='%s'\n",
+			size, fmt, expected, buf);
+	}
+}
+
+static void __init of_unittest_printf(void)
+{
+	struct device_node *np;
+	const char *full_name = "/testcase-data/platform-tests/test-device@1/dev@100";
+	char phandle_str[16] = "";
+
+	np = of_find_node_by_path(full_name);
+	if (!np) {
+		unittest(np, "testcase data missing\n");
+		return;
+	}
+
+	num_to_str(phandle_str, sizeof(phandle_str), np->phandle);
+
+	of_unittest_printf_one(np, "%pOn",  full_name);
+	of_unittest_printf_one(np, "%pOnf", full_name);
+	of_unittest_printf_one(np, "%pOnp", phandle_str);
+	of_unittest_printf_one(np, "%pOnP", "dev@100");
+	of_unittest_printf_one(np, "ABC %pOnP ABC", "ABC dev@100 ABC");
+	of_unittest_printf_one(np, "%10pOnP", "   dev@100");
+	of_unittest_printf_one(np, "%-10pOnP", "dev@100   ");
+	of_unittest_printf_one(of_root, "%pOnP", "/");
+	of_unittest_printf_one(np, "%pOnF", "----");
+	of_unittest_printf_one(np, "%pOnPF", "dev@100:----");
+	of_unittest_printf_one(np, "%pOnPFPc", "dev@100:----:dev@100:test-sub-device");
+	of_unittest_printf_one(np, "%pOnc", "test-sub-device");
+	of_unittest_printf_one(np, "%pOnC",
+			"\"test-sub-device\",\"test-compat2\",\"test-compat3\"");
+}
+
 struct node_hash {
 	struct hlist_node node;
 	struct device_node *np;
@@ -1888,6 +1945,7 @@ static int __init of_unittest(void)
 	of_unittest_find_node_by_name();
 	of_unittest_dynamic();
 	of_unittest_parse_phandle_with_args();
+	of_unittest_printf();
 	of_unittest_property_string();
 	of_unittest_property_copy();
 	of_unittest_changeset();
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index b235c96167d3..8a793a4560c2 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -29,6 +29,7 @@
 #include <linux/dcache.h>
 #include <linux/cred.h>
 #include <net/addrconf.h>
+#include <linux/of.h>
 
 #include <asm/page.h>		/* for PAGE_SIZE */
 #include <asm/sections.h>	/* for dereference_function_descriptor() */
@@ -1322,6 +1323,91 @@ char *address_val(char *buf, char *end, const void *addr,
 	return number(buf, end, num, spec);
 }
 
+static noinline_for_stack
+char *device_node_string(char *buf, char *end, struct device_node *dn,
+			 struct printf_spec spec, const char *fmt)
+{
+	char tbuf[] = "----";
+	struct property *prop;
+	const char *p;
+	int ret, i, j, n;
+	char c, *start = buf;
+	struct printf_spec str_spec =
+		{ .precision = -1, .field_width = 0, .base = 10, .flags = LEFT };
+
+	if (!IS_ENABLED(CONFIG_OF))
+		return string(buf, end, "(!OF)", spec);
+
+	if ((unsigned long)dn < PAGE_SIZE)
+		return string(buf, end, "(null)", spec);
+
+	/* simple case without anything any more format specifiers */
+	if (!isalnum(fmt[2]))
+		fmt = "Onf";
+
+	fmt += 2;
+	for (j = 0; isalnum((c = *fmt)); j++, fmt++) {
+		if (j)
+			buf = string(buf, end, ":", str_spec);
+
+		switch (c) {
+		case 'f':	/* full_name */
+			buf = string(buf, end, of_node_full_name(dn), str_spec);
+			break;
+		case 'n':	/* name */
+			buf = string(buf, end, dn->name, str_spec);
+			break;
+		case 'p':	/* phandle */
+			buf = number(buf, end, dn->phandle, str_spec);
+			break;
+		case 'P':	/* path-spec */
+			p = strrchr(of_node_full_name(dn), '/');
+			if (p[1]) /* Root node name is just "/" */
+				p++;
+			buf = string(buf, end, p, str_spec);
+			break;
+		case 'F':	/* flags */
+			snprintf(tbuf, sizeof(tbuf), "%c%c%c%c",
+				of_node_check_flag(dn, OF_DYNAMIC) ?  'D' : '-',
+				of_node_check_flag(dn, OF_DETACHED) ?  'd' : '-',
+				of_node_check_flag(dn, OF_POPULATED) ?  'P' : '-',
+				of_node_check_flag(dn, OF_POPULATED_BUS) ?  'B' : '-');
+			buf = string(buf, end, tbuf, str_spec);
+			break;
+		case 'c':	/* first compatible string */
+			ret = of_property_read_string(dn, "compatible", &p);
+			if (ret == 0)
+				buf = string(buf, end, p, str_spec);
+			break;
+		case 'C':	/* full compatible string list */
+			i = 0;
+			of_property_for_each_string(dn, "compatible", prop, p) {
+				buf = string(buf, end, i ? "\",\"" : "\"", str_spec);
+				buf = string(buf, end, p, str_spec);
+				i++;
+			}
+			buf = string(buf, end, "\"", str_spec);
+			break;
+		}
+	}
+
+	/* Pad out at the front or back if field_width is specified */
+	n = buf - start;
+	if (n < spec.field_width) {
+		unsigned spaces = spec.field_width - n;
+		if (!(spec.flags & LEFT)) {
+			widen(start, end, n, spaces);
+			return buf + spaces;
+		}
+		while (spaces-- && (buf < end)) {
+			if (buf < end)
+				*buf = ' ';
+			++buf;
+		}
+	}
+	return buf;
+}
+
 int kptr_restrict __read_mostly;
 
 /*
@@ -1393,6 +1479,15 @@ 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
+ * - 'On[fnpPcCF]' For a device tree object
+ *                Without any optional arguments prints the full_name
+ *                f device node full_name
+ *                n device node name
+ *                p device node phandle
+ *                P device node path spec (name + @unit)
+ *                F device node flags
+ *                c major compatible string
+ *                C full compatible string
  * - 'h[CDN]' For a variable-length buffer, it prints it as a hex string with
  *            a certain separator (' ' by default):
  *              C colon
@@ -1544,6 +1639,12 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 			return netdev_feature_string(buf, end, ptr, spec);
 		}
 		break;
+	case 'O':
+		switch (fmt[1]) {
+		case 'n':
+			return device_node_string(buf, end, ptr, spec, fmt);
+		}
+		break;
 	case 'a':
 		return address_val(buf, end, ptr, spec, fmt);
 	case 'd':
-- 
2.1.0

Re: [PATCH] of: Custom printk format specifier for device node

[Pantelis Antoniou]

Hi Grant,

> On Mar 30, 2015, at 22:04 , Grant Likely <grant.likely@secretlab.ca> wrote:
> 
> On Thu, 22 Jan 2015 22:31:46 +0200
> , Pantelis Antoniou <panto@antoniou-consulting.com>
> wrote:
>> Hi Joe,
>> 
>>> On Jan 21, 2015, at 19:37 , Joe Perches <joe@perches.com> wrote:
>>> 
>>> On Wed, 2015-01-21 at 19:06 +0200, Pantelis Antoniou wrote:
>>>> 90% of the usage of device node's full_name is printing it out
>>>> in a kernel message. Preparing for the eventual delayed allocation
>>>> introduce a custom printk format specifier that is both more
>>>> compact and more pleasant to the eye.
>>>> 
>>>> For instance typical use is:
>>>> 	pr_info("Frobbing node %s\n", node->full_name);
>>>> 
>>>> Which can be written now as:
>>>> 	pr_info("Frobbing node %pO\n", node);
>> 
>>> Still disliking use of %p0.
>>> 
>> 
>> pO - Open Firmware
>> 
>> pT for tree is bad, cause we plan to use a tree type in the future in OF.
> 
> So, here's a radical thought. How about we reserve '%pO' for objects, as
> in kobjects.  We'll use extra flags to narrow down specifically to
> device tree nodes, but we could teach vsprintf() to treat a plain '%pO'
> as plain kobject pointer, and if it is able to recognize the kobj_type,
> then run a specific decoder to format it.
> 
> This also gives us a namespace for various kinds of firmware data
> output. %Od could be a struct device, %On for device tree node, %Oa for
> an ACPI node, etc.
> 

I’m fine with this. I also have a patch to turn an overlay to a kobj
so this fits naturally.

OTOH if we do this, I would expect to rework the custom printk infrastructure
to be more generic.

IMHO having the format specifier and the format print methods in lib/vsprintf.c
is not very nice.

How about having a way to register object printk handlers with something like that?
We could put that in a special linker section and have the printk method pass control
there.

PRINTK_OBJFMT(’n’, printk_objfmt_device_node);

We might have to make a few printk methods public however.

>> 
>>>> More fine-grained control of formatting includes printing the name,
>>>> flag, path-spec name, reference count and others, explained in the
>>>> documentation entry.
>>>> 
>>>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>>>> 
>>>> dt-print
>>>> ---
>>>> Documentation/printk-formats.txt |  29 ++++++++
>>>> lib/vsprintf.c                   | 151 +++++++++++++++++++++++++++++++++++++++
>>>> 2 files changed, 180 insertions(+)
>>>> 
>>>> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
>>>> index 5a615c1..2d42c04 100644
>>>> --- a/Documentation/printk-formats.txt
>>>> +++ b/Documentation/printk-formats.txt
>>>> @@ -231,6 +231,35 @@ struct va_format:
>>>> 	Do not use this feature without some mechanism to verify the
>>>> 	correctness of the format string and va_list arguments.
>>>> 
>>>> +Device tree nodes:
>>>> +
>>>> +	%pO[fnpPcCFr]
>>>> +
>>>> +	For printing device tree nodes. The optional arguments are:
>>>> +            f device node full_name
>>>> +            n device node name
>>>> +            p device node phandle
>>>> +            P device node path spec (name + @unit)
>>>> +            F device node flags
>>>> +            c major compatible string
>>>> +            C full compatible string
>>>> +            r node reference count
>>>> +	Without any arguments prints full_name (same as %pOf)
>>>> +	The separator when using multiple arguments is '|'
>>>> +
>>>> +	Examples:
>>>> +
>>>> +	%pO	/foo/bar@0			- Node full name
>>>> +	%pOf	/foo/bar@0			- Same as above
>>>> +	%pOfp	/foo/bar@0|10			- Node full name + phandle
>>>> +	%pOfcF	/foo/bar@0|foo,device|--P-	- Node full name +
>>>> +	                                          major compatible string +
>>>> +						  node flags
>>>> +							D - dynamic
>>>> +							d - detached
>>>> +							P - Populated
>>>> +							B - Populated bus
>>>> +
>>>> u64 SHOULD be printed with %llu/%llx:
>>>> 
>>>> 	printk("%llu", u64_var);
>>>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>>> []
>>> 
>>> Add #ifdef back ?
>>> 
>> 
>> The whole thing is optimized away when CONFIG_OF is not defined, leaving only
>> the return statement.
>> 
>>>> +static noinline_for_stack
>>>> +char *device_node_string(char *buf, char *end, struct device_node *dn,
>>>> +			 struct printf_spec spec, const char *fmt)
>>>> +{
>>>> +	char tbuf[sizeof("xxxxxxxxxx") + 1];
>>>> +	const char *fmtp, *p;
>>>> +	int len, ret, i, j, pass;
>>>> +	char c;
>>>> +
>>>> +	if (!IS_ENABLED(CONFIG_OF))
>>>> +		return string(buf, end, "(!OF)", spec);
>>> 
>>> Not very descriptive output, maybe the address would be better.
>>> 
>> 
>> OK
>> 
>>>> +
>>>> +	if ((unsigned long)dn < PAGE_SIZE)
>>>> +		return string(buf, end, "(null)", spec);
>>>> +
>>>> +	/* simple case without anything any more format specifiers */
>>>> +	if (fmt[1] == '\0' || isspace(fmt[1]))
>>>> +		fmt = "Of";
> 
> s/isspace()/!isalnum() to match with what the pointer() function does
> when finding the end of a format string.
> 
>>> 
>>> why lower case here but upper case above?
>>> 
>> 
>> Cause '(null)' is what’s printed in string() when null is passed as a pointer.
>> 
>>>> +
>>>> +	len = 0;
>>>> +
>>>> +	/* two passes; the first calculates length, the second fills in */
>>>> +	for (pass = 1; pass <= 2; pass++) {
>>>> +		if (pass == 2 && !(spec.flags & LEFT)) {
>>>> +			/* padding */
>>>> +			while (len < spec.field_width--) {
>>>> +				if (buf < end)
>>>> +					*buf = ' ';
>>>> +				++buf;
>>>> +			}
>>>> +		}
>>>> +#undef _HANDLE_CH
>>>> +#define _HANDLE_CH(_ch) \
>>>> +	do { \
>>>> +		if (pass == 1) \
>>>> +			len++; \
>>>> +		else \
>>>> +			if (buf < end) \
>>>> +				*buf++ = (_ch); \
>>>> +	} while (0)
>>>> +#undef _HANDLE_STR
>>>> +#define _HANDLE_STR(_str) \
>>>> +	do { \
>>>> +		const char *str = (_str); \
>>>> +		if (pass == 1) \
>>>> +			len += strlen(str); \
>>>> +		else \
>>>> +			while (*str && buf < end) \
>>>> +				*buf++ = *str++; \
>>>> +	} while (0)
>>> 
>>> This isn't pretty.  Perhaps there's a better way?
>>> 
>> 
>> It’s the simplest way to do the different operations for the two passes, without
>> bloating the code or adding superfluous methods.
>> 
>> We don’t want to allocate memory, we don’t want to use stack space. We’re probably
>> printing in atomic context too since device nodes don’t usually printed out
>> during normal operation.
> 
> As far as I can tell from the code, the only thing that 2 passes is used
> for is when the LEFT spec.flags value is not set. Instead of doing two,
> what if the code generated the string right-aligned, and then if the
> LEFT flag is set, shift it over the required amount, ' ' padding as
> necessary? In fact, that's exactly what the dentry_name() helper does.
> 

Hmm, that might work.

> This is a complicated enough block of code, that I'd like to see
> unittests for it added at the same time.
> 
> ...
> 
> So, I'm keen enough on this feature that I've just gone ahead and played
> with it this morning. The following is what I've come up with. I got rid
> of the two passes, fixed up the boundary conditions, and added
> unittests. I've also switched the format key to %pOn, and the separator
> to ':'. ':' is never a valid character in a node name, so it should be
> safe to use as a delimiter.

OK.

> 
> I've dropped the refcount decoder. I know it is useful for debugging the
> core DT code, but it isn't something that will be used generally. Plus
> the returned value cannot be relied upon to be stable because there may
> be other code currently iterating over the tree.
> 

Yeah, I know it’s not something to rely on. If we do %pOk to be kobj
debug I can add it back in.

> ---
> 
> of: Custom printk format specifier for device node
> 
> 90% of the usage of device node's full_name is printing it out
> in a kernel message. Preparing for the eventual delayed allocation
> introduce a custom printk format specifier that is both more
> compact and more pleasant to the eye.
> 
> For instance typical use is:
> 	pr_info("Frobbing node %s\n", node->full_name);
> 
> Which can be written now as:
> 	pr_info("Frobbing node %pOn\n", node);
> 
> More fine-grained control of formatting includes printing the name,
> flag, path-spec name, reference count and others, explained in the
> documentation entry.
> 

Remove reference count comment?

> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> [grant.likely: Rework to be simpler]
> Signed-off-by: Grant Likely <grant.likely@linaro.org>
> ---
> Documentation/printk-formats.txt             |  28 ++++++++
> drivers/of/unittest-data/tests-platform.dtsi |   4 +-
> drivers/of/unittest.c                        |  58 +++++++++++++++
> lib/vsprintf.c                               | 101 +++++++++++++++++++++++++++
> 4 files changed, 190 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
> index 5a615c14f75d..f0dc2fd229e4 100644
> --- a/Documentation/printk-formats.txt
> +++ b/Documentation/printk-formats.txt
> @@ -192,6 +192,34 @@ IPv4/IPv6 addresses (generic, with port, flowinfo, scope):
> 	%pISsc		1.2.3.4		or [1:2:3:4:5:6:7:8]%1234567890
> 	%pISpfc		1.2.3.4:12345	or [1:2:3:4:5:6:7:8]:12345/123456789
> 
> +Device tree nodes:
> +
> +	%pOn[fnpPcCFr]
> +
> +	For printing device tree nodes. The optional arguments are:
> +	    f device node full_name
> +	    n device node name
> +	    p device node phandle
> +	    P device node path spec (name + @unit)
> +	    F device node flags
> +	    c major compatible string
> +	    C full compatible string
> +	Without any arguments prints full_name (same as %pOnf)
> +	The separator when using multiple arguments is ‘:’
^ separator is ‘.'
> +

> +	Examples:
> +
> +	%pOn	/foo/bar@0			- Node full name
> +	%pOnf	/foo/bar@0			- Same as above
> +	%pOnfp	/foo/bar@0:10			- Node full name + phandle
> +	%pOnfcF	/foo/bar@0:foo,device:--P-	- Node full name +
> +	                                          major compatible string +
> +						  node flags
> +							D - dynamic
> +							d - detached
> +							P - Populated
> +							B - Populated bus
> +

Same here.

> UUID/GUID addresses:
> 
> 	%pUb	00010203-0405-0607-0809-0a0b0c0d0e0f
> diff --git a/drivers/of/unittest-data/tests-platform.dtsi b/drivers/of/unittest-data/tests-platform.dtsi
> index eb20eeb2b062..a0c93822aee3 100644
> --- a/drivers/of/unittest-data/tests-platform.dtsi
> +++ b/drivers/of/unittest-data/tests-platform.dtsi
> @@ -26,7 +26,9 @@
> 				#size-cells = <0>;
> 
> 				dev@100 {
> -					compatible = "test-sub-device";
> +					compatible = "test-sub-device",
> +						     "test-compat2",
> +						     "test-compat3";
> 					reg = <0x100>;
> 				};
> 			};
> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> index e844907c9efa..dc43209f2064 100644
> --- a/drivers/of/unittest.c
> +++ b/drivers/of/unittest.c
> @@ -234,6 +234,63 @@ static void __init of_unittest_check_tree_linkage(void)
> 	pr_debug("allnodes list size (%i); sibling lists size (%i)\n", allnode_count, child_count);
> }
> 
> +static void __init of_unittest_printf_one(struct device_node *np, const char *fmt,
> +					  const char *expected)
> +{
> +	char buf[strlen(expected)+10];
> +	int size, i;
> +
> +	/* Baseline; check conversion with a large size limit */
> +	memset(buf, 0xff, sizeof(buf));
> +	size = snprintf(buf, sizeof(buf) - 2, fmt, np);
> +
> +	/* use strcmp() instead of strncmp() here to be absolutely sure strings match */
> +	unittest((strcmp(buf, expected) == 0) && (buf[size+1] == 0xff),
> +		"sprintf failed; fmt='%s' expected='%s' rslt='%s'\n",
> +		fmt, expected, buf);
> +
> +	/* Make sure length limits work */
> +	size++;
> +	for (i = 0; i < 2; i++, size--) {
> +		/* Clear the buffer, and make sure it works correctly still */
> +		memset(buf, 0xff, sizeof(buf));
> +		snprintf(buf, size+1, fmt, np);
> +		unittest(strncmp(buf, expected, size) == 0 && (buf[size+1] == 0xff),
> +			"snprintf failed; size=%i fmt='%s' expected='%s' rslt='%s'\n",
> +			size, fmt, expected, buf);
> +	}
> +}
> +
> +static void __init of_unittest_printf(void)
> +{
> +	struct device_node *np;
> +	const char *full_name = "/testcase-data/platform-tests/test-device@1/dev@100";
> +	char phandle_str[16] = "";
> +
> +	np = of_find_node_by_path(full_name);
> +	if (!np) {
> +		unittest(np, "testcase data missing\n");
> +		return;
> +	}
> +
> +	num_to_str(phandle_str, sizeof(phandle_str), np->phandle);
> +
> +	of_unittest_printf_one(np, "%pOn",  full_name);
> +	of_unittest_printf_one(np, "%pOnf", full_name);
> +	of_unittest_printf_one(np, "%pOnp", phandle_str);
> +	of_unittest_printf_one(np, "%pOnP", "dev@100");
> +	of_unittest_printf_one(np, "ABC %pOnP ABC", "ABC dev@100 ABC");
> +	of_unittest_printf_one(np, "%10pOnP", "   dev@100");
> +	of_unittest_printf_one(np, "%-10pOnP", "dev@100   ");
> +	of_unittest_printf_one(of_root, "%pOnP", "/");
> +	of_unittest_printf_one(np, "%pOnF", "----");
> +	of_unittest_printf_one(np, "%pOnPF", "dev@100:----");
> +	of_unittest_printf_one(np, "%pOnPFPc", "dev@100:----:dev@100:test-sub-device");
> +	of_unittest_printf_one(np, "%pOnc", "test-sub-device");
> +	of_unittest_printf_one(np, "%pOnC",
> +			"\"test-sub-device\",\"test-compat2\",\"test-compat3\"");
> +}
> +
> struct node_hash {
> 	struct hlist_node node;
> 	struct device_node *np;
> @@ -1888,6 +1945,7 @@ static int __init of_unittest(void)
> 	of_unittest_find_node_by_name();
> 	of_unittest_dynamic();
> 	of_unittest_parse_phandle_with_args();
> +	of_unittest_printf();
> 	of_unittest_property_string();
> 	of_unittest_property_copy();
> 	of_unittest_changeset();
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index b235c96167d3..8a793a4560c2 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -29,6 +29,7 @@
> #include <linux/dcache.h>
> #include <linux/cred.h>
> #include <net/addrconf.h>
> +#include <linux/of.h>
> 
> #include <asm/page.h>		/* for PAGE_SIZE */
> #include <asm/sections.h>	/* for dereference_function_descriptor() */
> @@ -1322,6 +1323,91 @@ char *address_val(char *buf, char *end, const void *addr,
> 	return number(buf, end, num, spec);
> }
> 
> +static noinline_for_stack
> +char *device_node_string(char *buf, char *end, struct device_node *dn,
> +			 struct printf_spec spec, const char *fmt)
> +{
> +	char tbuf[] = "----";
> +	struct property *prop;
> +	const char *p;
> +	int ret, i, j, n;
> +	char c, *start = buf;
> +	struct printf_spec str_spec =
> +		{ .precision = -1, .field_width = 0, .base = 10, .flags = LEFT };
> +
> +	if (!IS_ENABLED(CONFIG_OF))
> +		return string(buf, end, "(!OF)", spec);
> +
> +	if ((unsigned long)dn < PAGE_SIZE)
> +		return string(buf, end, "(null)", spec);
> +
> +	/* simple case without anything any more format specifiers */
> +	if (!isalnum(fmt[2]))
> +		fmt = "Onf";
> +
> +	fmt += 2;
> +	for (j = 0; isalnum((c = *fmt)); j++, fmt++) {
> +		if (j)
> +			buf = string(buf, end, ":", str_spec);
> +
^ separator is ‘.’ now?

> +		switch (c) {
> +		case 'f':	/* full_name */
> +			buf = string(buf, end, of_node_full_name(dn), str_spec);
> +			break;
> +		case 'n':	/* name */
> +			buf = string(buf, end, dn->name, str_spec);
> +			break;
> +		case 'p':	/* phandle */
> +			buf = number(buf, end, dn->phandle, str_spec);
> +			break;
> +		case 'P':	/* path-spec */
> +			p = strrchr(of_node_full_name(dn), '/');
> +			if (p[1]) /* Root node name is just "/" */
> +				p++;
> +			buf = string(buf, end, p, str_spec);
> +			break;
> +		case 'F':	/* flags */
> +			snprintf(tbuf, sizeof(tbuf), "%c%c%c%c",
> +				of_node_check_flag(dn, OF_DYNAMIC) ?  'D' : '-',
> +				of_node_check_flag(dn, OF_DETACHED) ?  'd' : '-',
> +				of_node_check_flag(dn, OF_POPULATED) ?  'P' : '-',
> +				of_node_check_flag(dn, OF_POPULATED_BUS) ?  'B' : '-');
> +			buf = string(buf, end, tbuf, str_spec);
> +			break;
> +		case 'c':	/* first compatible string */
> +			ret = of_property_read_string(dn, "compatible", &p);
> +			if (ret == 0)
> +				buf = string(buf, end, p, str_spec);
> +			break;
> +		case 'C':	/* full compatible string list */
> +			i = 0;
> +			of_property_for_each_string(dn, "compatible", prop, p) {
> +				buf = string(buf, end, i ? "\",\"" : "\"", str_spec);
> +				buf = string(buf, end, p, str_spec);
> +				i++;
> +			}
> +			buf = string(buf, end, "\"", str_spec);
> +			break;
> +		}
> +	}
> +
> +	/* Pad out at the front or back if field_width is specified */
> +	n = buf - start;
> +	if (n < spec.field_width) {
> +		unsigned spaces = spec.field_width - n;
> +		if (!(spec.flags & LEFT)) {
> +			widen(start, end, n, spaces);
> +			return buf + spaces;
> +		}
> +		while (spaces-- && (buf < end)) {
> +			if (buf < end)
> +				*buf = ' ';
> +			++buf;
> +		}
> +	}
> +	return buf;
> +}
> +
> int kptr_restrict __read_mostly;
> 
> /*
> @@ -1393,6 +1479,15 @@ 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
> + * - 'On[fnpPcCF]' For a device tree object
> + *                Without any optional arguments prints the full_name
> + *                f device node full_name
> + *                n device node name
> + *                p device node phandle
> + *                P device node path spec (name + @unit)
> + *                F device node flags
> + *                c major compatible string
> + *                C full compatible string
>  * - 'h[CDN]' For a variable-length buffer, it prints it as a hex string with
>  *            a certain separator (' ' by default):
>  *              C colon
> @@ -1544,6 +1639,12 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
> 			return netdev_feature_string(buf, end, ptr, spec);
> 		}
> 		break;
> +	case 'O':
> +		switch (fmt[1]) {
> +		case 'n':
> +			return device_node_string(buf, end, ptr, spec, fmt);
> +		}
> +		break;
> 	case 'a':
> 		return address_val(buf, end, ptr, spec, fmt);
> 	case 'd':
> -- 
> 2.1.0
> 
> 

Regards

— Pantelis

Re: [PATCH] of: Custom printk format specifier for device node

[Grant Likely]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 3807 bytes --]

Hi Pantelis,

Thanks for the quick reply. Comments below...

On Tue, 31 Mar 2015 13:03:05 +0300
, Pantelis Antoniou <pantelis.antoniou@konsulko.com>
 wrote:
> Hi Grant,
> 
> > On Mar 30, 2015, at 22:04 , Grant Likely <grant.likely@secretlab.ca> wrote:
> > 
> > On Thu, 22 Jan 2015 22:31:46 +0200
> > , Pantelis Antoniou <panto@antoniou-consulting.com>
> > wrote:
> >> Hi Joe,
> >> 
> >>> On Jan 21, 2015, at 19:37 , Joe Perches <joe@perches.com> wrote:
> >>> 
> >>> On Wed, 2015-01-21 at 19:06 +0200, Pantelis Antoniou wrote:
> >>>> 90% of the usage of device node's full_name is printing it out
> >>>> in a kernel message. Preparing for the eventual delayed allocation
> >>>> introduce a custom printk format specifier that is both more
> >>>> compact and more pleasant to the eye.
> >>>> 
> >>>> For instance typical use is:
> >>>> 	pr_info("Frobbing node %s\n", node->full_name);
> >>>> 
> >>>> Which can be written now as:
> >>>> 	pr_info("Frobbing node %pO\n", node);
> >> 
> >>> Still disliking use of %p0.
> >>> 
> >> 
> >> pO - Open Firmware
> >> 
> >> pT for tree is bad, cause we plan to use a tree type in the future in OF.
> > 
> > So, here's a radical thought. How about we reserve '%pO' for objects, as
> > in kobjects.  We'll use extra flags to narrow down specifically to
> > device tree nodes, but we could teach vsprintf() to treat a plain '%pO'
> > as plain kobject pointer, and if it is able to recognize the kobj_type,
> > then run a specific decoder to format it.
> > 
> > This also gives us a namespace for various kinds of firmware data
> > output. %Od could be a struct device, %On for device tree node, %Oa for
> > an ACPI node, etc.
> > 
> 
> I’m fine with this. I also have a patch to turn an overlay to a kobj
> so this fits naturally.
> 
> OTOH if we do this, I would expect to rework the custom printk infrastructure
> to be more generic.
> 
> IMHO having the format specifier and the format print methods in lib/vsprintf.c
> is not very nice.
> 
> How about having a way to register object printk handlers with something like that?
> We could put that in a special linker section and have the printk method pass control
> there.
> 
> PRINTK_OBJFMT(’n’, printk_objfmt_device_node);
> 
> We might have to make a few printk methods public however.

Honestly, I think trying to add registration is an overengineered
solution at this point. We're not hitting a wall on the complexity of
vsprintf.c, and having them all in one place helps to ensure we don't
have conflicts.

> 
> > I've dropped the refcount decoder. I know it is useful for debugging the
> > core DT code, but it isn't something that will be used generally. Plus
> > the returned value cannot be relied upon to be stable because there may
> > be other code currently iterating over the tree.
> > 
> 
> Yeah, I know it’s not something to rely on. If we do %pOk to be kobj
> debug I can add it back in.

Yes, that would be a good place to have refcount output.

> > +Device tree nodes:
> > +
> > +	%pOn[fnpPcCFr]
> > +
> > +	For printing device tree nodes. The optional arguments are:
> > +	    f device node full_name
> > +	    n device node name
> > +	    p device node phandle
> > +	    P device node path spec (name + @unit)
> > +	    F device node flags
> > +	    c major compatible string
> > +	    C full compatible string
> > +	Without any arguments prints full_name (same as %pOnf)
> > +	The separator when using multiple arguments is ‘:’
> ^ separator is ‘.'

? I'm confused? The separator that I'm using is a colon. ':'  Where do
you see ','? I don't think ',' would be a good separator because it
appears in node names and compatible strings. Originally, I think you
were using pipe '|', but my personal opinion is that ':' is better
because there is already precidence as a separator.

g.

Re: [PATCH] of: Custom printk format specifier for device node

[Grant Likely]

Hi Pantelis,

Thanks for the quick reply. Comments below...

On Tue, 31 Mar 2015 13:03:05 +0300
, Pantelis Antoniou <pantelis.antoniou@konsulko.com>
 wrote:
> Hi Grant,
> 
> > On Mar 30, 2015, at 22:04 , Grant Likely <grant.likely@secretlab.ca> wrote:
> > 
> > On Thu, 22 Jan 2015 22:31:46 +0200
> > , Pantelis Antoniou <panto@antoniou-consulting.com>
> > wrote:
> >> Hi Joe,
> >> 
> >>> On Jan 21, 2015, at 19:37 , Joe Perches <joe@perches.com> wrote:
> >>> 
> >>> On Wed, 2015-01-21 at 19:06 +0200, Pantelis Antoniou wrote:
> >>>> 90% of the usage of device node's full_name is printing it out
> >>>> in a kernel message. Preparing for the eventual delayed allocation
> >>>> introduce a custom printk format specifier that is both more
> >>>> compact and more pleasant to the eye.
> >>>> 
> >>>> For instance typical use is:
> >>>> 	pr_info("Frobbing node %s\n", node->full_name);
> >>>> 
> >>>> Which can be written now as:
> >>>> 	pr_info("Frobbing node %pO\n", node);
> >> 
> >>> Still disliking use of %p0.
> >>> 
> >> 
> >> pO - Open Firmware
> >> 
> >> pT for tree is bad, cause we plan to use a tree type in the future in OF.
> > 
> > So, here's a radical thought. How about we reserve '%pO' for objects, as
> > in kobjects.  We'll use extra flags to narrow down specifically to
> > device tree nodes, but we could teach vsprintf() to treat a plain '%pO'
> > as plain kobject pointer, and if it is able to recognize the kobj_type,
> > then run a specific decoder to format it.
> > 
> > This also gives us a namespace for various kinds of firmware data
> > output. %Od could be a struct device, %On for device tree node, %Oa for
> > an ACPI node, etc.
> > 
> 
> I’m fine with this. I also have a patch to turn an overlay to a kobj
> so this fits naturally.
> 
> OTOH if we do this, I would expect to rework the custom printk infrastructure
> to be more generic.
> 
> IMHO having the format specifier and the format print methods in lib/vsprintf.c
> is not very nice.
> 
> How about having a way to register object printk handlers with something like that?
> We could put that in a special linker section and have the printk method pass control
> there.
> 
> PRINTK_OBJFMT(’n’, printk_objfmt_device_node);
> 
> We might have to make a few printk methods public however.

Honestly, I think trying to add registration is an overengineered
solution at this point. We're not hitting a wall on the complexity of
vsprintf.c, and having them all in one place helps to ensure we don't
have conflicts.

> 
> > I've dropped the refcount decoder. I know it is useful for debugging the
> > core DT code, but it isn't something that will be used generally. Plus
> > the returned value cannot be relied upon to be stable because there may
> > be other code currently iterating over the tree.
> > 
> 
> Yeah, I know it’s not something to rely on. If we do %pOk to be kobj
> debug I can add it back in.

Yes, that would be a good place to have refcount output.

> > +Device tree nodes:
> > +
> > +	%pOn[fnpPcCFr]
> > +
> > +	For printing device tree nodes. The optional arguments are:
> > +	    f device node full_name
> > +	    n device node name
> > +	    p device node phandle
> > +	    P device node path spec (name + @unit)
> > +	    F device node flags
> > +	    c major compatible string
> > +	    C full compatible string
> > +	Without any arguments prints full_name (same as %pOnf)
> > +	The separator when using multiple arguments is ‘:’
> ^ separator is ‘.'

? I'm confused? The separator that I'm using is a colon. ':'  Where do
you see ','? I don't think ',' would be a good separator because it
appears in node names and compatible strings. Originally, I think you
were using pipe '|', but my personal opinion is that ':' is better
because there is already precidence as a separator.

g.

Re: [PATCH] of: Custom printk format specifier for device node

[Pantelis Antoniou]

Hi Grant,

> On Mar 31, 2015, at 20:02 , Grant Likely <grant.likely@secretlab.ca> wrote:
> 
> Hi Pantelis,
> 
> Thanks for the quick reply. Comments below...
> 
> On Tue, 31 Mar 2015 13:03:05 +0300
> , Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> wrote:
>> Hi Grant,
>> 
>>> On Mar 30, 2015, at 22:04 , Grant Likely <grant.likely@secretlab.ca> wrote:
>>> 
>>> On Thu, 22 Jan 2015 22:31:46 +0200
>>> , Pantelis Antoniou <panto@antoniou-consulting.com>
>>> wrote:
>>>> Hi Joe,
>>>> 
>>>>> On Jan 21, 2015, at 19:37 , Joe Perches <joe@perches.com> wrote:
>>>>> 
>>>>> On Wed, 2015-01-21 at 19:06 +0200, Pantelis Antoniou wrote:
>>>>>> 90% of the usage of device node's full_name is printing it out
>>>>>> in a kernel message. Preparing for the eventual delayed allocation
>>>>>> introduce a custom printk format specifier that is both more
>>>>>> compact and more pleasant to the eye.
>>>>>> 
>>>>>> For instance typical use is:
>>>>>> 	pr_info("Frobbing node %s\n", node->full_name);
>>>>>> 
>>>>>> Which can be written now as:
>>>>>> 	pr_info("Frobbing node %pO\n", node);
>>>> 
>>>>> Still disliking use of %p0.
>>>>> 
>>>> 
>>>> pO - Open Firmware
>>>> 
>>>> pT for tree is bad, cause we plan to use a tree type in the future in OF.
>>> 
>>> So, here's a radical thought. How about we reserve '%pO' for objects, as
>>> in kobjects.  We'll use extra flags to narrow down specifically to
>>> device tree nodes, but we could teach vsprintf() to treat a plain '%pO'
>>> as plain kobject pointer, and if it is able to recognize the kobj_type,
>>> then run a specific decoder to format it.
>>> 
>>> This also gives us a namespace for various kinds of firmware data
>>> output. %Od could be a struct device, %On for device tree node, %Oa for
>>> an ACPI node, etc.
>>> 
>> 
>> I’m fine with this. I also have a patch to turn an overlay to a kobj
>> so this fits naturally.
>> 
>> OTOH if we do this, I would expect to rework the custom printk infrastructure
>> to be more generic.
>> 
>> IMHO having the format specifier and the format print methods in lib/vsprintf.c
>> is not very nice.
>> 
>> How about having a way to register object printk handlers with something like that?
>> We could put that in a special linker section and have the printk method pass control
>> there.
>> 
>> PRINTK_OBJFMT(’n’, printk_objfmt_device_node);
>> 
>> We might have to make a few printk methods public however.
> 
> Honestly, I think trying to add registration is an overengineered
> solution at this point. We're not hitting a wall on the complexity of
> vsprintf.c, and having them all in one place helps to ensure we don't
> have conflicts.
> 
>> 
>>> I've dropped the refcount decoder. I know it is useful for debugging the
>>> core DT code, but it isn't something that will be used generally. Plus
>>> the returned value cannot be relied upon to be stable because there may
>>> be other code currently iterating over the tree.
>>> 
>> 
>> Yeah, I know it’s not something to rely on. If we do %pOk to be kobj
>> debug I can add it back in.
> 
> Yes, that would be a good place to have refcount output.
> 
>>> +Device tree nodes:
>>> +
>>> +	%pOn[fnpPcCFr]
>>> +
>>> +	For printing device tree nodes. The optional arguments are:
>>> +	    f device node full_name
>>> +	    n device node name
>>> +	    p device node phandle
>>> +	    P device node path spec (name + @unit)
>>> +	    F device node flags
>>> +	    c major compatible string
>>> +	    C full compatible string
>>> +	Without any arguments prints full_name (same as %pOnf)
>>> +	The separator when using multiple arguments is ‘:’
>> ^ separator is ‘.'
> 
> ? I'm confused? The separator that I'm using is a colon. ':'  Where do
> you see ','? I don't think ',' would be a good separator because it
> appears in node names and compatible strings. Originally, I think you
> were using pipe '|', but my personal opinion is that ':' is better
> because there is already precidence as a separator.
> 

Ugh, -EJETLAG.

You’re correct, sorry for the confusion.

> g.

Regards

— Pantelis

Re: [PATCH] of: Custom printk format specifier for device node

[Grant Likely]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1338 bytes --]

On Tue, 31 Mar 2015 13:03:05 +0300
, Pantelis Antoniou <pantelis.antoniou@konsulko.com>
 wrote:
> > +Device tree nodes:
> > +
> > +	%pOn[fnpPcCFr]
> > +
> > +	For printing device tree nodes. The optional arguments are:
> > +	    f device node full_name
> > +	    n device node name
> > +	    p device node phandle
> > +	    P device node path spec (name + @unit)
> > +	    F device node flags
> > +	    c major compatible string
> > +	    C full compatible string
> > +	Without any arguments prints full_name (same as %pOnf)
> > +	The separator when using multiple arguments is ‘:’
> ^ separator is ‘.'
> > +
> 
> > +	Examples:
> > +
> > +	%pOn	/foo/bar@0			- Node full name
> > +	%pOnf	/foo/bar@0			- Same as above
> > +	%pOnfp	/foo/bar@0:10			- Node full name + phandle
> > +	%pOnfcF	/foo/bar@0:foo,device:--P-	- Node full name +
> > +	                                          major compatible string +
> > +						  node flags
> > +							D - dynamic
> > +							d - detached
> > +							P - Populated
> > +							B - Populated bus
> > +

Thinking about this more, I'd like to suggest a different format that
gives us a nice hack on the name that makes it easy to remember:
	'%pOF[...]'
'O' still means 'object', but it is also overloaded for Open Firmware.
That still leaves %pO? for other object types.  What do you think?

g.

Re: [PATCH] of: Custom printk format specifier for device node

[Grant Likely]

On Tue, 31 Mar 2015 13:03:05 +0300
, Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
 wrote:
> > +Device tree nodes:
> > +
> > +	%pOn[fnpPcCFr]
> > +
> > +	For printing device tree nodes. The optional arguments are:
> > +	    f device node full_name
> > +	    n device node name
> > +	    p device node phandle
> > +	    P device node path spec (name + @unit)
> > +	    F device node flags
> > +	    c major compatible string
> > +	    C full compatible string
> > +	Without any arguments prints full_name (same as %pOnf)
> > +	The separator when using multiple arguments is ‘:’
> ^ separator is ‘.'
> > +
> 
> > +	Examples:
> > +
> > +	%pOn	/foo/bar@0			- Node full name
> > +	%pOnf	/foo/bar@0			- Same as above
> > +	%pOnfp	/foo/bar@0:10			- Node full name + phandle
> > +	%pOnfcF	/foo/bar@0:foo,device:--P-	- Node full name +
> > +	                                          major compatible string +
> > +						  node flags
> > +							D - dynamic
> > +							d - detached
> > +							P - Populated
> > +							B - Populated bus
> > +

Thinking about this more, I'd like to suggest a different format that
gives us a nice hack on the name that makes it easy to remember:
	'%pOF[...]'
'O' still means 'object', but it is also overloaded for Open Firmware.
That still leaves %pO? for other object types.  What do you think?

g.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] of: Custom printk format specifier for device node

[Joe Perches]

On Tue, 2015-03-31 at 21:52 -0700, Grant Likely wrote:
> Thinking about this more, I'd like to suggest a different format that
> gives us a nice hack on the name that makes it easy to remember:
> 	'%pOF[...]'
> 'O' still means 'object', but it is also overloaded for Open Firmware.
> That still leaves %pO? for other object types.  What do you think?

I think that's fine.

Re: [PATCH] of: Custom printk format specifier for device node

[Joe Perches]

On Tue, 2015-01-20 at 20:06 +0200, Pantelis Antoniou wrote:
> On Jan 20, 2015, at 19:59 , Joe Perches <joe@perches.com> wrote:
> > On Tue, 2015-01-20 at 16:52 +0200, Pantelis Antoniou wrote:
> >>> On Jan 20, 2015, at 16:47 , Rob Herring <robherring2@gmail.com> wrote:
> >>> On Tue, Jan 20, 2015 at 8:34 AM, Pantelis Antoniou
> >>> <pantelis.antoniou@konsulko.com> wrote:
> >>>> 90% of the usage of device node's full_name is printing it out
> >>>> in a kernel message. Preparing for the eventual delayed allocation
> >>>> introduce a custom printk format specifier that is both more
> >>>> compact and more pleasant to the eye.
> > []
> >>>> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
> > []
> >>>> +Device tree nodes:
> >>>> +
> >>>> +       %pO{,1,2}
> >>> 
> >>> 'O' is not very obvious, but I imagine we are somewhat limted in our
> >>> choice here?
> >>> 
> >> 
> >> All the good women are married, all the handsome men are gay, all the obvious
> >> format specifiers are taken.
> > 
> > Not really at all.
> > 
> > I quite dislike '0' as the format type specifier for
> > a device tree node as there's no mnemonic mapping.
> > 
> There’s only so many characters one can use; [DdNn] are taken. And it’s O != 0.

The '[Dd]' entry type doesn't just have to be used for dentries.
It could also be used for Device Tree Nodes via 'DTN[x]'

'T' for tree could also be used.

Re: [PATCH] of: Custom printk format specifier for device node

[Pantelis Antoniou]

Hi Joe,

> On Jan 20, 2015, at 19:59 , Joe Perches <joe@perches.com> wrote:
> 
> On Tue, 2015-01-20 at 16:52 +0200, Pantelis Antoniou wrote:
>> Hi Rob,
>> 
>>> On Jan 20, 2015, at 16:47 , Rob Herring <robherring2@gmail.com> wrote:
>>> 
>>> On Tue, Jan 20, 2015 at 8:34 AM, Pantelis Antoniou
>>> <pantelis.antoniou@konsulko.com> wrote:
>>>> 90% of the usage of device node's full_name is printing it out
>>>> in a kernel message. Preparing for the eventual delayed allocation
>>>> introduce a custom printk format specifier that is both more
>>>> compact and more pleasant to the eye.
> []
>>>> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
> []
>>>> +Device tree nodes:
>>>> +
>>>> +       %pO{,1,2}
>>> 
>>> 'O' is not very obvious, but I imagine we are somewhat limted in our
>>> choice here?
>>> 
>> 
>> All the good women are married, all the handsome men are gay, all the obvious
>> format specifiers are taken.
> 
> Not really at all.
> 
> I quite dislike '0' as the format type specifier for
> a device tree node as there's no mnemonic mapping.
> 

There’s only so many characters one can use; [DdNn] are taken. And it’s O != 0.

I am open to suggestions.

Regards

— Pantelis

Re: [PATCH] of: Custom printk format specifier for device node

[Pantelis Antoniou]

Hi Joe,

> On Jan 20, 2015, at 19:59 , Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
> 
> On Tue, 2015-01-20 at 16:52 +0200, Pantelis Antoniou wrote:
>> Hi Rob,
>> 
>>> On Jan 20, 2015, at 16:47 , Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> 
>>> On Tue, Jan 20, 2015 at 8:34 AM, Pantelis Antoniou
>>> <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
>>>> 90% of the usage of device node's full_name is printing it out
>>>> in a kernel message. Preparing for the eventual delayed allocation
>>>> introduce a custom printk format specifier that is both more
>>>> compact and more pleasant to the eye.
> []
>>>> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
> []
>>>> +Device tree nodes:
>>>> +
>>>> +       %pO{,1,2}
>>> 
>>> 'O' is not very obvious, but I imagine we are somewhat limted in our
>>> choice here?
>>> 
>> 
>> All the good women are married, all the handsome men are gay, all the obvious
>> format specifiers are taken.
> 
> Not really at all.
> 
> I quite dislike '0' as the format type specifier for
> a device tree node as there's no mnemonic mapping.
> 

There’s only so many characters one can use; [DdNn] are taken. And it’s O != 0.

I am open to suggestions.

Regards

— Pantelis

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] of: Custom printk format specifier for device node

[Rob Herring]

On Tue, Jan 20, 2015 at 11:59 AM, Joe Perches <joe@perches.com> wrote:
> On Tue, 2015-01-20 at 16:52 +0200, Pantelis Antoniou wrote:
>> Hi Rob,
>>
>> > On Jan 20, 2015, at 16:47 , Rob Herring <robherring2@gmail.com> wrote:
>> >
>> > On Tue, Jan 20, 2015 at 8:34 AM, Pantelis Antoniou
>> > <pantelis.antoniou@konsulko.com> wrote:
>> >> 90% of the usage of device node's full_name is printing it out
>> >> in a kernel message. Preparing for the eventual delayed allocation
>> >> introduce a custom printk format specifier that is both more
>> >> compact and more pleasant to the eye.
> []
>> >> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
> []
>> >> +Device tree nodes:
>> >> +
>> >> +       %pO{,1,2}
>> >
>> > 'O' is not very obvious, but I imagine we are somewhat limted in our
>> > choice here?
>> >
>>
>> All the good women are married, all the handsome men are gay, all the obvious
>> format specifiers are taken.
>
> Not really at all.
>
> I quite dislike '0' as the format type specifier for
> a device tree node as there's no mnemonic mapping.

It is O, not zero if that matters. Do you have 
>
>
>

Re: [PATCH] of: Custom printk format specifier for device node

[Joe Perches]

On Tue, 2015-01-20 at 16:52 +0200, Pantelis Antoniou wrote:
> Hi Rob,
> 
> > On Jan 20, 2015, at 16:47 , Rob Herring <robherring2@gmail.com> wrote:
> > 
> > On Tue, Jan 20, 2015 at 8:34 AM, Pantelis Antoniou
> > <pantelis.antoniou@konsulko.com> wrote:
> >> 90% of the usage of device node's full_name is printing it out
> >> in a kernel message. Preparing for the eventual delayed allocation
> >> introduce a custom printk format specifier that is both more
> >> compact and more pleasant to the eye.
[]
> >> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
[]
> >> +Device tree nodes:
> >> +
> >> +       %pO{,1,2}
> > 
> > 'O' is not very obvious, but I imagine we are somewhat limted in our
> > choice here?
> > 
> 
> All the good women are married, all the handsome men are gay, all the obvious
> format specifiers are taken.

Not really at all.

I quite dislike '0' as the format type specifier for
a device tree node as there's no mnemonic mapping.

Re: [PATCH] of: Custom printk format specifier for device node

[Rob Herring]

On Tue, Jan 20, 2015 at 10:05 AM, Måns Rullgård <mans@mansr.com> wrote:
> Pantelis Antoniou <pantelis.antoniou@konsulko.com> writes:
>
>> Hi Geert,
>>
>>> On Jan 20, 2015, at 17:24 , Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>
>>> On Tue, Jan 20, 2015 at 3:47 PM, Rob Herring <robherring2@gmail.com> wrote:
>>>>> +       Examples:
>>>>> +
>>>>> +       %pO     /foo/bar@0              - Node full name
>>>>> +       %pO0    /foo/bar@0              - Same as above
>>>>> +       %pO1    /foo/bar@0[10]          - Node full name + phandle
>>>>> +       %pO2    /foo/bar@0[10:DdPB]     - Node full name + phandle + node flags
>>>>> +                                        D - dynamic
>>>>> +                                        d - detached
>>>>> +                                        P - Populated
>>>>> +                                        B - Populated bus
>>>>
>>>> We should think about what else we want to print for a node. Perhaps
>>>> 'On' for name, 'Oc' for compatible, etc.
>>>
>>> I was just going to say "The least verbose variant is name, not full_name”.
>>>
>>
>> Unfortunately in the context of device tree nodes ‘name' is usually
>> not what you want to print to identify the node in question. ‘name’ is
>> usually not unique.
>
> Name and address without the full path is usually a good compromise
> between uniqueness (it is usually unique for memory-mapped things) and
> verbosity.

How much of the address is in the name depends on how the address
translation is done. I don't think we really need to do full address
translations here.

%pOn     /foo/bar@0              - Node full name
%pOn0   bar@0              - Node name and unit address
%pOn1    /foo/bar@0[10]          - Node full name + phandle
%pOn2    /foo/bar@0[10:DdPB]     - Node full name + phandle + node flags
%pOc      vendor,foo-bar          - Most significant compatible string

We could do phandle and/or node flags as separate specifiers such as
%pOf for flags.

I'm not proposing implementing all these now, but just want to make
sure we have a structure to do so later.

Rob

Re: [PATCH] of: Custom printk format specifier for device node

[Måns Rullgård]

Pantelis Antoniou <pantelis.antoniou@konsulko.com> writes:

> Hi Geert,
>
>> On Jan 20, 2015, at 17:24 , Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> 
>> On Tue, Jan 20, 2015 at 3:47 PM, Rob Herring <robherring2@gmail.com> wrote:
>>>> +       Examples:
>>>> +
>>>> +       %pO     /foo/bar@0              - Node full name
>>>> +       %pO0    /foo/bar@0              - Same as above
>>>> +       %pO1    /foo/bar@0[10]          - Node full name + phandle
>>>> +       %pO2    /foo/bar@0[10:DdPB]     - Node full name + phandle + node flags
>>>> +                                        D - dynamic
>>>> +                                        d - detached
>>>> +                                        P - Populated
>>>> +                                        B - Populated bus
>>> 
>>> We should think about what else we want to print for a node. Perhaps
>>> 'On' for name, 'Oc' for compatible, etc.
>> 
>> I was just going to say "The least verbose variant is name, not full_name”.
>> 
>
> Unfortunately in the context of device tree nodes ‘name' is usually
> not what you want to print to identify the node in question. ‘name’ is
> usually not unique.

Name and address without the full path is usually a good compromise
between uniqueness (it is usually unique for memory-mapped things) and
verbosity.

-- 
Måns Rullgård
mans@mansr.com

Re: [PATCH] of: Custom printk format specifier for device node

[Måns Rullgård]

Pantelis Antoniou <pantelis.antoniou@konsulko.com> writes:

> Hi Geert,
>
>> On Jan 20, 2015, at 17:24 , Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> 
>> On Tue, Jan 20, 2015 at 3:47 PM, Rob Herring <robherring2@gmail.com> wrote:
>>>> +       Examples:
>>>> +
>>>> +       %pO     /foo/bar@0              - Node full name
>>>> +       %pO0    /foo/bar@0              - Same as above
>>>> +       %pO1    /foo/bar@0[10]          - Node full name + phandle
>>>> +       %pO2    /foo/bar@0[10:DdPB]     - Node full name + phandle + node flags
>>>> +                                        D - dynamic
>>>> +                                        d - detached
>>>> +                                        P - Populated
>>>> +                                        B - Populated bus
>>> 
>>> We should think about what else we want to print for a node. Perhaps
>>> 'On' for name, 'Oc' for compatible, etc.
>> 
>> I was just going to say "The least verbose variant is name, not full_name”.
>> 
>
> Unfortunately in the context of device tree nodes ‘name' is usually
> not what you want to print to identify the node in question. ‘name’ is
> usually not unique.

Name and address without the full path is usually a good compromise
between uniqueness (it is usually unique for memory-mapped things) and
verbosity.

-- 
Måns Rullgård
mans@mansr.com

Re: [PATCH] of: Custom printk format specifier for device node

[Pantelis Antoniou]

Hi Geert,

> On Jan 20, 2015, at 17:24 , Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> 
> On Tue, Jan 20, 2015 at 3:47 PM, Rob Herring <robherring2@gmail.com> wrote:
>>> +       Examples:
>>> +
>>> +       %pO     /foo/bar@0              - Node full name
>>> +       %pO0    /foo/bar@0              - Same as above
>>> +       %pO1    /foo/bar@0[10]          - Node full name + phandle
>>> +       %pO2    /foo/bar@0[10:DdPB]     - Node full name + phandle + node flags
>>> +                                        D - dynamic
>>> +                                        d - detached
>>> +                                        P - Populated
>>> +                                        B - Populated bus
>> 
>> We should think about what else we want to print for a node. Perhaps
>> 'On' for name, 'Oc' for compatible, etc.
> 
> I was just going to say "The least verbose variant is name, not full_name”.
> 

Unfortunately in the context of device tree nodes ‘name' is usually not what
you want to print to identify the node in question. ‘name’ is usually not unique.

We can add a format specifier for it though.

> Gr{oetje,eeting}s,
> 
>                        Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                -- Linus Torvalds

Regards

— Pantelis

Re: [PATCH] of: Custom printk format specifier for device node

[Geert Uytterhoeven]

On Tue, Jan 20, 2015 at 3:47 PM, Rob Herring <robherring2@gmail.com> wrote:
>> +       Examples:
>> +
>> +       %pO     /foo/bar@0              - Node full name
>> +       %pO0    /foo/bar@0              - Same as above
>> +       %pO1    /foo/bar@0[10]          - Node full name + phandle
>> +       %pO2    /foo/bar@0[10:DdPB]     - Node full name + phandle + node flags
>> +                                        D - dynamic
>> +                                        d - detached
>> +                                        P - Populated
>> +                                        B - Populated bus
>
> We should think about what else we want to print for a node. Perhaps
> 'On' for name, 'Oc' for compatible, etc.

I was just going to say "The least verbose variant is name, not full_name".

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] of: Custom printk format specifier for device node

[Pantelis Antoniou]

Hi Rob,

> On Jan 20, 2015, at 16:47 , Rob Herring <robherring2@gmail.com> wrote:
> 
> On Tue, Jan 20, 2015 at 8:34 AM, Pantelis Antoniou
> <pantelis.antoniou@konsulko.com> wrote:
>> 90% of the usage of device node's full_name is printing it out
>> in a kernel message. Preparing for the eventual delayed allocation
>> introduce a custom printk format specifier that is both more
>> compact and more pleasant to the eye.
> 
> What a great idea. ;)
> 

It was a stroke of genius I tell you… :)

>> For instance typical use is:
>>        pr_info("Frobbing node %s\n", node->full_name);
>> 
>> Which can be written now as:
>>        pr_info("Frobbing node %pO\n", node);
>> 
>> A verbose format specifier (1-2) can be used to print extra
>> information about the node like its phandle and node flags.
>> 
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>> ---
>> Documentation/printk-formats.txt | 18 +++++++++++
>> lib/vsprintf.c                   | 67 ++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 85 insertions(+)
>> 
>> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
>> index 5a615c1..6ad199f 100644
>> --- a/Documentation/printk-formats.txt
>> +++ b/Documentation/printk-formats.txt
>> @@ -231,6 +231,24 @@ struct va_format:
>>        Do not use this feature without some mechanism to verify the
>>        correctness of the format string and va_list arguments.
>> 
>> +Device tree nodes:
>> +
>> +       %pO{,1,2}
> 
> 'O' is not very obvious, but I imagine we are somewhat limted in our
> choice here?
> 

All the good women are married, all the handsome men are gay, all the obvious
format specifiers are taken.

>> +
>> +       For printing device tree nodes. The (optional) number is the verbosity
>> +       level.
>> +
>> +       Examples:
>> +
>> +       %pO     /foo/bar@0              - Node full name
>> +       %pO0    /foo/bar@0              - Same as above
>> +       %pO1    /foo/bar@0[10]          - Node full name + phandle
>> +       %pO2    /foo/bar@0[10:DdPB]     - Node full name + phandle + node flags
>> +                                        D - dynamic
>> +                                        d - detached
>> +                                        P - Populated
>> +                                        B - Populated bus
> 
> We should think about what else we want to print for a node. Perhaps
> 'On' for name, 'Oc' for compatible, etc.
> 

The verbosity argument is something simple that works, but I take requests...

>> +
>> u64 SHOULD be printed with %llu/%llx:
>> 
>>        printk("%llu", u64_var);
>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>> index ec337f6..72896cc 100644
>> --- a/lib/vsprintf.c
>> +++ b/lib/vsprintf.c
>> @@ -29,6 +29,7 @@
>> #include <linux/dcache.h>
>> #include <linux/cred.h>
>> #include <net/addrconf.h>
>> +#include <linux/of.h>
>> 
>> #include <asm/page.h>          /* for PAGE_SIZE */
>> #include <asm/sections.h>      /* for dereference_function_descriptor() */
>> @@ -1240,6 +1241,68 @@ char *address_val(char *buf, char *end, const void *addr,
>>        return number(buf, end, num, spec);
>> }
>> 
>> +#if IS_ENABLED(CONFIG_OF)
> 
> You can move this into the function:
> 
> if (!IS_ENABLED(CONFIG_OF)
>  return NULL;
> 
> Looks like you'll need the patch for empty of_node_check_flag as well.
> 

Yes, I tried to avoid that.

>> +static noinline_for_stack
>> +char *device_node_string(char *buf, char *end, struct device_node *dn,
>> +                        struct printf_spec spec, const char *fmt)
>> +{
>> +       char tbuf[sizeof("[xxxxxxxxxx:xxxx]") + 1];
>> +       const char *full_name;
>> +       int verbosity, len, flen, i;
>> +
>> +       if ((unsigned long)dn < PAGE_SIZE)
>> +               return string(buf, end, "(null)", spec);
>> +
>> +       full_name = of_node_full_name(dn);
>> +       verbosity = 0;
>> +       if (fmt[1] >= '0' && fmt[1] <= '2')
>> +               verbosity = fmt[1] - '0';
>> +
>> +       /* fast and simple case */
>> +       switch (verbosity) {
>> +       default:
>> +       case 0:
>> +               tbuf[0] = '\0';
>> +               break;
>> +       case 1:
>> +               snprintf(tbuf, sizeof(tbuf), "[%u]",
>> +                       (u32)dn->phandle);
>> +               break;
>> +       case 2:
>> +               snprintf(tbuf, sizeof(tbuf), "[%u:%c%c%c%c]",
>> +                       (u32)dn->phandle,
>> +                       of_node_check_flag(dn, OF_DYNAMIC) ? 'D' : '-',
>> +                       of_node_check_flag(dn, OF_DETACHED) ? 'd' : '-',
>> +                       of_node_check_flag(dn, OF_POPULATED) ? 'P' : '-',
>> +                       of_node_check_flag(dn, OF_POPULATED_BUS) ? 'B' : '-');
>> +               break;
>> +       }
>> +       flen = strlen(full_name);
>> +       len = flen + strlen(tbuf);
>> +       if (spec.precision > 0 && len > spec.precision)
>> +               len = spec.precision;
>> +
>> +       if (!(spec.flags & LEFT)) {
>> +               while (len < spec.field_width--) {
>> +                       if (buf < end)
>> +                               *buf = ' ';
>> +                       ++buf;
>> +               }
>> +       }
>> +       for (i = 0; i < len; ++i) {
>> +               if (buf < end)
>> +                       *buf = i < flen ? full_name[i] : tbuf[i - flen];
>> +               ++buf;
>> +       }
>> +       while (len < spec.field_width--) {
>> +               if (buf < end)
>> +                       *buf = ' ';
>> +               ++buf;
>> +       }
>> +       return buf;
>> +}
>> +#endif
>> +
>> int kptr_restrict __read_mostly;
>> 
>> /*
>> @@ -1459,6 +1522,10 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>>                return dentry_name(buf, end,
>>                                   ((const struct file *)ptr)->f_path.dentry,
>>                                   spec, fmt);
>> +#if IS_ENABLED(CONFIG_OF)
> 
> With the above, then this ifdef should not be needed.
> 

Yes

>> +       case 'O':
>> +               return device_node_string(buf, end, ptr, spec, fmt);
>> +#endif
>>        }
>>        spec.flags |= SMALL;
>>        if (spec.field_width == -1) {
>> --
>> 1.7.12
>> 

OK, I’ll prepare the empty flag accessors patch and I’ll repost with the requested changes.

Regards

— Pantelis

Re: [PATCH] of: Custom printk format specifier for device node

[Rob Herring]

On Tue, Jan 20, 2015 at 8:34 AM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> 90% of the usage of device node's full_name is printing it out
> in a kernel message. Preparing for the eventual delayed allocation
> introduce a custom printk format specifier that is both more
> compact and more pleasant to the eye.

What a great idea. ;)

> For instance typical use is:
>         pr_info("Frobbing node %s\n", node->full_name);
>
> Which can be written now as:
>         pr_info("Frobbing node %pO\n", node);
>
> A verbose format specifier (1-2) can be used to print extra
> information about the node like its phandle and node flags.
>
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> ---
>  Documentation/printk-formats.txt | 18 +++++++++++
>  lib/vsprintf.c                   | 67 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 85 insertions(+)
>
> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
> index 5a615c1..6ad199f 100644
> --- a/Documentation/printk-formats.txt
> +++ b/Documentation/printk-formats.txt
> @@ -231,6 +231,24 @@ struct va_format:
>         Do not use this feature without some mechanism to verify the
>         correctness of the format string and va_list arguments.
>
> +Device tree nodes:
> +
> +       %pO{,1,2}

'O' is not very obvious, but I imagine we are somewhat limted in our
choice here?

> +
> +       For printing device tree nodes. The (optional) number is the verbosity
> +       level.
> +
> +       Examples:
> +
> +       %pO     /foo/bar@0              - Node full name
> +       %pO0    /foo/bar@0              - Same as above
> +       %pO1    /foo/bar@0[10]          - Node full name + phandle
> +       %pO2    /foo/bar@0[10:DdPB]     - Node full name + phandle + node flags
> +                                        D - dynamic
> +                                        d - detached
> +                                        P - Populated
> +                                        B - Populated bus

We should think about what else we want to print for a node. Perhaps
'On' for name, 'Oc' for compatible, etc.

> +
>  u64 SHOULD be printed with %llu/%llx:
>
>         printk("%llu", u64_var);
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index ec337f6..72896cc 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -29,6 +29,7 @@
>  #include <linux/dcache.h>
>  #include <linux/cred.h>
>  #include <net/addrconf.h>
> +#include <linux/of.h>
>
>  #include <asm/page.h>          /* for PAGE_SIZE */
>  #include <asm/sections.h>      /* for dereference_function_descriptor() */
> @@ -1240,6 +1241,68 @@ char *address_val(char *buf, char *end, const void *addr,
>         return number(buf, end, num, spec);
>  }
>
> +#if IS_ENABLED(CONFIG_OF)

You can move this into the function:

if (!IS_ENABLED(CONFIG_OF)
  return NULL;

Looks like you'll need the patch for empty of_node_check_flag as well.

> +static noinline_for_stack
> +char *device_node_string(char *buf, char *end, struct device_node *dn,
> +                        struct printf_spec spec, const char *fmt)
> +{
> +       char tbuf[sizeof("[xxxxxxxxxx:xxxx]") + 1];
> +       const char *full_name;
> +       int verbosity, len, flen, i;
> +
> +       if ((unsigned long)dn < PAGE_SIZE)
> +               return string(buf, end, "(null)", spec);
> +
> +       full_name = of_node_full_name(dn);
> +       verbosity = 0;
> +       if (fmt[1] >= '0' && fmt[1] <= '2')
> +               verbosity = fmt[1] - '0';
> +
> +       /* fast and simple case */
> +       switch (verbosity) {
> +       default:
> +       case 0:
> +               tbuf[0] = '\0';
> +               break;
> +       case 1:
> +               snprintf(tbuf, sizeof(tbuf), "[%u]",
> +                       (u32)dn->phandle);
> +               break;
> +       case 2:
> +               snprintf(tbuf, sizeof(tbuf), "[%u:%c%c%c%c]",
> +                       (u32)dn->phandle,
> +                       of_node_check_flag(dn, OF_DYNAMIC) ? 'D' : '-',
> +                       of_node_check_flag(dn, OF_DETACHED) ? 'd' : '-',
> +                       of_node_check_flag(dn, OF_POPULATED) ? 'P' : '-',
> +                       of_node_check_flag(dn, OF_POPULATED_BUS) ? 'B' : '-');
> +               break;
> +       }
> +       flen = strlen(full_name);
> +       len = flen + strlen(tbuf);
> +       if (spec.precision > 0 && len > spec.precision)
> +               len = spec.precision;
> +
> +       if (!(spec.flags & LEFT)) {
> +               while (len < spec.field_width--) {
> +                       if (buf < end)
> +                               *buf = ' ';
> +                       ++buf;
> +               }
> +       }
> +       for (i = 0; i < len; ++i) {
> +               if (buf < end)
> +                       *buf = i < flen ? full_name[i] : tbuf[i - flen];
> +               ++buf;
> +       }
> +       while (len < spec.field_width--) {
> +               if (buf < end)
> +                       *buf = ' ';
> +               ++buf;
> +       }
> +       return buf;
> +}
> +#endif
> +
>  int kptr_restrict __read_mostly;
>
>  /*
> @@ -1459,6 +1522,10 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>                 return dentry_name(buf, end,
>                                    ((const struct file *)ptr)->f_path.dentry,
>                                    spec, fmt);
> +#if IS_ENABLED(CONFIG_OF)

With the above, then this ifdef should not be needed.

> +       case 'O':
> +               return device_node_string(buf, end, ptr, spec, fmt);
> +#endif
>         }
>         spec.flags |= SMALL;
>         if (spec.field_width == -1) {
> --
> 1.7.12
>

[PATCH] of: Custom printk format specifier for device node

[Pantelis Antoniou]

90% of the usage of device node's full_name is printing it out
in a kernel message. Preparing for the eventual delayed allocation
introduce a custom printk format specifier that is both more
compact and more pleasant to the eye.

For instance typical use is:
	pr_info("Frobbing node %s\n", node->full_name);

Which can be written now as:
	pr_info("Frobbing node %pO\n", node);

A verbose format specifier (1-2) can be used to print extra
information about the node like its phandle and node flags.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 Documentation/printk-formats.txt | 18 +++++++++++
 lib/vsprintf.c                   | 67 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 85 insertions(+)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 5a615c1..6ad199f 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -231,6 +231,24 @@ struct va_format:
 	Do not use this feature without some mechanism to verify the
 	correctness of the format string and va_list arguments.
 
+Device tree nodes:
+
+	%pO{,1,2}
+
+	For printing device tree nodes. The (optional) number is the verbosity
+	level.
+
+	Examples:
+
+	%pO	/foo/bar@0		- Node full name
+	%pO0	/foo/bar@0		- Same as above
+	%pO1	/foo/bar@0[10]		- Node full name + phandle
+	%pO2	/foo/bar@0[10:DdPB]	- Node full name + phandle + node flags
+					 D - dynamic
+					 d - detached
+					 P - Populated
+					 B - Populated bus
+
 u64 SHOULD be printed with %llu/%llx:
 
 	printk("%llu", u64_var);
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index ec337f6..72896cc 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -29,6 +29,7 @@
 #include <linux/dcache.h>
 #include <linux/cred.h>
 #include <net/addrconf.h>
+#include <linux/of.h>
 
 #include <asm/page.h>		/* for PAGE_SIZE */
 #include <asm/sections.h>	/* for dereference_function_descriptor() */
@@ -1240,6 +1241,68 @@ char *address_val(char *buf, char *end, const void *addr,
 	return number(buf, end, num, spec);
 }
 
+#if IS_ENABLED(CONFIG_OF)
+static noinline_for_stack
+char *device_node_string(char *buf, char *end, struct device_node *dn,
+			 struct printf_spec spec, const char *fmt)
+{
+	char tbuf[sizeof("[xxxxxxxxxx:xxxx]") + 1];
+	const char *full_name;
+	int verbosity, len, flen, i;
+
+	if ((unsigned long)dn < PAGE_SIZE)
+		return string(buf, end, "(null)", spec);
+
+	full_name = of_node_full_name(dn);
+	verbosity = 0;
+	if (fmt[1] >= '0' && fmt[1] <= '2')
+		verbosity = fmt[1] - '0';
+
+	/* fast and simple case */
+	switch (verbosity) {
+	default:
+	case 0:
+		tbuf[0] = '\0';
+		break;
+	case 1:
+		snprintf(tbuf, sizeof(tbuf), "[%u]",
+			(u32)dn->phandle);
+		break;
+	case 2:
+		snprintf(tbuf, sizeof(tbuf), "[%u:%c%c%c%c]",
+			(u32)dn->phandle,
+			of_node_check_flag(dn, OF_DYNAMIC) ? 'D' : '-',
+			of_node_check_flag(dn, OF_DETACHED) ? 'd' : '-',
+			of_node_check_flag(dn, OF_POPULATED) ? 'P' : '-',
+			of_node_check_flag(dn, OF_POPULATED_BUS) ? 'B' : '-');
+		break;
+	}
+	flen = strlen(full_name);
+	len = flen + strlen(tbuf);
+	if (spec.precision > 0 && len > spec.precision)
+		len = spec.precision;
+
+	if (!(spec.flags & LEFT)) {
+		while (len < spec.field_width--) {
+			if (buf < end)
+				*buf = ' ';
+			++buf;
+		}
+	}
+	for (i = 0; i < len; ++i) {
+		if (buf < end)
+			*buf = i < flen ? full_name[i] : tbuf[i - flen];
+		++buf;
+	}
+	while (len < spec.field_width--) {
+		if (buf < end)
+			*buf = ' ';
+		++buf;
+	}
+	return buf;
+}
+#endif
+
 int kptr_restrict __read_mostly;
 
 /*
@@ -1459,6 +1522,10 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		return dentry_name(buf, end,
 				   ((const struct file *)ptr)->f_path.dentry,
 				   spec, fmt);
+#if IS_ENABLED(CONFIG_OF)
+	case 'O':
+		return device_node_string(buf, end, ptr, spec, fmt);
+#endif
 	}
 	spec.flags |= SMALL;
 	if (spec.field_width == -1) {
-- 
1.7.12

[PATCH] of: Custom printk format specifier for device node

[Pantelis Antoniou]

90% of the usage of device node's full_name is printing it out
in a kernel message. Preparing for the eventual delayed allocation
introduce a custom printk format specifier that is both more
compact and more pleasant to the eye.

For instance typical use is:
	pr_info("Frobbing node %s\n", node->full_name);

Which can be written now as:
	pr_info("Frobbing node %pO\n", node);

A verbose format specifier (1-2) can be used to print extra
information about the node like its phandle and node flags.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
 Documentation/printk-formats.txt | 18 +++++++++++
 lib/vsprintf.c                   | 67 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 85 insertions(+)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 5a615c1..6ad199f 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -231,6 +231,24 @@ struct va_format:
 	Do not use this feature without some mechanism to verify the
 	correctness of the format string and va_list arguments.
 
+Device tree nodes:
+
+	%pO{,1,2}
+
+	For printing device tree nodes. The (optional) number is the verbosity
+	level.
+
+	Examples:
+
+	%pO	/foo/bar@0		- Node full name
+	%pO0	/foo/bar@0		- Same as above
+	%pO1	/foo/bar@0[10]		- Node full name + phandle
+	%pO2	/foo/bar@0[10:DdPB]	- Node full name + phandle + node flags
+					 D - dynamic
+					 d - detached
+					 P - Populated
+					 B - Populated bus
+
 u64 SHOULD be printed with %llu/%llx:
 
 	printk("%llu", u64_var);
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index ec337f6..72896cc 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -29,6 +29,7 @@
 #include <linux/dcache.h>
 #include <linux/cred.h>
 #include <net/addrconf.h>
+#include <linux/of.h>
 
 #include <asm/page.h>		/* for PAGE_SIZE */
 #include <asm/sections.h>	/* for dereference_function_descriptor() */
@@ -1240,6 +1241,68 @@ char *address_val(char *buf, char *end, const void *addr,
 	return number(buf, end, num, spec);
 }
 
+#if IS_ENABLED(CONFIG_OF)
+static noinline_for_stack
+char *device_node_string(char *buf, char *end, struct device_node *dn,
+			 struct printf_spec spec, const char *fmt)
+{
+	char tbuf[sizeof("[xxxxxxxxxx:xxxx]") + 1];
+	const char *full_name;
+	int verbosity, len, flen, i;
+
+	if ((unsigned long)dn < PAGE_SIZE)
+		return string(buf, end, "(null)", spec);
+
+	full_name = of_node_full_name(dn);
+	verbosity = 0;
+	if (fmt[1] >= '0' && fmt[1] <= '2')
+		verbosity = fmt[1] - '0';
+
+	/* fast and simple case */
+	switch (verbosity) {
+	default:
+	case 0:
+		tbuf[0] = '\0';
+		break;
+	case 1:
+		snprintf(tbuf, sizeof(tbuf), "[%u]",
+			(u32)dn->phandle);
+		break;
+	case 2:
+		snprintf(tbuf, sizeof(tbuf), "[%u:%c%c%c%c]",
+			(u32)dn->phandle,
+			of_node_check_flag(dn, OF_DYNAMIC) ? 'D' : '-',
+			of_node_check_flag(dn, OF_DETACHED) ? 'd' : '-',
+			of_node_check_flag(dn, OF_POPULATED) ? 'P' : '-',
+			of_node_check_flag(dn, OF_POPULATED_BUS) ? 'B' : '-');
+		break;
+	}
+	flen = strlen(full_name);
+	len = flen + strlen(tbuf);
+	if (spec.precision > 0 && len > spec.precision)
+		len = spec.precision;
+
+	if (!(spec.flags & LEFT)) {
+		while (len < spec.field_width--) {
+			if (buf < end)
+				*buf = ' ';
+			++buf;
+		}
+	}
+	for (i = 0; i < len; ++i) {
+		if (buf < end)
+			*buf = i < flen ? full_name[i] : tbuf[i - flen];
+		++buf;
+	}
+	while (len < spec.field_width--) {
+		if (buf < end)
+			*buf = ' ';
+		++buf;
+	}
+	return buf;
+}
+#endif
+
 int kptr_restrict __read_mostly;
 
 /*
@@ -1459,6 +1522,10 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		return dentry_name(buf, end,
 				   ((const struct file *)ptr)->f_path.dentry,
 				   spec, fmt);
+#if IS_ENABLED(CONFIG_OF)
+	case 'O':
+		return device_node_string(buf, end, ptr, spec, fmt);
+#endif
 	}
 	spec.flags |= SMALL;
 	if (spec.field_width == -1) {
-- 
1.7.12

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html