Message-Id: <8421E7C5-C0D9-4C86-AA37-F6507B3BCC57@konsulko.com>
Date:	Tue, 31 Mar 2015 13:03:05 +0300
From:	Pantelis Antoniou <pantelis.antoniou@...sulko.com>
To:	Grant Likely <grant.likely@...retlab.ca>
Cc:	Joe Perches <joe@...ches.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Rob Herring <robherring2@...il.com>,
	Randy Dunlap <rdunlap@...radead.org>,
	linux-doc@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] of: Custom printk format specifier for device node

Hi Grant,

> On Mar 30, 2015, at 22:04 , Grant Likely <grant.likely@...retlab.ca> wrote:
> 
> On Thu, 22 Jan 2015 22:31:46 +0200
> , Pantelis Antoniou <panto@...oniou-consulting.com>
> wrote:
>> Hi Joe,
>> 
>>> On Jan 21, 2015, at 19:37 , Joe Perches <joe@...ches.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@...sulko.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			- Node full name + phandle
>>>> +	%pOfcF	/foo/bar@...oo,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@...sulko.com>
> [grant.likely: Rework to be simpler]
> Signed-off-by: Grant Likely <grant.likely@...aro.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@...ev@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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists