From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:400c:c09::243; helo=mail-wm0-x243.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wm0-x243.google.com (mail-wm0-x243.google.com [IPv6:2a00:1450:400c:c09::243]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id D341B203525E2 for ; Tue, 31 Oct 2017 20:41:45 -0700 (PDT) Received: by mail-wm0-x243.google.com with SMTP id b189so2388179wmd.4 for ; Tue, 31 Oct 2017 20:45:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=L9W2SB63JVrAT4lqX+2+5njf9hAQTWD4IWHPiGChePM=; b=W+e/eFcpqXd/9Ufo/ihWpzHGfUR8xi8Jvnfpc9DPKYI45hu/YMMUPG9qiW6mZHUT+s meYHjFdz7zwU3IDlr1M81p1EcbVS5mW+FvFr7U9ZV+eayr3HgD/g8Ba9k4WVeHGQVPTp mGJzKsrUOwHDb+9LXBtbsQFNjDLSQ3wckmlxA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=L9W2SB63JVrAT4lqX+2+5njf9hAQTWD4IWHPiGChePM=; b=pCW2BDiSvVlgvj6qcQZ3HH9rZgrjcMuxyybQRoBUH7RUhy3YIcG1LvhMt5S5/E1RG9 KSSaC5iBLCgq6V+75kkQOooZv3HzKF7nUmg+CnVibfHEn5mIOu+DS1V8ywcffzJSKLBE 242vCUANTB5chVcTQZ1+EsNGcmoTL4XJmjm3+7K+VyV1g/MwMHXXbF7MfjeTU9ic38o3 1xN82OU5psc9JZWhvGOyJzraqYihUmG7gkY7u9pllQbMxCtnIV5F71w4dINHPxhmC+bc so/I7JiknjzUEhMVQSkYjgf+O47i4ltR6EH+hGfHappLjMX+IKNGirVAlsr18S3YrS2v OGHg== X-Gm-Message-State: AMCzsaXtQ1EG3juoknCrCRI2B//f/jgkEWwkZ8qwLFPOkFKxAZ7IJ579 ZqlMCCFCBK++qNsu+zALLHJ7JA== X-Google-Smtp-Source: ABhQp+QBAvPL0llhU5DxgktkJKosluIWgrc4kwxje8AB4SGtK3I885+8mRSY9RkwQNFvzN1BY3lyPQ== X-Received: by 10.28.145.196 with SMTP id t187mr3335653wmd.119.1509507936737; Tue, 31 Oct 2017 20:45:36 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id p200sm2548630wmd.9.2017.10.31.20.45.35 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 31 Oct 2017 20:45:35 -0700 (PDT) Date: Wed, 1 Nov 2017 03:45:34 +0000 From: Leif Lindholm To: Marcin Wojtas Cc: edk2-devel@lists.01.org, ard.biesheuvel@linaro.org, nadavh@marvell.com, neta@marvell.com, kostap@marvell.com, jinghua@marvell.com, jsd@semihalf.com Message-ID: <20171101034534.bwm4qd3nuc76ccff@bivouac.eciton.net> References: <1509422375-20198-1-git-send-email-mw@semihalf.com> <1509422375-20198-5-git-send-email-mw@semihalf.com> MIME-Version: 1.0 In-Reply-To: <1509422375-20198-5-git-send-email-mw@semihalf.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [platforms: PATCH 4/6] Marvell/Applications: SpiTool: Do not override existing slave device 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: Wed, 01 Nov 2017 03:41:46 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 > --- > 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 >