[Intel-wired-lan] [next PATCH S80-V3 01/11] i40e: use the safe hash table iterator when deleting mac filters

[Intel-wired-lan] [next PATCH S80-V3 01/11] i40e: use the safe hash table iterator when deleting mac filters

[Alice Michael]

From: Lihong Yang <lihong.yang@intel.com>

This patch replaces hash_for_each function with hash_for_each_safe
when calling  __i40e_del_filter. The hash_for_each_safe function is
the right one to use when iterating over a hash table to safely remove
a hash entry. Otherwise, incorrect values may be read from freed memory.

Detected by CoverityScan, CID 1402048 Read from pointer after free

Signed-off-by: Lihong Yang <lihong.yang@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
index 8bedc74c..d8fbdda 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
@@ -2880,6 +2880,7 @@ int i40e_ndo_set_vf_mac(struct net_device *netdev, int vf_id, u8 *mac)
 	struct i40e_mac_filter *f;
 	struct i40e_vf *vf;
 	int ret = 0;
+	struct hlist_node *h;
 	int bkt;
 
 	/* validate the request */
@@ -2918,7 +2919,7 @@ int i40e_ndo_set_vf_mac(struct net_device *netdev, int vf_id, u8 *mac)
 	/* Delete all the filters for this VSI - we're going to kill it
 	 * anyway.
 	 */
-	hash_for_each(vsi->mac_filter_hash, bkt, f, hlist)
+	hash_for_each_safe(vsi->mac_filter_hash, bkt, h, f, hlist)
 		__i40e_del_filter(vsi, f);
 
 	spin_unlock_bh(&vsi->mac_filter_hash_lock);
-- 
2.9.4

[Intel-wired-lan] [next PATCH S80-V3 02/11] i40evf: fix mac filter removal timing issue

[Alice Michael]

From: Alan Brady <alan.brady@intel.com>

Due to the asynchronous nature in which mac filters are added and
deleted, there exists a bug in which filters are erroneously removed if
removed then added again quickly.

The events are as such:
    - filter marked for removal
    - same filter is re-added before watchdog that cleans up filters
    - we skip re-adding the filter because we have it already in the
list
    - watchdog filter cleanup kicks off and filter is removed

So when we were re-adding the same filter, it didn't actually get added
because it already existed in the list, but was marked for removal and
had yet to actually be removed.

This patch fixes the issue by making sure that when adding a filter, if
we find it already existing in our list, make sure it is not marked to
be removed.

Signed-off-by: Alan Brady <alan.brady@intel.com>
---
 drivers/net/ethernet/intel/i40evf/i40evf_main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/intel/i40evf/i40evf_main.c b/drivers/net/ethernet/intel/i40evf/i40evf_main.c
index c5371da..4496c48 100644
--- a/drivers/net/ethernet/intel/i40evf/i40evf_main.c
+++ b/drivers/net/ethernet/intel/i40evf/i40evf_main.c
@@ -880,6 +880,8 @@ i40evf_mac_filter *i40evf_add_filter(struct i40evf_adapter *adapter,
 		list_add_tail(&f->list, &adapter->mac_filter_list);
 		f->add = true;
 		adapter->aq_required |= I40EVF_FLAG_AQ_ADD_MAC_FILTER;
+	} else {
+		f->remove = false;
 	}
 
 	clear_bit(__I40EVF_IN_CRITICAL_TASK, &adapter->crit_section);
-- 
2.9.4

[Intel-wired-lan] [next PATCH S80-V3 03/11] i40e/i40evf: fix incorrect default ITR values on driver load

[Alice Michael]

From: Jacob Keller <jacob.e.keller@intel.com>

The ITR register expects to be programmed in units of 2 microseconds.
Because of this, all of the drivers I40E_ITR_* constants are in terms of
this 2 microsecond register.

Unfortunately, the rx_itr_default value is expected to be programmed in
microseconds.

Effectively the driver defaults to an ITR value of half the expected
value (in terms of minimum microseconds between interrupts).

Fix this by changing the default values to be calculated using
ITR_REG_TO_USEC macro which indicates that we're converting from the
register units into microseconds.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c     | 4 ++--
 drivers/net/ethernet/intel/i40e/i40e_txrx.h     | 6 ++++--
 drivers/net/ethernet/intel/i40evf/i40e_txrx.h   | 6 ++++--
 drivers/net/ethernet/intel/i40evf/i40evf_main.c | 4 ++--
 4 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 76b03f7..cc5327c 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -8983,8 +8983,8 @@ static int i40e_sw_init(struct i40e_pf *pf)
 		    I40E_FLAG_MSIX_ENABLED;
 
 	/* Set default ITR */
-	pf->rx_itr_default = I40E_ITR_DYNAMIC | I40E_ITR_RX_DEF;
-	pf->tx_itr_default = I40E_ITR_DYNAMIC | I40E_ITR_TX_DEF;
+	pf->rx_itr_default = I40E_ITR_RX_DEF;
+	pf->tx_itr_default = I40E_ITR_TX_DEF;
 
 	/* Depending on PF configurations, it is possible that the RSS
 	 * maximum might end up larger than the available queues
diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.h b/drivers/net/ethernet/intel/i40e/i40e_txrx.h
index a4e3e66..c3156aa 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.h
@@ -38,8 +38,10 @@
 #define I40E_ITR_8K                0x003E
 #define I40E_ITR_4K                0x007A
 #define I40E_MAX_INTRL             0x3B    /* reg uses 4 usec resolution */
-#define I40E_ITR_RX_DEF            I40E_ITR_20K
-#define I40E_ITR_TX_DEF            I40E_ITR_20K
+#define I40E_ITR_RX_DEF            (ITR_REG_TO_USEC(I40E_ITR_20K) | \
+				    I40E_ITR_DYNAMIC)
+#define I40E_ITR_TX_DEF            (ITR_REG_TO_USEC(I40E_ITR_20K) | \
+				    I40E_ITR_DYNAMIC)
 #define I40E_ITR_DYNAMIC           0x8000  /* use top bit as a flag */
 #define I40E_MIN_INT_RATE          250     /* ~= 1000000 / (I40E_MAX_ITR * 2) */
 #define I40E_MAX_INT_RATE          500000  /* == 1000000 / (I40E_MIN_ITR * 2) */
diff --git a/drivers/net/ethernet/intel/i40evf/i40e_txrx.h b/drivers/net/ethernet/intel/i40evf/i40e_txrx.h
index d8ca802..8f9830d 100644
--- a/drivers/net/ethernet/intel/i40evf/i40e_txrx.h
+++ b/drivers/net/ethernet/intel/i40evf/i40e_txrx.h
@@ -38,8 +38,10 @@
 #define I40E_ITR_8K                0x003E
 #define I40E_ITR_4K                0x007A
 #define I40E_MAX_INTRL             0x3B    /* reg uses 4 usec resolution */
-#define I40E_ITR_RX_DEF            I40E_ITR_20K
-#define I40E_ITR_TX_DEF            I40E_ITR_20K
+#define I40E_ITR_RX_DEF            (ITR_REG_TO_USEC(I40E_ITR_20K) | \
+				    I40E_ITR_DYNAMIC)
+#define I40E_ITR_TX_DEF            (ITR_REG_TO_USEC(I40E_ITR_20K) | \
+				    I40E_ITR_DYNAMIC)
 #define I40E_ITR_DYNAMIC           0x8000  /* use top bit as a flag */
 #define I40E_MIN_INT_RATE          250     /* ~= 1000000 / (I40E_MAX_ITR * 2) */
 #define I40E_MAX_INT_RATE          500000  /* == 1000000 / (I40E_MIN_ITR * 2) */
