From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 6FC7621A16EF7 for ; Thu, 18 May 2017 12:29:12 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C8A8B72488; Thu, 18 May 2017 19:29:11 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com C8A8B72488 Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com C8A8B72488 Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-86.phx2.redhat.com [10.3.116.86]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8B0DB18393; Thu, 18 May 2017 19:29:10 +0000 (UTC) To: Jordan Justen , edk2-devel-01 Cc: Ard Biesheuvel , Leif Lindholm References: <20170518150427.16435-1-lersek@redhat.com> <20170518150427.16435-2-lersek@redhat.com> <149512810792.14216.11044391728020039744@jljusten-skl> From: Laszlo Ersek Message-ID: Date: Thu, 18 May 2017 21:29:09 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: <149512810792.14216.11044391728020039744@jljusten-skl> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Thu, 18 May 2017 19:29:12 +0000 (UTC) Subject: Re: [PATCH 1/8] ArmPlatformPkg/NorFlashDxe: correct NumOfLba vararg type in EraseBlocks() X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 18 May 2017 19:29:12 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 05/18/17 19:21, Jordan Justen wrote: > On 2017-05-18 08:04:20, Laszlo Ersek wrote: >> According to the PI spec, Volume 3, >> EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL.EraseBlocks(): >> >>> The variable argument list is a list of tuples. Each tuple describes a >>> range of LBAs to erase and consists of the following: >>> * An EFI_LBA that indicates the starting LBA >>> * A UINTN that indicates the number of blocks to erase >> >> (NB, in edk2, EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL is a typedef to >> EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL.) >> >> In this driver, the NumOfLba local variable is defined with type UINTN, >> but the TYPE argument passed to VA_ARG() is UINT32. Fix the mismatch. >> >> In addition, update the DEBUG macro invocation where NumOfLba is formatted >> with the %d conversion specifier: UINTN values should be converted to >> UINT64 and printed with %Lu or %Lx for portability between 32-bit and >> 64-bit. >> >> Cc: Ard Biesheuvel >> Cc: Jordan Justen >> Cc: Leif Lindholm >> Reported-by: Jordan Justen >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Laszlo Ersek >> --- >> ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c | 12 +++++++++--- >> 1 file changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c >> index 12a861267a86..3ea858f46ffb 100644 >> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c >> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c >> @@ -628,10 +628,16 @@ FvbEraseBlocks ( >> } >> >> // How many Lba blocks are we requested to erase? >> - NumOfLba = VA_ARG (Args, UINT32); >> + NumOfLba = VA_ARG (Args, UINTN); >> >> // All blocks must be within range >> - DEBUG ((DEBUG_BLKIO, "FvbEraseBlocks: Check if: ( StartingLba=%ld + NumOfLba=%d - 1 ) > LastBlock=%ld.\n", Instance->StartLba + StartingLba, NumOfLba, Instance->Media.LastBlock)); >> + DEBUG (( >> + DEBUG_BLKIO, >> + "FvbEraseBlocks: Check if: ( StartingLba=%ld + NumOfLba=%Lu - 1 ) > LastBlock=%ld.\n", > > Notably this is still > 80 columns. Maybe? > > "FvbEraseBlocks: Check if: ( StartingLba=%ld + NumOfLba=%Lu - 1 ) " > "> LastBlock=%ld.\n", This file ("ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c") has extremely long lines, the longest one (line 774) has 172 columns. I broke up the above DEBUG so that it would at least fit in 120 chars per line (which is the "second level" recommendation in the coding spec). Also, the format string looked messy enough for me not to want to break it up. Finally, Leif has expressed a preference earlier for keeping DEBUG format strings on one line, if memory serves. (I certainly don't follow that in OvmfPkg and ArmVirtPkg, but ArmPlatformPkg is not my co-turf :) ) I think I'd like to leave the format string like this. > > I noticed the capital 'L' in "%Lu". Doesn't make a difference since > it's not hex... PrintLib handes "l" and "L" interchangeably. In BasePrintLibSPrintMarker(), file "MdePkg/Library/BasePrintLib/PrintLibInternal.c", we have case 'L': case 'l': Flags |= LONG_TYPE; break; and all hex characters are always printed upper-case (see "mHexStr"). Instead, I use the "L" length modifier rather than "l" because: - in ISO C, "l" (in %lx, %ld, %lu) stands for "long", and "L" is not valid for integers, - while in edk2, both "l" and "L" stand for 64-bit (which is a different question from "long"). In other words, "l" has conflicting meanings between ISO C and edk2 (long vs. 64-bit), while "L" is uniquely defined (it is undefined in ISO C, for integers, and in edk2 it stands for 64-bit). > The need for this debug message seems dubious to me, but I guess I > might not be familiar with situation. > > Thanks for noticing and fixing this bug in all the EDK II drivers. For noticing the bug (outside of OvmfPkg/EmuVariableFvbRuntimeDxe, where I originally fixed it), the credit goes to you :) The patches have the right Reported-by: tag. > > Series Reviewed-by: Jordan Justen Thanks! Laszlo >> + Instance->StartLba + StartingLba, >> + (UINT64)NumOfLba, >> + Instance->Media.LastBlock >> + )); >> if ((NumOfLba == 0) || ((Instance->StartLba + StartingLba + NumOfLba - 1) > Instance->Media.LastBlock)) { >> VA_END (Args); >> DEBUG ((EFI_D_ERROR, "FvbEraseBlocks: ERROR - Lba range goes past the last Lba.\n")); >> @@ -656,7 +662,7 @@ FvbEraseBlocks ( >> } >> >> // How many Lba blocks are we requested to erase? >> - NumOfLba = VA_ARG (Args, UINT32); >> + NumOfLba = VA_ARG (Args, UINTN); >> >> // Go through each one and erase it >> while (NumOfLba > 0) { >> -- >> 2.9.3 >> >>