From mboxrd@z Thu Jan 1 00:00:00 1970 From: Joe Perches Subject: Re: [PATCH 3/4] of: Custom printk format specifier for device node Date: Wed, 14 Jun 2017 13:56:48 -0700 Message-ID: <1497473808.18751.70.camel@perches.com> References: <20170614203025.7581-1-robh@kernel.org> <20170614203025.7581-4-robh@kernel.org> Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 8bit Return-path: In-Reply-To: <20170614203025.7581-4-robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Rob Herring , Frank Rowand , Mark Rutland Cc: Pantelis Antoniou , devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: devicetree@vger.kernel.org On Wed, 2017-06-14 at 15:30 -0500, Rob Herring wrote: > From: Pantelis Antoniou I think the commit subject is wrong. It adds an "of" specific bit to vsprintf.c. The subject should be 'vsprintf:  Add %p extension "%pO" for device tree' > 90% of the usage of device node's full_name is printing it out > in a kernel message. Preparing for the eventual delayed allocation > introduce a custom printk format specifier that is both more > compact and more pleasant to the eye. > > For instance typical use is: > pr_info("Frobbing node %s\n", node->full_name); > > Which can be written now as: > pr_info("Frobbing node %pOF\n", node); Somehow I think this example is poor as node->full_name is a pretty obvious to read use. %pOF requires you to look up or know what the output is going to be. > More fine-grained control of formatting includes printing the name, > flag, path-spec name, reference count and others, explained in the > documentation entry. > > Originally written by Pantelis, but pretty much rewrote the core > function using existing string/number functions. The 2 passes were > unnecessary and have been removed. Also, updated the checkpatch.pl > check. Some comments about the code. > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > [] > @@ -1470,6 +1471,123 @@ char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt) > return format_flags(buf, end, flags, names); > } > > +static noinline_for_stack > +char *device_node_gen_full_name(const struct device_node *np, char *buf, char *end) > +{ > + int len, ret; > + > + if (!np || !np->parent) > + return buf; > + > + buf = device_node_gen_full_name(np->parent, buf, end); This is recursive. How many levels of parents could there be? Perhaps there should be a recursion limit. > + > + if (buf < end) > + len = end - buf; > + else > + len = 0; > + ret = snprintf(buf, len, "/%s", kbasename(np->full_name)); > + if (ret <= 0) > + return buf; > + else if (len == 0 || ret < len) > + return buf + ret; > + return buf + len; > +} Does this work with %pOF for a right justified or padded length string? Perhaps widen_string should be added. > +static noinline_for_stack > +char *device_node_string(char *buf, char *end, struct device_node *dn, > + struct printf_spec spec, const char *fmt) > +{ > + char tbuf[sizeof("xxxxxxxxxx") + 1]; > + const char *fmtp, *p; > + int ret; > + char *buf_start = buf; > + struct property *prop; > + bool has_mult, pass; > + const struct printf_spec num_spec = { > + .flags = SMALL, > + .field_width = -1, > + .precision = -1, > + .base = 10, > + }; > + > + struct printf_spec str_spec = spec; > + str_spec.field_width = -1; > + > + if (!IS_ENABLED(CONFIG_OF)) > + return string(buf, end, "(!OF)", spec); > + > + if ((unsigned long)dn < PAGE_SIZE) > + return string(buf, end, "(null)", spec); > + > + /* simple case without anything any more format specifiers */ > + if (fmt[1] == '\0' || strcspn(fmt + 1,"fnpPFcCr") > 0) > + fmt = "Ff"; > + > + for (fmtp = fmt + 1, pass = false; strspn(fmtp,"fnpPFcCr"); fmtp++, pass = true) { why not while (isalpha(*++fmt)) like ip6 or isalnum like FORMAT_TYPE_PTR uses? > + if (pass && (*fmtp != 'f')) { > + if (buf < end) > + *buf = '|'; > + buf++; > + } > + > + switch (*fmtp) { > + case 'f': /* full_name */ > + if (pass) { > + if (buf < end) > + *buf = ':'; > + buf++; > + } > + buf = device_node_gen_full_name(dn, buf, end); > + break; > + case 'n': /* name */ > + buf = string(buf, end, dn->name, str_spec); > + break; > + case 'p': /* phandle */ > + buf = number(buf, end, (unsigned int)dn->phandle, num_spec); > + break; > + case 'P': /* path-spec */ > + buf = string(buf, end, kbasename(of_node_full_name(dn)), str_spec); > + break; > + case 'F': /* flags */ > + snprintf(tbuf, sizeof(tbuf), "%c%c%c%c", > + of_node_check_flag(dn, OF_DYNAMIC) ? > + 'D' : '-', > + of_node_check_flag(dn, OF_DETACHED) ? > + 'd' : '-', > + of_node_check_flag(dn, OF_POPULATED) ? > + 'P' : '-', > + of_node_check_flag(dn, > + OF_POPULATED_BUS) ? 'B' : '-'); I'd try to avoid all uses of snprintf as it's effectively another fairly large stack frame. It's probably better to avoid more recursion stack depth use and just use *buf++ as appropriate. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752429AbdFNU4x (ORCPT ); Wed, 14 Jun 2017 16:56:53 -0400 Received: from smtprelay0189.hostedemail.com ([216.40.44.189]:58866 "EHLO smtprelay.hostedemail.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751754AbdFNU4w (ORCPT ); Wed, 14 Jun 2017 16:56:52 -0400 X-Session-Marker: 6A6F6540706572636865732E636F6D X-Spam-Summary: 2,0,0,,d41d8cd98f00b204,joe@perches.com,:::::::::::,RULES_HIT:41:355:379:541:599:800:960:968:973:988:989:1260:1277:1311:1313:1314:1345:1359:1373:1394:1437:1515:1516:1518:1535:1544:1593:1594:1605:1711:1730:1747:1777:1792:2393:2559:2562:2828:2895:3138:3139:3140:3141:3142:3608:3622:3865:3866:3867:3868:3870:3871:3872:3873:3874:4321:5007:6117:6119:7576:7903:7974:8957:10004:10848:11026:11232:11473:11658:11783:11914:12043:12296:12438:12740:12895:13095:13161:13229:13439:13894:14096:14097:14181:14659:14721:21080:21324:21433:21451:21627:30012:30029:30030:30034:30036:30054:30070:30091,0,RBL:none,CacheIP:none,Bayesian:0.5,0.5,0.5,Netcheck:none,DomainCache:0,MSF:not bulk,SPF:,MSBL:0,DNSBL:none,Custom_rules:0:0:0,LFtime:2,LUA_SUMMARY:none X-HE-Tag: wash60_75cce3202bc1a X-Filterd-Recvd-Size: 5395 Message-ID: <1497473808.18751.70.camel@perches.com> Subject: Re: [PATCH 3/4] of: Custom printk format specifier for device node From: Joe Perches To: Rob Herring , Frank Rowand , Mark Rutland Cc: Pantelis Antoniou , devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Date: Wed, 14 Jun 2017 13:56:48 -0700 In-Reply-To: <20170614203025.7581-4-robh@kernel.org> References: <20170614203025.7581-1-robh@kernel.org> <20170614203025.7581-4-robh@kernel.org> Content-Type: text/plain; charset="ISO-8859-1" X-Mailer: Evolution 3.22.6-1ubuntu1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 2017-06-14 at 15:30 -0500, Rob Herring wrote: > From: Pantelis Antoniou I think the commit subject is wrong. It adds an "of" specific bit to vsprintf.c. The subject should be 'vsprintf:  Add %p extension "%pO" for device tree' > 90% of the usage of device node's full_name is printing it out > in a kernel message. Preparing for the eventual delayed allocation > introduce a custom printk format specifier that is both more > compact and more pleasant to the eye. > > For instance typical use is: > pr_info("Frobbing node %s\n", node->full_name); > > Which can be written now as: > pr_info("Frobbing node %pOF\n", node); Somehow I think this example is poor as node->full_name is a pretty obvious to read use. %pOF requires you to look up or know what the output is going to be. > More fine-grained control of formatting includes printing the name, > flag, path-spec name, reference count and others, explained in the > documentation entry. > > Originally written by Pantelis, but pretty much rewrote the core > function using existing string/number functions. The 2 passes were > unnecessary and have been removed. Also, updated the checkpatch.pl > check. Some comments about the code. > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > [] > @@ -1470,6 +1471,123 @@ char *flags_string(char *buf, char *end, void *flags_ptr, const char *fmt) > return format_flags(buf, end, flags, names); > } > > +static noinline_for_stack > +char *device_node_gen_full_name(const struct device_node *np, char *buf, char *end) > +{ > + int len, ret; > + > + if (!np || !np->parent) > + return buf; > + > + buf = device_node_gen_full_name(np->parent, buf, end); This is recursive. How many levels of parents could there be? Perhaps there should be a recursion limit. > + > + if (buf < end) > + len = end - buf; > + else > + len = 0; > + ret = snprintf(buf, len, "/%s", kbasename(np->full_name)); > + if (ret <= 0) > + return buf; > + else if (len == 0 || ret < len) > + return buf + ret; > + return buf + len; > +} Does this work with %pOF for a right justified or padded length string? Perhaps widen_string should be added. > +static noinline_for_stack > +char *device_node_string(char *buf, char *end, struct device_node *dn, > + struct printf_spec spec, const char *fmt) > +{ > + char tbuf[sizeof("xxxxxxxxxx") + 1]; > + const char *fmtp, *p; > + int ret; > + char *buf_start = buf; > + struct property *prop; > + bool has_mult, pass; > + const struct printf_spec num_spec = { > + .flags = SMALL, > + .field_width = -1, > + .precision = -1, > + .base = 10, > + }; > + > + struct printf_spec str_spec = spec; > + str_spec.field_width = -1; > + > + if (!IS_ENABLED(CONFIG_OF)) > + return string(buf, end, "(!OF)", spec); > + > + if ((unsigned long)dn < PAGE_SIZE) > + return string(buf, end, "(null)", spec); > + > + /* simple case without anything any more format specifiers */ > + if (fmt[1] == '\0' || strcspn(fmt + 1,"fnpPFcCr") > 0) > + fmt = "Ff"; > + > + for (fmtp = fmt + 1, pass = false; strspn(fmtp,"fnpPFcCr"); fmtp++, pass = true) { why not while (isalpha(*++fmt)) like ip6 or isalnum like FORMAT_TYPE_PTR uses? > + if (pass && (*fmtp != 'f')) { > + if (buf < end) > + *buf = '|'; > + buf++; > + } > + > + switch (*fmtp) { > + case 'f': /* full_name */ > + if (pass) { > + if (buf < end) > + *buf = ':'; > + buf++; > + } > + buf = device_node_gen_full_name(dn, buf, end); > + break; > + case 'n': /* name */ > + buf = string(buf, end, dn->name, str_spec); > + break; > + case 'p': /* phandle */ > + buf = number(buf, end, (unsigned int)dn->phandle, num_spec); > + break; > + case 'P': /* path-spec */ > + buf = string(buf, end, kbasename(of_node_full_name(dn)), str_spec); > + break; > + case 'F': /* flags */ > + snprintf(tbuf, sizeof(tbuf), "%c%c%c%c", > + of_node_check_flag(dn, OF_DYNAMIC) ? > + 'D' : '-', > + of_node_check_flag(dn, OF_DETACHED) ? > + 'd' : '-', > + of_node_check_flag(dn, OF_POPULATED) ? > + 'P' : '-', > + of_node_check_flag(dn, > + OF_POPULATED_BUS) ? 'B' : '-'); I'd try to avoid all uses of snprintf as it's effectively another fairly large stack frame. It's probably better to avoid more recursion stack depth use and just use *buf++ as appropriate.