Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1666205 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3/4] of: Custom printk format specifier for device node Rob Herring <robh@kernel.org> - 2017-06-14 22:40 +0200
    Re: [PATCH 3/4] of: Custom printk format specifier for device node Joe Perches <joe@perches.com> - 2017-06-14 23:00 +0200
      Re: [PATCH 3/4] of: Custom printk format specifier for device node Rob Herring <robh@kernel.org> - 2017-06-15 14:40 +0200
        Re: [PATCH 3/4] of: Custom printk format specifier for device node Joe Perches <joe@perches.com> - 2017-06-15 19:00 +0200
      Re: [PATCH 3/4] of: Custom printk format specifier for device node Rob Herring <robh@kernel.org> - 2017-06-15 23:30 +0200
        Re: [PATCH 3/4] of: Custom printk format specifier for device node Joe Perches <joe@perches.com> - 2017-06-16 00:00 +0200

#1666205 — [PATCH 3/4] of: Custom printk format specifier for device node

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

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 %pOF\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.

Originally written by Pantelis, but pretty much rewrote the core
function using existing string/number functions. The 2 passes were
unnecessary and have been removed. Also, updated the checkpatch.pl
check.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
Signed-off-by: Rob Herring <robh@kernel.org>
---
 Documentation/printk-formats.txt |  31 +++++++++
 lib/vsprintf.c                   | 135 ++++++++++++++++++++++++++++++++++++++-
 scripts/checkpatch.pl            |   2 +-
 3 files changed, 166 insertions(+), 2 deletions(-)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 5962949944fd..07bc088ba5f5 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -275,6 +275,37 @@ struct va_format:
 
 	Passed by reference.
 
