[net-next 0/6][pull request] 10GbE Intel Wired LAN Driver Updates 2018-07-26

[net-next 0/6][pull request] 10GbE Intel Wired LAN Driver Updates 2018-07-26

[Jeff Kirsher]

This series contains updates to ixgbe and igb.

Tony fixes ixgbe to add checks to ensure jumbo frames or LRO get enabled
after an XDP program is loaded.

Shannon Nelson adds the missing security configuration registers to the
ixgbe register dump, which will help in debugging.

Christian Grönke fixes an issue in igb that occurs on SGMII based SPF
mdoules, by reverting changes from 2 previous patches.  The issue was
that initialization would fail on the fore mentioned modules because the
driver would try to reset the PHY before obtaining the PHY address of
the SGMII attached PHY.

Venkatesh Srinivas replaces wmb() with dma_wmb() for doorbell writes,
which avoids SFENCEs before the doorbell writes.

Alex cleans up and refactors ixgbe Tx/Rx shutdown to reduce time needed
to stop the device.  The code refactor allows us to take the completion
time into account when disabling queues, so that on some platforms with
higher completion times, would not result in receive queues disabled
messages.

The following are changes since commit dc66fe43b7ebdb53628dcbc1f8f15de3e000aacf:
  rds: send: Fix dead code in rds_sendmsg
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/next-queue 10GbE

Alexander Duyck (2):
  ixgbe: Reorder Tx/Rx shutdown to reduce time needed to stop device
  ixgbe: Refactor queue disable logic to take completion time into
    account

Christian Grönke (1):
  igb: Remove superfluous reset to PHY and page 0 selection

Shannon Nelson (1):
  ixgbe: add ipsec security registers into ethtool register dump

Tony Nguyen (1):
  ixgbe: Do not allow LRO or MTU change with XDP

Venkatesh Srinivas (1):
  igb: Use dma_wmb() instead of wmb() before doorbell writes

 drivers/net/ethernet/intel/igb/e1000_82575.c  |  12 -
 drivers/net/ethernet/intel/igb/igb_main.c     |   4 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe.h      |   3 +-
 .../net/ethernet/intel/ixgbe/ixgbe_ethtool.c  |  42 +--
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 300 ++++++++++++++----
 5 files changed, 250 insertions(+), 111 deletions(-)

-- 
2.17.1

[net-next 1/6] ixgbe: Do not allow LRO or MTU change with XDP

[Jeff Kirsher]

From: Tony Nguyen <anthony.l.nguyen@intel.com>

XDP does not support jumbo frames or LRO.  These checks are being made
outside the driver when an XDP program is loaded, however, there is
nothing preventing these from changing after an XDP program is loaded.
Add the checks so that while an XDP program is loaded, do not allow MTU
to be changed or LRO to be enabled.

Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 5a6600f7b382..c42256e91997 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -6469,6 +6469,11 @@ static int ixgbe_change_mtu(struct net_device *netdev, int new_mtu)
 {
 	struct ixgbe_adapter *adapter = netdev_priv(netdev);
 
+	if (adapter->xdp_prog) {
+		e_warn(probe, "MTU cannot be changed while XDP program is loaded\n");
+		return -EPERM;
+	}
+
 	/*
 	 * For 82599EB we cannot allow legacy VFs to enable their receive
 	 * paths when MTU greater than 1500 is configured.  So display a
@@ -9407,6 +9412,11 @@ static netdev_features_t ixgbe_fix_features(struct net_device *netdev,
 	if (!(adapter->flags2 & IXGBE_FLAG2_RSC_CAPABLE))
 		features &= ~NETIF_F_LRO;
 
+	if (adapter->xdp_prog && (features & NETIF_F_LRO)) {
+		e_dev_err("LRO is not supported with XDP\n");
+		features &= ~NETIF_F_LRO;
+	}
+
 	return features;
 }
 
-- 
2.17.1

[net-next 2/6] ixgbe: add ipsec security registers into ethtool register dump

[Jeff Kirsher]

From: Shannon Nelson <shannon.nelson@oracle.com>

Add the ixgbe's security configuration registers into
the register dump.

Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
index bd1ba88ec1d5..1d688840cd6c 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
@@ -511,7 +511,7 @@ static void ixgbe_set_msglevel(struct net_device *netdev, u32 data)
 
 static int ixgbe_get_regs_len(struct net_device *netdev)
 {
-#define IXGBE_REGS_LEN  1139
+#define IXGBE_REGS_LEN  1145
 	return IXGBE_REGS_LEN * sizeof(u32);
 }
 
@@ -874,6 +874,14 @@ static void ixgbe_get_regs(struct net_device *netdev,
 	/* X540 specific DCB registers  */
 	regs_buff[1137] = IXGBE_READ_REG(hw, IXGBE_RTTQCNCR);
 	regs_buff[1138] = IXGBE_READ_REG(hw, IXGBE_RTTQCNTG);
+
+	/* Security config registers */
+	regs_buff[1139] = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
+	regs_buff[1140] = IXGBE_READ_REG(hw, IXGBE_SECTXSTAT);
+	regs_buff[1141] = IXGBE_READ_REG(hw, IXGBE_SECTXBUFFAF);
+	regs_buff[1142] = IXGBE_READ_REG(hw, IXGBE_SECTXMINIFG);
+	regs_buff[1143] = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
+	regs_buff[1144] = IXGBE_READ_REG(hw, IXGBE_SECRXSTAT);
 }
 
 static int ixgbe_get_eeprom_len(struct net_device *netdev)
