DT printf format specifiers
Subsystems (5)
Rob Herring
maintainercover DT printf format specifiers
This resurrects an old patch[1] from Pantelis adding printf format specifiers for DT nodes. The previous versions didn't get applied after debate about the what character(s) to use. Grant suggested %pO for base kobject and %pOF for struct device_node. Everyone agreed, but no new version was posted.
I ended up re-writing the core implementation to be more inline with how other format specifiers are written which allowed removing #define code fragments. The other 3 patches convert the core DT code to use %pOF and prepare for changing device_node.full_name to stop storing the full path for every node.
My plan is to merge this series for v4.13 and post follow-up patches to convert all arches and subsystems to %pOF for v4.14. The full series is available here[2]. I tested this on QEMU running the DT unittests.
Rob
[1] https://patchwork.kernel.org/patch/6127521/
[2] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git dt-printf
Pantelis Antoniou (1):
of: Custom printk format specifier for device node
Rob Herring (3):
of: use kbasename instead of open coding
of: find_node_by_full_name rewrite to compare each level
of: Convert to using %pOF instead of full_name
Documentation/printk-formats.txt | 31 +++++++++ drivers/of/address.c | 21 +++--- drivers/of/base.c | 76 ++++++++++++---------- drivers/of/device.c | 2 +- drivers/of/dynamic.c | 33 +++++----- drivers/of/irq.c | 10 +-- drivers/of/of_mdio.c | 10 +-- drivers/of/of_pci.c | 29 ++++----- drivers/of/of_private.h | 3 + drivers/of/overlay.c | 21 +++--- drivers/of/platform.c | 34 +++++----- drivers/of/resolver.c | 34 ++-------- drivers/of/unittest.c | 66 +++++++++++-------- lib/vsprintf.c | 135 ++++++++++++++++++++++++++++++++++++++- scripts/checkpatch.pl | 2 +- 15 files changed, 331 insertions(+), 176 deletions(-)
Signature · 6 lines
2.11.0 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Rob Herring
maintainerPATCH 1/4 of: use kbasename instead of open coding
Several places in DT code open code the equivalent of kbasename. Replace them.
The behavior for root nodes in node_name_cmp will be slightly different. Instead of comparing "/", "" will be compared. The comparison will be the same.
--- drivers/of/base.c | 5 +---- drivers/of/platform.c | 2 +- drivers/of/resolver.c | 4 ++-- 3 files changed, 4 insertions(+), 7 deletions(-)
drivers/of/base.c +1 -4
@@ -773,10 +773,7 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,return NULL ;__for_each_child_of_node (parent, child) { -const char *name =strrchr (child->full_name,'/' ); -if (WARN (!name,"malformed device_node %s\n" , child->full_name)) -continue ; - name++; +const char *name =kbasename (child->full_name);if (strncmp (path, name, len) ==0 && (strlen (name) == len))return child; }
drivers/of/platform.c +1 -1
@@ -99,7 +99,7 @@ static void of_device_make_bus_id(struct device *dev)/* format arguments only used if dev_name() resolves to NULL */ dev_set_name (dev,dev_name (dev) ?"%s:%s" :"%s" , -strrchr (node->full_name,'/' ) +1 ,dev_name (dev)); +kbasename (node->full_name),dev_name (dev)); node = node->parent; } }
drivers/of/resolver.c +2 -2
@@ -165,8 +165,8 @@ static int update_usages_of_a_phandle_reference(struct device_node *overlay,static int node_name_cmp (const struct device_node *dn1,const struct device_node *dn2) { -const char *n1 =strrchr (dn1->full_name,'/' ) ? :"/" ; -const char *n2 =strrchr (dn2->full_name,'/' ) ? :"/" ; +const char *n1 =kbasename (dn1->full_name); +const char *n2 =kbasename (dn2->full_name);return of_node_cmp (n1, n2); }
Signature · 6 lines
2.11.0 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] of: use kbasename instead of open coding
Quoted 6 lines · Rob Herring
Several places in DT code open code the equivalent of kbasename. Replace them.
The behavior for root nodes in
node_name_cmpwill be slightly different. Instead of comparing "/", "" will be compared. The comparison will be the same.
Quoted 55 lines
Signed-off-by Rob Herring <[email protected]>--- drivers/of/base.c | 5 +---- drivers/of/platform.c | 2 +- drivers/of/resolver.c | 4 ++-- 3 files changed, 4 insertions(+), 7 deletions(-)drivers/of/base.c +1
-4@@ -773,10 +773,7 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,return NULL ;__for_each_child_of_node (parent, child) { -const char *name =strrchr (child->full_name,'/' ); -if (WARN (!name,"malformed device_node %s\n" , child->full_name)) -continue ; - name++; +const char *name =kbasename (child->full_name);if (strncmp (path, name, len) ==0 && (strlen (name) == len))return child; }drivers/of/platform.c +1
-1@@ -99,7 +99,7 @@ static void of_device_make_bus_id(struct device *dev)/* format arguments only used if dev_name() resolves to NULL */ dev_set_name (dev,dev_name (dev) ?"%s:%s" :"%s" , -strrchr (node->full_name,'/' ) +1 ,dev_name (dev)); +kbasename (node->full_name),dev_name (dev)); node = node->parent; } }drivers/of/resolver.c +2
-3@@ -165,8 +165,8 @@ static int update_usages_of_a_phandle_reference(struct device_node *overlay,static int node_name_cmp (const struct device_node *dn1,const struct device_node *dn2) { -const char *n1 =strrchr (dn1->full_name,'/' ) ? :"/" ; -const char *n2 =strrchr (dn2->full_name,'/' ) ? :"/" ; +const char *n1 =kbasename (dn1->full_name); +const char *n2 =kbasename (dn2->full_name);return of_node_cmp (n1, n2); } --2.11.0
Signature · 6 lines
With Best Regards, Andy Shevchenko -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Rob Herring
maintainerPATCH 2/4 of: find_node_by_full_name rewrite to compare each level
In reply to Rob Herringfind_node_by_full_name() does the same thing as of_find_node_by_path(), but takes no locks and doesn't work on aliases. Refactor of_find_node_opts_by_path() into __of_find_node_by_full_path() and replace find_node_by_full_name() with it.
--- drivers/of/base.c | 29 +++++++++++++++++++---------- drivers/of/of_private.h | 3 +++ drivers/of/resolver.c | 30 +++--------------------------- 3 files changed, 25 insertions(+), 37 deletions(-)
drivers/of/base.c +19 -10
@@ -780,6 +780,24 @@ static struct device_node *__of_find_node_by_path(struct device_node *parent,return NULL ; } +struct device_node *__of_find_node_by_full_path (struct device_node *node, +const char *path) +{ +const char *separator =strchr (path,':' ); + +while (node && *path =='/' ) { +struct device_node *tmp = node; + + path++;/* Increment past '/' delimiter */ + node =__of_find_node_by_path (node, path); +of_node_put (tmp); + path =strchrnul (path,'/' ); +if (separator && separator < path) +break ; + } +return node; +} +/** * of_find_node_opts_by_path - Find a node matching a full OF path * @path: Either the full path to match, or if the path does not
@@ -839,16 +857,7 @@ struct device_node *of_find_node_opts_by_path(const char *path, const char **optraw_spin_lock_irqsave (&devtree_lock, flags);if (!np) np =of_node_get (of_root); -while (np && *path =='/' ) { -struct device_node *tmp = np; - - path++;/* Increment past '/' delimiter */ - np =__of_find_node_by_path (np, path); -of_node_put (tmp); - path =strchrnul (path,'/' ); -if (separator && separator < path) -break ; - } + np =__of_find_node_by_full_path (np, path);raw_spin_unlock_irqrestore (&devtree_lock, flags);return np; }
drivers/of/of_private.h +3 -0
@@ -77,6 +77,9 @@ extern void *__unflatten_device_tree(const void *blob,struct property *__of_prop_dup (const struct property *prop,gfp_t allocflags); __printf(2, 3) struct device_node *__of_node_dup(const struct device_node *np, const char *fmt, ...); +struct device_node *__of_find_node_by_full_path (struct device_node *node, +const char *path); +extern const void *__of_get_property (const struct device_node *np,const char *name,int *lenp);extern int __of_add_property (struct device_node *np,struct property *prop);
drivers/of/resolver.c +3 -27
@@ -20,35 +20,11 @@#include <linux/errno.h> #include <linux/slab.h> +#include "of_private.h" +/* illegal phandle value (set when unresolved) */ #define OF_PHANDLE_ILLEGAL 0xdeadbeef -/** -* Find a node with the give full name by recursively following any of -* the child node links. -*/ -static struct device_node *find_node_by_full_name (struct device_node *node, -const char *full_name) -{ -struct device_node *child, *found; - -if (!node) -return NULL ; - -if (!of_node_cmp (node->full_name, full_name)) -return of_node_get (node); - -for_each_child_of_node (node, child) { - found =find_node_by_full_name (child, full_name); -if (found !=NULL ) { -of_node_put (child); -return found; - } - } - -return NULL ; -} -static phandle live_tree_max_phandle (void ) {struct device_node *node;
@@ -138,7 +114,7 @@ static int update_usages_of_a_phandle_reference(struct device_node *overlay,if (err) goto err_fail; - refnode =find_node_by_full_name (overlay, node_path); + refnode =__of_find_node_by_full_path (overlay, node_path);if (!refnode)continue ;
Signature · 1 line
2.11.0
Rob Herring
maintainerPATCH 3/4 of: Custom printk format specifier for device node
In reply to Rob HerringFrom: Pantelis Antoniou <[email protected]>
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.
--- Documentation/printk-formats.txt | 31 +++++++++ lib/vsprintf.c | 135 ++++++++++++++++++++++++++++++++++++++- scripts/checkpatch.pl | 2 +- 3 files changed, 166 insertions(+), 2 deletions(-)
Documentation/printk-formats.txt +31 -0
@@ -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
lib/vsprintf.c +134 -1
165 lines not on this page · raw message
scripts/checkpatch.pl +1 -1
@@ -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; }
Signature · 1 line
2.11.0
Joe Perches
maintainerRe: [PATCH 3/4] of: Custom printk format specifier for device node
Quoted 1 line · Rob Herring
From: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/[email protected]>
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'
Quoted 10 lines
90% of the usage of device node's
full_nameis 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.
Quoted 8 lines
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.
Quoted 5 lines
lib/vsprintf.c +0
-0[]
lib/vsprintf.c +0
-0@@ -1470,6 +1471,123 @@ char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt)return format_flags (buf, end, flags, names); }
lib/vsprintf.c char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt) · line 1482
Quoted 10 lines
lib/vsprintf.c +9
-0+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.
lib/vsprintf.c char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt) · line 1494
Quoted 12 lines
lib/vsprintf.c +12
-0+ +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.
Quoted 21 lines
lib/vsprintf.c +21
-0+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 ))
lib/vsprintf.c char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt) · line 1526
Quoted 10 lines
lib/vsprintf.c +10
-0+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?
Quoted 24 lines
lib/vsprintf.c +24
-0+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 ;
lib/vsprintf.c char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt) · line 1560
Quoted 10 lines
lib/vsprintf.c +10
-0+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.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Rob Herring
maintainerRe: [PATCH 3/4] of: Custom printk format specifier for device node
Quoted 7 lines · Joe Perches
Quoted 1 line · Rob Herring
From: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/[email protected]>
I think the commit subject is wrong.
It adds an "of" specific bit tovsprintf.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...
Quoted 8 lines
Quoted 8 lines
90% of the usage of device node's
full_nameis 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);
Quoted 6 lines
Quoted 2 lines
Which can be written now as:
pr_info("Frobbing node %pOF\n", node);Somehow I think this example is poor as
node->full_nameis 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.
Quoted 18 lines
Quoted 9 lines
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.
Quoted 6 lines
lib/vsprintf.c +0
-0[]
lib/vsprintf.c +0
-0@@ -1470,6 +1471,123 @@ char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt)return format_flags (buf, end, flags, names); }
Quoted 12 lines
lib/vsprintf.c char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt) · line 1482
Quoted 9 lines
lib/vsprintf.c +9
-0+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.
Quoted 7 lines
lib/vsprintf.c char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt) · line 1488
Quoted 7 lines
lib/vsprintf.c +6
-0+ +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.
Quoted 9 lines
lib/vsprintf.c char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt) · line 1494
Quoted 6 lines
lib/vsprintf.c +6
-0+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_stringshould be added.
widen_string is called at the end of device_node_string.
Quoted 33 lines
Quoted 21 lines
lib/vsprintf.c +21
-0+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 ))lib/vsprintf.c char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt) · line 1526
Quoted 10 lines
lib/vsprintf.c +10
-0+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
Quoted 2 lines
while (isalpha(*++fmt))like ip6 or isalnum like
FORMAT_TYPE_PTRuses?
Okay.
Quoted 36 lines
Quoted 25 lines
lib/vsprintf.c +24
-0+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 ;lib/vsprintf.c char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt) · line 1560
Quoted 10 lines
lib/vsprintf.c +10
-0+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' :'-' );
Quoted 3 lines
I'd try to avoid all uses of snprintf as it's effectively another fairly
large stack frame.
Okay.
Quoted 2 lines
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Joe Perches
maintainerRe: [PATCH 3/4] of: Custom printk format specifier for device node
Quoted 10 lines · Rob Herring
Quoted 7 lines · Joe Perches
Quoted 1 line · Rob Herring
From: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/[email protected]>
I think the commit subject is wrong.
It adds an "of" specific bit tovsprintf.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.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
90% of the usage of device node's
full_nameis printing it out in a kernel message. Preparing for the eventual delayed allocation
The "eventual delayed allocation" bit doesn't
mean anything to me.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
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);
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
Which can be written now as:
pr_info("Frobbing node %pOF\n", node);Somehow I think this example is poor as
node->full_nameis 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.
Quoted 30 lines
Quoted 17 lines
Quoted 8 lines
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.
Quoted 6 lines
lib/vsprintf.c +0
-0[]
lib/vsprintf.c +0
-0@@ -1470,6 +1471,123 @@ char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt)return format_flags (buf, end, flags, names); }Quoted 12 lines
lib/vsprintf.c char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt) · line 1482
Quoted 9 lines
lib/vsprintf.c +9
-0+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.
Quoted 5 lines
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.
Quoted 14 lines
Quoted 11 lines
lib/vsprintf.c char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt) · line 1560
Quoted 10 lines
lib/vsprintf.c +10
-0+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' :'-' );Quoted 2 lines
I'd try to avoid all uses of snprintf as it's effectively another fairly large stack frame.
Quoted 7 lines
Okay.
Quoted 2 lines
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.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Rob Herring
maintainerRe: [PATCH 3/4] of: Custom printk format specifier for device node
In reply to Joe PerchesQuoted 40 lines · Joe Perches
Quoted 1 line · Rob Herring
From: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/[email protected]>
I think the commit subject is wrong.
It adds an "of" specific bit tovsprintf.c. The subject should be
'vsprintf: Add %p extension "%pO" for device tree'Quoted 10 lines
90% of the usage of device node's
full_nameis 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_nameis a pretty obvious to read use. %pOF requires you to look up or know what the output is going to be.Quoted 8 lines
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.
Quoted 6 lines
lib/vsprintf.c +0
-0[]
lib/vsprintf.c +0
-0@@ -1470,6 +1471,123 @@ char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt)return format_flags (buf, end, flags, names); }
Quoted 12 lines
lib/vsprintf.c char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt) · line 1482
Quoted 9 lines
lib/vsprintf.c +9
-0+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;
}Quoted 9 lines
lib/vsprintf.c char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt) · line 1526
Quoted 5 lines
lib/vsprintf.c +5
-0+/* 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_PTRuses?
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Joe Perches
maintainerRe: [PATCH 3/4] of: Custom printk format specifier for device node
Quoted 3 lines · Rob Herring
Quoted 2 lines · Joe Perches
Quoted 1 line · Rob Herring
From: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/[email protected]>
[]
Quoted 35 lines
Quoted 6 lines
Quoted 6 lines
lib/vsprintf.c +0
-0[]
lib/vsprintf.c +0
-0@@ -1470,6 +1471,123 @@ char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt)return format_flags (buf, end, flags, names); }Quoted 12 lines
lib/vsprintf.c char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt) · line 1482
Quoted 9 lines
lib/vsprintf.c +9
-0+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 structdevice_node*np, char *buf, char *end)
Quoted 8 lines
{
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
Quoted 10 lines
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;Quoted 8 lines
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;
}--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Rob Herring
maintainerPATCH v2 1/1 vsprintf: Add %p extension "%pOF" for device tree
In reply to Rob HerringFrom: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/[email protected]>
90% of the usage of device node's full_name is printing it out in a kernel message. However, storing the full path for every node is wasteful and redundant. With a custom format specifier, we can generate the full path at run-time and eventually remove the full path from every node.
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, flags, path-spec name 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. The unittest code was written by Grant Likely.
---
I missed some comments and changes Grant had done and incorporated them.
v2:
- Change subject
- Rewrite device_node_gen_full_name() to avoid recursion.
- Avoid using sprintf.
- Add unittests Grant L. wrote.
- Drop ref count printing (from discussion 2 years ago).
- Remove fmtp local var.
Documentation/printk-formats.txt | 30 ++++++ drivers/of/unittest-data/tests-platform.dtsi | 4 +- drivers/of/unittest.c | 58 +++++++++++ lib/vsprintf.c | 140 +++++++++++++++++++++++++++ scripts/checkpatch.pl | 2 +- 5 files changed, 232 insertions(+), 2 deletions(-)
Documentation/printk-formats.txt +30 -0
@@ -275,6 +275,36 @@ struct va_format: Passed by reference. +Device tree nodes: + + %pOn[fnpPcCF] + + 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 %pOFf) + 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 + + Passed by reference. + struct clk: %pC pll1
drivers/of/unittest-data/tests-platform.dtsi +3 -1
@@ -26,7 +26,9 @@ #size-cells = <0>; dev@100 { - compatible = "test-sub-device"; + compatible = "test-sub-device", + "test-compat2", + "test-compat3"; reg = <0x100>; }; };
drivers/of/unittest.c +58 -0
70 lines not on this page · raw message
lib/vsprintf.c +140 -0
164 lines not on this page · raw message
scripts/checkpatch.pl +1 -1
@@ -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; }
Signature · 6 lines
2.11.0 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Randy Dunlap
reviewerRe: [PATCH v2] vsprintf: Add %p extension "%pOF" for device tree
Quoted 8 lines · Rob Herring
From: Pantelis Antoniou <[email protected]>
90% of the usage of device node's
full_nameis printing it out in a kernel message. However, storing the full path for every node is wasteful and redundant. With a custom format specifier, we can generate the full path at run-time and eventually remove the full path from every node.
Quoted 5 lines
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);
isn't OF for flags -- and Of for full name?
Typo or a change in the last 2 years?
Quoted 70 lines
More fine-grained control of formatting includes printing the name, flags, path-spec name 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. The unittest code was written by Grant Likely.
Signed-off-by Pantelis Antoniou <[email protected]>Signed-off-by Rob Herring <[email protected]>---
I missed some comments and changes Grant had done and incorporated them.v2:
- Change subject
- Rewritedevice_node_gen_full_name()to avoid recursion.
- Avoid using sprintf.
- Add unittests Grant L. wrote.
- Drop ref count printing (from discussion 2 years ago).
- Remove fmtp local var.Documentation/printk-formats.txt | 30 ++++++ drivers/of/unittest-data/tests-platform.dtsi | 4 +- drivers/of/unittest.c | 58 +++++++++++ lib/vsprintf.c | 140 +++++++++++++++++++++++++++ scripts/checkpatch.pl | 2 +- 5 files changed, 232 insertions(+), 2 deletions(-)Documentation/printk-formats.txt +30
-0@@ -275,6 +275,36 @@ struct va_format: Passed by reference. +Device tree nodes: + + %pOn[fnpPcCF] + + 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 %pOFf) + 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 + + Passed by reference. + struct clk: %pC pll1
Signature · 1 line
~Randy
Rob Herring
maintainerRe: [PATCH v2] vsprintf: Add %p extension "%pOF" for device tree
Quoted 12 lines · Randy Dunlap
Quoted 11 lines · Rob Herring
From: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/[email protected]>
90% of the usage of device node's
full_nameis printing it out in a kernel message. However, storing the full path for every node is wasteful and redundant. With a custom format specifier, we can generate the full path at run-time and eventually remove the full path from every node.For instance typical use is:
pr_info("Frobbing node %s\n", node->full_name);
Quoted 6 lines
Quoted 2 lines
Which can be written now as:
pr_info("Frobbing node %pOF\n", node);isn't OF for flags -- and Of for full name?
Typo or a change in the last 2 years?
It changed in the discussion 2 years ago. The intent is:
%pO - kobj
%pOF - device_node, defaulting to full name
There's no support of base kobj's, but I guess I should make that intent clear in the documentation.
Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Joe Perches
maintainerRe: [PATCH v2] vsprintf: Add %p extension "%pOF" for device tree
In reply to Rob HerringQuoted 8 lines · Rob Herring
From: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/[email protected]>
90% of the usage of device node's
full_nameis printing it out in a kernel message. However, storing the full path for every node is wasteful and redundant. With a custom format specifier, we can generate the full path at run-time and eventually remove the full path from every node.
Quoted 5 lines
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);
I still think this should use another identifier like %pO for object then another letter for type of object maybe N for node.
And F is flags, f is name
lib/vsprintf.c
Quoted 1 line
lib/vsprintf.c +0
-0
[]
Quoted 32 lines
lib/vsprintf.c +28
-0@@ -1470,6 +1471,131 @@ char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt)return format_flags (buf, end, flags, names); } +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); +static const struct printf_spec strspec = { + .field_width =-1 , + .precision =-1 , + }; +const struct device_node *nodes[depth]; + +if (!depth) + returnuf; +/* special case for root node */ +if (depth ==1 ) +return string (buf, end,"/" , strspec); + + depth--;
lib/vsprintf.c char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt) · line 1511
Quoted 10 lines
lib/vsprintf.c +10
-0+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; +}
I think there should be another function to get
a particular struct device_node * at a particular
depth to avoid all the pointers on stack.
Basically, adapt the for (i = depth - 1 loop to
a function that returns nodes[i]
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Rob Herring
maintainerRe: [PATCH v2] vsprintf: Add %p extension "%pOF" for device tree
Quoted 12 lines · Joe Perches
Quoted 11 lines · Rob Herring
From: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/[email protected]>
90% of the usage of device node's
full_nameis printing it out in a kernel message. However, storing the full path for every node is wasteful and redundant. With a custom format specifier, we can generate the full path at run-time and eventually remove the full path from every node.For instance typical use is:
pr_info("Frobbing node %s\n", node->full_name);
Quoted 6 lines
Quoted 2 lines
Which can be written now as:
pr_info("Frobbing node %pOF\n", node);I still think this should use another identifier like %pO for object then another letter for type of object maybe N for node.
It is. O is for kobj and F is for device node. It doesn't make a much sense separately, but OF together makes sense as Open Firmware.
Quoted 52 lines
And F is flags, f is name
lib/vsprintf.c
Quoted 1 line
lib/vsprintf.c +0
-0[]
Quoted 32 lines
lib/vsprintf.c +28
-0@@ -1470,6 +1471,131 @@ char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt)return format_flags (buf, end, flags, names); } +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); +static const struct printf_spec strspec = { + .field_width =-1 , + .precision =-1 , + }; +const struct device_node *nodes[depth]; + +if (!depth) + returnuf; +/* special case for root node */ +if (depth ==1 ) +return string (buf, end,"/" , strspec); + + depth--;lib/vsprintf.c char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt) · line 1511
Quoted 10 lines
lib/vsprintf.c +10
-0+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; +}I think there should be another function to get a particular struct
device_node* at a particular depth to avoid all the pointers on stack.
Quoted 2 lines
Basically, adapt the for (i = depth - 1 loop to a function that returns nodes[i]
Okay.
Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Rob Herring
maintainerPATCH v3 1/1 vsprintf: Add %p extension "%pOF" for device tree
In reply to Rob HerringFrom: Pantelis Antoniou <[email protected]>
90% of the usage of device node's full_name is printing it out in a kernel message. However, storing the full path for every node is wasteful and redundant. With a custom format specifier, we can generate the full path at run-time and eventually remove the full path from every node.
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);'%pO' is the base specifier to represent kobjects with '%pOF' representing struct device_node. Currently, struct device_node is the only supported type of kobject.
More fine-grained control of formatting includes printing the name, flags, path-spec name 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. The unittest code was written by Grant Likely.
---
v3:
- Fix missing documentation updates using '%pOF' as the device_node specifier.
- Update the commit msg and documentation to clearly define '%pO' is the base specifier for kobjects.
- Rework device_node_gen_full_name() to avoid creating an array of node pointers on the stack.
v2:
- Change subject
- Rewrite device_node_gen_full_name() to avoid recursion.
- Avoid using sprintf.
- Add unittests Grant L. wrote.
- Drop ref count printing (from discussion 2 years ago).
- Remove fmtp local var.
Documentation/printk-formats.txt | 36 +++++++ drivers/of/unittest-data/tests-platform.dtsi | 4 +- drivers/of/unittest.c | 58 ++++++++++++ lib/vsprintf.c | 135 +++++++++++++++++++++++++++ scripts/checkpatch.pl | 2 +- 5 files changed, 233 insertions(+), 2 deletions(-)
Documentation/printk-formats.txt +36 -0
@@ -275,6 +275,42 @@ struct va_format: Passed by reference. +kobjects: + %pO + + Base specifier for kobject based structs. Must be followed with + character for specific type of kobject as listed below: + + Device tree nodes: + + %pOF[fnpPcCF] + + 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 %pOFf) + 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
drivers/of/unittest-data/tests-platform.dtsi +3 -1
@@ -26,7 +26,9 @@ #size-cells = <0>; dev@100 { - compatible = "test-sub-device"; + compatible = "test-sub-device", + "test-compat2", + "test-compat3"; reg = <0x100>; }; };
drivers/of/unittest.c +58 -0
70 lines not on this page · raw message
lib/vsprintf.c +135 -0
159 lines not on this page · raw message
scripts/checkpatch.pl +1 -1
@@ -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; }
Signature · 1 line
2.11.0
Re: [PATCH v3] vsprintf: Add %p extension "%pOF" for device tree
Quoted 28 lines · Rob Herring
From: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/[email protected]>
90% of the usage of device node's
full_nameis printing it out in a kernel message. However, storing the full path for every node is wasteful and redundant. With a custom format specifier, we can generate the full path at run-time and eventually remove the full path from every node.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);'%pO' is the base specifier to represent kobjects with '%pOF' representing struct
device_node. Currently, structdevice_nodeis the only supported type of kobject.More fine-grained control of formatting includes printing the name, flags, path-spec name 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. The unittest code was written by Grant Likely.
Signed-off-by Rob Herring <[email protected]>
Quoted 8 lines
---
v3:
- Fix missing documentation updates using '%pOF' as thedevice_nodespecifier.
- Update the commit msg and documentation to clearly define '%pO' is the base specifier for kobjects.
- Reworkdevice_node_gen_full_name()to avoid creating an array of node pointers on the stack.
Thanks Rob.
This all seems sensible to me now.
If you want:
cheers, Joe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Rob Herring
maintainerPATCH 4/4 of: Convert to using %pOF instead of full_name
In reply to Rob HerringNow that we have a custom printf format specifier, convert users of full_name to use %pOF instead. This is preparation to remove storing of the full path string for each node.
--- drivers/of/address.c | 21 ++++++++-------- drivers/of/base.c | 42 ++++++++++++++++---------------- drivers/of/device.c | 2 +- drivers/of/dynamic.c | 33 +++++++++++++------------- drivers/of/irq.c | 10 ++++---- drivers/of/of_mdio.c | 10 ++++---- drivers/of/of_pci.c | 29 +++++++++++----------- drivers/of/overlay.c | 21 ++++++++-------- drivers/of/platform.c | 32 ++++++++++++------------- drivers/of/unittest.c | 66 +++++++++++++++++++++++++++++---------------------- 10 files changed, 136 insertions(+), 130 deletions(-)
drivers/of/address.c +10 -11
@@ -559,7 +559,7 @@ static u64 __of_translate_address(struct device_node *dev,int na, ns, pna, pns;u64 result =OF_BAD_ADDR ; -pr_debug ("** translation for device %s **\n" ,of_node_full_name (dev)); +pr_debug ("** translation for device %pOF **\n" , dev);/* Increase refcount at current level */ of_node_get (dev);
@@ -573,13 +573,13 @@ static u64 __of_translate_address(struct device_node *dev,/* Count address cells & copy address locally */ bus->count_cells (dev, &na, &ns);if (!OF_CHECK_COUNTS (na, ns)) { -pr_debug ("Bad cell count for %s\n" ,of_node_full_name (dev)); +pr_debug ("Bad cell count for %pOF\n" , dev); goto bail; }memcpy (addr, in_addr, na *4 ); -pr_debug ("bus is %s (na=%d, ns=%d) on %s\n" , - bus->name, na, ns,of_node_full_name (parent)); +pr_debug ("bus is %s (na=%d, ns=%d) on %pOF\n" , + bus->name, na, ns, parent);of_dump_addr ("translating address:" , addr, na);/* Translate */
@@ -600,13 +600,12 @@ static u64 __of_translate_address(struct device_node *dev, pbus =of_match_bus (parent); pbus->count_cells (dev, &pna, &pns);if (!OF_CHECK_COUNTS (pna, pns)) { -pr_err ("Bad cell count for %s\n" , -of_node_full_name (dev)); +pr_err ("Bad cell count for %pOF\n" , dev);break ; } -pr_debug ("parent bus is %s (na=%d, ns=%d) on %s\n" , - pbus->name, pna, pns,of_node_full_name (parent)); +pr_debug ("parent bus is %s (na=%d, ns=%d) on %pOF\n" , + pbus->name, pna, pns, parent);/* Apply bus translation */ if (of_translate_one (dev, bus, pbus, addr, na, ns, pna, rprop))
@@ -855,7 +854,7 @@ int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *siz }if (!ranges) { -pr_debug ("no dma-ranges found for node(%s)\n" , np->full_name); +pr_debug ("no dma-ranges found for node(%pOF)\n" , np); ret = -ENODEV ; goto out; }
@@ -872,8 +871,8 @@ int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *siz dmaaddr =of_read_number (ranges, naddr); *paddr =of_translate_dma_address (np, ranges);if (*paddr ==OF_BAD_ADDR ) { -pr_err ("translation of DMA address(%pad) to CPU address failed node(%s)\n" , - dma_addr, np->full_name); +pr_err ("translation of DMA address(%pad) to CPU address failed node(%pOF)\n" , + dma_addr, np); ret = -EINVAL ; goto out; }
drivers/of/base.c +21 -21
106 lines not on this page · raw message
drivers/of/device.c +1 -1
@@ -294,7 +294,7 @@ void of_device_uevent(struct device *dev, struct kobj_uevent_env *env)return ;add_uevent_var (env,"OF_NAME=%s" , dev->of_node->name); -add_uevent_var (env,"OF_FULLNAME=%s" , dev->of_node->full_name); +add_uevent_var (env,"OF_FULLNAME=%pOF" , dev->of_node);if (dev->of_node->type &&strcmp ("<NULL>" , dev->of_node->type) !=0 )add_uevent_var (env,"OF_TYPE=%s" , dev->of_node->type);
drivers/of/dynamic.c +16 -17
@@ -98,14 +98,14 @@ int of_reconfig_notify(unsigned long action, struct of_reconfig_data *p)switch (action) {case OF_RECONFIG_ATTACH_NODE :case OF_RECONFIG_DETACH_NODE : -pr_debug ("notify %-15s %s\n" , action_names[action], - pr->dn->full_name); +pr_debug ("notify %-15s %pOF\n" , action_names[action], + pr->dn);break ;case OF_RECONFIG_ADD_PROPERTY :case OF_RECONFIG_REMOVE_PROPERTY :case OF_RECONFIG_UPDATE_PROPERTY : -pr_debug ("notify %-15s %s:%s\n" , action_names[action], - pr->dn->full_name, pr->prop->name); +pr_debug ("notify %-15s %pOF:%s\n" , action_names[action], + pr->dn, pr->prop->name);break ; }
@@ -328,11 +328,10 @@ void of_node_release(struct kobject *kobj)/* We should never be releasing nodes that haven't been detached. */ if (!of_node_check_flag (node,OF_DETACHED )) { -pr_err ("ERROR: Bad of_node_put() on %s\n" , node->full_name); +pr_err ("ERROR: Bad of_node_put() on %pOF\n" , node);dump_stack ();return ; } -if (!of_node_check_flag (node,OF_DYNAMIC ))return ;
@@ -462,13 +461,13 @@ static void __of_changeset_entry_dump(struct of_changeset_entry *ce)case OF_RECONFIG_ADD_PROPERTY :case OF_RECONFIG_REMOVE_PROPERTY :case OF_RECONFIG_UPDATE_PROPERTY : -pr_debug ("cset<%p> %-15s %s/%s\n" , ce, action_names[ce->action], - ce->np->full_name, ce->prop->name); +pr_debug ("cset<%p> %-15s %pOF/%s\n" , ce, action_names[ce->action], + ce->np, ce->prop->name);break ;case OF_RECONFIG_ATTACH_NODE :case OF_RECONFIG_DETACH_NODE : -pr_debug ("cset<%p> %-15s %s\n" , ce, action_names[ce->action], - ce->np->full_name); +pr_debug ("cset<%p> %-15s %pOF\n" , ce, action_names[ce->action], + ce->np);break ; } }
@@ -539,7 +538,7 @@ static void __of_changeset_entry_notify(struct of_changeset_entry *ce, bool reve }if (ret) -pr_err ("changeset notifier error @%s\n" , ce->np->full_name); +pr_err ("changeset notifier error @%pOF\n" , ce->np); }static int __of_changeset_entry_apply (struct of_changeset_entry *ce)
@@ -570,8 +569,8 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce) ret =__of_add_property (ce->np, ce->prop);if (ret) { -pr_err ("changeset: add_property failed @%s/%s\n" , - ce->np->full_name, +pr_err ("changeset: add_property failed @%pOF/%s\n" , + ce->np, ce->prop->name);break ; }
@@ -579,8 +578,8 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce)case OF_RECONFIG_REMOVE_PROPERTY : ret =__of_remove_property (ce->np, ce->prop);if (ret) { -pr_err ("changeset: remove_property failed @%s/%s\n" , - ce->np->full_name, +pr_err ("changeset: remove_property failed @%pOF/%s\n" , + ce->np, ce->prop->name);break ; }
@@ -598,8 +597,8 @@ static int __of_changeset_entry_apply(struct of_changeset_entry *ce) ret =__of_update_property (ce->np, ce->prop, &old_prop);if (ret) { -pr_err ("changeset: update_property failed @%s/%s\n" , - ce->np->full_name, +pr_err ("changeset: update_property failed @%pOF/%s\n" , + ce->np, ce->prop->name);break ; }
drivers/of/irq.c +5 -5
@@ -131,7 +131,7 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq) goto fail; } -pr_debug ("of_irq_parse_raw: ipar=%s, size=%d\n" ,of_node_full_name (ipar), intsize); +pr_debug ("of_irq_parse_raw: ipar=%pOF, size=%d\n" , ipar, intsize);if (out_irq->args_count != intsize) goto fail;
@@ -269,7 +269,7 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq) skiplevel:/* Iterate again with new parent */ out_irq->np = newpar; -pr_debug (" -> new parent: %s\n" ,of_node_full_name (newpar)); +pr_debug (" -> new parent: %pOF\n" , newpar);of_node_put (ipar); ipar = newpar; newpar =NULL ;
@@ -301,7 +301,7 @@ int of_irq_parse_one(struct device_node *device, int index, struct of_phandle_aru32 intsize, intlen;int i, res; -pr_debug ("of_irq_parse_one: dev=%s, index=%d\n" ,of_node_full_name (device), index); +pr_debug ("of_irq_parse_one: dev=%pOF, index=%d\n" , device, index);/* OldWorld mac stuff is "special", handle out of line */ if (of_irq_workarounds &OF_IMAP_OLDWORLD_MAC )
@@ -555,8 +555,8 @@ void __init of_irq_init(const struct of_device_id *matches)of_node_set_flag (desc->dev,OF_POPULATED ); -pr_debug ("of_irq_init: init %s (%p), parent %p\n" , - desc->dev->full_name, +pr_debug ("of_irq_init: init %pOF (%p), parent %p\n" , + desc->dev, desc->dev, desc->interrupt_parent); ret = desc->irq_init_cb (desc->dev, desc->interrupt_parent);
drivers/of/of_mdio.c +5 -5
@@ -126,14 +126,14 @@ int of_mdio_parse_addr(struct device *dev, const struct device_node *np) ret =of_property_read_u32 (np,"reg" , &addr);if (ret <0 ) { -dev_err (dev,"%s has invalid PHY address\n" , np->full_name); +dev_err (dev,"%pOF has invalid PHY address\n" , np);return ret; }/* A PHY must have a reg property in the range [0-31] */ if (addr >=PHY_MAX_ADDR ) { -dev_err (dev,"%s PHY address %i is too large\n" , - np->full_name, addr); +dev_err (dev,"%pOF PHY address %i is too large\n" , + np, addr);return -EINVAL ; }
@@ -188,8 +188,8 @@ static bool of_mdiobus_child_is_phy(struct device_node *child)if (of_match_node (whitelist_phys, child)) {pr_warn (FW_WARN -"%s: Whitelisted compatible string. Please remove\n" , - child->full_name); +"%pOF: Whitelisted compatible string. Please remove\n" , + child);return true; }
drivers/of/of_pci.c +14 -15
@@ -204,15 +204,15 @@ int of_pci_get_host_bridge_resources(struct device_node *dev,if (!bus_range)return -ENOMEM ; -pr_info ("host bridge %s ranges:\n" , dev->full_name); +pr_info ("host bridge %pOF ranges:\n" , dev); err =of_pci_parse_bus_range (dev, bus_range);if (err) { bus_range->start = busno; bus_range->end = bus_max; bus_range->flags =IORESOURCE_BUS ; -pr_info (" No bus range found for %s, using %pR\n" , - dev->full_name, bus_range); +pr_info (" No bus range found for %pOF, using %pR\n" , + dev, bus_range); }else {if (bus_range->end > bus_range->start + bus_max) bus_range->end = bus_range->start + bus_max;
@@ -258,14 +258,14 @@ int of_pci_get_host_bridge_resources(struct device_node *dev,if (resource_type (res) ==IORESOURCE_IO ) {if (!io_base) { -pr_err ("I/O range found for %s. Please provide an io_base pointer to save CPU base address\n" , - dev->full_name); +pr_err ("I/O range found for %pOF. Please provide an io_base pointer to save CPU base address\n" , + dev); err = -EINVAL ; goto conversion_failed; }if (*io_base != (resource_size_t )OF_BAD_ADDR ) -pr_warn ("More than one I/O resource converted for %s. CPU base address for old range lost!\n" , - dev->full_name); +pr_warn ("More than one I/O resource converted for %pOF. CPU base address for old range lost!\n" , + dev); *io_base = range.cpu_addr; }
@@ -325,7 +325,7 @@ int of_pci_map_rid(struct device_node *np, u32 rid, }if (!map_len || map_len % (4 *sizeof (*map))) { -pr_err ("%s: Error: Bad %s length: %d\n" , np->full_name, +pr_err ("%pOF: Error: Bad %s length: %d\n" , np, map_name, map_len);return -EINVAL ; }
@@ -349,8 +349,8 @@ int of_pci_map_rid(struct device_node *np, u32 rid,u32 rid_len =be32_to_cpup (map +3 );if (rid_base & ~map_mask) { -pr_err ("%s: Invalid %s translation - %s-mask (0x%x) ignores rid-base (0x%x)\n" , - np->full_name, map_name, map_name, +pr_err ("%pOF: Invalid %s translation - %s-mask (0x%x) ignores rid-base (0x%x)\n" , + np, map_name, map_name, map_mask, rid_base);return -EFAULT ; }
@@ -375,14 +375,13 @@ int of_pci_map_rid(struct device_node *np, u32 rid,if (id_out) *id_out = masked_rid - rid_base + out_base; -pr_debug ("%s: %s, using mask %08x, rid-base: %08x, out-base: %08x, length: %08x, rid: %08x -> %08x\n" , - np->full_name, map_name, map_mask, rid_base, out_base, +pr_debug ("%pOF: %s, using mask %08x, rid-base: %08x, out-base: %08x, length: %08x, rid: %08x -> %08x\n" , + np, map_name, map_mask, rid_base, out_base, rid_len, rid, *id_out);return 0 ; } -pr_err ("%s: Invalid %s translation - no match for rid 0x%x on %s\n" , - np->full_name, map_name, rid, - target && *target ? (*target)->full_name :"any target" ); +pr_err ("%pOF: Invalid %s translation - no match for rid 0x%x on %pOF\n" , + np, map_name, rid, target && *target ? *target :NULL );return -EFAULT ; }
drivers/of/overlay.c +10 -11
@@ -137,7 +137,7 @@ static int of_overlay_apply_single_device_node(struct of_overlay *ov,of_node_put (tchild); }else {/* create empty tree as a target */ - tchild =__of_node_dup (child,"%s/%s" , target->full_name, cname); + tchild =__of_node_dup (child,"%pOF/%s" , target, cname);if (!tchild)return -ENOMEM ;
@@ -173,8 +173,8 @@ static int of_overlay_apply_one(struct of_overlay *ov,for_each_property_of_node (overlay, prop) { ret =of_overlay_apply_single_property (ov, target, prop);if (ret) { -pr_err ("Failed to apply prop @%s/%s\n" , - target->full_name, prop->name); +pr_err ("Failed to apply prop @%pOF/%s\n" , + target, prop->name);return ret; } }
@@ -182,8 +182,8 @@ static int of_overlay_apply_one(struct of_overlay *ov,for_each_child_of_node (overlay, child) { ret =of_overlay_apply_single_device_node (ov, target, child);if (ret !=0 ) { -pr_err ("Failed to apply single node @%s/%s\n" , - target->full_name, child->name); +pr_err ("Failed to apply single node @%pOF/%s\n" , + target, child->name);of_node_put (child);return ret; }
@@ -211,7 +211,7 @@ static int of_overlay_apply(struct of_overlay *ov) err =of_overlay_apply_one (ov, ovinfo->target, ovinfo->overlay);if (err !=0 ) { -pr_err ("apply failed '%s'\n" , ovinfo->target->full_name); +pr_err ("apply failed '%pOF'\n" , ovinfo->target);return err; } }
@@ -396,8 +396,8 @@ int of_overlay_create(struct device_node *tree)/* build the overlay info structures */ err =of_build_overlay_info (ov, tree);if (err) { -pr_err ("of_build_overlay_info() failed for tree@%s\n" , - tree->full_name); +pr_err ("of_build_overlay_info() failed for tree@%pOF\n" , + tree); goto err_free_idr; }
@@ -476,9 +476,8 @@ static int overlay_is_topmost(struct of_overlay *ov, struct device_node *dn)/* check against each subtree affected by this overlay */ list_for_each_entry (ce, &ovt->cset.entries, node) {if (overlay_subtree_check (ce->np, dn)) { -pr_err ("%s: #%d clashes #%d @%s\n" , - __func__, ov->id, ovt->id, - dn->full_name); +pr_err ("%s: #%d clashes #%d @%pOF\n" , + __func__, ov->id, ovt->id, dn);return 0 ; } }
drivers/of/platform.c +16 -16
@@ -228,7 +228,7 @@ static struct amba_device *of_amba_device_create(struct device_node *node,const void *prop;int i, ret; -pr_debug ("Creating amba device %s\n" , node->full_name); +pr_debug ("Creating amba device %pOF\n" , node);if (!of_device_is_available (node) ||of_node_test_and_set_flag (node,OF_POPULATED ))
@@ -259,15 +259,15 @@ static struct amba_device *of_amba_device_create(struct device_node *node, ret =of_address_to_resource (node,0 , &dev->res);if (ret) { -pr_err ("amba: of_address_to_resource() failed (%d) for %s\n" , - ret, node->full_name); +pr_err ("amba: of_address_to_resource() failed (%d) for %pOF\n" , + ret, node); goto err_free; } ret =amba_device_add (dev, &iomem_resource);if (ret) { -pr_err ("amba_device_add() failed (%d) for %s\n" , - ret, node->full_name); +pr_err ("amba_device_add() failed (%d) for %pOF\n" , + ret, node); goto err_free; }
@@ -310,7 +310,7 @@ static const struct of_dev_auxdata *of_dev_lookup(const struct of_dev_auxdata *lif (!of_address_to_resource (np,0 , &res))if (res.start != auxdata->phys_addr)continue ; -pr_debug ("%s: devname=%s\n" , np->full_name, auxdata->name); +pr_debug ("%pOF: devname=%s\n" , np, auxdata->name);return auxdata; }
@@ -323,7 +323,7 @@ static const struct of_dev_auxdata *of_dev_lookup(const struct of_dev_auxdata *lif (!of_device_is_compatible (np, auxdata->compatible))continue ;if (!auxdata->phys_addr && !auxdata->name) { -pr_debug ("%s: compatible match\n" , np->full_name); +pr_debug ("%pOF: compatible match\n" , np);return auxdata; } }
@@ -356,14 +356,14 @@ static int of_platform_bus_create(struct device_node *bus,/* Make sure it has a compatible property */ if (strict && (!of_get_property (bus,"compatible" ,NULL ))) { -pr_debug ("%s() - skipping %s, no compatible prop\n" , - __func__, bus->full_name); +pr_debug ("%s() - skipping %pOF, no compatible prop\n" , + __func__, bus);return 0 ; }if (of_node_check_flag (bus,OF_POPULATED_BUS )) { -pr_debug ("%s() - skipping %s, already populated\n" , - __func__, bus->full_name); +pr_debug ("%s() - skipping %pOF, already populated\n" , + __func__, bus);return 0 ; }
@@ -387,7 +387,7 @@ static int of_platform_bus_create(struct device_node *bus,return 0 ;for_each_child_of_node (bus, child) { -pr_debug (" create child: %s\n" , child->full_name); +pr_debug (" create child: %pOF\n" , child); rc =of_platform_bus_create (child, matches, lookup, &dev->dev, strict);if (rc) {of_node_put (child);
@@ -419,7 +419,7 @@ int of_platform_bus_probe(struct device_node *root,return -EINVAL ;pr_debug ("%s()\n" , __func__); -pr_debug (" starting at: %s\n" , root->full_name); +pr_debug (" starting at: %pOF\n" , root);/* Do a self check of bus type, if there's a match, create children */ if (of_match_node (matches, root)) {
@@ -471,7 +471,7 @@ int of_platform_populate(struct device_node *root,return -EINVAL ;pr_debug ("%s()\n" , __func__); -pr_debug (" starting at: %s\n" , root->full_name); +pr_debug (" starting at: %pOF\n" , root);for_each_child_of_node (root, child) { rc =of_platform_bus_create (child, matches, lookup, parent, true);
@@ -659,8 +659,8 @@ static int of_platform_notify(struct notifier_block *nb,of_dev_put (pdev_parent);if (pdev ==NULL ) { -pr_err ("%s: failed to create for '%s'\n" , - __func__, rd->dn->full_name); +pr_err ("%s: failed to create for '%pOF'\n" , + __func__, rd->dn);/* of_platform_device_create tosses the error code */ return notifier_from_errno (-EINVAL ); }
drivers/of/unittest.c +38 -28
194 lines not on this page · raw message
Signature · 1 line
2.11.0
Joe Perches
maintainerRe: [PATCH 4/4] of: Convert to using %pOF instead of full_name
Quoted 3 lines · Rob Herring
Now that we have a custom printf format specifier, convert users of
full_nameto use %pOF instead. This is preparation to remove storing of the full path string for each node.
Well, a full_name removal makes my reply in 2/4 less sensible. Maybe 2/4 should say that full_name is going away.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html