diff --git a/drivers/net/ethernet/intel/i40evf/i40evf_main.c b/drivers/net/ethernet/intel/i40evf/i40evf_main.c
index 4496c48..d621daa 100644
--- a/drivers/net/ethernet/intel/i40evf/i40evf_main.c
+++ b/drivers/net/ethernet/intel/i40evf/i40evf_main.c
@@ -1223,7 +1223,7 @@ static int i40evf_alloc_queues(struct i40evf_adapter *adapter)
 		tx_ring->netdev = adapter->netdev;
 		tx_ring->dev = &adapter->pdev->dev;
 		tx_ring->count = adapter->tx_desc_count;
-		tx_ring->tx_itr_setting = (I40E_ITR_DYNAMIC | I40E_ITR_TX_DEF);
+		tx_ring->tx_itr_setting = I40E_ITR_TX_DEF;
 		if (adapter->flags & I40EVF_FLAG_WB_ON_ITR_CAPABLE)
 			tx_ring->flags |= I40E_TXR_FLAGS_WB_ON_ITR;
 
@@ -1232,7 +1232,7 @@ static int i40evf_alloc_queues(struct i40evf_adapter *adapter)
 		rx_ring->netdev = adapter->netdev;
 		rx_ring->dev = &adapter->pdev->dev;
 		rx_ring->count = adapter->rx_desc_count;
-		rx_ring->rx_itr_setting = (I40E_ITR_DYNAMIC | I40E_ITR_RX_DEF);
+		rx_ring->rx_itr_setting = I40E_ITR_RX_DEF;
 	}
 
 	adapter->num_active_queues = num_active_queues;
-- 
2.9.4

[Intel-wired-lan] [next PATCH S80-V3 04/11] i40e/i40evf: always set the CLEARPBA flag when re-enabling interrupts

[Alice Michael]

From: Jacob Keller <jacob.e.keller@intel.com>

In the past we changed driver behavior to not clear the PBA when
re-enabling interrupts. This change was motivated by the flawed belief
that clearing the PBA would cause a lost interrupt if a receive
interrupt occurred while interrupts were disabled.

According to empirical testing this isn't the case. Additionally, the
data sheet specifically says that we should set the CLEARPBA bit when
re-enabling interrupts in a polling setup.

This reverts commit 40d72a509862 ("i40e/i40evf: don't lose interrupts")

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e.h             |  5 +----
 drivers/net/ethernet/intel/i40e/i40e_main.c        | 11 +++++------
 drivers/net/ethernet/intel/i40e/i40e_txrx.c        |  6 ++----
 drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c |  2 +-
 drivers/net/ethernet/intel/i40evf/i40e_txrx.c      |  4 +---
 5 files changed, 10 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e.h b/drivers/net/ethernet/intel/i40e/i40e.h
index 18c453a..46df389 100644
--- a/drivers/net/ethernet/intel/i40e/i40e.h
+++ b/drivers/net/ethernet/intel/i40e/i40e.h
@@ -949,9 +949,6 @@ static inline void i40e_irq_dynamic_enable(struct i40e_vsi *vsi, int vector)
 	struct i40e_hw *hw = &pf->hw;
 	u32 val;
 
-	/* definitely clear the PBA here, as this function is meant to
-	 * clean out all previous interrupts AND enable the interrupt
-	 */
 	val = I40E_PFINT_DYN_CTLN_INTENA_MASK |
 	      I40E_PFINT_DYN_CTLN_CLEARPBA_MASK |
 	      (I40E_ITR_NONE << I40E_PFINT_DYN_CTLN_ITR_INDX_SHIFT);
@@ -960,7 +957,7 @@ static inline void i40e_irq_dynamic_enable(struct i40e_vsi *vsi, int vector)
 }
 
 void i40e_irq_dynamic_disable_icr0(struct i40e_pf *pf);
-void i40e_irq_dynamic_enable_icr0(struct i40e_pf *pf, bool clearpba);
+void i40e_irq_dynamic_enable_icr0(struct i40e_pf *pf);
 int i40e_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd);
 int i40e_open(struct net_device *netdev);
 int i40e_close(struct net_device *netdev);
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index cc5327c..3d9b65f 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -3403,15 +3403,14 @@ void i40e_irq_dynamic_disable_icr0(struct i40e_pf *pf)
 /**
  * i40e_irq_dynamic_enable_icr0 - Enable default interrupt generation for icr0
  * @pf: board private structure
- * @clearpba: true when all pending interrupt events should be cleared
  **/
-void i40e_irq_dynamic_enable_icr0(struct i40e_pf *pf, bool clearpba)
+void i40e_irq_dynamic_enable_icr0(struct i40e_pf *pf)
 {
 	struct i40e_hw *hw = &pf->hw;
 	u32 val;
 
 	val = I40E_PFINT_DYN_CTL0_INTENA_MASK   |
-	      (clearpba ? I40E_PFINT_DYN_CTL0_CLEARPBA_MASK : 0) |
+	      I40E_PFINT_DYN_CTL0_CLEARPBA_MASK |
 	      (I40E_ITR_NONE << I40E_PFINT_DYN_CTL0_ITR_INDX_SHIFT);
 
 	wr32(hw, I40E_PFINT_DYN_CTL0, val);
@@ -3597,7 +3596,7 @@ static int i40e_vsi_enable_irq(struct i40e_vsi *vsi)
 		for (i = 0; i < vsi->num_q_vectors; i++)
 			i40e_irq_dynamic_enable(vsi, i);
 	} else {
-		i40e_irq_dynamic_enable_icr0(pf, true);
+		i40e_irq_dynamic_enable_icr0(pf);
 	}
 
 	i40e_flush(&pf->hw);
@@ -3746,7 +3745,7 @@ static irqreturn_t i40e_intr(int irq, void *data)
 	wr32(hw, I40E_PFINT_ICR0_ENA, ena_mask);
 	if (!test_bit(__I40E_DOWN, pf->state)) {
 		i40e_service_event_schedule(pf);
-		i40e_irq_dynamic_enable_icr0(pf, false);
+		i40e_irq_dynamic_enable_icr0(pf);
 	}
 
 	return ret;
@@ -8455,7 +8454,7 @@ static int i40e_setup_misc_vector(struct i40e_pf *pf)
 
 	i40e_flush(hw);
 
-	i40e_irq_dynamic_enable_icr0(pf, true);
+	i40e_irq_dynamic_enable_icr0(pf);
 
 	return err;
 }
diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
index 53e1998..0e9a910 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -2201,9 +2201,7 @@ static u32 i40e_buildreg_itr(const int type, const u16 itr)
 	u32 val;
 
 	val = I40E_PFINT_DYN_CTLN_INTENA_MASK |
-	      /* Don't clear PBA because that can cause lost interrupts that
-	       * came in while we were cleaning/polling
-	       */
+	      I40E_PFINT_DYN_CTLN_CLEARPBA_MASK |
 	      (type << I40E_PFINT_DYN_CTLN_ITR_INDX_SHIFT) |
 	      (itr << I40E_PFINT_DYN_CTLN_INTERVAL_SHIFT);
 