-- 
2.17.1

[net-next 3/6] igb: Remove superfluous reset to PHY and page 0 selection

[Jeff Kirsher]

From: Christian Grönke <c.groenke@infodas.de>

This patch reverts two previous applied patches to fix an issue
that appeared when using SGMII based SFP modules. In the current
state the driver will try to reset the PHY before obtaining the
phy_addr of the SGMII attached PHY. That leads to an error in
e1000_write_phy_reg_sgmii_82575. Causing the initialization to
fail:

    igb: Intel(R) Gigabit Ethernet Network Driver - version 5.4.0-k
    igb: Copyright (c) 2007-2014 Intel Corporation.
    igb: probe of ????:??:??.? failed with error -3

The patches being reverted are:

    commit 182785335447957409282ca745aa5bc3968facee
    Author: Aaron Sierra <asierra@xes-inc.com>
    Date:   Tue Nov 29 10:03:56 2016 -0600

        igb: reset the PHY before reading the PHY ID

    commit 440aeca4b9858248d8f16d724d9fa87a4f65fa33
    Author: Matwey V Kornilov <matwey@sai.msu.ru>
    Date:   Thu Nov 24 13:32:48 2016 +0300

         igb: Explicitly select page 0 at initialization

The first reverted patch directly causes the problem mentioned above.
In case of SGMII the phy_addr is not known at this point and will
only be obtained by 'igb_get_phy_id_82575' further down in the code.
The second removed patch selects forces selection of page 0 in the
PHY. Something that the reset tries to address as well.

As pointed out by Alexander Duzck, the patch below fixes the same
issue but in the proper location:

    commit 4e684f59d760a2c7c716bb60190783546e2d08a1
    Author: Chris J Arges <christopherarges@gmail.com>
    Date:   Wed Nov 2 09:13:42 2016 -0500

        igb: Workaround for igb i210 firmware issue

Reverts: 440aeca4b9858248d8f16d724d9fa87a4f65fa33.
Reverts: 182785335447957409282ca745aa5bc3968facee.

Signed-off-by: Christian Grönke <c.groenke@infodas.de>
Reviewed-by: Alexander Duyck <alexander.h.duyck@intel.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/igb/e1000_82575.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.c b/drivers/net/ethernet/intel/igb/e1000_82575.c
index b13b42e5a1d9..a795c07d0df7 100644
--- a/drivers/net/ethernet/intel/igb/e1000_82575.c
+++ b/drivers/net/ethernet/intel/igb/e1000_82575.c
@@ -225,19 +225,7 @@ static s32 igb_init_phy_params_82575(struct e1000_hw *hw)
 	hw->bus.func = (rd32(E1000_STATUS) & E1000_STATUS_FUNC_MASK) >>
 			E1000_STATUS_FUNC_SHIFT;
 
-	/* Make sure the PHY is in a good state. Several people have reported
-	 * firmware leaving the PHY's page select register set to something
-	 * other than the default of zero, which causes the PHY ID read to
-	 * access something other than the intended register.
-	 */
-	ret_val = hw->phy.ops.reset(hw);
-	if (ret_val) {
-		hw_dbg("Error resetting the PHY.\n");
-		goto out;
-	}
-
 	/* Set phy->phy_addr and phy->id. */
-	igb_write_phy_reg_82580(hw, I347AT4_PAGE_SELECT, 0);
 	ret_val = igb_get_phy_id_82575(hw);
 	if (ret_val)
 		return ret_val;
-- 
2.17.1

[net-next 4/6] igb: Use dma_wmb() instead of wmb() before doorbell writes

[Jeff Kirsher]

From: Venkatesh Srinivas <venkateshs@google.com>

igb writes to doorbells to post transmit and receive descriptors;
after writing descriptors to memory but before writing to doorbells,
use dma_wmb() rather than wmb(). wmb() is more heavyweight than
necessary before doorbell writes.

