From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932122AbcAYIde (ORCPT ); Mon, 25 Jan 2016 03:33:34 -0500 Received: from mga02.intel.com ([134.134.136.20]:27860 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932069AbcAYIda (ORCPT ); Mon, 25 Jan 2016 03:33:30 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,343,1449561600"; d="scan'208";a="640948619" Message-ID: <1453710713.2521.200.camel@linux.intel.com> Subject: Re: [PATCH v3 3/4] x86/efi: print size in binary units in efi_print_memmap From: Andy Shevchenko To: James Bottomley , Andy Shevchenko Cc: Matt Fleming , Thomas Gleixner , Ingo Molnar , "H . Peter Anvin" , linux-efi@vger.kernel.org, Rasmus Villemoes , Andrew Morton , "linux-kernel @ vger . kernel . org" , Robert Elliott Date: Mon, 25 Jan 2016 10:31:53 +0200 In-Reply-To: <1453572191.2470.52.camel@HansenPartnership.com> References: <1453560913-134672-1-git-send-email-andriy.shevchenko@linux.intel.com> <1453560913-134672-4-git-send-email-andriy.shevchenko@linux.intel.com> <1453567445.2470.24.camel@HansenPartnership.com> <1453572191.2470.52.camel@HansenPartnership.com> Organization: Intel Finland Oy Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.18.3-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 Sat, 2016-01-23 at 10:03 -0800, James Bottomley wrote: > On Sat, 2016-01-23 at 19:18 +0200, Andy Shevchenko wrote: > > On Sat, Jan 23, 2016 at 6:44 PM, James Bottomley > > wrote: > > > On Sat, 2016-01-23 at 16:55 +0200, Andy Shevchenko wrote: > > > > > > +static char * __init efi_size_format(char *buf, size_t size, > > > > u64 > > > > bytes) > > > > +{ > > > > +     unsigned long i = bytes ? __ffs64(bytes) / 10 : 0; > > > > > > What if size is zero, which might happen on a UEFI screw up? > > > > size of what? Of input buffer? > > I mean when bytes == 0 ffs is undefined. Well, someone misread the above code ;-) There is ternary operator exactly to serve this purpose. > > > >  Also it gives really odd results for non power of two memory  > > > sizes. 16384MB prints as 16GiB but 16385 prints as 16385MiB. > > > > Adaptive precision. I don't think the idea is to print a nearby > > numbers here. > > Well either there's a point to reducing to the nearest exponent or we > simply print everything in MB as the original did.  Doing it > inconsistently is asking for trouble ... and lots of user queries.  I > mean, supposing there's a range off by one ... now we print a huge > number in B. > I really advise against hacking around like this.  In any event if > efi > must have this, please don't involve the parts of the kernel that try > to do this correctly, like lib/string_helpers.h > > > > If the goal is to have a clean interface reporting only the > > > first  > > > four significant figures and a size exponent, then a helper > > > would  > > > be much better than trying to open code this ad hoc. > > > > No. You get it wrong. The initial idea was (actually not mine, see > > authorship) to print an exact number with units and reduce whenever > > it's possible, i.e number is a multiplication of certain unit. > > so you must implement the original idea no matter how inconsistent it > leads us to be?  Is it wrong to try to do better? For both comments I prefer to hear Matt's opinion as he is maintainer of EFI stuff. My role in this all is to reduce the code base by avoiding 'not invented here' syndrome. -- Andy Shevchenko Intel Finland Oy