From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) (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 6BF6721A13499 for ; Thu, 18 May 2017 10:21:49 -0700 (PDT) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 May 2017 10:21:49 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.38,359,1491289200"; d="scan'208";a="858717840" Received: from jljusten-skl.jf.intel.com (HELO localhost) ([10.54.75.25]) by FMSMGA003.fm.intel.com with ESMTP; 18 May 2017 10:21:48 -0700 MIME-Version: 1.0 To: Laszlo Ersek , edk2-devel-01 Message-ID: <149512810792.14216.11044391728020039744@jljusten-skl> From: Jordan Justen In-Reply-To: <20170518150427.16435-2-lersek@redhat.com> Cc: Ard Biesheuvel , Leif Lindholm References: <20170518150427.16435-1-lersek@redhat.com> <20170518150427.16435-2-lersek@redhat.com> User-Agent: alot/0.5.1 Date: Thu, 18 May 2017 10:21:47 -0700 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 17:21:49 -0000 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable 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/ArmPla= tformPkg/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 =3D VA_ARG (Args, UINT32); > + NumOfLba =3D VA_ARG (Args, UINTN); > = > // All blocks must be within range > - DEBUG ((DEBUG_BLKIO, "FvbEraseBlocks: Check if: ( StartingLba=3D%ld = + NumOfLba=3D%d - 1 ) > LastBlock=3D%ld.\n", Instance->StartLba + StartingL= ba, NumOfLba, Instance->Media.LastBlock)); > + DEBUG (( > + DEBUG_BLKIO, > + "FvbEraseBlocks: Check if: ( StartingLba=3D%ld + NumOfLba=3D%Lu - = 1 ) > LastBlock=3D%ld.\n", Notably this is still > 80 columns. Maybe? "FvbEraseBlocks: Check if: ( StartingLba=3D%ld + NumOfLba=3D%Lu - 1 )= " "> LastBlock=3D%ld.\n", I noticed the capital 'L' in "%Lu". Doesn't make a difference since it's not hex... 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. Series Reviewed-by: Jordan Justen > + Instance->StartLba + StartingLba, > + (UINT64)NumOfLba, > + Instance->Media.LastBlock > + )); > if ((NumOfLba =3D=3D 0) || ((Instance->StartLba + StartingLba + NumO= fLba - 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 =3D VA_ARG (Args, UINT32); > + NumOfLba =3D VA_ARG (Args, UINTN); > = > // Go through each one and erase it > while (NumOfLba > 0) { > -- = > 2.9.3 > = >=20