Re: [PATCH] of: Custom printk format specifier for device node
On Tue, 2015-03-31 at 21:52 -0700, Grant Likely wrote: > Thinking about this more, I'd like to suggest a different format that > gives us a nice hack on the name that makes it easy to remember: > '%pOF[...]' > 'O' still means 'object', but it is also overloaded for Open Firmware. > That still leaves %pO? for other object types. What do you think? I think that's fine. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] of: Custom printk format specifier for device node
On Tue, 31 Mar 2015 13:03:05 +0300 , Pantelis Antoniou wrote: > > +Device tree nodes: > > + > > + %pOn[fnpPcCFr] > > + > > + For printing device tree nodes. The optional arguments are: > > + f device node full_name > > + n device node name > > + p device node phandle > > + P device node path spec (name + @unit) > > + F device node flags > > + c major compatible string > > + C full compatible string > > + Without any arguments prints full_name (same as %pOnf) > > + The separator when using multiple arguments is â:â > ^ separator is â.' > > + > > > + Examples: > > + > > + %pOn/foo/bar@0 - Node full name > > + %pOnf /foo/bar@0 - Same as above > > + %pOnfp /foo/bar@0:10 - Node full name + phandle > > + %pOnfcF /foo/bar@0:foo,device:--P- - Node full name + > > + major compatible string + > > + node flags > > + D - dynamic > > + d - detached > > + P - Populated > > + B - Populated bus > > + Thinking about this more, I'd like to suggest a different format that gives us a nice hack on the name that makes it easy to remember: '%pOF[...]' 'O' still means 'object', but it is also overloaded for Open Firmware. That still leaves %pO? for other object types. What do you think? g. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] of: Custom printk format specifier for device node
Hi Grant,
> On Mar 31, 2015, at 20:02 , Grant Likely wrote:
>
> Hi Pantelis,
>
> Thanks for the quick reply. Comments below...
>
> On Tue, 31 Mar 2015 13:03:05 +0300
> , Pantelis Antoniou
> wrote:
>> Hi Grant,
>>
>>> On Mar 30, 2015, at 22:04 , Grant Likely wrote:
>>>
>>> On Thu, 22 Jan 2015 22:31:46 +0200
>>> , Pantelis Antoniou
>>> wrote:
Hi Joe,
> On Jan 21, 2015, at 19:37 , Joe Perches wrote:
>
> On Wed, 2015-01-21 at 19:06 +0200, Pantelis Antoniou wrote:
>> 90% of the usage of device node's full_name is printing it out
>> in a kernel message. Preparing for the eventual delayed allocation
>> introduce a custom printk format specifier that is both more
>> compact and more pleasant to the eye.
>>
>> For instance typical use is:
>> pr_info("Frobbing node %s\n", node->full_name);
>>
>> Which can be written now as:
>> pr_info("Frobbing node %pO\n", node);
> Still disliking use of %p0.
>
pO - Open Firmware
pT for tree is bad, cause we plan to use a tree type in the future in OF.
>>>
>>> So, here's a radical thought. How about we reserve '%pO' for objects, as
>>> in kobjects. We'll use extra flags to narrow down specifically to
>>> device tree nodes, but we could teach vsprintf() to treat a plain '%pO'
>>> as plain kobject pointer, and if it is able to recognize the kobj_type,
>>> then run a specific decoder to format it.
>>>
>>> This also gives us a namespace for various kinds of firmware data
>>> output. %Od could be a struct device, %On for device tree node, %Oa for
>>> an ACPI node, etc.
>>>
>>
>> I’m fine with this. I also have a patch to turn an overlay to a kobj
>> so this fits naturally.
>>
>> OTOH if we do this, I would expect to rework the custom printk infrastructure
>> to be more generic.
>>
>> IMHO having the format specifier and the format print methods in
>> lib/vsprintf.c
>> is not very nice.
>>
>> How about having a way to register object printk handlers with something
>> like that?
>> We could put that in a special linker section and have the printk method
>> pass control
>> there.
>>
>> PRINTK_OBJFMT(’n’, printk_objfmt_device_node);
>>
>> We might have to make a few printk methods public however.
>
> Honestly, I think trying to add registration is an overengineered
> solution at this point. We're not hitting a wall on the complexity of
> vsprintf.c, and having them all in one place helps to ensure we don't
> have conflicts.
>
>>
>>> I've dropped the refcount decoder. I know it is useful for debugging the
>>> core DT code, but it isn't something that will be used generally. Plus
>>> the returned value cannot be relied upon to be stable because there may
>>> be other code currently iterating over the tree.
>>>
>>
>> Yeah, I know it’s not something to rely on. If we do %pOk to be kobj
>> debug I can add it back in.
>
> Yes, that would be a good place to have refcount output.
>
>>> +Device tree nodes:
>>> +
>>> + %pOn[fnpPcCFr]
>>> +
>>> + For printing device tree nodes. The optional arguments are:
>>> + f device node full_name
>>> + n device node name
>>> + p device node phandle
>>> + P device node path spec (name + @unit)
>>> + F device node flags
>>> + c major compatible string
>>> + C full compatible string
>>> + Without any arguments prints full_name (same as %pOnf)
>>> + The separator when using multiple arguments is ‘:’
>> ^ separator is ‘.'
>
> ? I'm confused? The separator that I'm using is a colon. ':' Where do
> you see ','? I don't think ',' would be a good separator because it
> appears in node names and compatible strings. Originally, I think you
> were using pipe '|', but my personal opinion is that ':' is better
> because there is already precidence as a separator.
>
Ugh, -EJETLAG.
You’re correct, sorry for the confusion.
> g.
Regards
— Pantelis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] of: Custom printk format specifier for device node
Hi Pantelis,
Thanks for the quick reply. Comments below...
On Tue, 31 Mar 2015 13:03:05 +0300
, Pantelis Antoniou
wrote:
> Hi Grant,
>
> > On Mar 30, 2015, at 22:04 , Grant Likely wrote:
> >
> > On Thu, 22 Jan 2015 22:31:46 +0200
> > , Pantelis Antoniou
> > wrote:
> >> Hi Joe,
> >>
> >>> On Jan 21, 2015, at 19:37 , Joe Perches wrote:
> >>>
> >>> On Wed, 2015-01-21 at 19:06 +0200, Pantelis Antoniou wrote:
> 90% of the usage of device node's full_name is printing it out
> in a kernel message. Preparing for the eventual delayed allocation
> introduce a custom printk format specifier that is both more
> compact and more pleasant to the eye.
>
> For instance typical use is:
> pr_info("Frobbing node %s\n", node->full_name);
>
> Which can be written now as:
> pr_info("Frobbing node %pO\n", node);
> >>
> >>> Still disliking use of %p0.
> >>>
> >>
> >> pO - Open Firmware
> >>
> >> pT for tree is bad, cause we plan to use a tree type in the future in OF.
> >
> > So, here's a radical thought. How about we reserve '%pO' for objects, as
> > in kobjects. We'll use extra flags to narrow down specifically to
> > device tree nodes, but we could teach vsprintf() to treat a plain '%pO'
> > as plain kobject pointer, and if it is able to recognize the kobj_type,
> > then run a specific decoder to format it.
> >
> > This also gives us a namespace for various kinds of firmware data
> > output. %Od could be a struct device, %On for device tree node, %Oa for
> > an ACPI node, etc.
> >
>
> Iâm fine with this. I also have a patch to turn an overlay to a kobj
> so this fits naturally.
>
> OTOH if we do this, I would expect to rework the custom printk infrastructure
> to be more generic.
>
> IMHO having the format specifier and the format print methods in
> lib/vsprintf.c
> is not very nice.
>
> How about having a way to register object printk handlers with something like
> that?
> We could put that in a special linker section and have the printk method pass
> control
> there.
>
> PRINTK_OBJFMT(ânâ, printk_objfmt_device_node);
>
> We might have to make a few printk methods public however.
Honestly, I think trying to add registration is an overengineered
solution at this point. We're not hitting a wall on the complexity of
vsprintf.c, and having them all in one place helps to ensure we don't
have conflicts.
>
> > I've dropped the refcount decoder. I know it is useful for debugging the
> > core DT code, but it isn't something that will be used generally. Plus
> > the returned value cannot be relied upon to be stable because there may
> > be other code currently iterating over the tree.
> >
>
> Yeah, I know itâs not something to rely on. If we do %pOk to be kobj
> debug I can add it back in.
Yes, that would be a good place to have refcount output.
> > +Device tree nodes:
> > +
> > + %pOn[fnpPcCFr]
> > +
> > + For printing device tree nodes. The optional arguments are:
> > + f device node full_name
> > + n device node name
> > + p device node phandle
> > + P device node path spec (name + @unit)
> > + F device node flags
> > + c major compatible string
> > + C full compatible string
> > + Without any arguments prints full_name (same as %pOnf)
> > + The separator when using multiple arguments is â:â
> ^ separator is â.'
? I'm confused? The separator that I'm using is a colon. ':' Where do
you see ','? I don't think ',' would be a good separator because it
appears in node names and compatible strings. Originally, I think you
were using pipe '|', but my personal opinion is that ':' is better
because there is already precidence as a separator.
g.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] of: Custom printk format specifier for device node
Hi Grant,
> On Mar 30, 2015, at 22:04 , Grant Likely wrote:
>
> On Thu, 22 Jan 2015 22:31:46 +0200
> , Pantelis Antoniou
> wrote:
>> Hi Joe,
>>
>>> On Jan 21, 2015, at 19:37 , Joe Perches wrote:
>>>
>>> On Wed, 2015-01-21 at 19:06 +0200, Pantelis Antoniou wrote:
90% of the usage of device node's full_name is printing it out
in a kernel message. Preparing for the eventual delayed allocation
introduce a custom printk format specifier that is both more
compact and more pleasant to the eye.
For instance typical use is:
pr_info("Frobbing node %s\n", node->full_name);
Which can be written now as:
pr_info("Frobbing node %pO\n", node);
>>
>>> Still disliking use of %p0.
>>>
>>
>> pO - Open Firmware
>>
>> pT for tree is bad, cause we plan to use a tree type in the future in OF.
>
> So, here's a radical thought. How about we reserve '%pO' for objects, as
> in kobjects. We'll use extra flags to narrow down specifically to
> device tree nodes, but we could teach vsprintf() to treat a plain '%pO'
> as plain kobject pointer, and if it is able to recognize the kobj_type,
> then run a specific decoder to format it.
>
> This also gives us a namespace for various kinds of firmware data
> output. %Od could be a struct device, %On for device tree node, %Oa for
> an ACPI node, etc.
>
I’m fine with this. I also have a patch to turn an overlay to a kobj
so this fits naturally.
OTOH if we do this, I would expect to rework the custom printk infrastructure
to be more generic.
IMHO having the format specifier and the format print methods in lib/vsprintf.c
is not very nice.
How about having a way to register object printk handlers with something like
that?
We could put that in a special linker section and have the printk method pass
control
there.
PRINTK_OBJFMT(’n’, printk_objfmt_device_node);
We might have to make a few printk methods public however.
>>
More fine-grained control of formatting includes printing the name,
flag, path-spec name, reference count and others, explained in the
documentation entry.
Signed-off-by: Pantelis Antoniou
dt-print
---
Documentation/printk-formats.txt | 29
lib/vsprintf.c | 151
+++
2 files changed, 180 insertions(+)
diff --git a/Documentation/printk-formats.txt
b/Documentation/printk-formats.txt
index 5a615c1..2d42c04 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -231,6 +231,35 @@ struct va_format:
Do not use this feature without some mechanism to verify the
correctness of the format string and va_list arguments.
+Device tree nodes:
+
+ %pO[fnpPcCFr]
+
+ For printing device tree nodes. The optional arguments are:
+f device node full_name
+n device node name
+p device node phandle
+P device node path spec (name + @unit)
+F device node flags
+c major compatible string
+C full compatible string
+r node reference count
+ Without any arguments prints full_name (same as %pOf)
+ The separator when using multiple arguments is '|'
+
+ Examples:
+
+ %pO /foo/bar@0 - Node full name
+ %pOf/foo/bar@0 - Same as above
+ %pOfp /foo/bar@0|10 - Node full name + phandle
+ %pOfcF /foo/bar@0|foo,device|--P- - Node full name +
+major compatible string +
+node flags
+ D - dynamic
+ d - detached
+ P - Populated
+ B - Populated bus
+
u64 SHOULD be printed with %llu/%llx:
printk("%llu", u64_var);
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>>> []
>>>
>>> Add #ifdef back ?
>>>
>>
>> The whole thing is optimized away when CONFIG_OF is not defined, leaving only
>> the return statement.
>>
+static noinline_for_stack
+char *device_node_string(char *buf, char *end, struct device_node *dn,
+ struct printf_spec spec, const char *fmt)
+{
+ char tbuf[sizeof("xx") + 1];
+ const char *fmtp, *p;
+ int len, ret, i, j, pass;
+ char c;
+
+ if (!IS_ENABLED(CONFIG_OF))
+ return string(buf, end, "(!OF)", spec);
>>>
>>> Not very descriptive output, maybe the address would be better.
>>>
>>
>> OK
>>
+
+ if ((unsigned long)dn < PAGE_SIZE)
+ return string(buf, end, "(null)", spec);
+
+
Re: [PATCH] of: Custom printk format specifier for device node
On Thu, 22 Jan 2015 22:31:46 +0200
, Pantelis Antoniou
wrote:
> Hi Joe,
>
> > On Jan 21, 2015, at 19:37 , Joe Perches wrote:
> >
> > On Wed, 2015-01-21 at 19:06 +0200, Pantelis Antoniou wrote:
> >> 90% of the usage of device node's full_name is printing it out
> >> in a kernel message. Preparing for the eventual delayed allocation
> >> introduce a custom printk format specifier that is both more
> >> compact and more pleasant to the eye.
> >>
> >> For instance typical use is:
> >>pr_info("Frobbing node %s\n", node->full_name);
> >>
> >> Which can be written now as:
> >>pr_info("Frobbing node %pO\n", node);
>
> > Still disliking use of %p0.
> >
>
> pO - Open Firmware
>
> pT for tree is bad, cause we plan to use a tree type in the future in OF.
So, here's a radical thought. How about we reserve '%pO' for objects, as
in kobjects. We'll use extra flags to narrow down specifically to
device tree nodes, but we could teach vsprintf() to treat a plain '%pO'
as plain kobject pointer, and if it is able to recognize the kobj_type,
then run a specific decoder to format it.
This also gives us a namespace for various kinds of firmware data
output. %Od could be a struct device, %On for device tree node, %Oa for
an ACPI node, etc.
>
> >> More fine-grained control of formatting includes printing the name,
> >> flag, path-spec name, reference count and others, explained in the
> >> documentation entry.
> >>
> >> Signed-off-by: Pantelis Antoniou
> >>
> >> dt-print
> >> ---
> >> Documentation/printk-formats.txt | 29
> >> lib/vsprintf.c | 151
> >> +++
> >> 2 files changed, 180 insertions(+)
> >>
> >> diff --git a/Documentation/printk-formats.txt
> >> b/Documentation/printk-formats.txt
> >> index 5a615c1..2d42c04 100644
> >> --- a/Documentation/printk-formats.txt
> >> +++ b/Documentation/printk-formats.txt
> >> @@ -231,6 +231,35 @@ struct va_format:
> >>Do not use this feature without some mechanism to verify the
> >>correctness of the format string and va_list arguments.
> >>
> >> +Device tree nodes:
> >> +
> >> + %pO[fnpPcCFr]
> >> +
> >> + For printing device tree nodes. The optional arguments are:
> >> +f device node full_name
> >> +n device node name
> >> +p device node phandle
> >> +P device node path spec (name + @unit)
> >> +F device node flags
> >> +c major compatible string
> >> +C full compatible string
> >> +r node reference count
> >> + Without any arguments prints full_name (same as %pOf)
> >> + The separator when using multiple arguments is '|'
> >> +
> >> + Examples:
> >> +
> >> + %pO /foo/bar@0 - Node full name
> >> + %pOf/foo/bar@0 - Same as above
> >> + %pOfp /foo/bar@0|10 - Node full name + phandle
> >> + %pOfcF /foo/bar@0|foo,device|--P- - Node full name +
> >> +major compatible string +
> >> +node flags
> >> + D - dynamic
> >> + d - detached
> >> + P - Populated
> >> + B - Populated bus
> >> +
> >> u64 SHOULD be printed with %llu/%llx:
> >>
> >>printk("%llu", u64_var);
> >> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > []
> >
> > Add #ifdef back ?
> >
>
> The whole thing is optimized away when CONFIG_OF is not defined, leaving only
> the return statement.
>
> >> +static noinline_for_stack
> >> +char *device_node_string(char *buf, char *end, struct device_node *dn,
> >> + struct printf_spec spec, const char *fmt)
> >> +{
> >> + char tbuf[sizeof("xx") + 1];
> >> + const char *fmtp, *p;
> >> + int len, ret, i, j, pass;
> >> + char c;
> >> +
> >> + if (!IS_ENABLED(CONFIG_OF))
> >> + return string(buf, end, "(!OF)", spec);
> >
> > Not very descriptive output, maybe the address would be better.
> >
>
> OK
>
> >> +
> >> + if ((unsigned long)dn < PAGE_SIZE)
> >> + return string(buf, end, "(null)", spec);
> >> +
> >> + /* simple case without anything any more format specifiers */
> >> + if (fmt[1] == '\0' || isspace(fmt[1]))
> >> + fmt = "Of";
s/isspace()/!isalnum() to match with what the pointer() function does
when finding the end of a format string.
> >
> > why lower case here but upper case above?
> >
>
> Cause '(null)' is whatâs printed in string() when null is passed as a
> pointer.
>
> >> +
> >> + len = 0;
> >> +
> >> + /* two passes; the first calculates length, the second fills in */
> >> + for (pass = 1; pass <= 2; pass++) {
> >> + if (pass == 2 && !(spec.flags & LEFT)) {
> >> + /* padding */
> >> + while (len < spec.f
Re: [PATCH] of: Custom printk format specifier for device node
Hi Joe,
> On Jan 21, 2015, at 19:37 , Joe Perches wrote:
>
> On Wed, 2015-01-21 at 19:06 +0200, Pantelis Antoniou wrote:
>> 90% of the usage of device node's full_name is printing it out
>> in a kernel message. Preparing for the eventual delayed allocation
>> introduce a custom printk format specifier that is both more
>> compact and more pleasant to the eye.
>>
>> For instance typical use is:
>> pr_info("Frobbing node %s\n", node->full_name);
>>
>> Which can be written now as:
>> pr_info("Frobbing node %pO\n", node);
> Still disliking use of %p0.
>
pO - Open Firmware
pT for tree is bad, cause we plan to use a tree type in the future in OF.
>> More fine-grained control of formatting includes printing the name,
>> flag, path-spec name, reference count and others, explained in the
>> documentation entry.
>>
>> Signed-off-by: Pantelis Antoniou
>>
>> dt-print
>> ---
>> Documentation/printk-formats.txt | 29
>> lib/vsprintf.c | 151
>> +++
>> 2 files changed, 180 insertions(+)
>>
>> diff --git a/Documentation/printk-formats.txt
>> b/Documentation/printk-formats.txt
>> index 5a615c1..2d42c04 100644
>> --- a/Documentation/printk-formats.txt
>> +++ b/Documentation/printk-formats.txt
>> @@ -231,6 +231,35 @@ struct va_format:
>> Do not use this feature without some mechanism to verify the
>> correctness of the format string and va_list arguments.
>>
>> +Device tree nodes:
>> +
>> +%pO[fnpPcCFr]
>> +
>> +For printing device tree nodes. The optional arguments are:
>> +f device node full_name
>> +n device node name
>> +p device node phandle
>> +P device node path spec (name + @unit)
>> +F device node flags
>> +c major compatible string
>> +C full compatible string
>> +r node reference count
>> +Without any arguments prints full_name (same as %pOf)
>> +The separator when using multiple arguments is '|'
>> +
>> +Examples:
>> +
>> +%pO /foo/bar@0 - Node full name
>> +%pOf/foo/bar@0 - Same as above
>> +%pOfp /foo/bar@0|10 - Node full name + phandle
>> +%pOfcF /foo/bar@0|foo,device|--P- - Node full name +
>> + major compatible string +
>> + node flags
>> +D - dynamic
>> +d - detached
>> +P - Populated
>> +B - Populated bus
>> +
>> u64 SHOULD be printed with %llu/%llx:
>>
>> printk("%llu", u64_var);
>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> []
>
> Add #ifdef back ?
>
The whole thing is optimized away when CONFIG_OF is not defined, leaving only
the return statement.
>> +static noinline_for_stack
>> +char *device_node_string(char *buf, char *end, struct device_node *dn,
>> + struct printf_spec spec, const char *fmt)
>> +{
>> +char tbuf[sizeof("xx") + 1];
>> +const char *fmtp, *p;
>> +int len, ret, i, j, pass;
>> +char c;
>> +
>> +if (!IS_ENABLED(CONFIG_OF))
>> +return string(buf, end, "(!OF)", spec);
>
> Not very descriptive output, maybe the address would be better.
>
OK
>> +
>> +if ((unsigned long)dn < PAGE_SIZE)
>> +return string(buf, end, "(null)", spec);
>> +
>> +/* simple case without anything any more format specifiers */
>> +if (fmt[1] == '\0' || isspace(fmt[1]))
>> +fmt = "Of";
>
> why lower case here but upper case above?
>
Cause '(null)' is what’s printed in string() when null is passed as a pointer.
>> +
>> +len = 0;
>> +
>> +/* two passes; the first calculates length, the second fills in */
>> +for (pass = 1; pass <= 2; pass++) {
>> +if (pass == 2 && !(spec.flags & LEFT)) {
>> +/* padding */
>> +while (len < spec.field_width--) {
>> +if (buf < end)
>> +*buf = ' ';
>> +++buf;
>> +}
>> +}
>> +#undef _HANDLE_CH
>> +#define _HANDLE_CH(_ch) \
>> +do { \
>> +if (pass == 1) \
>> +len++; \
>> +else \
>> +if (buf < end) \
>> +*buf++ = (_ch); \
>> +} while (0)
>> +#undef _HANDLE_STR
>> +#define _HANDLE_STR(_str) \
>> +do { \
>> +const char *str = (_str); \
>> +if (pass == 1) \
>> +len += strlen(str); \
>> +else \
>> +while (*str && buf < end) \
>> +*buf++ = *str++; \
>> +} while (0)
>
> This isn't pretty. Perhaps the
Re: [PATCH] of: Custom printk format specifier for device node
Pantelis Antoniou writes:
> Hi Joe,
>
>> On Jan 21, 2015, at 19:37 , Joe Perches wrote:
>>
>> On Wed, 2015-01-21 at 19:06 +0200, Pantelis Antoniou wrote:
>>> 90% of the usage of device node's full_name is printing it out
>>> in a kernel message. Preparing for the eventual delayed allocation
>>> introduce a custom printk format specifier that is both more
>>> compact and more pleasant to the eye.
>>>
>>> For instance typical use is:
>>> pr_info("Frobbing node %s\n", node->full_name);
>>>
>>> Which can be written now as:
>>> pr_info("Frobbing node %pO\n", node);
>>
>> Still disliking use of %p0.
>>
>
> Choices are limited. And it’s pO not p0.
O as in OF. Makes sense.
--
Måns Rullgård
m...@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] of: Custom printk format specifier for device node
On Wed, 2015-01-21 at 19:39 +0200, Pantelis Antoniou wrote:
> Hi Joe,
>
> > On Jan 21, 2015, at 19:37 , Joe Perches wrote:
> >
> > On Wed, 2015-01-21 at 19:06 +0200, Pantelis Antoniou wrote:
> >> 90% of the usage of device node's full_name is printing it out
> >> in a kernel message. Preparing for the eventual delayed allocation
> >> introduce a custom printk format specifier that is both more
> >> compact and more pleasant to the eye.
> >>
> >> For instance typical use is:
> >>pr_info("Frobbing node %s\n", node->full_name);
> >>
> >> Which can be written now as:
> >>pr_info("Frobbing node %pO\n", node);
> >
> > Still disliking use of %p0.
> >
>
> Choices are limited. And it’s pO not p0.
Yet another reason to dislike it.
> > This isn't pretty. Perhaps there's a better way?
> >
>
> No there isn’t.
There always is.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] of: Custom printk format specifier for device node
Hi Joe,
> On Jan 21, 2015, at 19:37 , Joe Perches wrote:
>
> On Wed, 2015-01-21 at 19:06 +0200, Pantelis Antoniou wrote:
>> 90% of the usage of device node's full_name is printing it out
>> in a kernel message. Preparing for the eventual delayed allocation
>> introduce a custom printk format specifier that is both more
>> compact and more pleasant to the eye.
>>
>> For instance typical use is:
>> pr_info("Frobbing node %s\n", node->full_name);
>>
>> Which can be written now as:
>> pr_info("Frobbing node %pO\n", node);
>
> Still disliking use of %p0.
>
Choices are limited. And it’s pO not p0.
>> More fine-grained control of formatting includes printing the name,
>> flag, path-spec name, reference count and others, explained in the
>> documentation entry.
>>
>> Signed-off-by: Pantelis Antoniou
>>
>> dt-print
>> ---
>> Documentation/printk-formats.txt | 29
>> lib/vsprintf.c | 151
>> +++
>> 2 files changed, 180 insertions(+)
>>
>> diff --git a/Documentation/printk-formats.txt
>> b/Documentation/printk-formats.txt
>> index 5a615c1..2d42c04 100644
>> --- a/Documentation/printk-formats.txt
>> +++ b/Documentation/printk-formats.txt
>> @@ -231,6 +231,35 @@ struct va_format:
>> Do not use this feature without some mechanism to verify the
>> correctness of the format string and va_list arguments.
>>
>> +Device tree nodes:
>> +
>> +%pO[fnpPcCFr]
>> +
>> +For printing device tree nodes. The optional arguments are:
>> +f device node full_name
>> +n device node name
>> +p device node phandle
>> +P device node path spec (name + @unit)
>> +F device node flags
>> +c major compatible string
>> +C full compatible string
>> +r node reference count
>> +Without any arguments prints full_name (same as %pOf)
>> +The separator when using multiple arguments is '|'
>> +
>> +Examples:
>> +
>> +%pO /foo/bar@0 - Node full name
>> +%pOf/foo/bar@0 - Same as above
>> +%pOfp /foo/bar@0|10 - Node full name + phandle
>> +%pOfcF /foo/bar@0|foo,device|--P- - Node full name +
>> + major compatible string +
>> + node flags
>> +D - dynamic
>> +d - detached
>> +P - Populated
>> +B - Populated bus
>> +
>> u64 SHOULD be printed with %llu/%llx:
>>
>> printk("%llu", u64_var);
>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> []
>
> Add #ifdef back ?
>
>> +static noinline_for_stack
>> +char *device_node_string(char *buf, char *end, struct device_node *dn,
>> + struct printf_spec spec, const char *fmt)
>> +{
>> +char tbuf[sizeof("xx") + 1];
>> +const char *fmtp, *p;
>> +int len, ret, i, j, pass;
>> +char c;
>> +
>> +if (!IS_ENABLED(CONFIG_OF))
>> +return string(buf, end, "(!OF)", spec);
>
> Not very descriptive output, maybe the address would be better.
>
>> +
>> +if ((unsigned long)dn < PAGE_SIZE)
>> +return string(buf, end, "(null)", spec);
>> +
>> +/* simple case without anything any more format specifiers */
>> +if (fmt[1] == '\0' || isspace(fmt[1]))
>> +fmt = "Of";
>
> why lower case here but upper case above?
>
>> +
>> +len = 0;
>> +
>> +/* two passes; the first calculates length, the second fills in */
>> +for (pass = 1; pass <= 2; pass++) {
>> +if (pass == 2 && !(spec.flags & LEFT)) {
>> +/* padding */
>> +while (len < spec.field_width--) {
>> +if (buf < end)
>> +*buf = ' ';
>> +++buf;
>> +}
>> +}
>> +#undef _HANDLE_CH
>> +#define _HANDLE_CH(_ch) \
>> +do { \
>> +if (pass == 1) \
>> +len++; \
>> +else \
>> +if (buf < end) \
>> +*buf++ = (_ch); \
>> +} while (0)
>> +#undef _HANDLE_STR
>> +#define _HANDLE_STR(_str) \
>> +do { \
>> +const char *str = (_str); \
>> +if (pass == 1) \
>> +len += strlen(str); \
>> +else \
>> +while (*str && buf < end) \
>> +*buf++ = *str++; \
>> +} while (0)
>
> This isn't pretty. Perhaps there's a better way?
>
No there isn’t.
>> +
>> +for (fmtp = fmt + 1, j = 0; (c = *fmtp++) != '\0'; ) {
>> +switch (c) {
>> +case 'f': /* full_name */
>> +
Re: [PATCH] of: Custom printk format specifier for device node
On Wed, 2015-01-21 at 19:06 +0200, Pantelis Antoniou wrote:
> 90% of the usage of device node's full_name is printing it out
> in a kernel message. Preparing for the eventual delayed allocation
> introduce a custom printk format specifier that is both more
> compact and more pleasant to the eye.
>
> For instance typical use is:
> pr_info("Frobbing node %s\n", node->full_name);
>
> Which can be written now as:
> pr_info("Frobbing node %pO\n", node);
Still disliking use of %p0.
> More fine-grained control of formatting includes printing the name,
> flag, path-spec name, reference count and others, explained in the
> documentation entry.
>
> Signed-off-by: Pantelis Antoniou
>
> dt-print
> ---
> Documentation/printk-formats.txt | 29
> lib/vsprintf.c | 151
> +++
> 2 files changed, 180 insertions(+)
>
> diff --git a/Documentation/printk-formats.txt
> b/Documentation/printk-formats.txt
> index 5a615c1..2d42c04 100644
> --- a/Documentation/printk-formats.txt
> +++ b/Documentation/printk-formats.txt
> @@ -231,6 +231,35 @@ struct va_format:
> Do not use this feature without some mechanism to verify the
> correctness of the format string and va_list arguments.
>
> +Device tree nodes:
> +
> + %pO[fnpPcCFr]
> +
> + For printing device tree nodes. The optional arguments are:
> +f device node full_name
> +n device node name
> +p device node phandle
> +P device node path spec (name + @unit)
> +F device node flags
> +c major compatible string
> +C full compatible string
> +r node reference count
> + Without any arguments prints full_name (same as %pOf)
> + The separator when using multiple arguments is '|'
> +
> + Examples:
> +
> + %pO /foo/bar@0 - Node full name
> + %pOf/foo/bar@0 - Same as above
> + %pOfp /foo/bar@0|10 - Node full name + phandle
> + %pOfcF /foo/bar@0|foo,device|--P- - Node full name +
> + major compatible string +
> + node flags
> + D - dynamic
> + d - detached
> + P - Populated
> + B - Populated bus
> +
> u64 SHOULD be printed with %llu/%llx:
>
> printk("%llu", u64_var);
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
[]
Add #ifdef back ?
> +static noinline_for_stack
> +char *device_node_string(char *buf, char *end, struct device_node *dn,
> + struct printf_spec spec, const char *fmt)
> +{
> + char tbuf[sizeof("xx") + 1];
> + const char *fmtp, *p;
> + int len, ret, i, j, pass;
> + char c;
> +
> + if (!IS_ENABLED(CONFIG_OF))
> + return string(buf, end, "(!OF)", spec);
Not very descriptive output, maybe the address would be better.
> +
> + if ((unsigned long)dn < PAGE_SIZE)
> + return string(buf, end, "(null)", spec);
> +
> + /* simple case without anything any more format specifiers */
> + if (fmt[1] == '\0' || isspace(fmt[1]))
> + fmt = "Of";
why lower case here but upper case above?
> +
> + len = 0;
> +
> + /* two passes; the first calculates length, the second fills in */
> + for (pass = 1; pass <= 2; pass++) {
> + if (pass == 2 && !(spec.flags & LEFT)) {
> + /* padding */
> + while (len < spec.field_width--) {
> + if (buf < end)
> + *buf = ' ';
> + ++buf;
> + }
> + }
> +#undef _HANDLE_CH
> +#define _HANDLE_CH(_ch) \
> + do { \
> + if (pass == 1) \
> + len++; \
> + else \
> + if (buf < end) \
> + *buf++ = (_ch); \
> + } while (0)
> +#undef _HANDLE_STR
> +#define _HANDLE_STR(_str) \
> + do { \
> + const char *str = (_str); \
> + if (pass == 1) \
> + len += strlen(str); \
> + else \
> + while (*str && buf < end) \
> + *buf++ = *str++; \
> + } while (0)
This isn't pretty. Perhaps there's a better way?
> +
> + for (fmtp = fmt + 1, j = 0; (c = *fmtp++) != '\0'; ) {
> + switch (c) {
> + case 'f': /* full_name */
> + if (j++ > 0)
> + _HANDLE_CH(':');
> + _HANDLE_STR(of_node_full_name(dn));
> + break;
> + case '
[PATCH] of: Custom printk format specifier for device node
90% of the usage of device node's full_name is printing it out
in a kernel message. Preparing for the eventual delayed allocation
introduce a custom printk format specifier that is both more
compact and more pleasant to the eye.
For instance typical use is:
pr_info("Frobbing node %s\n", node->full_name);
Which can be written now as:
pr_info("Frobbing node %pO\n", node);
More fine-grained control of formatting includes printing the name,
flag, path-spec name, reference count and others, explained in the
documentation entry.
Signed-off-by: Pantelis Antoniou
dt-print
---
Documentation/printk-formats.txt | 29
lib/vsprintf.c | 151 +++
2 files changed, 180 insertions(+)
diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 5a615c1..2d42c04 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -231,6 +231,35 @@ struct va_format:
Do not use this feature without some mechanism to verify the
correctness of the format string and va_list arguments.
+Device tree nodes:
+
+ %pO[fnpPcCFr]
+
+ For printing device tree nodes. The optional arguments are:
+f device node full_name
+n device node name
+p device node phandle
+P device node path spec (name + @unit)
+F device node flags
+c major compatible string
+C full compatible string
+r node reference count
+ Without any arguments prints full_name (same as %pOf)
+ The separator when using multiple arguments is '|'
+
+ Examples:
+
+ %pO /foo/bar@0 - Node full name
+ %pOf/foo/bar@0 - Same as above
+ %pOfp /foo/bar@0|10 - Node full name + phandle
+ %pOfcF /foo/bar@0|foo,device|--P- - Node full name +
+ major compatible string +
+ node flags
+ D - dynamic
+ d - detached
+ P - Populated
+ B - Populated bus
+
u64 SHOULD be printed with %llu/%llx:
printk("%llu", u64_var);
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index ec337f6..f3e49b9 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -29,6 +29,7 @@
#include
#include
#include
+#include
#include /* for PAGE_SIZE */
#include /* for dereference_function_descriptor() */
@@ -1240,6 +1241,144 @@ char *address_val(char *buf, char *end, const void
*addr,
return number(buf, end, num, spec);
}
+static noinline_for_stack
+char *device_node_string(char *buf, char *end, struct device_node *dn,
+struct printf_spec spec, const char *fmt)
+{
+ char tbuf[sizeof("xx") + 1];
+ const char *fmtp, *p;
+ int len, ret, i, j, pass;
+ char c;
+
+ if (!IS_ENABLED(CONFIG_OF))
+ return string(buf, end, "(!OF)", spec);
+
+ if ((unsigned long)dn < PAGE_SIZE)
+ return string(buf, end, "(null)", spec);
+
+ /* simple case without anything any more format specifiers */
+ if (fmt[1] == '\0' || isspace(fmt[1]))
+ fmt = "Of";
+
+ len = 0;
+
+ /* two passes; the first calculates length, the second fills in */
+ for (pass = 1; pass <= 2; pass++) {
+ if (pass == 2 && !(spec.flags & LEFT)) {
+ /* padding */
+ while (len < spec.field_width--) {
+ if (buf < end)
+ *buf = ' ';
+ ++buf;
+ }
+ }
+#undef _HANDLE_CH
+#define _HANDLE_CH(_ch) \
+ do { \
+ if (pass == 1) \
+ len++; \
+ else \
+ if (buf < end) \
+ *buf++ = (_ch); \
+ } while (0)
+#undef _HANDLE_STR
+#define _HANDLE_STR(_str) \
+ do { \
+ const char *str = (_str); \
+ if (pass == 1) \
+ len += strlen(str); \
+ else \
+ while (*str && buf < end) \
+ *buf++ = *str++; \
+ } while (0)
+
+ for (fmtp = fmt + 1, j = 0; (c = *fmtp++) != '\0'; ) {
+ switch (c) {
+ case 'f': /* full_name */
+ if (j++ > 0)
+ _HANDLE_CH(':');
+ _HANDLE_STR(of_node_full_name(dn));
+ break;
+ case 'n': /* name */
+
Re: [PATCH] of: Custom printk format specifier for device node
On Tue, 2015-01-20 at 20:06 +0200, Pantelis Antoniou wrote:
> On Jan 20, 2015, at 19:59 , Joe Perches wrote:
> > On Tue, 2015-01-20 at 16:52 +0200, Pantelis Antoniou wrote:
> >>> On Jan 20, 2015, at 16:47 , Rob Herring wrote:
> >>> On Tue, Jan 20, 2015 at 8:34 AM, Pantelis Antoniou
> >>> wrote:
> 90% of the usage of device node's full_name is printing it out
> in a kernel message. Preparing for the eventual delayed allocation
> introduce a custom printk format specifier that is both more
> compact and more pleasant to the eye.
> > []
> diff --git a/Documentation/printk-formats.txt
> b/Documentation/printk-formats.txt
> > []
> +Device tree nodes:
> +
> + %pO{,1,2}
> >>>
> >>> 'O' is not very obvious, but I imagine we are somewhat limted in our
> >>> choice here?
> >>>
> >>
> >> All the good women are married, all the handsome men are gay, all the
> >> obvious
> >> format specifiers are taken.
> >
> > Not really at all.
> >
> > I quite dislike '0' as the format type specifier for
> > a device tree node as there's no mnemonic mapping.
> >
> There’s only so many characters one can use; [DdNn] are taken. And it’s O !=
> 0.
The '[Dd]' entry type doesn't just have to be used for dentries.
It could also be used for Device Tree Nodes via 'DTN[x]'
'T' for tree could also be used.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] of: Custom printk format specifier for device node
Hi Joe,
> On Jan 20, 2015, at 19:59 , Joe Perches wrote:
>
> On Tue, 2015-01-20 at 16:52 +0200, Pantelis Antoniou wrote:
>> Hi Rob,
>>
>>> On Jan 20, 2015, at 16:47 , Rob Herring wrote:
>>>
>>> On Tue, Jan 20, 2015 at 8:34 AM, Pantelis Antoniou
>>> wrote:
90% of the usage of device node's full_name is printing it out
in a kernel message. Preparing for the eventual delayed allocation
introduce a custom printk format specifier that is both more
compact and more pleasant to the eye.
> []
diff --git a/Documentation/printk-formats.txt
b/Documentation/printk-formats.txt
> []
+Device tree nodes:
+
+ %pO{,1,2}
>>>
>>> 'O' is not very obvious, but I imagine we are somewhat limted in our
>>> choice here?
>>>
>>
>> All the good women are married, all the handsome men are gay, all the obvious
>> format specifiers are taken.
>
> Not really at all.
>
> I quite dislike '0' as the format type specifier for
> a device tree node as there's no mnemonic mapping.
>
There’s only so many characters one can use; [DdNn] are taken. And it’s O != 0.
I am open to suggestions.
Regards
— Pantelis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] of: Custom printk format specifier for device node
On Tue, 2015-01-20 at 16:52 +0200, Pantelis Antoniou wrote:
> Hi Rob,
>
> > On Jan 20, 2015, at 16:47 , Rob Herring wrote:
> >
> > On Tue, Jan 20, 2015 at 8:34 AM, Pantelis Antoniou
> > wrote:
> >> 90% of the usage of device node's full_name is printing it out
> >> in a kernel message. Preparing for the eventual delayed allocation
> >> introduce a custom printk format specifier that is both more
> >> compact and more pleasant to the eye.
[]
> >> diff --git a/Documentation/printk-formats.txt
> >> b/Documentation/printk-formats.txt
[]
> >> +Device tree nodes:
> >> +
> >> + %pO{,1,2}
> >
> > 'O' is not very obvious, but I imagine we are somewhat limted in our
> > choice here?
> >
>
> All the good women are married, all the handsome men are gay, all the obvious
> format specifiers are taken.
Not really at all.
I quite dislike '0' as the format type specifier for
a device tree node as there's no mnemonic mapping.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] of: Custom printk format specifier for device node
On Tue, Jan 20, 2015 at 10:05 AM, Måns Rullgård wrote: > Pantelis Antoniou writes: > >> Hi Geert, >> >>> On Jan 20, 2015, at 17:24 , Geert Uytterhoeven wrote: >>> >>> On Tue, Jan 20, 2015 at 3:47 PM, Rob Herring wrote: > + Examples: > + > + %pO /foo/bar@0 - Node full name > + %pO0/foo/bar@0 - Same as above > + %pO1/foo/bar@0[10] - Node full name + phandle > + %pO2/foo/bar@0[10:DdPB] - Node full name + phandle + node > flags > +D - dynamic > +d - detached > +P - Populated > +B - Populated bus We should think about what else we want to print for a node. Perhaps 'On' for name, 'Oc' for compatible, etc. >>> >>> I was just going to say "The least verbose variant is name, not full_name”. >>> >> >> Unfortunately in the context of device tree nodes ‘name' is usually >> not what you want to print to identify the node in question. ‘name’ is >> usually not unique. > > Name and address without the full path is usually a good compromise > between uniqueness (it is usually unique for memory-mapped things) and > verbosity. How much of the address is in the name depends on how the address translation is done. I don't think we really need to do full address translations here. %pOn /foo/bar@0 - Node full name %pOn0 bar@0 - Node name and unit address %pOn1/foo/bar@0[10] - Node full name + phandle %pOn2/foo/bar@0[10:DdPB] - Node full name + phandle + node flags %pOc vendor,foo-bar - Most significant compatible string We could do phandle and/or node flags as separate specifiers such as %pOf for flags. I'm not proposing implementing all these now, but just want to make sure we have a structure to do so later. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] of: Custom printk format specifier for device node
Pantelis Antoniou writes: > Hi Geert, > >> On Jan 20, 2015, at 17:24 , Geert Uytterhoeven wrote: >> >> On Tue, Jan 20, 2015 at 3:47 PM, Rob Herring wrote: + Examples: + + %pO /foo/bar@0 - Node full name + %pO0/foo/bar@0 - Same as above + %pO1/foo/bar@0[10] - Node full name + phandle + %pO2/foo/bar@0[10:DdPB] - Node full name + phandle + node flags +D - dynamic +d - detached +P - Populated +B - Populated bus >>> >>> We should think about what else we want to print for a node. Perhaps >>> 'On' for name, 'Oc' for compatible, etc. >> >> I was just going to say "The least verbose variant is name, not full_name”. >> > > Unfortunately in the context of device tree nodes ‘name' is usually > not what you want to print to identify the node in question. ‘name’ is > usually not unique. Name and address without the full path is usually a good compromise between uniqueness (it is usually unique for memory-mapped things) and verbosity. -- Måns Rullgård m...@mansr.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] of: Custom printk format specifier for device node
Hi Geert,
> On Jan 20, 2015, at 17:24 , Geert Uytterhoeven wrote:
>
> On Tue, Jan 20, 2015 at 3:47 PM, Rob Herring wrote:
>>> + Examples:
>>> +
>>> + %pO /foo/bar@0 - Node full name
>>> + %pO0/foo/bar@0 - Same as above
>>> + %pO1/foo/bar@0[10] - Node full name + phandle
>>> + %pO2/foo/bar@0[10:DdPB] - Node full name + phandle + node
>>> flags
>>> +D - dynamic
>>> +d - detached
>>> +P - Populated
>>> +B - Populated bus
>>
>> We should think about what else we want to print for a node. Perhaps
>> 'On' for name, 'Oc' for compatible, etc.
>
> I was just going to say "The least verbose variant is name, not full_name”.
>
Unfortunately in the context of device tree nodes ‘name' is usually not what
you want to print to identify the node in question. ‘name’ is usually not
unique.
We can add a format specifier for it though.
> Gr{oetje,eeting}s,
>
>Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like
> that.
>-- Linus Torvalds
Regards
— Pantelis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] of: Custom printk format specifier for device node
On Tue, Jan 20, 2015 at 3:47 PM, Rob Herring wrote:
>> + Examples:
>> +
>> + %pO /foo/bar@0 - Node full name
>> + %pO0/foo/bar@0 - Same as above
>> + %pO1/foo/bar@0[10] - Node full name + phandle
>> + %pO2/foo/bar@0[10:DdPB] - Node full name + phandle + node
>> flags
>> +D - dynamic
>> +d - detached
>> +P - Populated
>> +B - Populated bus
>
> We should think about what else we want to print for a node. Perhaps
> 'On' for name, 'Oc' for compatible, etc.
I was just going to say "The least verbose variant is name, not full_name".
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] of: Custom printk format specifier for device node
Hi Rob,
> On Jan 20, 2015, at 16:47 , Rob Herring wrote:
>
> On Tue, Jan 20, 2015 at 8:34 AM, Pantelis Antoniou
> wrote:
>> 90% of the usage of device node's full_name is printing it out
>> in a kernel message. Preparing for the eventual delayed allocation
>> introduce a custom printk format specifier that is both more
>> compact and more pleasant to the eye.
>
> What a great idea. ;)
>
It was a stroke of genius I tell you… :)
>> For instance typical use is:
>>pr_info("Frobbing node %s\n", node->full_name);
>>
>> Which can be written now as:
>>pr_info("Frobbing node %pO\n", node);
>>
>> A verbose format specifier (1-2) can be used to print extra
>> information about the node like its phandle and node flags.
>>
>> Signed-off-by: Pantelis Antoniou
>> ---
>> Documentation/printk-formats.txt | 18 +++
>> lib/vsprintf.c | 67
>>
>> 2 files changed, 85 insertions(+)
>>
>> diff --git a/Documentation/printk-formats.txt
>> b/Documentation/printk-formats.txt
>> index 5a615c1..6ad199f 100644
>> --- a/Documentation/printk-formats.txt
>> +++ b/Documentation/printk-formats.txt
>> @@ -231,6 +231,24 @@ struct va_format:
>>Do not use this feature without some mechanism to verify the
>>correctness of the format string and va_list arguments.
>>
>> +Device tree nodes:
>> +
>> + %pO{,1,2}
>
> 'O' is not very obvious, but I imagine we are somewhat limted in our
> choice here?
>
All the good women are married, all the handsome men are gay, all the obvious
format specifiers are taken.
>> +
>> + For printing device tree nodes. The (optional) number is the
>> verbosity
>> + level.
>> +
>> + Examples:
>> +
>> + %pO /foo/bar@0 - Node full name
>> + %pO0/foo/bar@0 - Same as above
>> + %pO1/foo/bar@0[10] - Node full name + phandle
>> + %pO2/foo/bar@0[10:DdPB] - Node full name + phandle + node
>> flags
>> +D - dynamic
>> +d - detached
>> +P - Populated
>> +B - Populated bus
>
> We should think about what else we want to print for a node. Perhaps
> 'On' for name, 'Oc' for compatible, etc.
>
The verbosity argument is something simple that works, but I take requests...
>> +
>> u64 SHOULD be printed with %llu/%llx:
>>
>>printk("%llu", u64_var);
>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>> index ec337f6..72896cc 100644
>> --- a/lib/vsprintf.c
>> +++ b/lib/vsprintf.c
>> @@ -29,6 +29,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> #include /* for PAGE_SIZE */
>> #include /* for dereference_function_descriptor() */
>> @@ -1240,6 +1241,68 @@ char *address_val(char *buf, char *end, const void
>> *addr,
>>return number(buf, end, num, spec);
>> }
>>
>> +#if IS_ENABLED(CONFIG_OF)
>
> You can move this into the function:
>
> if (!IS_ENABLED(CONFIG_OF)
> return NULL;
>
> Looks like you'll need the patch for empty of_node_check_flag as well.
>
Yes, I tried to avoid that.
>> +static noinline_for_stack
>> +char *device_node_string(char *buf, char *end, struct device_node *dn,
>> +struct printf_spec spec, const char *fmt)
>> +{
>> + char tbuf[sizeof("[xx:]") + 1];
>> + const char *full_name;
>> + int verbosity, len, flen, i;
>> +
>> + if ((unsigned long)dn < PAGE_SIZE)
>> + return string(buf, end, "(null)", spec);
>> +
>> + full_name = of_node_full_name(dn);
>> + verbosity = 0;
>> + if (fmt[1] >= '0' && fmt[1] <= '2')
>> + verbosity = fmt[1] - '0';
>> +
>> + /* fast and simple case */
>> + switch (verbosity) {
>> + default:
>> + case 0:
>> + tbuf[0] = '\0';
>> + break;
>> + case 1:
>> + snprintf(tbuf, sizeof(tbuf), "[%u]",
>> + (u32)dn->phandle);
>> + break;
>> + case 2:
>> + snprintf(tbuf, sizeof(tbuf), "[%u:%c%c%c%c]",
>> + (u32)dn->phandle,
>> + of_node_check_flag(dn, OF_DYNAMIC) ? 'D' : '-',
>> + of_node_check_flag(dn, OF_DETACHED) ? 'd' : '-',
>> + of_node_check_flag(dn, OF_POPULATED) ? 'P' : '-',
>> + of_node_check_flag(dn, OF_POPULATED_BUS) ? 'B' :
>> '-');
>> + break;
>> + }
>> + flen = strlen(full_name);
>> + len = flen + strlen(tbuf);
>> + if (spec.precision > 0 && len > spec.precision)
>> + len = spec.precision;
>> +
>> + if (!(spec.flags & LEFT)) {
>> + while (len < spec.field_width--) {
>> + if (buf < end)
>> + *buf = ' ';
>> +
Re: [PATCH] of: Custom printk format specifier for device node
On Tue, Jan 20, 2015 at 8:34 AM, Pantelis Antoniou
wrote:
> 90% of the usage of device node's full_name is printing it out
> in a kernel message. Preparing for the eventual delayed allocation
> introduce a custom printk format specifier that is both more
> compact and more pleasant to the eye.
What a great idea. ;)
> For instance typical use is:
> pr_info("Frobbing node %s\n", node->full_name);
>
> Which can be written now as:
> pr_info("Frobbing node %pO\n", node);
>
> A verbose format specifier (1-2) can be used to print extra
> information about the node like its phandle and node flags.
>
> Signed-off-by: Pantelis Antoniou
> ---
> Documentation/printk-formats.txt | 18 +++
> lib/vsprintf.c | 67
>
> 2 files changed, 85 insertions(+)
>
> diff --git a/Documentation/printk-formats.txt
> b/Documentation/printk-formats.txt
> index 5a615c1..6ad199f 100644
> --- a/Documentation/printk-formats.txt
> +++ b/Documentation/printk-formats.txt
> @@ -231,6 +231,24 @@ struct va_format:
> Do not use this feature without some mechanism to verify the
> correctness of the format string and va_list arguments.
>
> +Device tree nodes:
> +
> + %pO{,1,2}
'O' is not very obvious, but I imagine we are somewhat limted in our
choice here?
> +
> + For printing device tree nodes. The (optional) number is the verbosity
> + level.
> +
> + Examples:
> +
> + %pO /foo/bar@0 - Node full name
> + %pO0/foo/bar@0 - Same as above
> + %pO1/foo/bar@0[10] - Node full name + phandle
> + %pO2/foo/bar@0[10:DdPB] - Node full name + phandle + node
> flags
> +D - dynamic
> +d - detached
> +P - Populated
> +B - Populated bus
We should think about what else we want to print for a node. Perhaps
'On' for name, 'Oc' for compatible, etc.
> +
> u64 SHOULD be printed with %llu/%llx:
>
> printk("%llu", u64_var);
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index ec337f6..72896cc 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -29,6 +29,7 @@
> #include
> #include
> #include
> +#include
>
> #include /* for PAGE_SIZE */
> #include /* for dereference_function_descriptor() */
> @@ -1240,6 +1241,68 @@ char *address_val(char *buf, char *end, const void
> *addr,
> return number(buf, end, num, spec);
> }
>
> +#if IS_ENABLED(CONFIG_OF)
You can move this into the function:
if (!IS_ENABLED(CONFIG_OF)
return NULL;
Looks like you'll need the patch for empty of_node_check_flag as well.
> +static noinline_for_stack
> +char *device_node_string(char *buf, char *end, struct device_node *dn,
> +struct printf_spec spec, const char *fmt)
> +{
> + char tbuf[sizeof("[xx:]") + 1];
> + const char *full_name;
> + int verbosity, len, flen, i;
> +
> + if ((unsigned long)dn < PAGE_SIZE)
> + return string(buf, end, "(null)", spec);
> +
> + full_name = of_node_full_name(dn);
> + verbosity = 0;
> + if (fmt[1] >= '0' && fmt[1] <= '2')
> + verbosity = fmt[1] - '0';
> +
> + /* fast and simple case */
> + switch (verbosity) {
> + default:
> + case 0:
> + tbuf[0] = '\0';
> + break;
> + case 1:
> + snprintf(tbuf, sizeof(tbuf), "[%u]",
> + (u32)dn->phandle);
> + break;
> + case 2:
> + snprintf(tbuf, sizeof(tbuf), "[%u:%c%c%c%c]",
> + (u32)dn->phandle,
> + of_node_check_flag(dn, OF_DYNAMIC) ? 'D' : '-',
> + of_node_check_flag(dn, OF_DETACHED) ? 'd' : '-',
> + of_node_check_flag(dn, OF_POPULATED) ? 'P' : '-',
> + of_node_check_flag(dn, OF_POPULATED_BUS) ? 'B' : '-');
> + break;
> + }
> + flen = strlen(full_name);
> + len = flen + strlen(tbuf);
> + if (spec.precision > 0 && len > spec.precision)
> + len = spec.precision;
> +
> + if (!(spec.flags & LEFT)) {
> + while (len < spec.field_width--) {
> + if (buf < end)
> + *buf = ' ';
> + ++buf;
> + }
> + }
> + for (i = 0; i < len; ++i) {
> + if (buf < end)
> + *buf = i < flen ? full_name[i] : tbuf[i - flen];
> + ++buf;
> + }
> + while (len < spec.field_width--) {
> + if (buf < end)
> + *buf = ' ';
> + ++buf;
> + }
> + return buf;
> +}
> +#endif
> +
> int kptr_restrict __read_mostly;
>
> /*
> @@ -145
[PATCH] of: Custom printk format specifier for device node
90% of the usage of device node's full_name is printing it out
in a kernel message. Preparing for the eventual delayed allocation
introduce a custom printk format specifier that is both more
compact and more pleasant to the eye.
For instance typical use is:
pr_info("Frobbing node %s\n", node->full_name);
Which can be written now as:
pr_info("Frobbing node %pO\n", node);
A verbose format specifier (1-2) can be used to print extra
information about the node like its phandle and node flags.
Signed-off-by: Pantelis Antoniou
---
Documentation/printk-formats.txt | 18 +++
lib/vsprintf.c | 67
2 files changed, 85 insertions(+)
diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 5a615c1..6ad199f 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -231,6 +231,24 @@ struct va_format:
Do not use this feature without some mechanism to verify the
correctness of the format string and va_list arguments.
+Device tree nodes:
+
+ %pO{,1,2}
+
+ For printing device tree nodes. The (optional) number is the verbosity
+ level.
+
+ Examples:
+
+ %pO /foo/bar@0 - Node full name
+ %pO0/foo/bar@0 - Same as above
+ %pO1/foo/bar@0[10] - Node full name + phandle
+ %pO2/foo/bar@0[10:DdPB] - Node full name + phandle + node flags
+D - dynamic
+d - detached
+P - Populated
+B - Populated bus
+
u64 SHOULD be printed with %llu/%llx:
printk("%llu", u64_var);
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index ec337f6..72896cc 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -29,6 +29,7 @@
#include
#include
#include
+#include
#include /* for PAGE_SIZE */
#include /* for dereference_function_descriptor() */
@@ -1240,6 +1241,68 @@ char *address_val(char *buf, char *end, const void *addr,
return number(buf, end, num, spec);
}
+#if IS_ENABLED(CONFIG_OF)
+static noinline_for_stack
+char *device_node_string(char *buf, char *end, struct device_node *dn,
+struct printf_spec spec, const char *fmt)
+{
+ char tbuf[sizeof("[xx:]") + 1];
+ const char *full_name;
+ int verbosity, len, flen, i;
+
+ if ((unsigned long)dn < PAGE_SIZE)
+ return string(buf, end, "(null)", spec);
+
+ full_name = of_node_full_name(dn);
+ verbosity = 0;
+ if (fmt[1] >= '0' && fmt[1] <= '2')
+ verbosity = fmt[1] - '0';
+
+ /* fast and simple case */
+ switch (verbosity) {
+ default:
+ case 0:
+ tbuf[0] = '\0';
+ break;
+ case 1:
+ snprintf(tbuf, sizeof(tbuf), "[%u]",
+ (u32)dn->phandle);
+ break;
+ case 2:
+ snprintf(tbuf, sizeof(tbuf), "[%u:%c%c%c%c]",
+ (u32)dn->phandle,
+ of_node_check_flag(dn, OF_DYNAMIC) ? 'D' : '-',
+ of_node_check_flag(dn, OF_DETACHED) ? 'd' : '-',
+ of_node_check_flag(dn, OF_POPULATED) ? 'P' : '-',
+ of_node_check_flag(dn, OF_POPULATED_BUS) ? 'B' : '-');
+ break;
+ }
+ flen = strlen(full_name);
+ len = flen + strlen(tbuf);
+ if (spec.precision > 0 && len > spec.precision)
+ len = spec.precision;
+
+ if (!(spec.flags & LEFT)) {
+ while (len < spec.field_width--) {
+ if (buf < end)
+ *buf = ' ';
+ ++buf;
+ }
+ }
+ for (i = 0; i < len; ++i) {
+ if (buf < end)
+ *buf = i < flen ? full_name[i] : tbuf[i - flen];
+ ++buf;
+ }
+ while (len < spec.field_width--) {
+ if (buf < end)
+ *buf = ' ';
+ ++buf;
+ }
+ return buf;
+}
+#endif
+
int kptr_restrict __read_mostly;
/*
@@ -1459,6 +1522,10 @@ char *pointer(const char *fmt, char *buf, char *end,
void *ptr,
return dentry_name(buf, end,
((const struct file *)ptr)->f_path.dentry,
spec, fmt);
+#if IS_ENABLED(CONFIG_OF)
+ case 'O':
+ return device_node_string(buf, end, ptr, spec, fmt);
+#endif
}
spec.flags |= SMALL;
if (spec.field_width == -1) {
--
1.7.12
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://w
