| Message ID | 1502940316-13384-3-git-send-email-dingtianhong@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
On 2017/8/17 22:17, Tantilov, Emil S wrote: >> ret_val = ixgbe_start_hw_generic(hw); >> >> -#ifndef CONFIG_SPARC >> - /* Disable relaxed ordering */ >> - for (i = 0; ((i < hw->mac.max_tx_queues) && >> - (i < IXGBE_DCA_MAX_QUEUES_82598)); i++) { >> - regval = IXGBE_READ_REG(hw, IXGBE_DCA_TXCTRL(i)); >> - regval &= ~IXGBE_DCA_TXCTRL_DESC_WRO_EN; >> - IXGBE_WRITE_REG(hw, IXGBE_DCA_TXCTRL(i), regval); >> - } >> + if (!pcie_relaxed_ordering_enabled(adapter->pdev)) { > > As Alex mentioned there is no need for this check in any form. > > The HW defaults to Relaxed Ordering enabled unless it is disabled in > the PCIe Device Control Register. So the above logic is already done by HW. > > All you have to do is strip the code disabling relaxed ordering. > Hi Tantilov: I misunderstood Alex's suggestion, But I still couldn't find the logic where the HW disable the Relaxed Ordering when the PCIe Device Control Register disable it, can you point it out? Thanks Ding > Thanks, > Emil > > > . >
>-----Original Message----- >From: Ding Tianhong [mailto:dingtianhong@huawei.com] >Sent: Thursday, August 17, 2017 5:39 PM >To: Tantilov, Emil S <emil.s.tantilov@intel.com>; davem@davemloft.net; >Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>; keescook@chromium.org; >linux-kernel@vger.kernel.org; sparclinux@vger.kernel.org; intel-wired- >lan@lists.osuosl.org; alexander.duyck@gmail.com; netdev@vger.kernel.org; >linuxarm@huawei.com >Subject: Re: [PATCH net v2 2/2] net: ixgbe: Use new >PCI_DEV_FLAGS_NO_RELAXED_ORDERING flag > > > >On 2017/8/17 22:17, Tantilov, Emil S wrote: > >>> ret_val = ixgbe_start_hw_generic(hw); >>> >>> -#ifndef CONFIG_SPARC >>> - /* Disable relaxed ordering */ >>> - for (i = 0; ((i < hw->mac.max_tx_queues) && >>> - (i < IXGBE_DCA_MAX_QUEUES_82598)); i++) { >>> - regval = IXGBE_READ_REG(hw, IXGBE_DCA_TXCTRL(i)); >>> - regval &= ~IXGBE_DCA_TXCTRL_DESC_WRO_EN; >>> - IXGBE_WRITE_REG(hw, IXGBE_DCA_TXCTRL(i), regval); >>> - } >>> + if (!pcie_relaxed_ordering_enabled(adapter->pdev)) { >> >> As Alex mentioned there is no need for this check in any form. >> >> The HW defaults to Relaxed Ordering enabled unless it is disabled in >> the PCIe Device Control Register. So the above logic is already done by >HW. >> >> All you have to do is strip the code disabling relaxed ordering. >> > >Hi Tantilov: > >I misunderstood Alex's suggestion, But I still couldn't find the logic >where >the HW disable the Relaxed Ordering when the PCIe Device Control Register >disable it, can you point it out? If you look at the datasheet (82599) - the description of CTRL_EXT.RO_DIS (bit 17, 0b): Relaxed Ordering Disable. When set to 1b, the device does not request any relaxed ordering transactions. When this bit is cleared and the Enable Relaxed Ordering bit in the Device Control register is set, the device requests relaxed ordering transactions per queues as configured in the DCA_RXCTRL[n] and DCA_TXCTRL[n] registers. So if you remove the code that clears the bits in DCA_T/RXCTRL relaxed ordering should be enabled by HW when the bus allows it. Thanks, Emil
On 2017/8/18 13:04, Tantilov, Emil S wrote: >> -----Original Message----- >> From: Ding Tianhong [mailto:dingtianhong@huawei.com] >> Sent: Thursday, August 17, 2017 5:39 PM >> To: Tantilov, Emil S <emil.s.tantilov@intel.com>; davem@davemloft.net; >> Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>; keescook@chromium.org; >> linux-kernel@vger.kernel.org; sparclinux@vger.kernel.org; intel-wired- >> lan@lists.osuosl.org; alexander.duyck@gmail.com; netdev@vger.kernel.org; >> linuxarm@huawei.com >> Subject: Re: [PATCH net v2 2/2] net: ixgbe: Use new >> PCI_DEV_FLAGS_NO_RELAXED_ORDERING flag >> >> >> >> On 2017/8/17 22:17, Tantilov, Emil S wrote: >> >>>> ret_val = ixgbe_start_hw_generic(hw); >>>> >>>> -#ifndef CONFIG_SPARC >>>> - /* Disable relaxed ordering */ >>>> - for (i = 0; ((i < hw->mac.max_tx_queues) && >>>> - (i < IXGBE_DCA_MAX_QUEUES_82598)); i++) { >>>> - regval = IXGBE_READ_REG(hw, IXGBE_DCA_TXCTRL(i)); >>>> - regval &= ~IXGBE_DCA_TXCTRL_DESC_WRO_EN; >>>> - IXGBE_WRITE_REG(hw, IXGBE_DCA_TXCTRL(i), regval); >>>> - } >>>> + if (!pcie_relaxed_ordering_enabled(adapter->pdev)) { >>> >>> As Alex mentioned there is no need for this check in any form. >>> >>> The HW defaults to Relaxed Ordering enabled unless it is disabled in >>> the PCIe Device Control Register. So the above logic is already done by >> HW. >>> >>> All you have to do is strip the code disabling relaxed ordering. >>> >> >> Hi Tantilov: >> >> I misunderstood Alex's suggestion, But I still couldn't find the logic >> where >> the HW disable the Relaxed Ordering when the PCIe Device Control Register >> disable it, can you point it out? > > If you look at the datasheet (82599) - the description of CTRL_EXT.RO_DIS (bit 17, 0b): > > Relaxed Ordering Disable. When set to 1b, the device does not request any relaxed > ordering transactions. When this bit is cleared and the Enable Relaxed Ordering bit in > the Device Control register is set, the device requests relaxed ordering transactions per queues as configured in the DCA_RXCTRL[n] and DCA_TXCTRL[n] registers. > > So if you remove the code that clears the bits in DCA_T/RXCTRL relaxed ordering should > be enabled by HW when the bus allows it. > Great, Thanks for your explanation. > Thanks, > Emil > > > . >
Hi Ding,
[auto build test ERROR on net/master]
url: https://github.com/0day-ci/linux/commits/Ding-Tianhong/Revert-commit-1a8b6d76dc5b-net-add-one-common-config/20170820-053530
config: i386-randconfig-x011-201734 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/net//ethernet/intel/ixgbe/ixgbe_common.c: In function 'ixgbe_start_hw_gen2':
>> drivers/net//ethernet/intel/ixgbe/ixgbe_common.c:354:7: error: implicit declaration of function 'pcie_relaxed_ordering_enabled' [-Werror=implicit-function-declaration]
if (!pcie_relaxed_ordering_enabled(adapter->pdev)) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
--
drivers/net//ethernet/intel/ixgbe/ixgbe_82598.c: In function 'ixgbe_start_hw_82598':
>> drivers/net//ethernet/intel/ixgbe/ixgbe_82598.c:184:7: error: implicit declaration of function 'pcie_relaxed_ordering_enabled' [-Werror=implicit-function-declaration]
if (!pcie_relaxed_ordering_enabled(adapter->pdev)) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/pcie_relaxed_ordering_enabled +354 drivers/net//ethernet/intel/ixgbe/ixgbe_common.c
331
332 /**
333 * ixgbe_start_hw_gen2 - Init sequence for common device family
334 * @hw: pointer to hw structure
335 *
336 * Performs the init sequence common to the second generation
337 * of 10 GbE devices.
338 * Devices in the second generation:
339 * 82599
340 * X540
341 **/
342 s32 ixgbe_start_hw_gen2(struct ixgbe_hw *hw)
343 {
344 u32 i;
345 struct ixgbe_adapter *adapter = hw->back;
346
347 /* Clear the rate limiters */
348 for (i = 0; i < hw->mac.max_tx_queues; i++) {
349 IXGBE_WRITE_REG(hw, IXGBE_RTTDQSEL, i);
350 IXGBE_WRITE_REG(hw, IXGBE_RTTBCNRC, 0);
351 }
352 IXGBE_WRITE_FLUSH(hw);
353
> 354 if (!pcie_relaxed_ordering_enabled(adapter->pdev)) {
355 /* Disable relaxed ordering */
356 for (i = 0; i < hw->mac.max_tx_queues; i++) {
357 u32 regval;
358
359 regval = IXGBE_READ_REG(hw, IXGBE_DCA_TXCTRL_82599(i));
360 regval &= ~IXGBE_DCA_TXCTRL_DESC_WRO_EN;
361 IXGBE_WRITE_REG(hw, IXGBE_DCA_TXCTRL_82599(i), regval);
362 }
363
364 for (i = 0; i < hw->mac.max_rx_queues; i++) {
365 u32 regval;
366
367 regval = IXGBE_READ_REG(hw, IXGBE_DCA_RXCTRL(i));
368 regval &= ~(IXGBE_DCA_RXCTRL_DATA_WRO_EN |
369 IXGBE_DCA_RXCTRL_HEAD_WRO_EN);
370 IXGBE_WRITE_REG(hw, IXGBE_DCA_RXCTRL(i), regval);
371 }
372 }
373
374 return 0;
375 }
376
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_82598.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_82598.c index 523f9d0..d1571e3 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_82598.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_82598.c @@ -175,31 +175,30 @@ static s32 ixgbe_init_phy_ops_82598(struct ixgbe_hw *hw) **/ static s32 ixgbe_start_hw_82598(struct ixgbe_hw *hw) { -#ifndef CONFIG_SPARC - u32 regval; - u32 i; -#endif + u32 regval, i; s32 ret_val; + struct ixgbe_adapter *adapter = hw->back; ret_val = ixgbe_start_hw_generic(hw); -#ifndef CONFIG_SPARC - /* Disable relaxed ordering */ - for (i = 0; ((i < hw->mac.max_tx_queues) && - (i < IXGBE_DCA_MAX_QUEUES_82598)); i++) { - regval = IXGBE_READ_REG(hw, IXGBE_DCA_TXCTRL(i)); - regval &= ~IXGBE_DCA_TXCTRL_DESC_WRO_EN; - IXGBE_WRITE_REG(hw, IXGBE_DCA_TXCTRL(i), regval); - } + if (!pcie_relaxed_ordering_enabled(adapter->pdev)) { + /* Disable relaxed ordering */ + for (i = 0; ((i < hw->mac.max_tx_queues) && + (i < IXGBE_DCA_MAX_QUEUES_82598)); i++) { + regval = IXGBE_READ_REG(hw, IXGBE_DCA_TXCTRL(i)); + regval &= ~IXGBE_DCA_TXCTRL_DESC_WRO_EN; + IXGBE_WRITE_REG(hw, IXGBE_DCA_TXCTRL(i), regval); + } - for (i = 0; ((i < hw->mac.max_rx_queues) && - (i < IXGBE_DCA_MAX_QUEUES_82598)); i++) { - regval = IXGBE_READ_REG(hw, IXGBE_DCA_RXCTRL(i)); - regval &= ~(IXGBE_DCA_RXCTRL_DATA_WRO_EN | - IXGBE_DCA_RXCTRL_HEAD_WRO_EN); - IXGBE_WRITE_REG(hw, IXGBE_DCA_RXCTRL(i), regval); + for (i = 0; ((i < hw->mac.max_rx_queues) && + (i < IXGBE_DCA_MAX_QUEUES_82598)); i++) { + regval = IXGBE_READ_REG(hw, IXGBE_DCA_RXCTRL(i)); + regval &= ~(IXGBE_DCA_RXCTRL_DATA_WRO_EN | + IXGBE_DCA_RXCTRL_HEAD_WRO_EN); + IXGBE_WRITE_REG(hw, IXGBE_DCA_RXCTRL(i), regval); + } } -#endif + if (ret_val) return ret_val; diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c index d4933d2..d1052ee 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c @@ -342,6 +342,7 @@ s32 ixgbe_start_hw_generic(struct ixgbe_hw *hw) s32 ixgbe_start_hw_gen2(struct ixgbe_hw *hw) { u32 i; + struct ixgbe_adapter *adapter = hw->back; /* Clear the rate limiters */ for (i = 0; i < hw->mac.max_tx_queues; i++) { @@ -350,25 +351,26 @@ s32 ixgbe_start_hw_gen2(struct ixgbe_hw *hw) } IXGBE_WRITE_FLUSH(hw); -#ifndef CONFIG_SPARC - /* Disable relaxed ordering */ - for (i = 0; i < hw->mac.max_tx_queues; i++) { - u32 regval; + if (!pcie_relaxed_ordering_enabled(adapter->pdev)) { + /* Disable relaxed ordering */ + for (i = 0; i < hw->mac.max_tx_queues; i++) { + u32 regval; - regval = IXGBE_READ_REG(hw, IXGBE_DCA_TXCTRL_82599(i)); - regval &= ~IXGBE_DCA_TXCTRL_DESC_WRO_EN; - IXGBE_WRITE_REG(hw, IXGBE_DCA_TXCTRL_82599(i), regval); - } + regval = IXGBE_READ_REG(hw, IXGBE_DCA_TXCTRL_82599(i)); + regval &= ~IXGBE_DCA_TXCTRL_DESC_WRO_EN; + IXGBE_WRITE_REG(hw, IXGBE_DCA_TXCTRL_82599(i), regval); + } - for (i = 0; i < hw->mac.max_rx_queues; i++) { - u32 regval; + for (i = 0; i < hw->mac.max_rx_queues; i++) { + u32 regval; - regval = IXGBE_READ_REG(hw, IXGBE_DCA_RXCTRL(i)); - regval &= ~(IXGBE_DCA_RXCTRL_DATA_WRO_EN | - IXGBE_DCA_RXCTRL_HEAD_WRO_EN); - IXGBE_WRITE_REG(hw, IXGBE_DCA_RXCTRL(i), regval); + regval = IXGBE_READ_REG(hw, IXGBE_DCA_RXCTRL(i)); + regval &= ~(IXGBE_DCA_RXCTRL_DATA_WRO_EN | + IXGBE_DCA_RXCTRL_HEAD_WRO_EN); + IXGBE_WRITE_REG(hw, IXGBE_DCA_RXCTRL(i), regval); + } } -#endif + return 0; }
The ixgbe driver use the compile check to determine if it can send TLPs to Root Port with the Relaxed Ordering Attribute set, this is too inconvenient, now the new flag PCI_DEV_FLAGS_NO_RELAXED_ORDERING has been added to the kernel and we could check the bit4 in the PCIe Device Control register to determine whether we should use the Relaxed Ordering Attributes or not, so use this new way in the ixgbe driver. Signed-off-by: Ding Tianhong <dingtianhong@huawei.com> --- drivers/net/ethernet/intel/ixgbe/ixgbe_82598.c | 37 ++++++++++++------------- drivers/net/ethernet/intel/ixgbe/ixgbe_common.c | 32 +++++++++++---------- 2 files changed, 35 insertions(+), 34 deletions(-)