On x86, this avoids SFENCEs before doorbell writes in both the
tx and rx refill paths.

Signed-off-by: Venkatesh Srinivas <venkateshs@google.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index e3a0c02721c9..25720d95d4ea 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -6031,7 +6031,7 @@ static int igb_tx_map(struct igb_ring *tx_ring,
 	 * We also need this memory barrier to make certain all of the
 	 * status bits have been updated before next_to_watch is written.
 	 */
-	wmb();
+	dma_wmb();
 
 	/* set next_to_watch value indicating a packet is present */
 	first->next_to_watch = tx_desc;
@@ -8531,7 +8531,7 @@ void igb_alloc_rx_buffers(struct igb_ring *rx_ring, u16 cleaned_count)
 		 * applicable for weak-ordered memory model archs,
 		 * such as IA-64).
 		 */
-		wmb();
+		dma_wmb();
 		writel(i, rx_ring->tail);
 	}
 }
-- 
2.17.1

[net-next 5/6] ixgbe: Reorder Tx/Rx shutdown to reduce time needed to stop device

[Jeff Kirsher]

From: Alexander Duyck <alexander.h.duyck@intel.com>

This change is meant to help reduce the time needed to shutdown the
transmit and receive paths for the device. Specifically what we now do
after this patch is disable the transmit path first at the netdev level,
and then work on disabling the Rx. This way while we are waiting on the Rx
queues to be disabled the Tx queues have an opportunity to drain out.

In addition I have dropped the 10ms timeout that was left in the ixgbe_down
function that seems to have been carried through from back in e1000 as far
as I can tell. We shouldn't need it since we don't actually disable the Tx
until much later and we have additional logic in place for verifying the Tx
queues have been disabled.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
Tested-by: Don Buchholz <donald.buchholz@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index c42256e91997..aa4f05c36260 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -5814,6 +5814,13 @@ void ixgbe_down(struct ixgbe_adapter *adapter)
 	if (test_and_set_bit(__IXGBE_DOWN, &adapter->state))
 		return; /* do nothing if already down */
 
+	/* Shut off incoming Tx traffic */
+	netif_tx_stop_all_queues(netdev);
+
+	/* call carrier off first to avoid false dev_watchdog timeouts */
+	netif_carrier_off(netdev);
+	netif_tx_disable(netdev);
+
 	/* disable receives */
 	hw->mac.ops.disable_rx(hw);
 
@@ -5822,16 +5829,9 @@ void ixgbe_down(struct ixgbe_adapter *adapter)
 		/* this call also flushes the previous write */
 		ixgbe_disable_rx_queue(adapter, adapter->rx_ring[i]);
 
-	usleep_range(10000, 20000);
-
 	/* synchronize_sched() needed for pending XDP buffers to drain */
 	if (adapter->xdp_ring[0])
 		synchronize_sched();
-	netif_tx_stop_all_queues(netdev);
-
-	/* call carrier off first to avoid false dev_watchdog timeouts */
-	netif_carrier_off(netdev);
-	netif_tx_disable(netdev);
 
 	ixgbe_irq_disable(adapter);
 
-- 
2.17.1

Re: [net-next 0/6][pull request] 10GbE Intel Wired LAN Driver Updates 2018-07-26

[David Miller]

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Thu, 26 Jul 2018 10:40:44 -0700

> This series contains updates to ixgbe and igb.

Pulled, thanks Jeff.

Re: [net-next 1/6] ixgbe: Do not allow LRO or MTU change with XDP

[Jakub Kicinski]