@@ -2240,7 +2238,7 @@ static inline void i40e_update_enable_itr(struct i40e_vsi *vsi,
 
 	/* If we don't have MSIX, then we only need to re-enable icr0 */
 	if (!(vsi->back->flags & I40E_FLAG_MSIX_ENABLED)) {
-		i40e_irq_dynamic_enable_icr0(vsi->back, false);
+		i40e_irq_dynamic_enable_icr0(vsi->back);
 		return;
 	}
 
diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
index d8fbdda..c7f0a72 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
@@ -1355,7 +1355,7 @@ int i40e_alloc_vfs(struct i40e_pf *pf, u16 num_alloc_vfs)
 		i40e_free_vfs(pf);
 err_iov:
 	/* Re-enable interrupt 0. */
-	i40e_irq_dynamic_enable_icr0(pf, false);
+	i40e_irq_dynamic_enable_icr0(pf);
 	return ret;
 }
 
diff --git a/drivers/net/ethernet/intel/i40evf/i40e_txrx.c b/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
index 8167617..4ae054d 100644
--- a/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
@@ -1409,9 +1409,7 @@ static u32 i40e_buildreg_itr(const int type, const u16 itr)
 	u32 val;
 
 	val = I40E_VFINT_DYN_CTLN1_INTENA_MASK |
-	      /* Don't clear PBA because that can cause lost interrupts that
-	       * came in while we were cleaning/polling
-	       */
+	      I40E_VFINT_DYN_CTLN1_CLEARPBA_MASK |
 	      (type << I40E_VFINT_DYN_CTLN1_ITR_INDX_SHIFT) |
 	      (itr << I40E_VFINT_DYN_CTLN1_INTERVAL_SHIFT);
 
-- 
2.9.4

[Intel-wired-lan] [next PATCH S80-V3 05/11] i40e: reduce lrxqthresh from 2 to 1

[Alice Michael]

From: Jacob Keller <jacob.e.keller@intel.com>

The lrxq thresh value tells hardware to immediately interrupt when there
are fewer than N*64 packets left in the ring.

Counter intuitively, empirical testing has shown that decreasing this
value from 2 to 1, and thus changing from an immediate interrupt at
fewer than 128 descriptors down to 64 descriptors causes a small
increase in the maximum total packets per second we can receive. This
increase occurs even when we're polling with interrupts masked, as the
hardware must still handle interrupts internally even if we've disabled
them in software.

Also reduce the value for any VFs we allocate.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c        | 2 +-
 drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 3d9b65f..a47f4ed 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -3030,7 +3030,7 @@ static int i40e_configure_rx_ring(struct i40e_ring *ring)
 	if (hw->revision_id == 0)
 		rx_ctx.lrxqthresh = 0;
 	else
-		rx_ctx.lrxqthresh = 2;
+		rx_ctx.lrxqthresh = 1;
 	rx_ctx.crcstrip = 1;
 	rx_ctx.l2tsel = 1;
 	/* this controls whether VLAN is stripped from inner headers */
diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
index c7f0a72..7dc042b 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
@@ -636,7 +636,7 @@ static int i40e_config_vsi_rx_queue(struct i40e_vf *vf, u16 vsi_id,
 	rx_ctx.dsize = 1;
 
 	/* default values */
-	rx_ctx.lrxqthresh = 2;
+	rx_ctx.lrxqthresh = 1;
 	rx_ctx.crcstrip = 1;
 	rx_ctx.prefena = 1;
 	rx_ctx.l2tsel = 1;
-- 
2.9.4

[Intel-wired-lan] [next PATCH S80-V3 06/11] i40e/i40evf: bump tail only in multiples of 8

[Alice Michael]

From: Jacob Keller <jacob.e.keller@intel.com>

Hardware only fetches descriptors on cachelines of 8, essentially
ignoring the lower 3 bits of the tail register. Thus, it is pointless to
bump tail by an unaligned access as the hardware will ignore some of the
new descriptors we allocated. Thus, it's ideal if we can ensure tail
writes are always aligned to 8.

At first, it seems like we'd already do this, since we allocate
descriptors in batches which are a multiple of 8. Since we'd always
increment by a multiple of 8, it seems like the value should always be
aligned.

However, this ignores allocation failures. If we fail to allocate
a buffer, our tail register will become unaligned. Once it has become
unaligned it will essentially be stuck unaligned until a buffer
allocation happens to fail at the exact amount necessary to re-align it.

We can do better, by simply rounding down the number of buffers we're
about to allocate (cleaned_count) such that "next_to_clean
+ cleaned_count" is rounded to the nearest multiple of 8.

We do this by calculating how far off that value is and subtracting it
from the cleaned_count. This essentially defers allocation of buffers if
they're going to be ignored by hardware anyways, and re-aligns our
next_to_use and tail values after a failure to allocate a descriptor.

This calculation ensures that we always align the tail writes in a way
the hardware expects and don't unnecessarily allocate buffers which
won't be fetched immediately.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_txrx.c   | 9 +++++++++
 drivers/net/ethernet/intel/i40evf/i40e_txrx.c | 9 +++++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
index 0e9a910..94311e3 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -1372,6 +1372,15 @@ bool i40e_alloc_rx_buffers(struct i40e_ring *rx_ring, u16 cleaned_count)
 	union i40e_rx_desc *rx_desc;
 	struct i40e_rx_buffer *bi;
 
+	/* Hardware only fetches new descriptors in cache lines of 8,
+	 * essentially ignoring the lower 3 bits of the tail register. We want
+	 * to ensure our tail writes are aligned to avoid unnecessary work. We
+	 * can't simply round down the cleaned count, since we might fail to
+	 * allocate some buffers. What we really want is to ensure that
+	 * next_to_used + cleaned_count produces an aligned value.
+	 */
+	cleaned_count -= (ntu + cleaned_count) & 0x7;
+
 	/* do nothing if no valid netdev defined */
 	if (!rx_ring->netdev || !cleaned_count)
 		return false;
diff --git a/drivers/net/ethernet/intel/i40evf/i40e_txrx.c b/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
index 4ae054d..212fc1f 100644
--- a/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
@@ -711,6 +711,15 @@ bool i40evf_alloc_rx_buffers(struct i40e_ring *rx_ring, u16 cleaned_count)
 	union i40e_rx_desc *rx_desc;
 	struct i40e_rx_buffer *bi;
 
+	/* Hardware only fetches new descriptors in cache lines of 8,
+	 * essentially ignoring the lower 3 bits of the tail register. We want
+	 * to ensure our tail writes are aligned to avoid unnecessary work. We
+	 * can't simply round down the cleaned count, since we might fail to
+	 * allocate some buffers. What we really want is to ensure that
+	 * next_to_used + cleaned_count produces an aligned value.
+	 */
+	cleaned_count -= (ntu + cleaned_count) & 0x7;
+
 	/* do nothing if no valid netdev defined */
 	if (!rx_ring->netdev || !cleaned_count)
 		return false;
-- 
2.9.4

[Intel-wired-lan] [next PATCH S80-V3 07/11] i40e/i40evf: bundle more descriptors when allocating buffers

[Alice Michael]

From: Jacob Keller <jacob.e.keller@intel.com>

Double the number of descriptors we'll bundle into one tail bump when
receiving. Empirical testing has shown that we reduce CPU utilization
and don't appear to reduce throughput or packet rate. 32 seems to be the
sweet spot, as it's half the default polling budget, so we'd essentially
reduce from 4 tail writes when polling down to 2. Increasing this up to
64 appears to have negative impacts as it may become possible that we
don't bump the tail each time we get polled, which could cause a long
delay between returning descriptors to the hardware.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_txrx.h   | 2 +-
 drivers/net/ethernet/intel/i40evf/i40e_txrx.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.h b/drivers/net/ethernet/intel/i40e/i40e_txrx.h
index c3156aa..ff57ae4 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.h
@@ -208,7 +208,7 @@ static inline bool i40e_test_staterr(union i40e_rx_desc *rx_desc,
 }
 
 /* How many Rx Buffers do we bundle into one write to the hardware ? */
-#define I40E_RX_BUFFER_WRITE	16	/* Must be power of 2 */
+#define I40E_RX_BUFFER_WRITE	32	/* Must be power of 2 */
 #define I40E_RX_INCREMENT(r, i) \
 	do {					\
 		(i)++;				\
diff --git a/drivers/net/ethernet/intel/i40evf/i40e_txrx.h b/drivers/net/ethernet/intel/i40evf/i40e_txrx.h
index 8f9830d..8d26c85 100644
--- a/drivers/net/ethernet/intel/i40evf/i40e_txrx.h
+++ b/drivers/net/ethernet/intel/i40evf/i40e_txrx.h
@@ -191,7 +191,7 @@ static inline bool i40e_test_staterr(union i40e_rx_desc *rx_desc,
 }
 
 /* How many Rx Buffers do we bundle into one write to the hardware ? */
-#define I40E_RX_BUFFER_WRITE	16	/* Must be power of 2 */
+#define I40E_RX_BUFFER_WRITE	32	/* Must be power of 2 */
 #define I40E_RX_INCREMENT(r, i) \
 	do {					\
 		(i)++;				\
-- 
2.9.4

[Intel-wired-lan] [next PATCH S80-V3 08/11] i40e: allow XPS with QoS enabled

[Alice Michael]

From: Jacob Keller <jacob.e.keller@intel.com>

Recently, the kernel gained support for enabling XPS and QoS at the
same time. Thus, we no longer need to worry about the number of
traffic classes when enabling XPS.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index a47f4ed..0dcae11 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -2879,23 +2879,18 @@ static void i40e_vsi_free_rx_resources(struct i40e_vsi *vsi)
  **/
 static void i40e_config_xps_tx_ring(struct i40e_ring *ring)
 {
-	struct i40e_vsi *vsi = ring->vsi;
 	int cpu;
 
 	if (!ring->q_vector || !ring->netdev)
 		return;
 
-	if ((vsi->tc_config.numtc <= 1) &&
-	    !test_and_set_bit(__I40E_TX_XPS_INIT_DONE, ring->state)) {
-		cpu = cpumask_local_spread(ring->q_vector->v_idx, -1);
-		netif_set_xps_queue(ring->netdev, get_cpu_mask(cpu),
-				    ring->queue_index);
-	}
+	/* We only initialize XPS once, so as not to overwrite user settings */
+	if (test_and_set_bit(__I40E_TX_XPS_INIT_DONE, ring->state))
+		return;
 
-	/* schedule our worker thread which will take care of
-	 * applying the new filter changes
-	 */
-	i40e_service_event_schedule(vsi->back);
+	cpu = cpumask_local_spread(ring->q_vector->v_idx, -1);
+	netif_set_xps_queue(ring->netdev, get_cpu_mask(cpu),
+			    ring->queue_index);
 }
 
 /**
-- 
2.9.4

[Intel-wired-lan] [next PATCH S80-V3 09/11] i40e: add check for return from find_first_bit call

[Alice Michael]

From: Lihong Yang <lihong.yang@intel.com>

The find_first_bit function will return the size passed to search
if the first set bit is not found. This patch adds the check in case
that happens as the return value would be used as the index in an array
and that would have caused the out-of-bounds access.

Detected by CoverityScan, CID 1295969 Out-of-bounds access

Signed-off-by: Lihong Yang <lihong.yang@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
index 7dc042b..b2bdf27 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
@@ -306,6 +306,10 @@ static void i40e_config_irq_link_list(struct i40e_vf *vf, u16 vsi_id,
 	next_q = find_first_bit(&linklistmap,
 				(I40E_MAX_VSI_QP *
 				 I40E_VIRTCHNL_SUPPORTED_QTYPES));
+	if (unlikely(next_q == (I40E_MAX_VSI_QP *
+				I40E_VIRTCHNL_SUPPORTED_QTYPES)))
+		goto irq_list_done;
+
 	vsi_queue_id = next_q / I40E_VIRTCHNL_SUPPORTED_QTYPES;
 	qtype = next_q % I40E_VIRTCHNL_SUPPORTED_QTYPES;
 	pf_queue_id = i40e_vc_get_pf_queue_id(vf, vsi_id, vsi_queue_id);
-- 
2.9.4

[Intel-wired-lan] [next PATCH S80-V3 10/11] i40e: Retry AQC GetPhyAbilities to overcome I2CRead hangs

[Alice Michael]

From: Jayaprakash Shanmugam <jayaprakash.shanmugam@intel.com>

- When the I2C is busy, the PHY reads are delayed.  The firmware will
  return EGAIN in these cases with an expectation that the SW will
  trigger the reads again
- This patch retries the operation for a maximum period of 500ms

Signed-off-by: Jayaprakash Shanmugam <jayaprakash.shanmugam@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_common.c | 42 ++++++++++++++++++---------
 drivers/net/ethernet/intel/i40e/i40e_type.h   |  3 ++
 drivers/net/ethernet/intel/i40evf/i40e_type.h |  3 ++
 3 files changed, 35 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c b/drivers/net/ethernet/intel/i40e/i40e_common.c
index cb4468c..e7d8a01 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_common.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_common.c
@@ -1567,30 +1567,46 @@ i40e_status i40e_aq_get_phy_capabilities(struct i40e_hw *hw,
 	struct i40e_aq_desc desc;
 	i40e_status status;
 	u16 abilities_size = sizeof(struct i40e_aq_get_phy_abilities_resp);
+	u16 max_delay = I40E_MAX_PHY_TIMEOUT, total_delay = 0;
 
 	if (!abilities)
 		return I40E_ERR_PARAM;
 
-	i40e_fill_default_direct_cmd_desc(&desc,
-					  i40e_aqc_opc_get_phy_abilities);
+	do {
+		i40e_fill_default_direct_cmd_desc(&desc,
+					       i40e_aqc_opc_get_phy_abilities);
 
-	desc.flags |= cpu_to_le16((u16)I40E_AQ_FLAG_BUF);
-	if (abilities_size > I40E_AQ_LARGE_BUF)
-		desc.flags |= cpu_to_le16((u16)I40E_AQ_FLAG_LB);
+		desc.flags |= cpu_to_le16((u16)I40E_AQ_FLAG_BUF);
+		if (abilities_size > I40E_AQ_LARGE_BUF)
+			desc.flags |= cpu_to_le16((u16)I40E_AQ_FLAG_LB);
 
-	if (qualified_modules)
-		desc.params.external.param0 |=
+		if (qualified_modules)
+			desc.params.external.param0 |=
 			cpu_to_le32(I40E_AQ_PHY_REPORT_QUALIFIED_MODULES);
 
-	if (report_init)
-		desc.params.external.param0 |=
+		if (report_init)
+			desc.params.external.param0 |=
 			cpu_to_le32(I40E_AQ_PHY_REPORT_INITIAL_VALUES);
 
-	status = i40e_asq_send_command(hw, &desc, abilities, abilities_size,
-				       cmd_details);
+		status = i40e_asq_send_command(hw, &desc, abilities,
+					       abilities_size, cmd_details);
 
-	if (hw->aq.asq_last_status == I40E_AQ_RC_EIO)
-		status = I40E_ERR_UNKNOWN_PHY;
+		if (status)
+			break;
+
+		if (hw->aq.asq_last_status == I40E_AQ_RC_EIO) {
+			status = I40E_ERR_UNKNOWN_PHY;
+			break;
+		} else if (hw->aq.asq_last_status == I40E_AQ_RC_EAGAIN) {
+			usleep_range(1000, 2000);
+			total_delay++;
+			status = I40E_ERR_TIMEOUT;
+		}
+	} while ((hw->aq.asq_last_status != I40E_AQ_RC_OK) &&
+		 (total_delay < max_delay));
+
+	if (status)
+		return status;
 
 	if (report_init) {
 		if (hw->mac.type ==  I40E_MAC_XL710 &&
diff --git a/drivers/net/ethernet/intel/i40e/i40e_type.h b/drivers/net/ethernet/intel/i40e/i40e_type.h
index 4b32b1d..0410fcb 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_type.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_type.h
@@ -46,6 +46,9 @@
 /* Max default timeout in ms, */
 #define I40E_MAX_NVM_TIMEOUT		18000
 
+/* Max timeout in ms for the phy to respond */
+#define I40E_MAX_PHY_TIMEOUT		500
+
 /* Switch from ms to the 1usec global time (this is the GTIME resolution) */
 #define I40E_MS_TO_GTIME(time)		((time) * 1000)
 
diff --git a/drivers/net/ethernet/intel/i40evf/i40e_type.h b/drivers/net/ethernet/intel/i40evf/i40e_type.h
index 9364b67..213b773 100644
--- a/drivers/net/ethernet/intel/i40evf/i40e_type.h
+++ b/drivers/net/ethernet/intel/i40evf/i40e_type.h
@@ -46,6 +46,9 @@
 /* Max default timeout in ms, */
 #define I40E_MAX_NVM_TIMEOUT		18000
 
+/* Max timeout in ms for the phy to respond */
+#define I40E_MAX_PHY_TIMEOUT		500
+
 /* Switch from ms to the 1usec global time (this is the GTIME resolution) */
 #define I40E_MS_TO_GTIME(time)		((time) * 1000)
 
-- 
2.9.4

[Intel-wired-lan] [next PATCH S80-V3 11/11] i40e: use a local variable instead of calculating multiple times

[Alice Michael]

From: Lihong Yang <lihong.yang@intel.com>

The computed result of I40E_MAX_VSI_QP * I40E_VIRTCHNL_SUPPORTED_QTYPES
is used more than three times in function i40e_config_irq_link_list.
Simply declare a local variable to store it to improve readability.

Signed-off-by: Lihong Yang <lihong.yang@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
index b2bdf27..341944b 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
@@ -273,7 +273,7 @@ static void i40e_config_irq_link_list(struct i40e_vf *vf, u16 vsi_id,
 	struct i40e_hw *hw = &pf->hw;
 	u16 vsi_queue_id, pf_queue_id;
 	enum i40e_queue_type qtype;
-	u16 next_q, vector_id;
+	u16 next_q, vector_id, size;
 	u32 reg, reg_idx;
 	u16 itr_idx = 0;
 
@@ -303,11 +303,9 @@ static void i40e_config_irq_link_list(struct i40e_vf *vf, u16 vsi_id,
 				     vsi_queue_id + 1));
 	}
 
-	next_q = find_first_bit(&linklistmap,
-				(I40E_MAX_VSI_QP *
-				 I40E_VIRTCHNL_SUPPORTED_QTYPES));
-	if (unlikely(next_q == (I40E_MAX_VSI_QP *
-				I40E_VIRTCHNL_SUPPORTED_QTYPES)))
+	size = I40E_MAX_VSI_QP * I40E_VIRTCHNL_SUPPORTED_QTYPES;
+	next_q = find_first_bit(&linklistmap, size);
+	if (unlikely(next_q == size))
 		goto irq_list_done;
 
 	vsi_queue_id = next_q / I40E_VIRTCHNL_SUPPORTED_QTYPES;
@@ -317,7 +315,7 @@ static void i40e_config_irq_link_list(struct i40e_vf *vf, u16 vsi_id,
 
 	wr32(hw, reg_idx, reg);
 
-	while (next_q < (I40E_MAX_VSI_QP * I40E_VIRTCHNL_SUPPORTED_QTYPES)) {
+	while (next_q < size) {
 		switch (qtype) {
 		case I40E_QUEUE_TYPE_RX:
 			reg_idx = I40E_QINT_RQCTL(pf_queue_id);
@@ -331,12 +329,8 @@ static void i40e_config_irq_link_list(struct i40e_vf *vf, u16 vsi_id,
 			break;
 		}
 
-		next_q = find_next_bit(&linklistmap,
-				       (I40E_MAX_VSI_QP *
-					I40E_VIRTCHNL_SUPPORTED_QTYPES),
-				       next_q + 1);
-		if (next_q <
-		    (I40E_MAX_VSI_QP * I40E_VIRTCHNL_SUPPORTED_QTYPES)) {
+		next_q = find_next_bit(&linklistmap, size, next_q + 1);
+		if (next_q < size) {
 			vsi_queue_id = next_q / I40E_VIRTCHNL_SUPPORTED_QTYPES;
 			qtype = next_q % I40E_VIRTCHNL_SUPPORTED_QTYPES;
 			pf_queue_id = i40e_vc_get_pf_queue_id(vf, vsi_id,
-- 
2.9.4

[Intel-wired-lan] [next PATCH S80-V3 01/11] i40e: use the safe hash table iterator when deleting mac filters

[Bowers, AndrewX]

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On
> Behalf Of Alice Michael
> Sent: Thursday, September 7, 2017 5:06 AM
> To: Michael, Alice <alice.michael@intel.com>; intel-wired-
> lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [next PATCH S80-V3 01/11] i40e: use the safe hash
> table iterator when deleting mac filters
> 
> From: Lihong Yang <lihong.yang@intel.com>
> 
> This patch replaces hash_for_each function with hash_for_each_safe when
> calling  __i40e_del_filter. The hash_for_each_safe function is the right one
> to use when iterating over a hash table to safely remove a hash entry.
> Otherwise, incorrect values may be read from freed memory.
> 
> Detected by CoverityScan, CID 1402048 Read from pointer after free
> 
> Signed-off-by: Lihong Yang <lihong.yang@intel.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Tested-by: Andrew Bowers <andrewx.bowers@intel.com>

[Intel-wired-lan] [next PATCH S80-V3 02/11] i40evf: fix mac filter removal timing issue

[Bowers, AndrewX]

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On
> Behalf Of Alice Michael
> Sent: Thursday, September 7, 2017 5:06 AM
> To: Michael, Alice <alice.michael@intel.com>; intel-wired-
> lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [next PATCH S80-V3 02/11] i40evf: fix mac filter
> removal timing issue
> 
> From: Alan Brady <alan.brady@intel.com>
> 
> Due to the asynchronous nature in which mac filters are added and deleted,
> there exists a bug in which filters are erroneously removed if removed then
> added again quickly.
> 
> The events are as such:
>     - filter marked for removal
>     - same filter is re-added before watchdog that cleans up filters
>     - we skip re-adding the filter because we have it already in the list
>     - watchdog filter cleanup kicks off and filter is removed
> 
> So when we were re-adding the same filter, it didn't actually get added
> because it already existed in the list, but was marked for removal and had yet
> to actually be removed.
> 
> This patch fixes the issue by making sure that when adding a filter, if we find
> it already existing in our list, make sure it is not marked to be removed.
> 
> Signed-off-by: Alan Brady <alan.brady@intel.com>
> ---
>  drivers/net/ethernet/intel/i40evf/i40evf_main.c | 2 ++
>  1 file changed, 2 insertions(+)

Tested-by: Andrew Bowers <andrewx.bowers@intel.com>

[Intel-wired-lan] [next PATCH S80-V3 03/11] i40e/i40evf: fix incorrect default ITR values on driver load

[Bowers, AndrewX]

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On
> Behalf Of Alice Michael
> Sent: Thursday, September 7, 2017 5:06 AM
> To: Michael, Alice <alice.michael@intel.com>; intel-wired-
> lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [next PATCH S80-V3 03/11] i40e/i40evf: fix
> incorrect default ITR values on driver load
> 
> From: Jacob Keller <jacob.e.keller@intel.com>
> 
> The ITR register expects to be programmed in units of 2 microseconds.
> Because of this, all of the drivers I40E_ITR_* constants are in terms of this 2
> microsecond register.
> 
> Unfortunately, the rx_itr_default value is expected to be programmed in
> microseconds.
> 
> Effectively the driver defaults to an ITR value of half the expected value (in
> terms of minimum microseconds between interrupts).
> 
> Fix this by changing the default values to be calculated using
> ITR_REG_TO_USEC macro which indicates that we're converting from the
> register units into microseconds.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_main.c     | 4 ++--
>  drivers/net/ethernet/intel/i40e/i40e_txrx.h     | 6 ++++--
>  drivers/net/ethernet/intel/i40evf/i40e_txrx.h   | 6 ++++--
>  drivers/net/ethernet/intel/i40evf/i40evf_main.c | 4 ++--
>  4 files changed, 12 insertions(+), 8 deletions(-)

Tested-by: Andrew Bowers <andrewx.bowers@intel.com>

[Intel-wired-lan] [next PATCH S80-V3 04/11] i40e/i40evf: always set the CLEARPBA flag when re-enabling interrupts

[Bowers, AndrewX]

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On
> Behalf Of Alice Michael
> Sent: Thursday, September 7, 2017 5:06 AM
> To: Michael, Alice <alice.michael@intel.com>; intel-wired-
> lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [next PATCH S80-V3 04/11] i40e/i40evf: always set
> the CLEARPBA flag when re-enabling interrupts
> 
> From: Jacob Keller <jacob.e.keller@intel.com>
> 
> In the past we changed driver behavior to not clear the PBA when re-
> enabling interrupts. This change was motivated by the flawed belief that
> clearing the PBA would cause a lost interrupt if a receive interrupt occurred
> while interrupts were disabled.
> 
> According to empirical testing this isn't the case. Additionally, the data sheet
> specifically says that we should set the CLEARPBA bit when re-enabling
> interrupts in a polling setup.
> 
> This reverts commit 40d72a509862 ("i40e/i40evf: don't lose interrupts")
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e.h             |  5 +----
>  drivers/net/ethernet/intel/i40e/i40e_main.c        | 11 +++++------
>  drivers/net/ethernet/intel/i40e/i40e_txrx.c        |  6 ++----
>  drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c |  2 +-
>  drivers/net/ethernet/intel/i40evf/i40e_txrx.c      |  4 +---
>  5 files changed, 10 insertions(+), 18 deletions(-)

Tested-by: Andrew Bowers <andrewx.bowers@intel.com>

[Intel-wired-lan] [next PATCH S80-V3 05/11] i40e: reduce lrxqthresh from 2 to 1

[Bowers, AndrewX]

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On
> Behalf Of Alice Michael
> Sent: Thursday, September 7, 2017 5:06 AM
> To: Michael, Alice <alice.michael@intel.com>; intel-wired-
> lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [next PATCH S80-V3 05/11] i40e: reduce lrxqthresh
> from 2 to 1
> 
> From: Jacob Keller <jacob.e.keller@intel.com>
> 
> The lrxq thresh value tells hardware to immediately interrupt when there are
> fewer than N*64 packets left in the ring.
> 
> Counter intuitively, empirical testing has shown that decreasing this value
> from 2 to 1, and thus changing from an immediate interrupt at fewer than
> 128 descriptors down to 64 descriptors causes a small increase in the
> maximum total packets per second we can receive. This increase occurs even
> when we're polling with interrupts masked, as the hardware must still handle
> interrupts internally even if we've disabled them in software.
> 
> Also reduce the value for any VFs we allocate.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_main.c        | 2 +-
>  drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Tested-by: Andrew Bowers <andrewx.bowers@intel.com>

[Intel-wired-lan] [next PATCH S80-V3 06/11] i40e/i40evf: bump tail only in multiples of 8

[Bowers, AndrewX]

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On
> Behalf Of Alice Michael
> Sent: Thursday, September 7, 2017 5:06 AM
> To: Michael, Alice <alice.michael@intel.com>; intel-wired-
> lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [next PATCH S80-V3 06/11] i40e/i40evf: bump tail
> only in multiples of 8
> 
> From: Jacob Keller <jacob.e.keller@intel.com>
> 
> Hardware only fetches descriptors on cachelines of 8, essentially ignoring the
> lower 3 bits of the tail register. Thus, it is pointless to bump tail by an
> unaligned access as the hardware will ignore some of the new descriptors we
> allocated. Thus, it's ideal if we can ensure tail writes are always aligned to 8.
> 
> At first, it seems like we'd already do this, since we allocate descriptors in
> batches which are a multiple of 8. Since we'd always increment by a multiple
> of 8, it seems like the value should always be aligned.
> 
> However, this ignores allocation failures. If we fail to allocate a buffer, our tail
> register will become unaligned. Once it has become unaligned it will
> essentially be stuck unaligned until a buffer allocation happens to fail at the
> exact amount necessary to re-align it.
> 
> We can do better, by simply rounding down the number of buffers we're
> about to allocate (cleaned_count) such that "next_to_clean
> + cleaned_count" is rounded to the nearest multiple of 8.
> 
> We do this by calculating how far off that value is and subtracting it from the
> cleaned_count. This essentially defers allocation of buffers if they're going to
> be ignored by hardware anyways, and re-aligns our next_to_use and tail
> values after a failure to allocate a descriptor.
> 
> This calculation ensures that we always align the tail writes in a way the
> hardware expects and don't unnecessarily allocate buffers which won't be
> fetched immediately.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_txrx.c   | 9 +++++++++
>  drivers/net/ethernet/intel/i40evf/i40e_txrx.c | 9 +++++++++
>  2 files changed, 18 insertions(+)

Tested-by: Andrew Bowers <andrewx.bowers@intel.com>

[Intel-wired-lan] [next PATCH S80-V3 07/11] i40e/i40evf: bundle more descriptors when allocating buffers

[Bowers, AndrewX]

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On
> Behalf Of Alice Michael
> Sent: Thursday, September 7, 2017 5:06 AM
> To: Michael, Alice <alice.michael@intel.com>; intel-wired-
> lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [next PATCH S80-V3 07/11] i40e/i40evf: bundle
> more descriptors when allocating buffers
> 
> From: Jacob Keller <jacob.e.keller@intel.com>
> 
> Double the number of descriptors we'll bundle into one tail bump when
> receiving. Empirical testing has shown that we reduce CPU utilization and
> don't appear to reduce throughput or packet rate. 32 seems to be the sweet
> spot, as it's half the default polling budget, so we'd essentially reduce from 4
> tail writes when polling down to 2. Increasing this up to
> 64 appears to have negative impacts as it may become possible that we don't
> bump the tail each time we get polled, which could cause a long delay
> between returning descriptors to the hardware.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_txrx.h   | 2 +-
>  drivers/net/ethernet/intel/i40evf/i40e_txrx.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Tested-by: Andrew Bowers <andrewx.bowers@intel.com>

[Intel-wired-lan] [next PATCH S80-V3 08/11] i40e: allow XPS with QoS enabled

[Bowers, AndrewX]

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On
> Behalf Of Alice Michael
> Sent: Thursday, September 7, 2017 5:06 AM
> To: Michael, Alice <alice.michael@intel.com>; intel-wired-
> lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [next PATCH S80-V3 08/11] i40e: allow XPS with
> QoS enabled
> 
> From: Jacob Keller <jacob.e.keller@intel.com>
> 
> Recently, the kernel gained support for enabling XPS and QoS at the same
> time. Thus, we no longer need to worry about the number of traffic classes
> when enabling XPS.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_main.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)

Tested-by: Andrew Bowers <andrewx.bowers@intel.com>

[Intel-wired-lan] [next PATCH S80-V3 09/11] i40e: add check for return from find_first_bit call

[Bowers, AndrewX]

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On
> Behalf Of Alice Michael
> Sent: Thursday, September 7, 2017 5:06 AM
> To: Michael, Alice <alice.michael@intel.com>; intel-wired-
> lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [next PATCH S80-V3 09/11] i40e: add check for
> return from find_first_bit call
> 
> From: Lihong Yang <lihong.yang@intel.com>
> 
> The find_first_bit function will return the size passed to search if the first set
> bit is not found. This patch adds the check in case that happens as the return
> value would be used as the index in an array and that would have caused the
> out-of-bounds access.
> 
> Detected by CoverityScan, CID 1295969 Out-of-bounds access
> 
> Signed-off-by: Lihong Yang <lihong.yang@intel.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 4 ++++
>  1 file changed, 4 insertions(+)

Tested-by: Andrew Bowers <andrewx.bowers@intel.com>

[Intel-wired-lan] [next PATCH S80-V3 11/11] i40e: use a local variable instead of calculating multiple times

[Bowers, AndrewX]

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On
> Behalf Of Alice Michael
> Sent: Thursday, September 7, 2017 5:06 AM
> To: Michael, Alice <alice.michael@intel.com>; intel-wired-
> lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [next PATCH S80-V3 11/11] i40e: use a local variable
> instead of calculating multiple times
> 
> From: Lihong Yang <lihong.yang@intel.com>
> 
> The computed result of I40E_MAX_VSI_QP *
> I40E_VIRTCHNL_SUPPORTED_QTYPES is used more than three times in
> function i40e_config_irq_link_list.
> Simply declare a local variable to store it to improve readability.
> 
> Signed-off-by: Lihong Yang <lihong.yang@intel.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)

Tested-by: Andrew Bowers <andrewx.bowers@intel.com>

[Intel-wired-lan] [next PATCH S80-V3 10/11] i40e: Retry AQC GetPhyAbilities to overcome I2CRead hangs

[Bowers, AndrewX]

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On
> Behalf Of Alice Michael
> Sent: Thursday, September 7, 2017 5:06 AM
> To: Michael, Alice <alice.michael@intel.com>; intel-wired-
> lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [next PATCH S80-V3 10/11] i40e: Retry AQC
> GetPhyAbilities to overcome I2CRead hangs
> 
> From: Jayaprakash Shanmugam <jayaprakash.shanmugam@intel.com>
> 
> - When the I2C is busy, the PHY reads are delayed.  The firmware will
>   return EGAIN in these cases with an expectation that the SW will
>   trigger the reads again
> - This patch retries the operation for a maximum period of 500ms
> 
> Signed-off-by: Jayaprakash Shanmugam
> <jayaprakash.shanmugam@intel.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_common.c | 42
> ++++++++++++++++++---------
>  drivers/net/ethernet/intel/i40e/i40e_type.h   |  3 ++
>  drivers/net/ethernet/intel/i40evf/i40e_type.h |  3 ++
>  3 files changed, 35 insertions(+), 13 deletions(-)

Tested-by: Andrew Bowers <andrewx.bowers@intel.com>

[Intel-wired-lan] [next PATCH S80-V3 04/11] i40e/i40evf: always set the CLEARPBA flag when re-enabling interrupts

[Duyck, Alexander H]

On Tue, 2017-09-12 at 20:46 +0000, Bowers, AndrewX wrote:
> > -----Original Message-----
> > From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On
> > Behalf Of Alice Michael
> > Sent: Thursday, September 7, 2017 5:06 AM
> > To: Michael, Alice <alice.michael@intel.com>; intel-wired-
> > lan at lists.osuosl.org
> > Subject: [Intel-wired-lan] [next PATCH S80-V3 04/11] i40e/i40evf: always set
> > the CLEARPBA flag when re-enabling interrupts
> > 
> > From: Jacob Keller <jacob.e.keller@intel.com>
> > 
> > In the past we changed driver behavior to not clear the PBA when re-
> > enabling interrupts. This change was motivated by the flawed belief that
> > clearing the PBA would cause a lost interrupt if a receive interrupt occurred
> > while interrupts were disabled.
> > 
> > According to empirical testing this isn't the case. Additionally, the data sheet
> > specifically says that we should set the CLEARPBA bit when re-enabling
> > interrupts in a polling setup.
> > 
> > This reverts commit 40d72a509862 ("i40e/i40evf: don't lose interrupts")
> > 
> > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> > ---
> >  drivers/net/ethernet/intel/i40e/i40e.h             |  5 +----
> >  drivers/net/ethernet/intel/i40e/i40e_main.c        | 11 +++++------
> >  drivers/net/ethernet/intel/i40e/i40e_txrx.c        |  6 ++----
> >  drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c |  2 +-
> >  drivers/net/ethernet/intel/i40evf/i40e_txrx.c      |  4 +---
> >  5 files changed, 10 insertions(+), 18 deletions(-)
> 
> Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
> 

This patch just got pointed out to me in a hallway conversation.

I am pretty sure this _will_ cause you to lose interrupts. Specifically
you shouldn't be clearing the PBA bit at the end of the polling routine
unless you know you are going to poll again.

The PBA bit should only be cleared if:
1. You are at the start of your clean-up routine and want to clear it
in case any additional work has come in since the original vector
fired. (currently handled via an auto-clear function for MSI-X)
2. You are at the end of your polling routine and you know you are
going to be polling again.

The original patch was more aggressive than it needed to be. For
example you could probably go ahead and clear the PBA in the
i40e_intr()q interrupt routine itself since all it is doing is
scheduling the polling routine which will run after the interrupt
routine has completed. You shouldn't be clearing the PBA at the end of
NAPI poll unless you know you are not going to be exiting polling.

I hope this helps to clarify things.

- Alex

[Intel-wired-lan] [next PATCH S80-V3 04/11] i40e/i40evf: always set the CLEARPBA flag when re-enabling interrupts

[Keller, Jacob E]

> -----Original Message-----
> From: Duyck, Alexander H
> Sent: Thursday, October 05, 2017 11:32 AM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: Bowers, AndrewX <andrewx.bowers@intel.com>; intel-wired-
> lan at lists.osuosl.org
> Subject: Re: [Intel-wired-lan] [next PATCH S80-V3 04/11] i40e/i40evf: always set
> the CLEARPBA flag when re-enabling interrupts
> 
> On Tue, 2017-09-12 at 20:46 +0000, Bowers, AndrewX wrote:
> > > -----Original Message-----
> > > From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On
> > > Behalf Of Alice Michael
> > > Sent: Thursday, September 7, 2017 5:06 AM
> > > To: Michael, Alice <alice.michael@intel.com>; intel-wired-
> > > lan at lists.osuosl.org
> > > Subject: [Intel-wired-lan] [next PATCH S80-V3 04/11] i40e/i40evf: always set
> > > the CLEARPBA flag when re-enabling interrupts
> > >
> > > From: Jacob Keller <jacob.e.keller@intel.com>
> > >
> > > In the past we changed driver behavior to not clear the PBA when re-
> > > enabling interrupts. This change was motivated by the flawed belief that
> > > clearing the PBA would cause a lost interrupt if a receive interrupt occurred
> > > while interrupts were disabled.
> > >
> > > According to empirical testing this isn't the case. Additionally, the data sheet
> > > specifically says that we should set the CLEARPBA bit when re-enabling
> > > interrupts in a polling setup.
> > >
> > > This reverts commit 40d72a509862 ("i40e/i40evf: don't lose interrupts")
> > >
> > > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> > > ---
> > >  drivers/net/ethernet/intel/i40e/i40e.h             |  5 +----
> > >  drivers/net/ethernet/intel/i40e/i40e_main.c        | 11 +++++------
> > >  drivers/net/ethernet/intel/i40e/i40e_txrx.c        |  6 ++----
> > >  drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c |  2 +-
> > >  drivers/net/ethernet/intel/i40evf/i40e_txrx.c      |  4 +---
> > >  5 files changed, 10 insertions(+), 18 deletions(-)
> >
> > Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
> >
> 
> This patch just got pointed out to me in a hallway conversation.
> 
> I am pretty sure this _will_ cause you to lose interrupts. Specifically
> you shouldn't be clearing the PBA bit at the end of the polling routine
> unless you know you are going to poll again.
> 
> The PBA bit should only be cleared if:
> 1. You are at the start of your clean-up routine and want to clear it
> in case any additional work has come in since the original vector
> fired. (currently handled via an auto-clear function for MSI-X)
> 2. You are at the end of your polling routine and you know you are
> going to be polling again.
> 
> The original patch was more aggressive than it needed to be. For
> example you could probably go ahead and clear the PBA in the
> i40e_intr()q interrupt routine itself since all it is doing is
> scheduling the polling routine which will run after the interrupt
> routine has completed. You shouldn't be clearing the PBA at the end of
> NAPI poll unless you know you are not going to be exiting polling.
> 
> I hope this helps to clarify things.
> 
> - Alex

Ok so only clear it if we're continuing to poll. I'll cook up a patch for that.

Thanks,
Jake

[Intel-wired-lan] [next PATCH S80-V3 04/11] i40e/i40evf: always set the CLEARPBA flag when re-enabling interrupts

[Jesse Brandeburg]

On Thu, 5 Oct 2017 22:24:19 +0000
"Keller, Jacob E" <jacob.e.keller@intel.com> wrote:

> > The original patch was more aggressive than it needed to be. For
> > example you could probably go ahead and clear the PBA in the
> > i40e_intr()q interrupt routine itself since all it is doing is
> > scheduling the polling routine which will run after the interrupt
> > routine has completed. You shouldn't be clearing the PBA at the end of
> > NAPI poll unless you know you are not going to be exiting polling.
> > 
> > I hope this helps to clarify things.
> > 
> > - Alex  
> 
> Ok so only clear it if we're continuing to poll. I'll cook up a patch for that.

Hi Jake, we may just need to cook up a better test for this instead of
generating further patches.  I believe based on updated understanding
of the hardware functionality that this patch is correct, but it is
difficult to prove and we should probably spend more time doing the
actual proof.

At least this patch follow the actual directions in the hardware
manual, so I think we should stick with it.

Jesse

[Intel-wired-lan] [next PATCH S80-V3 04/11] i40e/i40evf: always set the CLEARPBA flag when re-enabling interrupts

[Keller, Jacob E]

> -----Original Message-----
> From: Brandeburg, Jesse
> Sent: Thursday, October 05, 2017 4:01 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: Duyck, Alexander H <alexander.h.duyck@intel.com>; intel-wired-
> lan at lists.osuosl.org; Brandeburg, Jesse <jesse.brandeburg@intel.com>
> Subject: Re: [Intel-wired-lan] [next PATCH S80-V3 04/11] i40e/i40evf: always set
> the CLEARPBA flag when re-enabling interrupts
> 
> On Thu, 5 Oct 2017 22:24:19 +0000
> "Keller, Jacob E" <jacob.e.keller@intel.com> wrote:
> 
> > > The original patch was more aggressive than it needed to be. For
> > > example you could probably go ahead and clear the PBA in the
> > > i40e_intr()q interrupt routine itself since all it is doing is
> > > scheduling the polling routine which will run after the interrupt
> > > routine has completed. You shouldn't be clearing the PBA at the end of
> > > NAPI poll unless you know you are not going to be exiting polling.
> > >
> > > I hope this helps to clarify things.
> > >
> > > - Alex
> >
> > Ok so only clear it if we're continuing to poll. I'll cook up a patch for that.
> 
> Hi Jake, we may just need to cook up a better test for this instead of
> generating further patches.  I believe based on updated understanding
> of the hardware functionality that this patch is correct, but it is
> difficult to prove and we should probably spend more time doing the
> actual proof.
> 
> At least this patch follow the actual directions in the hardware
> manual, so I think we should stick with it.
> 
> Jesse

Ok. Lets continue the discussion in person to make sure everyone's on the same page and understanding.

I think I agree that we should keep this patch as is since it's based on the directions in our hardware manual.

Regards,
Jake