From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752775AbbL1VmK (ORCPT ); Mon, 28 Dec 2015 16:42:10 -0500 Received: from mail-wm0-f49.google.com ([74.125.82.49]:34901 "EHLO mail-wm0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752534AbbL1VmH (ORCPT ); Mon, 28 Dec 2015 16:42:07 -0500 From: Rasmus Villemoes To: Joe Perches Cc: Andy Shevchenko , Andrew Morton , linux-kernel@vger.kernel.org Subject: Re: [PATCH v1 1/1] lib/vsprintf: refactor duplicate code to xnumber() Organization: D03 References: <1451326703-122826-1-git-send-email-andriy.shevchenko@linux.intel.com> <1451327112.3219.14.camel@perches.com> X-Hashcash: 1:20:151228:akpm@linux-foundation.org::Rldcfm2KZ8PnpASY:00000000000000000000000000000000000009u4 X-Hashcash: 1:20:151228:andriy.shevchenko@linux.intel.com::qNi+voOAdMEXLp4X:00000000000000000000000000000kUL X-Hashcash: 1:20:151228:joe@perches.com::QBaacEIb/3MmXb56:0011qV X-Hashcash: 1:20:151228:linux-kernel@vger.kernel.org::u2SgA9/cYOWGStkz:00000000000000000000000000000000028kK Date: Mon, 28 Dec 2015 22:42:02 +0100 In-Reply-To: <1451327112.3219.14.camel@perches.com> (Joe Perches's message of "Mon, 28 Dec 2015 10:25:12 -0800") Message-ID: <87h9j245ed.fsf@rasmusvillemoes.dk> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 %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 >> --- >> 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? > xnumber isn't a great name. Maybe 'hexnumber'. That's a bit further away from 'number', and 'x' might stand for something other than hex. > 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. >> 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 ;-) num = *(const dma_addr_t *)addr; size = sizeof(dma_addr_t); Rasmus