RE: [net-next 05/14] i40e/i40evf: always set the CLEARPBA flag when re-enabling interrupts

[David Laight]

From: Jeff Kirsher
> Sent: 09 October 2017 23:39
> 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.

I presume this if the MSI-X Pending Bit Array?
Normally this bit is cleared when the interrupt is actioned.

If request the device clear the PBA then it (probably) won't
raise an interrupt when it is unmasked (by clearing the 'masked' bit).

If you've just checked all the rings (with the interrupt masked)
and you clear the PBA bit when you unmask interrupts then you will
need to do another scan of the rings to pick up any packets that
arrived (or tried to signal an interrupt) in that small gap.

'Empirical testing' probably won't hit the timing window.

David

[net-next 05/14] i40e/i40evf: always set the CLEARPBA flag when re-enabling interrupts

[Jeff Kirsher]

From: Jacob Keller 

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 
Tested-by: Andrew Bowers 
Signed-off-by: Jeff Kirsher 
---
 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 7baf6d8a84dd..8139b4ee1dc3 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 = >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 d4b0cc36afb1..00a83afb02e9 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 = >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(>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 3bd176606c09..616abf79253e 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -2202,9 +2202,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);
 
@@ -2241,7 +2239,7 @@ static inline void i40e_update_enable_itr(struct i40e_vsi 
*vsi,
 
/* If we don't have MSIX, then we only