[edk2,platforms:,07/11] Applications/FirmwareUpdate: Fix 32-bit issues

Message ID 1504271303-1782-8-git-send-email-mw@semihalf.com
State New
Headers show
Series
  • Untitled series #3719
Related show

Commit Message

Marcin Wojtas Sept. 1, 2017, 1:08 p.m.
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(-)

-- 
1.8.3.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Leif Lindholm Sept. 1, 2017, 2:54 p.m. | #1
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?

/
     Leif

>    // Read Image header into buffer

>    Buffer = AllocateZeroPool (*FileSize);

>  

> -- 

> 1.8.3.1

> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Sept. 1, 2017, 3:16 p.m. | #2
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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif Lindholm Sept. 1, 2017, 3:51 p.m. | #3
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch

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;
+
   // Read Image header into buffer
   Buffer = AllocateZeroPool (*FileSize);