From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753506AbbL2PHc (ORCPT ); Tue, 29 Dec 2015 10:07:32 -0500 Received: from mga02.intel.com ([134.134.136.20]:64918 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752868AbbL2PH2 (ORCPT ); Tue, 29 Dec 2015 10:07:28 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,495,1444719600"; d="scan'208";a="625262439" Message-ID: <1451401664.30729.368.camel@linux.intel.com> Subject: Re: [PATCH v1 1/1] lib/vsprintf: refactor duplicate code to xnumber() From: Andy Shevchenko To: Andy Shevchenko , Rasmus Villemoes Cc: Joe Perches , Andrew Morton , "linux-kernel@vger.kernel.org" Date: Tue, 29 Dec 2015 17:07:44 +0200 In-Reply-To: References: <1451326703-122826-1-git-send-email-andriy.shevchenko@linux.intel.com> <1451327112.3219.14.camel@perches.com> <87h9j245ed.fsf@rasmusvillemoes.dk> Organization: Intel Finland Oy Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.18.2-1 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 Tue, 2015-12-29 at 00:20 +0200, Andy Shevchenko wrote: > On Mon, Dec 28, 2015 at 11:42 PM, Rasmus Villemoes > wrote: > > On Mon, Dec 28 2015, Joe Perches wrote: > > > > > On Mon, 2015-12-28 at 20:18 +0200, Andy Shevchenko wrote: > > > > xnumber() is a special helper to print a fixed size type in a > > > > hex format with > > > > '0x' prefix with padding and reduced size. In the module we > > > > have already > > > > several copies of such code. Consolidate them under xnumber() > > > > helper. > > > > > > > > There are couple of differences though. > > > > > > > > It seems nobody cared about the output in case of > > > > CONFIG_KALLSYMS=n when > > > > printing symbol address because the asked width is not enough > > > > to care either > > > > prefix or last byte. Fixed here. > > > > ok, though I'm curious what 'last byte' refers to here? > > The last byte ('78') as it appears in the string carrying the number > '0x12345678'. Yeah, might be confusing, I'm open for suggestion how > to > phrase it. > > > > > > > The %pNF specifier used to be allowed with a specific field > > > > width, though there > > > > is neither any user of it nor mention in the documentation. > > > > > > > > Signed-off-by: Andy Shevchenko > > > om> > > > > --- > > > >  lib/vsprintf.c | 43 +++++++++++++++--------------------------- > > > > - > > > >  1 file changed, 15 insertions(+), 28 deletions(-) > > > > > > > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > > > > index dcf5646..e971549 100644 > > > > --- a/lib/vsprintf.c > > > > +++ b/lib/vsprintf.c > > > > @@ -514,6 +514,16 @@ char *number(char *buf, char *end, > > > > unsigned long long num, > > > >      return buf; > > > >  } > > > > > > > > +static noinline_for_stack > > > > +char *xnumber(char *buf, char *end, unsigned long long value, > > > > unsigned int type, > > > > +          struct printf_spec spec) > > > > Is there any aspect of the passed-through printf_spec which isn't > > overridden in xnumber? The users are/will be various %p extensions, > > which probably means that no-one passes a non-default precision > > (gcc > > complains about %.*p), and the remaining possible flags (PLUS, > > LEFT, > > SPACE) are useless and/or impossible to pass to %p without gcc > > complaining. In other words, why pass the spec at all instead of > > just > > building it inside xnumber? > > Wow, good catch! > I slightly suspected something like that, but didn't made up my mind > to check this. > > > > > > xnumber isn't a great name. > > > > Maybe 'hexnumber'. > > We already have similar for %*ph. And as you noticed below… > > > That's a bit further away from 'number', and 'x' > > might stand for something other than hex. > > …isn't only about hex. I don't know how to play on words the all > three > flags including 16 base. > > > > > > unsigned int type should probably be size_t size > > > > Compromise: 'unsigned int size'. The name should be size since it's > > supposed to be the size of the actual type being printed. But the > > type > > carrying that information need not be 8 bytes wide on 64bits. > > Exactly, the result anyway as for now only 8 bits as a part of > unsigned int. Oops, 24 bits of signed int. Incorrectly caught wrong line. So, I will change this to be int size then. > > > > > > > static noinline_for_stack > > > >  char *address_val(char *buf, char *end, const void *addr, > > > >                struct printf_spec spec, const char *fmt) > > > >  { > > > > -    unsigned long long num; > > > > - > > > > -    spec.flags |= SPECIAL | SMALL | ZEROPAD; > > > > -    spec.base = 16; > > > > - > > > >      switch (fmt[1]) { > > > >      case 'd': > > > > -            num = *(const dma_addr_t *)addr; > > > > -            spec.field_width = sizeof(dma_addr_t) * 2 + 2; > > > > -            break; > > > > +            return xnumber(buf, end, *(const dma_addr_t > > > > *)addr, sizeof(dma_addr_t), spec); > > > >      case 'p': > > > >      default: > > > > -            num = *(const phys_addr_t *)addr; > > > > -            spec.field_width = sizeof(phys_addr_t) * 2 + 2; > > > > -            break; > > > > +            return xnumber(buf, end, *(const phys_addr_t > > > > *)addr, sizeof(phys_addr_t), spec); > > > >      } > > > > - > > > > -    return number(buf, end, num, spec); > > > >  } > > > > Nit: I think it would be a bit easier to read if the > > cast+dereference > > are kept outside the function calls. I'd suggest just introducing > > 'unsigned int size', assign the appropriate value in the two cases, > > and > > fall through to a common 'xnumber(buf, end, num, size);'. It'll > > even > > line up nicely ;-) > > Will try that. > > > > > num = *(const dma_addr_t *)addr; > > size = sizeof(dma_addr_t); > > Thanks, Rasmus, for review. > -- Andy Shevchenko Intel Finland Oy