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 <[email protected]> > --- > 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 > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
