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: Thu, 15 Jun 2017 09:51:21 -0700 Message-ID: <1497545481.14396.10.camel@perches.com> References: <20170614203025.7581-1-robh@kernel.org> <20170614203025.7581-4-robh@kernel.org> <1497473808.18751.70.camel@perches.com> Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Rob Herring Cc: Frank Rowand , Mark Rutland , Pantelis Antoniou , "devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" List-Id: devicetree@vger.kernel.org On Thu, 2017-06-15 at 07:30 -0500, Rob Herring wrote: > On Wed, Jun 14, 2017 at 3:56 PM, Joe Perches wrote: > > 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' > > Okay, but it was good enough for the 2-3 versions Pantelis did before... Which were not applied. > > > 90% of the usage of device node's full_name is printing it out > > > in a kernel message. Preparing for the eventual delayed allocation The "eventual delayed allocation" bit doesn't mean anything to me. > > > 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. > > So %pOFfullname? We've beat this one to death IMO. I don't doubt the utility, just the example. Just mention that full_name is going away. > > > 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. > > 2-6 I'd say is typical. The FDT unflattening code limits things to 64 > (which is probably way more than needed). > > I could re-write it to be non-recursive, but then I'll just have the > max sized array of pointers on the stack. Which would be less stack than how many recursive calls? 5? In any case, 64 * 8 for pointers or 5+ stack frames is a fair amount of stack. Maybe too much. > > > + 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. > > Okay. > > > It's probably better to avoid more recursion stack depth use > > and just use *buf++ as appropriate. > > You can't use *buf++ as this code must work and increment buf even > when buf is NULL. tbuf then. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to 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 S1752171AbdFOQv0 (ORCPT ); Thu, 15 Jun 2017 12:51:26 -0400 Received: from smtprelay0141.hostedemail.com ([216.40.44.141]:58247 "EHLO smtprelay.hostedemail.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750961AbdFOQvZ (ORCPT ); Thu, 15 Jun 2017 12:51:25 -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:1437:1515:1516:1518:1535:1543:1593:1594:1605:1711:1730:1747:1777:1792:1801:2393:2553:2559:2562:2828:2895:3138:3139:3140:3141:3142:3608:3622:3865:3866:3867:3868:3870:3871:3872:3873:3874:4250:4321:4605:5007:6119:7576:7903:7974:9151:10004:10400:10482:10848:11026:11232:11233:11473:11658:11914:12043:12296:12438:12740:12760:12895:13095:13161:13229:13439:13869:14096:14097:14181:14659:14721:19901:19997:21080:21324:21433:21451:21627:30005:30012:30029:30030:30034:30054:30070:30090: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:3,LUA_SUMMARY:none X-HE-Tag: walk03_46451e8edd94e X-Filterd-Recvd-Size: 5081 Message-ID: <1497545481.14396.10.camel@perches.com> Subject: Re: [PATCH 3/4] of: Custom printk format specifier for device node From: Joe Perches To: Rob Herring Cc: Frank Rowand , Mark Rutland , Pantelis Antoniou , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" Date: Thu, 15 Jun 2017 09:51:21 -0700 In-Reply-To: References: <20170614203025.7581-1-robh@kernel.org> <20170614203025.7581-4-robh@kernel.org> <1497473808.18751.70.camel@perches.com> Content-Type: text/plain; charset="ISO-8859-1" X-Mailer: Evolution 3.22.6-1ubuntu1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 2017-06-15 at 07:30 -0500, Rob Herring wrote: > On Wed, Jun 14, 2017 at 3:56 PM, Joe Perches wrote: > > 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' > > Okay, but it was good enough for the 2-3 versions Pantelis did before... Which were not applied. > > > 90% of the usage of device node's full_name is printing it out > > > in a kernel message. Preparing for the eventual delayed allocation The "eventual delayed allocation" bit doesn't mean anything to me. > > > 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. > > So %pOFfullname? We've beat this one to death IMO. I don't doubt the utility, just the example. Just mention that full_name is going away. > > > 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. > > 2-6 I'd say is typical. The FDT unflattening code limits things to 64 > (which is probably way more than needed). > > I could re-write it to be non-recursive, but then I'll just have the > max sized array of pointers on the stack. Which would be less stack than how many recursive calls? 5? In any case, 64 * 8 for pointers or 5+ stack frames is a fair amount of stack. Maybe too much. > > > + 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. > > Okay. > > > It's probably better to avoid more recursion stack depth use > > and just use *buf++ as appropriate. > > You can't use *buf++ as this code must work and increment buf even > when buf is NULL. tbuf then.