Re: [platforms: PATCH 07/11] Applications/FirmwareUpdate: Fix 32-bit issues

[Leif Lindholm <leif.lindholm@linaro.org>]

On Fri, Sep 01, 2017 at 04:16:24PM +0100, Ard Biesheuvel wrote:
> On 1 September 2017 at 15:54, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Fri, Sep 01, 2017 at 03:08:19PM +0200, Marcin Wojtas wrote:
> >> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>
> >> Fix casting and related issues to make this code build for 32-bit ARM.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> >> ---
> >>  Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c | 5 ++++-
> >>  1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
> >> index edb6986..0951734 100644
> >> --- a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
> >> +++ b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
> >> @@ -172,6 +172,7 @@ PrepareFirmwareImage (
> >>    EFI_STATUS           Status;
> >>    UINT64               OpenMode;
> >>    UINTN                *Buffer;
> >> +  UINT64               Size;
> >>
> >>    // Parse string from commandline
> >>    FileStr = ShellCommandLineGetRawValue (CheckPackage, 1);
> >> @@ -195,11 +196,13 @@ PrepareFirmwareImage (
> >>        return EFI_DEVICE_ERROR;
> >>      }
> >>
> >> -  Status = FileHandleGetSize (*FileHandle, FileSize);
> >> +  Status = FileHandleGetSize (*FileHandle, &Size);
> >>      if (EFI_ERROR (Status)) {
> >>        Print (L"%s: Cannot get Image file size\n", CMD_NAME_STRING);
> >>      }
> >>
> >> +  *FileSize = (UINTN)Size;
> >> +
> >
> > Rather than juggling around with temporary variables, why not make
> > FileSize in ShellCommandRunFUpdate() UINT64 and update
> > PrepareFirmwareImage() prototype accordingly?
> 
> I don't remember /exactly/, but I think it breaks in another place then.

The only thing I see that _could_ be affected is
    Status = SpiFlashProtocol->Update (Slave, 0, FileSize, (UINT8 *)FileBuffer);

And I would prefer a (UINTN) cast there over temporary variable
shuffling in a subfunction.

/
    Leif