Hi Grant,
> On Mar 30, 2015, at 22:04 , Grant Likely <grant.likely@xxxxxxxxxxxx> wrote:
>
> On Thu, 22 Jan 2015 22:31:46 +0200
> , Pantelis Antoniou <panto@xxxxxxxxxxxxxxxxxxxxxxx>
> wrote:
>> Hi Joe,
>>
>>> On Jan 21, 2015, at 19:37 , Joe Perches <joe@xxxxxxxxxxx> wrote:
>>>
>>> On Wed, 2015-01-21 at 19:06 +0200, Pantelis Antoniou wrote:
>>>> 90% of the usage of device node's full_name is printing it out
>>>> in a kernel message. Preparing for the eventual delayed allocation
>>>> introduce a custom printk format specifier that is both more
>>>> compact and more pleasant to the eye.
>>>>
>>>> For instance typical use is:
>>>> pr_info("Frobbing node %s\n", node->full_name);
>>>>
>>>> Which can be written now as:
>>>> pr_info("Frobbing node %pO\n", node);
>>
>>> Still disliking use of %p0.
>>>
>>
>> pO - Open Firmware
>>
>> pT for tree is bad, cause we plan to use a tree type in the future in OF.
>
> So, here's a radical thought. How about we reserve '%pO' for objects, as
> in kobjects. We'll use extra flags to narrow down specifically to
> device tree nodes, but we could teach vsprintf() to treat a plain '%pO'
> as plain kobject pointer, and if it is able to recognize the kobj_type,
> then run a specific decoder to format it.
>
> This also gives us a namespace for various kinds of firmware data
> output. %Od could be a struct device, %On for device tree node, %Oa for
> an ACPI node, etc.
>
I’m fine with this. I also have a patch to turn an overlay to a kobj
so this fits naturally.
OTOH if we do this, I would expect to rework the custom printk infrastructure
to be more generic.
IMHO having the format specifier and the format print methods in lib/vsprintf.c
is not very nice.
How about having a way to register object printk handlers with something like that?
We could put that in a special linker section and have the printk method pass control
there.
PRINTK_OBJFMT(’n’, printk_objfmt_device_node);
We might have to make a few printk methods public however.
>>
>>>> More fine-grained control of formatting includes printing the name,
>>>> flag, path-spec name, reference count and others, explained in the
>>>> documentation entry.
>>>>
>>>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@xxxxxxxxxxxx>
>>>>
>>>> dt-print
>>>> ---
>>>> Documentation/printk-formats.txt | 29 ++++++++
>>>> lib/vsprintf.c | 151 +++++++++++++++++++++++++++++++++++++++
>>>> 2 files changed, 180 insertions(+)
>>>>
>>>> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
>>>> index 5a615c1..2d42c04 100644
>>>> --- a/Documentation/printk-formats.txt
>>>> +++ b/Documentation/printk-formats.txt
>>>> @@ -231,6 +231,35 @@ struct va_format:
>>>> Do not use this feature without some mechanism to verify the
>>>> correctness of the format string and va_list arguments.
>>>>
>>>> +Device tree nodes:
>>>> +
>>>> + %pO[fnpPcCFr]
>>>> +
>>>> + For printing device tree nodes. The optional arguments are:
>>>> + f device node full_name
>>>> + n device node name
>>>> + p device node phandle
>>>> + P device node path spec (name + @unit)
>>>> + F device node flags
>>>> + c major compatible string
>>>> + C full compatible string
>>>> + r node reference count
>>>> + Without any arguments prints full_name (same as %pOf)
>>>> + The separator when using multiple arguments is '|'
>>>> +
>>>> + Examples:
>>>> +
>>>> + %pO /foo/bar@0 - Node full name
>>>> + %pOf /foo/bar@0 - Same as above
>>>> + %pOfp /foo/bar@0|10 - Node full name + phandle
>>>> + %pOfcF /foo/bar@0|foo,device|--P- - Node full name +
>>>> + major compatible string +
>>>> + node flags
>>>> + D - dynamic
>>>> + d - detached
>>>> + P - Populated
>>>> + B - Populated bus
>>>> +
>>>> u64 SHOULD be printed with %llu/%llx:
>>>>
>>>> printk("%llu", u64_var);
>>>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>>> []
>>>
>>> Add #ifdef back ?
>>>
>>
>> The whole thing is optimized away when CONFIG_OF is not defined, leaving only
>> the return statement.
>>
>>>> +static noinline_for_stack
>>>> +char *device_node_string(char *buf, char *end, struct device_node *dn,
>>>> + struct printf_spec spec, const char *fmt)
>>>> +{
>>>> + char tbuf[sizeof("xxxxxxxxxx") + 1];
>>>> + const char *fmtp, *p;
>>>> + int len, ret, i, j, pass;
>>>> + char c;
>>>> +
>>>> + if (!IS_ENABLED(CONFIG_OF))
>>>> + return string(buf, end, "(!OF)", spec);
>>>
>>> Not very descriptive output, maybe the address would be better.
>>>
>>
>> OK
>>
>>>> +
>>>> + if ((unsigned long)dn < PAGE_SIZE)
>>>> + return string(buf, end, "(null)", spec);
>>>> +
>>>> + /* simple case without anything any more format specifiers */
>>>> + if (fmt[1] == '\0' || isspace(fmt[1]))
>>>> + fmt = "Of";
>
> s/isspace()/!isalnum() to match with what the pointer() function does
> when finding the end of a format string.
>
>>>
>>> why lower case here but upper case above?
>>>
>>
>> Cause '(null)' is what’s printed in string() when null is passed as a pointer.
>>
>>>> +
>>>> + len = 0;
>>>> +
>>>> + /* two passes; the first calculates length, the second fills in */
>>>> + for (pass = 1; pass <= 2; pass++) {
>>>> + if (pass == 2 && !(spec.flags & LEFT)) {
>>>> + /* padding */
>>>> + while (len < spec.field_width--) {
>>>> + if (buf < end)
>>>> + *buf = ' ';
>>>> + ++buf;
>>>> + }
>>>> + }
>>>> +#undef _HANDLE_CH
>>>> +#define _HANDLE_CH(_ch) \
>>>> + do { \
>>>> + if (pass == 1) \
>>>> + len++; \
>>>> + else \
>>>> + if (buf < end) \
>>>> + *buf++ = (_ch); \
>>>> + } while (0)
>>>> +#undef _HANDLE_STR
>>>> +#define _HANDLE_STR(_str) \
>>>> + do { \
>>>> + const char *str = (_str); \
>>>> + if (pass == 1) \
>>>> + len += strlen(str); \
>>>> + else \
>>>> + while (*str && buf < end) \
>>>> + *buf++ = *str++; \
>>>> + } while (0)
>>>
>>> This isn't pretty. Perhaps there's a better way?
>>>
>>
>> It’s the simplest way to do the different operations for the two passes, without
>> bloating the code or adding superfluous methods.
>>
>> We don’t want to allocate memory, we don’t want to use stack space. We’re probably
>> printing in atomic context too since device nodes don’t usually printed out
>> during normal operation.
>
> As far as I can tell from the code, the only thing that 2 passes is used
> for is when the LEFT spec.flags value is not set. Instead of doing two,
> what if the code generated the string right-aligned, and then if the
> LEFT flag is set, shift it over the required amount, ' ' padding as
> necessary? In fact, that's exactly what the dentry_name() helper does.
>
Hmm, that might work.
> This is a complicated enough block of code, that I'd like to see
> unittests for it added at the same time.
>
> ...
>
> So, I'm keen enough on this feature that I've just gone ahead and played
> with it this morning. The following is what I've come up with. I got rid
> of the two passes, fixed up the boundary conditions, and added
> unittests. I've also switched the format key to %pOn, and the separator
> to ':'. ':' is never a valid character in a node name, so it should be
> safe to use as a delimiter.
OK.
>
> I've dropped the refcount decoder. I know it is useful for debugging the
> core DT code, but it isn't something that will be used generally. Plus
> the returned value cannot be relied upon to be stable because there may
> be other code currently iterating over the tree.
>
Yeah, I know it’s not something to rely on. If we do %pOk to be kobj
debug I can add it back in.
> ---
>
> of: Custom printk format specifier for device node
>
> 90% of the usage of device node's full_name is printing it out
> in a kernel message. Preparing for the eventual delayed allocation
> introduce a custom printk format specifier that is both more
> compact and more pleasant to the eye.
>
> For instance typical use is:
> pr_info("Frobbing node %s\n", node->full_name);
>
> Which can be written now as:
> pr_info("Frobbing node %pOn\n", node);
>
> More fine-grained control of formatting includes printing the name,
> flag, path-spec name, reference count and others, explained in the
> documentation entry.
>
Remove reference count comment?
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@xxxxxxxxxxxx>
> [grant.likely: Rework to be simpler]
> Signed-off-by: Grant Likely <grant.likely@xxxxxxxxxx>
> ---
> Documentation/printk-formats.txt | 28 ++++++++
> drivers/of/unittest-data/tests-platform.dtsi | 4 +-
> drivers/of/unittest.c | 58 +++++++++++++++
> lib/vsprintf.c | 101 +++++++++++++++++++++++++++
> 4 files changed, 190 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
> index 5a615c14f75d..f0dc2fd229e4 100644
> --- a/Documentation/printk-formats.txt
> +++ b/Documentation/printk-formats.txt
> @@ -192,6 +192,34 @@ IPv4/IPv6 addresses (generic, with port, flowinfo, scope):
> %pISsc 1.2.3.4 or [1:2:3:4:5:6:7:8]%1234567890
> %pISpfc 1.2.3.4:12345 or [1:2:3:4:5:6:7:8]:12345/123456789
>
> +Device tree nodes:
> +
> + %pOn[fnpPcCFr]
> +
> + For printing device tree nodes. The optional arguments are:
> + f device node full_name
> + n device node name
> + p device node phandle
> + P device node path spec (name + @unit)
> + F device node flags
> + c major compatible string
> + C full compatible string
> + Without any arguments prints full_name (same as %pOnf)
> + The separator when using multiple arguments is ‘:’
^ separator is ‘.'
> +
> + Examples:
> +
> + %pOn /foo/bar@0 - Node full name
> + %pOnf /foo/bar@0 - Same as above
> + %pOnfp /foo/bar@0:10 - Node full name + phandle
> + %pOnfcF /foo/bar@0:foo,device:--P- - Node full name +
> + major compatible string +
> + node flags
> + D - dynamic
> + d - detached
> + P - Populated
> + B - Populated bus
> +
Same here.
> UUID/GUID addresses:
>
> %pUb 00010203-0405-0607-0809-0a0b0c0d0e0f
> diff --git a/drivers/of/unittest-data/tests-platform.dtsi b/drivers/of/unittest-data/tests-platform.dtsi
> index eb20eeb2b062..a0c93822aee3 100644
> --- a/drivers/of/unittest-data/tests-platform.dtsi
> +++ b/drivers/of/unittest-data/tests-platform.dtsi
> @@ -26,7 +26,9 @@
> #size-cells = <0>;
>
> dev@100 {
> - compatible = "test-sub-device";
> + compatible = "test-sub-device",
> + "test-compat2",
> + "test-compat3";
> reg = <0x100>;
> };
> };
> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> index e844907c9efa..dc43209f2064 100644
> --- a/drivers/of/unittest.c
> +++ b/drivers/of/unittest.c
> @@ -234,6 +234,63 @@ static void __init of_unittest_check_tree_linkage(void)
> pr_debug("allnodes list size (%i); sibling lists size (%i)\n", allnode_count, child_count);
> }
>
> +static void __init of_unittest_printf_one(struct device_node *np, const char *fmt,
> + const char *expected)
> +{
> + char buf[strlen(expected)+10];
> + int size, i;
> +
> + /* Baseline; check conversion with a large size limit */
> + memset(buf, 0xff, sizeof(buf));
> + size = snprintf(buf, sizeof(buf) - 2, fmt, np);
> +
> + /* use strcmp() instead of strncmp() here to be absolutely sure strings match */
> + unittest((strcmp(buf, expected) == 0) && (buf[size+1] == 0xff),
> + "sprintf failed; fmt='%s' expected='%s' rslt='%s'\n",
> + fmt, expected, buf);
> +
> + /* Make sure length limits work */
> + size++;
> + for (i = 0; i < 2; i++, size--) {
> + /* Clear the buffer, and make sure it works correctly still */
> + memset(buf, 0xff, sizeof(buf));
> + snprintf(buf, size+1, fmt, np);
> + unittest(strncmp(buf, expected, size) == 0 && (buf[size+1] == 0xff),
> + "snprintf failed; size=%i fmt='%s' expected='%s' rslt='%s'\n",
> + size, fmt, expected, buf);
> + }
> +}
> +
> +static void __init of_unittest_printf(void)
> +{
> + struct device_node *np;
> + const char *full_name = "/testcase-data/platform-tests/test-device@1/dev@100";
> + char phandle_str[16] = "";
> +
> + np = of_find_node_by_path(full_name);
> + if (!np) {
> + unittest(np, "testcase data missing\n");
> + return;
> + }
> +
> + num_to_str(phandle_str, sizeof(phandle_str), np->phandle);
> +
> + of_unittest_printf_one(np, "%pOn", full_name);
> + of_unittest_printf_one(np, "%pOnf", full_name);
> + of_unittest_printf_one(np, "%pOnp", phandle_str);
> + of_unittest_printf_one(np, "%pOnP", "dev@100");
> + of_unittest_printf_one(np, "ABC %pOnP ABC", "ABC dev@100 ABC");
> + of_unittest_printf_one(np, "%10pOnP", " dev@100");
> + of_unittest_printf_one(np, "%-10pOnP", "dev@100 ");
> + of_unittest_printf_one(of_root, "%pOnP", "/");
> + of_unittest_printf_one(np, "%pOnF", "----");
> + of_unittest_printf_one(np, "%pOnPF", "dev@100:----");
> + of_unittest_printf_one(np, "%pOnPFPc", "dev@100:----:dev@100:test-sub-device");
> + of_unittest_printf_one(np, "%pOnc", "test-sub-device");
> + of_unittest_printf_one(np, "%pOnC",
> + "\"test-sub-device\",\"test-compat2\",\"test-compat3\"");
> +}
> +
> struct node_hash {
> struct hlist_node node;
> struct device_node *np;
> @@ -1888,6 +1945,7 @@ static int __init of_unittest(void)
> of_unittest_find_node_by_name();
> of_unittest_dynamic();
> of_unittest_parse_phandle_with_args();
> + of_unittest_printf();
> of_unittest_property_string();
> of_unittest_property_copy();
> of_unittest_changeset();
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index b235c96167d3..8a793a4560c2 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -29,6 +29,7 @@
> #include <linux/dcache.h>
> #include <linux/cred.h>
> #include <net/addrconf.h>
> +#include <linux/of.h>
>
> #include <asm/page.h> /* for PAGE_SIZE */
> #include <asm/sections.h> /* for dereference_function_descriptor() */
> @@ -1322,6 +1323,91 @@ char *address_val(char *buf, char *end, const void *addr,
> return number(buf, end, num, spec);
> }
>
> +static noinline_for_stack
> +char *device_node_string(char *buf, char *end, struct device_node *dn,
> + struct printf_spec spec, const char *fmt)
> +{
> + char tbuf[] = "----";
> + struct property *prop;
> + const char *p;
> + int ret, i, j, n;
> + char c, *start = buf;
> + struct printf_spec str_spec =
> + { .precision = -1, .field_width = 0, .base = 10, .flags = LEFT };
> +
> + if (!IS_ENABLED(CONFIG_OF))
> + return string(buf, end, "(!OF)", spec);
> +
> + if ((unsigned long)dn < PAGE_SIZE)
> + return string(buf, end, "(null)", spec);
> +
> + /* simple case without anything any more format specifiers */
> + if (!isalnum(fmt[2]))
> + fmt = "Onf";
> +
> + fmt += 2;
> + for (j = 0; isalnum((c = *fmt)); j++, fmt++) {
> + if (j)
> + buf = string(buf, end, ":", str_spec);
> +
^ separator is ‘.’ now?
> + switch (c) {
> + case 'f': /* full_name */
> + buf = string(buf, end, of_node_full_name(dn), str_spec);
> + break;
> + case 'n': /* name */
> + buf = string(buf, end, dn->name, str_spec);
> + break;
> + case 'p': /* phandle */
> + buf = number(buf, end, dn->phandle, str_spec);
> + break;
> + case 'P': /* path-spec */
> + p = strrchr(of_node_full_name(dn), '/');
> + if (p[1]) /* Root node name is just "/" */
> + p++;
> + buf = string(buf, end, p, str_spec);
> + break;
> + case 'F': /* flags */
> + snprintf(tbuf, sizeof(tbuf), "%c%c%c%c",
> + of_node_check_flag(dn, OF_DYNAMIC) ? 'D' : '-',
> + of_node_check_flag(dn, OF_DETACHED) ? 'd' : '-',
> + of_node_check_flag(dn, OF_POPULATED) ? 'P' : '-',
> + of_node_check_flag(dn, OF_POPULATED_BUS) ? 'B' : '-');
> + buf = string(buf, end, tbuf, str_spec);
> + break;
> + case 'c': /* first compatible string */
> + ret = of_property_read_string(dn, "compatible", &p);
> + if (ret == 0)
> + buf = string(buf, end, p, str_spec);
> + break;
> + case 'C': /* full compatible string list */
> + i = 0;
> + of_property_for_each_string(dn, "compatible", prop, p) {
> + buf = string(buf, end, i ? "\",\"" : "\"", str_spec);
> + buf = string(buf, end, p, str_spec);
> + i++;
> + }
> + buf = string(buf, end, "\"", str_spec);
> + break;
> + }
> + }
> +
> + /* Pad out at the front or back if field_width is specified */
> + n = buf - start;
> + if (n < spec.field_width) {
> + unsigned spaces = spec.field_width - n;
> + if (!(spec.flags & LEFT)) {
> + widen(start, end, n, spaces);
> + return buf + spaces;
> + }
> + while (spaces-- && (buf < end)) {
> + if (buf < end)
> + *buf = ' ';
> + ++buf;
> + }
> + }
> + return buf;
> +}
> +
> int kptr_restrict __read_mostly;
>
> /*
> @@ -1393,6 +1479,15 @@ int kptr_restrict __read_mostly;
> * correctness of the format string and va_list arguments.
> * - 'K' For a kernel pointer that should be hidden from unprivileged users
> * - 'NF' For a netdev_features_t
> + * - 'On[fnpPcCF]' For a device tree object
> + * Without any optional arguments prints the full_name
> + * f device node full_name
> + * n device node name
> + * p device node phandle
> + * P device node path spec (name + @unit)
> + * F device node flags
> + * c major compatible string
> + * C full compatible string
> * - 'h[CDN]' For a variable-length buffer, it prints it as a hex string with
> * a certain separator (' ' by default):
> * C colon
> @@ -1544,6 +1639,12 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
> return netdev_feature_string(buf, end, ptr, spec);
> }
> break;
> + case 'O':
> + switch (fmt[1]) {
> + case 'n':
> + return device_node_string(buf, end, ptr, spec, fmt);
> + }
> + break;
> case 'a':
> return address_val(buf, end, ptr, spec, fmt);
> case 'd':
> --
> 2.1.0
>
>
Regards
— Pantelis
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
