Re: [platforms: PATCH 4/6] Marvell/Applications: SpiTool: Do not override existing slave device

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

On Tue, Oct 31, 2017 at 04:59:33AM +0100, Marcin Wojtas wrote:
> Current usage of sf command requires running 'sf probe' prior to
> executing any other option. Because it is done in two separate steps,
> it turned out that SpiMasterProtocol->SetupDevice could easily
> overwrite valid Slave pointer when performing second operation.
> 
> Fix the issue by allocating Slave device only once and keep it
> as global variable in the SpiTool application. This patch
> also updates FirmwareUpdate command to follow the modified
> SetupDevice operation.

Really an unrelated question, but would we not expect to use capsule
updates instead now? Do we have other uses for this tool?

> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c |  4 ++--
>  Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c    |  8 ++++----
>  Platform/Marvell/Drivers/Spi/MvSpiDxe.c                | 17 +++++++++--------
>  Platform/Marvell/Drivers/Spi/MvSpiDxe.h                |  1 +
>  Platform/Marvell/Include/Protocol/Spi.h                |  1 +
>  5 files changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
> index 750e52a..9ccb1c7 100644
> --- a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
> +++ b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.c
> @@ -240,7 +240,7 @@ ShellCommandRunFUpdate (
>    )
>  {
>    IN SHELL_FILE_HANDLE    FileHandle;
> -  SPI_DEVICE              *Slave;
> +  SPI_DEVICE              *Slave = NULL;
>    UINT64                  FileSize;
>    UINTN                   *FileBuffer = NULL;
>    CHAR16                  *ProblemParam;
> @@ -302,7 +302,7 @@ ShellCommandRunFUpdate (
>    }
>  
>    // Setup and probe SPI flash
> -  Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, 0, 0);
> +  Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, Slave, 0, 0);
>    if (Slave == NULL) {
>      Print(L"%s: Cannot allocate SPI device!\n", CMD_NAME_STRING);
>      goto HeaderError;
> diff --git a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
> index 68a6cf7..1084f68 100644
> --- a/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
> +++ b/Platform/Marvell/Applications/SpiTool/SpiFlashCmd.c
> @@ -191,7 +191,7 @@ ShellCommandRunSpiFlash (
>    )
>  {
>  EFI_STATUS              Status;
> -  SPI_DEVICE            *Slave;
> +  STATIC SPI_DEVICE     *Slave;

If this is a safe thing to do, please use a global variable called
mSlave instead, to make it clear that it persists across calls.

>    LIST_ENTRY            *CheckPackage;
>    EFI_PHYSICAL_ADDRESS  Address = 0, Offset = 0;
>    SHELL_FILE_HANDLE     FileHandle = NULL;
> @@ -273,7 +273,7 @@ EFI_STATUS              Status;
>    Cs = PcdGet32 (PcdSpiFlashCs);
>  
>    // Setup new spi device
> -  Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, Cs, Mode);
> +  Slave = SpiMasterProtocol->SetupDevice (SpiMasterProtocol, Slave, Cs, Mode);
>      if (Slave == NULL) {
>        Print(L"sf: Cannot allocate SPI device!\n");
>        return SHELL_ABORTED;
> @@ -285,6 +285,8 @@ EFI_STATUS              Status;
>      Status = FlashProbe (Slave);
>      if (EFI_ERROR(Status)) {
>        // No supported spi flash detected
> +      SpiMasterProtocol->FreeDevice(Slave);

Space before (.

/
    Leif

> +      Slave = NULL;
>        return SHELL_ABORTED;
>      } else {
>        return Status;
> @@ -426,8 +428,6 @@ EFI_STATUS              Status;
>      break;
>    }
>  
> -  SpiMasterProtocol->FreeDevice(Slave);
> -
>    if (EFI_ERROR (Status)) {
>      Print (L"sf: Error while performing spi transfer\n");
>      return SHELL_ABORTED;
> diff --git a/Platform/Marvell/Drivers/Spi/MvSpiDxe.c b/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
> index 3b49147..a7db5f2 100755
> --- a/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
> +++ b/Platform/Marvell/Drivers/Spi/MvSpiDxe.c
> @@ -296,21 +296,22 @@ SPI_DEVICE *
>  EFIAPI
>  MvSpiSetupSlave (
>    IN MARVELL_SPI_MASTER_PROTOCOL *This,
> +  IN SPI_DEVICE *Slave,
>    IN UINTN Cs,
>    IN SPI_MODE Mode
>    )
>  {
> -  SPI_DEVICE  *Slave;
> +  if (!Slave) {
> +    Slave = AllocateZeroPool (sizeof(SPI_DEVICE));
> +    if (Slave == NULL) {
> +      DEBUG((DEBUG_ERROR, "Cannot allocate memory\n"));
> +      return NULL;
> +    }
>  
> -  Slave = AllocateZeroPool (sizeof(SPI_DEVICE));
> -  if (Slave == NULL) {
> -    DEBUG((DEBUG_ERROR, "Cannot allocate memory\n"));
> -    return NULL;
> +    Slave->Cs   = Cs;
> +    Slave->Mode = Mode;
>    }
>  
> -  Slave->Cs   = Cs;
> -  Slave->Mode = Mode;
> -
>    SpiSetupTransfer (This, Slave);
>  
>    return Slave;
> diff --git a/Platform/Marvell/Drivers/Spi/MvSpiDxe.h b/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
> index 1401f62..e7e280a 100644
> --- a/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
> +++ b/Platform/Marvell/Drivers/Spi/MvSpiDxe.h
> @@ -125,6 +125,7 @@ SPI_DEVICE *
>  EFIAPI
>  MvSpiSetupSlave (
>    IN MARVELL_SPI_MASTER_PROTOCOL     * This,
> +  IN SPI_DEVICE *Slave,
>    IN UINTN Cs,
>    IN SPI_MODE Mode
>    );
> diff --git a/Platform/Marvell/Include/Protocol/Spi.h b/Platform/Marvell/Include/Protocol/Spi.h
> index 93a8ec0..0cf7914 100644
> --- a/Platform/Marvell/Include/Protocol/Spi.h
> +++ b/Platform/Marvell/Include/Protocol/Spi.h
> @@ -87,6 +87,7 @@ typedef
>  SPI_DEVICE *
>  (EFIAPI *MV_SPI_SETUP_DEVICE) (
>    IN MARVELL_SPI_MASTER_PROTOCOL *This,
> +  IN SPI_DEVICE *Slave,
>    IN UINTN    Cs,
>    IN SPI_MODE Mode
>    );
> -- 
> 2.7.4
>