+Device tree nodes:
+
+	%pOF[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:
+
+	%pOF	/foo/bar@0			- Node full name
+	%pOFf	/foo/bar@0			- Same as above
+	%pOFfp	/foo/bar@0|10			- Node full name + phandle
+	%pOFfcF	/foo/bar@0|foo,device|--P-	- Node full name +
+	                                          major compatible string +
+						  node flags
+							D - dynamic
+							d - detached
+							P - Populated
+							B - Populated bus
+
+	Passed by reference.
+
 struct clk:
 
 	%pC	pll1
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 2d41de3f98a1..80b3e237f8d1 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -31,6 +31,7 @@
 #include <linux/dcache.h>
 #include <linux/cred.h>
 #include <linux/uuid.h>
+#include <linux/of.h>
 #include <net/addrconf.h>
 #ifdef CONFIG_BLOCK
 #include <linux/blkdev.h>
@@ -650,7 +651,7 @@ char *bdev_name(char *buf, char *end, struct block_device *bdev,
 		struct printf_spec spec, const char *fmt)
 {
 	struct gendisk *hd = bdev->bd_disk;
-	
+
 	buf = string(buf, end, hd->disk_name, spec);
 	if (bdev->bd_part->partno) {
 		if (isdigit(hd->disk_name[strlen(hd->disk_name)-1])) {
@@ -1470,6 +1471,123 @@ char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt)
 	return format_flags(buf, end, flags, names);
 }
 
+static noinline_for_stack
+char *device_node_gen_full_name(const struct device_node *np, char *buf, char *end)
+{
+	int len, ret;
+
+	if (!np || !np->parent)
+		return buf;
+
+	buf = device_node_gen_full_name(np->parent, buf, end);
+
+	if (buf < end)
+		len = end - buf;
+	else
+		len = 0;
+	ret = snprintf(buf, len, "/%s", kbasename(np->full_name));
+	if (ret <= 0)
+		return buf;
+	else if (len == 0 || ret < len)
+		return buf + ret;
+	return buf + len;
+}
+
+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 ret;
+	char *buf_start = buf;
+	struct property *prop;
+	bool has_mult, pass;
+	const struct printf_spec num_spec = {
+		.flags = SMALL,
+		.field_width = -1,
+		.precision = -1,
+		.base = 10,
+	};
+
+	struct printf_spec str_spec = spec;
+	str_spec.field_width = -1;
+
+	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' || strcspn(fmt + 1,"fnpPFcCr") > 0)
+		fmt = "Ff";
+
+	for (fmtp = fmt + 1, pass = false; strspn(fmtp,"fnpPFcCr"); fmtp++, pass = true) {
+		if (pass && (*fmtp != 'f')) {
+			if (buf < end)
+				*buf = '|';
+			buf++;
+		}
+
+		switch (*fmtp) {
+		case 'f':	/* full_name */
+			if (pass) {
+				if (buf < end)
+					*buf = ':';
+				buf++;
+			}
+			buf = device_node_gen_full_name(dn, buf, end);
+			break;
+		case 'n':	/* name */
+			buf = string(buf, end, dn->name, str_spec);
+			break;
+		case 'p':	/* phandle */
+			buf = number(buf, end, (unsigned int)dn->phandle, num_spec);
+			break;
+		case 'P':	/* path-spec */
+			buf = string(buf, end, kbasename(of_node_full_name(dn)), 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':	/* major compatible string */
+			ret = of_property_read_string(dn, "compatible", &p);
+			if (!ret)
+				buf = string(buf, end, p, str_spec);
+			break;
+		case 'C':	/* full compatible string */
+			has_mult = false;
+			of_property_for_each_string(dn, "compatible", prop, p) {
+				if (has_mult)
+					buf = string(buf, end, ",", str_spec);
+				buf = string(buf, end, "\"", str_spec);
+				buf = string(buf, end, p, str_spec);
+				buf = string(buf, end, "\"", str_spec);
+
+				has_mult = true;
+			}
+			break;
+		case 'r':	/* node reference count */
+			buf = number(buf, end, refcount_read(&dn->kobj.kref.refcount), num_spec);
+			break;
+		default:
+			break;
+		}
+	}
+
+	return widen_string(buf, buf - buf_start, end, spec);
+}
+
 int kptr_restrict __read_mostly;
 
 /*
@@ -1566,6 +1684,16 @@ int kptr_restrict __read_mostly;
  *       p page flags (see struct page) given as pointer to unsigned long
  *       g gfp flags (GFP_* and __GFP_*) given as pointer to gfp_t
  *       v vma flags (VM_*) given as pointer to unsigned long
+ * - 'OF[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
  *
  * ** Please update also Documentation/printk-formats.txt when making changes **
  *
@@ -1721,6 +1849,11 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 
 	case 'G':
 		return flags_string(buf, end, ptr, fmt);
+	case 'O':
+		switch (fmt[1]) {
+		case 'F':
+			return device_node_string(buf, end, ptr, spec, fmt + 1);
+		}
 	}
 	spec.flags |= SMALL;
 	if (spec.field_width == -1) {
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 4b9569fa931b..411f2098fa6b 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5709,7 +5709,7 @@ sub process {
 		        for (my $count = $linenr; $count <= $lc; $count++) {
 				my $fmt = get_quoted_string($lines[$count - 1], raw_line($count, 0));
 				$fmt =~ s/%%//g;
-				if ($fmt =~ /(\%[\*\d\.]*p(?![\WFfSsBKRraEhMmIiUDdgVCbGN]).)/) {
+				if ($fmt =~ /(\%[\*\d\.]*p(?![\WFfSsBKRraEhMmIiUDdgVCbGNO]).)/) {
 					$bad_extension = $1;
 					last;
 				}
-- 
2.11.0

[toc] | [next] | [standalone]

#1666224

On Wed, 2017-06-14 at 15:30 -0500, Rob Herring wrote:
> From: Pantelis Antoniou <pantelis.antoniou@konsulko.com>

I think the commit subject is wrong.
It adds an "of" specific bit to vsprintf.c.
The subject should be
'vsprintf:  Add %p extension "%pO" for device tree'

> 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 %pOF\n", node);

Somehow I think this example is poor as node->full_name
is a pretty obvious to read use.  %pOF requires you to
look up or know what the output is going to be.

> More fine-grained control of formatting includes printing the name,
> flag, path-spec name, reference count and others, explained in the
> documentation entry.
> 
> Originally written by Pantelis, but pretty much rewrote the core
> function using existing string/number functions. The 2 passes were
> unnecessary and have been removed. Also, updated the checkpatch.pl
> check.

Some comments about the code.

> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> []
> @@ -1470,6 +1471,123 @@ char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt)
>  	return format_flags(buf, end, flags, names);
>  }
>  
> +static noinline_for_stack
> +char *device_node_gen_full_name(const struct device_node *np, char *buf, char *end)
> +{
> +	int len, ret;
> +
> +	if (!np || !np->parent)
> +		return buf;
> +
> +	buf = device_node_gen_full_name(np->parent, buf, end);

This is recursive.  How many levels of parents could there be?
Perhaps there should be a recursion limit.

> +
> +	if (buf < end)
> +		len = end - buf;
> +	else
> +		len = 0;
> +	ret = snprintf(buf, len, "/%s", kbasename(np->full_name));
> +	if (ret <= 0)
> +		return buf;
> +	else if (len == 0 || ret < len)
> +		return buf + ret;
> +	return buf + len;
> +}

Does this work with %p<len>OF for a right justified or padded
length string?  Perhaps widen_string should be added.

> +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 ret;
> +	char *buf_start = buf;
> +	struct property *prop;
> +	bool has_mult, pass;
> +	const struct printf_spec num_spec = {
> +		.flags = SMALL,
> +		.field_width = -1,
> +		.precision = -1,
> +		.base = 10,
> +	};
> +
> +	struct printf_spec str_spec = spec;
> +	str_spec.field_width = -1;
> +
> +	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' || strcspn(fmt + 1,"fnpPFcCr") > 0)
> +		fmt = "Ff";
> +
> +	for (fmtp = fmt + 1, pass = false; strspn(fmtp,"fnpPFcCr"); fmtp++, pass = true) {

why not
	while (isalpha(*++fmt))
like ip6 or isalnum like FORMAT_TYPE_PTR uses?

> +		if (pass && (*fmtp != 'f')) {
> +			if (buf < end)
> +				*buf = '|';
> +			buf++;
> +		}
> +
> +		switch (*fmtp) {
> +		case 'f':	/* full_name */
> +			if (pass) {
> +				if (buf < end)
> +					*buf = ':';
> +				buf++;
> +			}
> +			buf = device_node_gen_full_name(dn, buf, end);
> +			break;
> +		case 'n':	/* name */
> +			buf = string(buf, end, dn->name, str_spec);
> +			break;
> +		case 'p':	/* phandle */
> +			buf = number(buf, end, (unsigned int)dn->phandle, num_spec);
> +			break;
> +		case 'P':	/* path-spec */
> +			buf = string(buf, end, kbasename(of_node_full_name(dn)), 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' : '-');

I'd try to avoid all uses of snprintf as it's effectively
another fairly
large stack frame.

It's probably better to avoid more recursion stack depth use
and just use *buf++ as appropriate.

[toc] | [prev] | [next] | [standalone]

#1666699

On Wed, Jun 14, 2017 at 3:56 PM, Joe Perches <joe@perches.com> wrote:
> On Wed, 2017-06-14 at 15:30 -0500, Rob Herring wrote:
>> From: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>
> I think the commit subject is wrong.
> It adds an "of" specific bit to vsprintf.c.
> The subject should be
> 'vsprintf:  Add %p extension "%pO" for device tree'

Okay, but it was good enough for the 2-3 versions Pantelis did before...

>> 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 %pOF\n", node);
>
> Somehow I think this example is poor as node->full_name
> is a pretty obvious to read use.  %pOF requires you to
> look up or know what the output is going to be.

So %pOFfullname? We've beat this one to death IMO.

>
>> More fine-grained control of formatting includes printing the name,
>> flag, path-spec name, reference count and others, explained in the
>> documentation entry.
>>
>> Originally written by Pantelis, but pretty much rewrote the core
>> function using existing string/number functions. The 2 passes were
>> unnecessary and have been removed. Also, updated the checkpatch.pl
>> check.
>
> Some comments about the code.
>
>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>> []
>> @@ -1470,6 +1471,123 @@ char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt)
>>       return format_flags(buf, end, flags, names);
>>  }
>>
>> +static noinline_for_stack
>> +char *device_node_gen_full_name(const struct device_node *np, char *buf, char *end)
>> +{
>> +     int len, ret;
>> +
>> +     if (!np || !np->parent)
>> +             return buf;
>> +
>> +     buf = device_node_gen_full_name(np->parent, buf, end);
>
> This is recursive.  How many levels of parents could there be?
> Perhaps there should be a recursion limit.

2-6 I'd say is typical. The FDT unflattening code limits things to 64
(which is probably way more than needed).

I could re-write it to be non-recursive, but then I'll just have the
max sized array of pointers on the stack.

>
>> +
>> +     if (buf < end)
>> +             len = end - buf;
>> +     else
>> +             len = 0;
>> +     ret = snprintf(buf, len, "/%s", kbasename(np->full_name));

I can replace this one too with a strcat and save some stack space.

>> +     if (ret <= 0)
>> +             return buf;
>> +     else if (len == 0 || ret < len)
>> +             return buf + ret;
>> +     return buf + len;
>> +}
>
> Does this work with %p<len>OF for a right justified or padded
> length string?  Perhaps widen_string should be added.

widen_string is called at the end of device_node_string.

>> +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 ret;
>> +     char *buf_start = buf;
>> +     struct property *prop;
>> +     bool has_mult, pass;
>> +     const struct printf_spec num_spec = {
>> +             .flags = SMALL,
>> +             .field_width = -1,
>> +             .precision = -1,
>> +             .base = 10,
>> +     };
>> +
>> +     struct printf_spec str_spec = spec;
>> +     str_spec.field_width = -1;
>> +
>> +     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' || strcspn(fmt + 1,"fnpPFcCr") > 0)
>> +             fmt = "Ff";
>> +
>> +     for (fmtp = fmt + 1, pass = false; strspn(fmtp,"fnpPFcCr"); fmtp++, pass = true) {
>
> why not
>         while (isalpha(*++fmt))
> like ip6 or isalnum like FORMAT_TYPE_PTR uses?

Okay.

>
>> +             if (pass && (*fmtp != 'f')) {
>> +                     if (buf < end)
>> +                             *buf = '|';
>> +                     buf++;
>> +             }
>> +
>> +             switch (*fmtp) {
>> +             case 'f':       /* full_name */
>> +                     if (pass) {
>> +                             if (buf < end)
>> +                                     *buf = ':';
>> +                             buf++;
>> +                     }
>> +                     buf = device_node_gen_full_name(dn, buf, end);
>> +                     break;
>> +             case 'n':       /* name */
>> +                     buf = string(buf, end, dn->name, str_spec);
>> +                     break;
>> +             case 'p':       /* phandle */
>> +                     buf = number(buf, end, (unsigned int)dn->phandle, num_spec);
>> +                     break;
>> +             case 'P':       /* path-spec */
>> +                     buf = string(buf, end, kbasename(of_node_full_name(dn)), 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' : '-');
>
> I'd try to avoid all uses of snprintf as it's effectively
> another fairly
> large stack frame.

Okay.

> It's probably better to avoid more recursion stack depth use
> and just use *buf++ as appropriate.

You can't use *buf++ as this code must work and increment buf even
when buf is NULL.

Rob

[toc] | [prev] | [next] | [standalone]

#1666905

On Thu, 2017-06-15 at 07:30 -0500, Rob Herring wrote:
> On Wed, Jun 14, 2017 at 3:56 PM, Joe Perches <joe@perches.com> wrote:
> > On Wed, 2017-06-14 at 15:30 -0500, Rob Herring wrote:
> > > From: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> > 
> > I think the commit subject is wrong.
> > It adds an "of" specific bit to vsprintf.c.
> > The subject should be
> > 'vsprintf:  Add %p extension "%pO" for device tree'
> 
> Okay, but it was good enough for the 2-3 versions Pantelis did before...

Which were not applied.

> > > 90% of the usage of device node's full_name is printing it out
> > > in a kernel message. Preparing for the eventual delayed allocation

The "eventual delayed allocation" bit doesn't
mean anything to me.

> > > 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 %pOF\n", node);
> > 
> > Somehow I think this example is poor as node->full_name
> > is a pretty obvious to read use.  %pOF requires you to
> > look up or know what the output is going to be.
> 
> So %pOFfullname? We've beat this one to death IMO.

I don't doubt the utility, just the example.
Just mention that full_name is going away.

> > > More fine-grained control of formatting includes printing the name,
> > > flag, path-spec name, reference count and others, explained in the
> > > documentation entry.
> > > 
> > > Originally written by Pantelis, but pretty much rewrote the core
> > > function using existing string/number functions. The 2 passes were
> > > unnecessary and have been removed. Also, updated the checkpatch.pl
> > > check.
> > 
> > Some comments about the code.
> > 
> > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > > []
> > > @@ -1470,6 +1471,123 @@ char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt)
> > >       return format_flags(buf, end, flags, names);
> > >  }
> > > 
> > > +static noinline_for_stack
> > > +char *device_node_gen_full_name(const struct device_node *np, char *buf, char *end)
> > > +{
> > > +     int len, ret;
> > > +
> > > +     if (!np || !np->parent)
> > > +             return buf;
> > > +
> > > +     buf = device_node_gen_full_name(np->parent, buf, end);
> > 
> > This is recursive.  How many levels of parents could there be?
> > Perhaps there should be a recursion limit.
> 
> 2-6 I'd say is typical. The FDT unflattening code limits things to 64
> (which is probably way more than needed).
> 
> I could re-write it to be non-recursive, but then I'll just have the
> max sized array of pointers on the stack.

Which would be less stack than how many recursive calls?  5?

In any case, 64 * 8 for pointers or 5+ stack
frames is a fair amount of stack.

Maybe too much.

> > > +             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' : '-');
> > 
> > I'd try to avoid all uses of snprintf as it's effectively
> > another fairly large stack frame.
> 
> Okay.
> 
> > It's probably better to avoid more recursion stack depth use
> > and just use *buf++ as appropriate.
> 
> You can't use *buf++ as this code must work and increment buf even
> when buf is NULL.

tbuf then.

[toc] | [prev] | [next] | [standalone]

#1667283

On Wed, Jun 14, 2017 at 01:56:48PM -0700, Joe Perches wrote:
> On Wed, 2017-06-14 at 15:30 -0500, Rob Herring wrote:
> > From: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> 
> I think the commit subject is wrong.
> It adds an "of" specific bit to vsprintf.c.
> The subject should be
> 'vsprintf:  Add %p extension "%pO" for device tree'
> 
> > 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 %pOF\n", node);
> 
> Somehow I think this example is poor as node->full_name
> is a pretty obvious to read use.  %pOF requires you to
> look up or know what the output is going to be.
> 
> > More fine-grained control of formatting includes printing the name,
> > flag, path-spec name, reference count and others, explained in the
> > documentation entry.
> > 
> > Originally written by Pantelis, but pretty much rewrote the core
> > function using existing string/number functions. The 2 passes were
> > unnecessary and have been removed. Also, updated the checkpatch.pl
> > check.
> 
> Some comments about the code.
> 
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > []
> > @@ -1470,6 +1471,123 @@ char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt)
> >  	return format_flags(buf, end, flags, names);
> >  }
> >  
> > +static noinline_for_stack
> > +char *device_node_gen_full_name(const struct device_node *np, char *buf, char *end)
> > +{
> > +	int len, ret;
> > +
> > +	if (!np || !np->parent)
> > +		return buf;
> > +
> > +	buf = device_node_gen_full_name(np->parent, buf, end);
> 
> This is recursive.  How many levels of parents could there be?
> Perhaps there should be a recursion limit.

Okay, unlike unflattening code, we can easily calculate the depth and 
then allocate an array on the stack. So this is what I've ended up 
with:

static int device_node_calc_depth(const struct device_node *np)
{
	int d;

	for (d = 0; np; d++)
		np = np->parent;

	return d;
}

static noinline_for_stack
char *device_node_gen_full_name(const struct device_node *np, char *buf, char *end)
{
	int i;
	int depth = device_node_calc_depth(np);
	const struct device_node *nodes[depth];
	const struct printf_spec strspec = {
		.field_width = -1,
		.precision = -1,
	};

	if (!depth)
		return buf;
	/* special case for root node */
	if (depth == 1)
		return string(buf, end, "/", strspec);

	depth--;
	for (i = depth - 1; i >= 0; i--) {
		nodes[i] = np;
		np = np->parent;
	}
	for (i = 0; i < depth; i++) {
		buf = string(buf, end, "/", strspec);
		buf = string(buf, end, kbasename(nodes[i]->full_name), strspec);
	}
	return buf;
}


> > +	/* simple case without anything any more format specifiers */
> > +	if (fmt[1] == '\0' || strcspn(fmt + 1,"fnpPFcCr") > 0)
> > +		fmt = "Ff";
> > +
> > +	for (fmtp = fmt + 1, pass = false; strspn(fmtp,"fnpPFcCr"); fmtp++, pass = true) {
> 
> why not
> 	while (isalpha(*++fmt))
> like ip6 or isalnum like FORMAT_TYPE_PTR uses?

This case is more complicated with the field separators. If we have 
something like %pOFfxyz where xyz are not valid format specifiers, we'd 
end up with extra field separators.

I did simplify things a bit and got rid of fmtp and just use fmt 
instead.

Rob

[toc] | [prev] | [next] | [standalone]

#1667296

On Thu, 2017-06-15 at 16:26 -0500, Rob Herring wrote:
> On Wed, Jun 14, 2017 at 01:56:48PM -0700, Joe Perches wrote:
> > On Wed, 2017-06-14 at 15:30 -0500, Rob Herring wrote:
> > > From: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
[]
> > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > > []
> > > @@ -1470,6 +1471,123 @@ char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt)
> > >  	return format_flags(buf, end, flags, names);
> > >  }
> > >  
> > > +static noinline_for_stack
> > > +char *device_node_gen_full_name(const struct device_node *np, char *buf, char *end)
> > > +{
> > > +	int len, ret;
> > > +
> > > +	if (!np || !np->parent)
> > > +		return buf;
> > > +
> > > +	buf = device_node_gen_full_name(np->parent, buf, end);
> > 
> > This is recursive.  How many levels of parents could there be?
> > Perhaps there should be a recursion limit.
> 
> Okay, unlike unflattening code, we can easily calculate the depth and 
> then allocate an array on the stack. So this is what I've ended up 
> with:
> 
> static int device_node_calc_depth(const struct device_node *np)
> {
> 	int d;
> 
> 	for (d = 0; np; d++)
> 		np = np->parent;
> 
> 	return d;
> }
> 
> static noinline_for_stack
> char *device_node_gen_full_name(const struct device_node *np, char *buf, char *end)
> {
> 	int i;
> 	int depth = device_node_calc_depth(np);
> 	const struct device_node *nodes[depth];
> 	const struct printf_spec strspec = {
> 		.field_width = -1,
> 		.precision = -1,
> 	};

static const struct printf_spec strspec = { etc...
and please move strspec above *nodes

> 
> 	if (!depth)
> 		return buf;
> 	/* special case for root node */
> 	if (depth == 1)
> 		return string(buf, end, "/", strspec);
> 
> 	depth--;
> 	for (i = depth - 1; i >= 0; i--) {
> 		nodes[i] = np;
> 		np = np->parent;
> 	}
> 	for (i = 0; i < depth; i++) {
> 		buf = string(buf, end, "/", strspec);
> 		buf = string(buf, end, kbasename(nodes[i]->full_name), strspec);
> 	}
> 	return buf;
> }

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web