| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 21 Jan 2015 19:39:44 +0200
From: Pantelis Antoniou <panto@...oniou-consulting.com>
To: Joe Perches <joe@...ches.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
Grant Likely <grant.likely@...retlab.ca>,
Rob Herring <robherring2@...il.com>,
Randy Dunlap <rdunlap@...radead.org>,
linux-doc@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] of: Custom printk format specifier for device node
Hi Joe,
> On Jan 21, 2015, at 19:37 , Joe Perches <joe@...ches.com> wrote:
>
> On Wed, 2015-01-21 at 19:06 +0200, Pantelis Antoniou wrote:
>> 90% of the usage of device node's full_name is printing it out
>> in a kernel message. Preparing for the eventual delayed allocation
>> introduce a custom printk format specifier that is both more
>> compact and more pleasant to the eye.
>>
>> For instance typical use is:
>> pr_info("Frobbing node %s\n", node->full_name);
>>
>> Which can be written now as:
>> pr_info("Frobbing node %pO\n", node);
>
> Still disliking use of %p0.
>
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 <pantelis.antoniou@...sulko.com>
>>
>> dt-print
>> ---
>> Documentation/printk-formats.txt | 29 ++++++++
>> lib/vsprintf.c | 151 +++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 180 insertions(+)
>>
>> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
>> index 5a615c1..2d42c04 100644
>> --- a/Documentation/printk-formats.txt
>> +++ b/Documentation/printk-formats.txt
>> @@ -231,6 +231,35 @@ struct va_format:
>> Do not use this feature without some mechanism to verify the
>> correctness of the format string and va_list arguments.
>>
>> +Device tree nodes:
>> +
>> + %pO[fnpPcCFr]
>> +
>> + For printing device tree nodes. The optional arguments are:
>> + f device node full_name
>> + n device node name
>> + p device node phandle
>> + P device node path spec (name + @unit)
>> + F device node flags
>> + c major compatible string
>> + C full compatible string
>> + r node reference count
>> + Without any arguments prints full_name (same as %pOf)
>> + The separator when using multiple arguments is '|'
>> +
>> + Examples:
>> +
>> + %pO /foo/bar@0 - Node full name
>> + %pOf /foo/bar@0 - Same as above
>> + %pOfp /foo/bar@...0 - Node full name + phandle
>> + %pOfcF /foo/bar@...oo,device|--P- - Node full name +
>> + major compatible string +
>> + node flags
>> + D - dynamic
>> + d - detached
>> + P - Populated
>> + B - Populated bus
>> +
>> u64 SHOULD be printed with %llu/%llx:
>>
>> printk("%llu", u64_var);
>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> []
>
> Add #ifdef back ?
>
>> +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.
>
>> +
>> + 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 */
>> + if (j++ > 0)
>> + _HANDLE_CH(':');
>> + _HANDLE_STR(of_node_full_name(dn));
>> + break;
>> + case 'n': /* name */
>> + if (j++ > 0)
>> + _HANDLE_CH('|');
>> + _HANDLE_STR(dn->name);
>> + break;
>> + case 'p': /* phandle */
>> + if (j++ > 0)
>> + _HANDLE_CH('|');
>> + snprintf(tbuf, sizeof(tbuf), "%u",
>> + (unsigned int)dn->phandle);
>> + _HANDLE_STR(tbuf);
>> + break;
>> + case 'P': /* path-spec */
>> + if (j++ > 0)
>> + _HANDLE_CH('|');
>> + _HANDLE_STR(dn->name);
>> + /* need to tack on the @ postfix */
>> + p = strchr(of_node_full_name(dn), '@');
>> + if (p)
>> + _HANDLE_STR(p);
>> + break;
>> + case 'F': /* flags */
>> + if (j++ > 0)
>> + _HANDLE_CH('|');
>> + 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' : '-');
>> + _HANDLE_STR(tbuf);
>> + break;
>> + case 'c': /* major compatible string */
>> + if (j++ > 0)
>> + _HANDLE_CH('|');
>> + ret = of_property_read_string(dn, "compatible",
>> + &p);
>> + if (ret == 0)
>> + _HANDLE_STR(p);
>> + break;
>> + case 'C': /* full compatible string */
>> + if (j++ > 0)
>> + _HANDLE_CH('|');
>> + i = 0;
>> + while (of_property_read_string_index(dn,
>> + "compatible", i, &p) == 0) {
>> + if (i == 0)
>> + _HANDLE_STR("\"");
>> + else
>> + _HANDLE_STR("\",\"");
>> + _HANDLE_STR(p);
>> + i++;
>> + }
>> + if (i > 0)
>> + _HANDLE_STR("\"");
>> + break;
>> + case 'r': /* node reference count */
>> + if (j++ > 0)
>> + _HANDLE_CH('|');
>> + snprintf(tbuf, sizeof(tbuf), "%u",
>> + atomic_read(&dn->kobj.kref.refcount));
>> + _HANDLE_STR(tbuf);
>> + break;
>> + default:
>> + break;
>> + }
>> + }
>> + }
>> + /* finish up */
>> + while (buf < end && len < spec.field_width--)
>> + *buf++ = ' ';
>> +#undef _HANDLE_CH
>> +#undef _HANDLE_STR
>
Regards
— Pantelis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists