From patchwork Fri May 6 17:19:06 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mark Rutland X-Patchwork-Id: 67286 Delivered-To: patch@linaro.org Received: by 10.140.92.199 with SMTP id b65csp428812qge; Fri, 6 May 2016 10:19:30 -0700 (PDT) X-Received: by 10.66.172.165 with SMTP id bd5mr30798068pac.128.1462555169990; Fri, 06 May 2016 10:19:29 -0700 (PDT) Return-Path: Received: from ml01.01.org (ml01.01.org. [198.145.21.10]) by mx.google.com with ESMTPS id 87si13329893pfp.114.2016.05.06.10.19.29 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 06 May 2016 10:19:29 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of edk2-devel-bounces@lists.01.org designates 198.145.21.10 as permitted sender) client-ip=198.145.21.10; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of edk2-devel-bounces@lists.01.org designates 198.145.21.10 as permitted sender) smtp.mailfrom=edk2-devel-bounces@lists.01.org Received: from [127.0.0.1] (localhost [IPv6:::1]) by ml01.01.org (Postfix) with ESMTP id 967311A1F03; Fri, 6 May 2016 10:19:29 -0700 (PDT) X-Original-To: edk2-devel@lists.01.org Delivered-To: edk2-devel@lists.01.org Received: from foss.arm.com (foss.arm.com [217.140.101.70]) by ml01.01.org (Postfix) with ESMTP id A1B471A1DFE for ; Fri, 6 May 2016 10:19:28 -0700 (PDT) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id C2FD849; Fri, 6 May 2016 10:19:36 -0700 (PDT) Received: from leverpostej.cambridge.arm.com (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id 9EFE03F246; Fri, 6 May 2016 10:19:27 -0700 (PDT) From: Mark Rutland To: edk2-devel@lists.01.org Date: Fri, 6 May 2016 18:19:06 +0100 Message-Id: <1462555149-18136-2-git-send-email-mark.rutland@arm.com> X-Mailer: git-send-email 1.9.1 In-Reply-To: <1462555149-18136-1-git-send-email-mark.rutland@arm.com> References: <1462555149-18136-1-git-send-email-mark.rutland@arm.com> Subject: [edk2] [PATCH 1/4] Revert "EmbeddedPkg/Lan9118Dxe: use MemoryFence" X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Cc: ryan.harkin@linaro.org, leif.lindholm@linaro.org, ard.biesheuvel@linaro.org MIME-Version: 1.0 Errors-To: edk2-devel-bounces@lists.01.org Sender: "edk2-devel" Commit a4626006bbf86113 ("EmbeddedPkg/Lan9118Dxe: use MemoryFence") replaced some stalls with memory fences, on the presumption that these were erroneously being used to order memory accesses. However, this was not the case. LAN9118 devices require a timing delay between state-changing reads/writes and subsequent reads, as updates to the register file are asynchronous and the effects of state-changes are not immediately visible to subsequent reads. This delay cannot be ensured through the use of memory barriers, which only enforce observable ordering, and not timing. Thus, converting these stalls to memory fences was erroneous, and may result in stale values being read. This reverts commit a4626006bbf86113453aeb7920895e66cdd04737. Cc: Ard Biesheuvel Cc: Leif Lindholm Cc: Ryan Harkin Signed-off-by: Mark Rutland Contributed-under: TianoCore Contribution Agreement 1.0 --- EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c | 9 +++--- EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c | 40 +++++++++++-------------- 2 files changed, 23 insertions(+), 26 deletions(-) -- 1.9.1 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel diff --git a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c index d0bf7be..50644e7 100644 --- a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c +++ b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118Dxe.c @@ -307,7 +307,8 @@ SnpInitialize ( // Write the current configuration to the register MmioWrite32 (LAN9118_PMT_CTRL, PmConf); - MemoryFence(); + gBS->Stall (LAN9118_STALL); + gBS->Stall (LAN9118_STALL); // Configure GPIO and HW Status = ConfigureHardware (HW_CONF_USE_LEDS, Snp); @@ -430,7 +431,7 @@ SnpReset ( // Write the current configuration to the register MmioWrite32 (LAN9118_PMT_CTRL, PmConf); - MemoryFence(); + gBS->Stall (LAN9118_STALL); // Reactivate the LEDs Status = ConfigureHardware (HW_CONF_USE_LEDS, Snp); @@ -445,7 +446,7 @@ SnpReset ( HwConf |= HW_CFG_TX_FIFO_SIZE(gTxBuffer); // assign size chosen in SnpInitialize MmioWrite32 (LAN9118_HW_CFG, HwConf); // Write the conf - MemoryFence(); + gBS->Stall (LAN9118_STALL); } // Enable the receiver and transmitter and clear their contents @@ -700,7 +701,7 @@ SnpReceiveFilters ( // Write the options to the MAC_CSR // IndirectMACWrite32 (INDIRECT_MAC_INDEX_CR, MacCSRValue); - MemoryFence(); + gBS->Stall (LAN9118_STALL); // // If we have to retrieve something, start packet reception. diff --git a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c index 3ef98ef..bd20eeb 100644 --- a/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c +++ b/EmbeddedPkg/Drivers/Lan9118Dxe/Lan9118DxeUtil.c @@ -236,7 +236,7 @@ IndirectEEPROMRead32 ( // Write to Eeprom command register MmioWrite32 (LAN9118_E2P_CMD, EepromCmd); - MemoryFence(); + gBS->Stall (LAN9118_STALL); // Wait until operation has completed while (MmioRead32 (LAN9118_E2P_CMD) & E2P_EPC_BUSY); @@ -284,7 +284,7 @@ IndirectEEPROMWrite32 ( // Write to Eeprom command register MmioWrite32 (LAN9118_E2P_CMD, EepromCmd); - MemoryFence(); + gBS->Stall (LAN9118_STALL); // Wait until operation has completed while (MmioRead32 (LAN9118_E2P_CMD) & E2P_EPC_BUSY); @@ -362,14 +362,13 @@ Lan9118Initialize ( if (((MmioRead32 (LAN9118_PMT_CTRL) & MPTCTRL_PM_MODE_MASK) >> 12) != 0) { DEBUG ((DEBUG_NET, "Waking from reduced power state.\n")); MmioWrite32 (LAN9118_BYTE_TEST, 0xFFFFFFFF); - MemoryFence(); + gBS->Stall (LAN9118_STALL); } // Check that device is active Retries = 20; while ((MmioRead32 (LAN9118_PMT_CTRL) & MPTCTRL_READY) == 0 && --Retries) { gBS->Stall (LAN9118_STALL); - MemoryFence(); } if (!Retries) { return EFI_TIMEOUT; @@ -379,7 +378,6 @@ Lan9118Initialize ( Retries = 20; while ((MmioRead32 (LAN9118_E2P_CMD) & E2P_EPC_BUSY) && --Retries){ gBS->Stall (LAN9118_STALL); - MemoryFence(); } if (!Retries) { return EFI_TIMEOUT; @@ -449,12 +447,11 @@ SoftReset ( // Write the configuration MmioWrite32 (LAN9118_HW_CFG, HwConf); - MemoryFence(); + gBS->Stall (LAN9118_STALL); // Wait for reset to complete while (MmioRead32 (LAN9118_HW_CFG) & HWCFG_SRST) { - MemoryFence(); gBS->Stall (LAN9118_STALL); ResetTime += 1; @@ -503,7 +500,7 @@ PhySoftReset ( // Wait for completion while (MmioRead32 (LAN9118_PMT_CTRL) & MPTCTRL_PHY_RST) { - MemoryFence(); + gBS->Stall (LAN9118_STALL); } // PHY Basic Control Register reset } else if (Flags & PHY_RESET_BCR) { @@ -511,7 +508,7 @@ PhySoftReset ( // Wait for completion while (IndirectPHYRead32 (PHY_INDEX_BASIC_CTRL) & PHYCR_RESET) { - MemoryFence(); + gBS->Stall (LAN9118_STALL); } } @@ -545,7 +542,7 @@ ConfigureHardware ( // Write the configuration MmioWrite32 (LAN9118_GPIO_CFG, GpioConf); - MemoryFence(); + gBS->Stall (LAN9118_STALL); } return EFI_SUCCESS; @@ -588,7 +585,6 @@ AutoNegotiate ( // Wait until it is up or until Time Out Retries = FixedPcdGet32 (PcdLan9118DefaultNegotiationTimeout) / LAN9118_STALL; while ((IndirectPHYRead32 (PHY_INDEX_BASIC_STATUS) & PHYSTS_LINK_STS) == 0) { - MemoryFence(); gBS->Stall (LAN9118_STALL); Retries--; if (!Retries) { @@ -675,7 +671,7 @@ StopTx ( TxCfg = MmioRead32 (LAN9118_TX_CFG); TxCfg |= TXCFG_TXS_DUMP | TXCFG_TXD_DUMP; MmioWrite32 (LAN9118_TX_CFG, TxCfg); - MemoryFence(); + gBS->Stall (LAN9118_STALL); } // Check if already stopped @@ -694,7 +690,7 @@ StopTx ( if (TxCfg & TXCFG_TX_ON) { TxCfg |= TXCFG_STOP_TX; MmioWrite32 (LAN9118_TX_CFG, TxCfg); - MemoryFence(); + gBS->Stall (LAN9118_STALL); // Wait for Tx to finish transmitting while (MmioRead32 (LAN9118_TX_CFG) & TXCFG_STOP_TX); @@ -729,7 +725,7 @@ StopRx ( RxCfg = MmioRead32 (LAN9118_RX_CFG); RxCfg |= RXCFG_RX_DUMP; MmioWrite32 (LAN9118_RX_CFG, RxCfg); - MemoryFence(); + gBS->Stall (LAN9118_STALL); while (MmioRead32 (LAN9118_RX_CFG) & RXCFG_RX_DUMP); } @@ -755,28 +751,28 @@ StartTx ( TxCfg = MmioRead32 (LAN9118_TX_CFG); TxCfg |= TXCFG_TXS_DUMP | TXCFG_TXD_DUMP; MmioWrite32 (LAN9118_TX_CFG, TxCfg); - MemoryFence(); + gBS->Stall (LAN9118_STALL); } // Check if tx was started from MAC and enable if not if (Flags & START_TX_MAC) { MacCsr = IndirectMACRead32 (INDIRECT_MAC_INDEX_CR); - MemoryFence(); + gBS->Stall (LAN9118_STALL); if ((MacCsr & MACCR_TX_EN) == 0) { MacCsr |= MACCR_TX_EN; IndirectMACWrite32 (INDIRECT_MAC_INDEX_CR, MacCsr); - MemoryFence(); + gBS->Stall (LAN9118_STALL); } } // Check if tx was started from TX_CFG and enable if not if (Flags & START_TX_CFG) { TxCfg = MmioRead32 (LAN9118_TX_CFG); - MemoryFence(); + gBS->Stall (LAN9118_STALL); if ((TxCfg & TXCFG_TX_ON) == 0) { TxCfg |= TXCFG_TX_ON; MmioWrite32 (LAN9118_TX_CFG, TxCfg); - MemoryFence(); + gBS->Stall (LAN9118_STALL); } } @@ -806,14 +802,14 @@ StartRx ( RxCfg = MmioRead32 (LAN9118_RX_CFG); RxCfg |= RXCFG_RX_DUMP; MmioWrite32 (LAN9118_RX_CFG, RxCfg); - MemoryFence(); + gBS->Stall (LAN9118_STALL); while (MmioRead32 (LAN9118_RX_CFG) & RXCFG_RX_DUMP); } MacCsr |= MACCR_RX_EN; IndirectMACWrite32 (INDIRECT_MAC_INDEX_CR, MacCsr); - MemoryFence(); + gBS->Stall (LAN9118_STALL); } return EFI_SUCCESS; @@ -1003,7 +999,7 @@ ChangeFifoAllocation ( HwConf &= ~(0xF0000); HwConf |= ((TxFifoOption & 0xF) << 16); MmioWrite32 (LAN9118_HW_CFG, HwConf); - MemoryFence(); + gBS->Stall (LAN9118_STALL); return EFI_SUCCESS; }