On Thu, 26 Jul 2018 10:40:45 -0700, Jeff Kirsher wrote:
> From: Tony Nguyen <anthony.l.nguyen@intel.com>
> 
> XDP does not support jumbo frames or LRO.  These checks are being made
> outside the driver when an XDP program is loaded, however, there is
> nothing preventing these from changing after an XDP program is loaded.
> Add the checks so that while an XDP program is loaded, do not allow MTU
> to be changed or LRO to be enabled.
> 
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 5a6600f7b382..c42256e91997 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -6469,6 +6469,11 @@ static int ixgbe_change_mtu(struct net_device *netdev, int new_mtu)
>  {
>  	struct ixgbe_adapter *adapter = netdev_priv(netdev);
>  
> +	if (adapter->xdp_prog) {
> +		e_warn(probe, "MTU cannot be changed while XDP program is loaded\n");
> +		return -EPERM;

EPERM looks wrong, EINVAL is common.  Also most drivers will just check
the bounds like you do in ixgbe_xdp_setup(), allowing the change if new
MTU still fits constraints.

FWIW are the IXGBE_FLAG_SRIOV_ENABLED and IXGBE_FLAG_DCB_ENABLED flag
changes also covered while xdp is enabled?  Quick grep doesn't reveal
them being checked against xdp_prog other than in ixgbe_xdp_setup().

> +	}
> +
>  	/*
>  	 * For 82599EB we cannot allow legacy VFs to enable their receive
>  	 * paths when MTU greater than 1500 is configured.  So display a
> @@ -9407,6 +9412,11 @@ static netdev_features_t ixgbe_fix_features(struct net_device *netdev,
>  	if (!(adapter->flags2 & IXGBE_FLAG2_RSC_CAPABLE))
>  		features &= ~NETIF_F_LRO;
>  
> +	if (adapter->xdp_prog && (features & NETIF_F_LRO)) {
> +		e_dev_err("LRO is not supported with XDP\n");
> +		features &= ~NETIF_F_LRO;
> +	}
> +
>  	return features;
>  }
>  

Re: [net-next 1/6] ixgbe: Do not allow LRO or MTU change with XDP

[Jakub Kicinski]

On Thu, 26 Jul 2018 14:21:08 -0700, Jakub Kicinski wrote:
> On Thu, 26 Jul 2018 10:40:45 -0700, Jeff Kirsher wrote:
> > From: Tony Nguyen <anthony.l.nguyen@intel.com>
> > 
> > XDP does not support jumbo frames or LRO.  These checks are being made
> > outside the driver when an XDP program is loaded, however, there is
> > nothing preventing these from changing after an XDP program is loaded.
> > Add the checks so that while an XDP program is loaded, do not allow MTU
> > to be changed or LRO to be enabled.
> > 
> > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> > Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > ---
> >  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > index 5a6600f7b382..c42256e91997 100644
> > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > @@ -6469,6 +6469,11 @@ static int ixgbe_change_mtu(struct net_device *netdev, int new_mtu)
> >  {
> >  	struct ixgbe_adapter *adapter = netdev_priv(netdev);
> >  
> > +	if (adapter->xdp_prog) {
> > +		e_warn(probe, "MTU cannot be changed while XDP program is loaded\n");
> > +		return -EPERM;  
> 
> EPERM looks wrong, EINVAL is common.  Also most drivers will just check
> the bounds like you do in ixgbe_xdp_setup(), allowing the change if new
> MTU still fits constraints.
> 
> FWIW are the IXGBE_FLAG_SRIOV_ENABLED and IXGBE_FLAG_DCB_ENABLED flag
> changes also covered while xdp is enabled?  Quick grep doesn't reveal
> them being checked against xdp_prog other than in ixgbe_xdp_setup().

Ah, I didn't make the review in time :)  Could you follow up?

RE: [net-next 1/6] ixgbe: Do not allow LRO or MTU change with XDP

[Nguyen, Anthony L]

> From: Jakub Kicinski [mailto:jakub.kicinski@netronome.com]
> Sent: Thursday, July 26, 2018 2:47 PM
> On Thu, 26 Jul 2018 14:21:08 -0700, Jakub Kicinski wrote:
> > On Thu, 26 Jul 2018 10:40:45 -0700, Jeff Kirsher wrote:
> > > From: Tony Nguyen <anthony.l.nguyen@intel.com>
> > >
> > > XDP does not support jumbo frames or LRO.  These checks are being
> > > made outside the driver when an XDP program is loaded, however,
> > > there is nothing preventing these from changing after an XDP program is
> loaded.
> > > Add the checks so that while an XDP program is loaded, do not allow
> > > MTU to be changed or LRO to be enabled.
> > >
> > > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> > > Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
> > > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > > ---
> > >  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > >
> > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > > b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > > index 5a6600f7b382..c42256e91997 100644
> > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > > @@ -6469,6 +6469,11 @@ static int ixgbe_change_mtu(struct net_device
> > > *netdev, int new_mtu)  {
> > >  	struct ixgbe_adapter *adapter = netdev_priv(netdev);
> > >
> > > +	if (adapter->xdp_prog) {
> > > +		e_warn(probe, "MTU cannot be changed while XDP program is
> loaded\n");
> > > +		return -EPERM;
> >
> > EPERM looks wrong, EINVAL is common.  Also most drivers will just
> > check the bounds like you do in ixgbe_xdp_setup(), allowing the change
> > if new MTU still fits constraints.
> >
> > FWIW are the IXGBE_FLAG_SRIOV_ENABLED and IXGBE_FLAG_DCB_ENABLED
> flag
> > changes also covered while xdp is enabled?  Quick grep doesn't reveal
> > them being checked against xdp_prog other than in ixgbe_xdp_setup().
> 
> Ah, I didn't make the review in time :)  Could you follow up?

Yes, I'll make a follow-on patch to address your comments.