RE: [Intel-wired-lan] [PATCH net-queue 3/3] e1000e: Avoid missed interrupts following ICR read.

2018-02-14 Thread Brown, Aaron F 
> From: Intel-wired-lan [mailto:intel-wired-lan-boun...@osuosl.org] On
> Behalf Of Benjamin Poirier
> Sent: Wednesday, February 7, 2018 10:47 PM
> To: Kirsher, Jeffrey T <jeffrey.t.kirs...@intel.com>
> Cc: netdev@vger.kernel.org; intel-wired-...@lists.osuosl.org; linux-
> ker...@vger.kernel.org
> Subject: [Intel-wired-lan] [PATCH net-queue 3/3] e1000e: Avoid missed
> interrupts following ICR read.
> 
> The 82574 specification update errata 12 states that interrupts may be
> missed if ICR is read while INT_ASSERTED is not set. Avoid that problem by
> setting all bits related to events that can trigger the Other interrupt in
> IMS.
> 
> The Other interrupt is raised for such events regardless of whether or not
> they are set in IMS. However, only when they are set is the INT_ASSERTED
> bit also set in ICR.
> 
> By doing this, we ensure that INT_ASSERTED is always set when we read ICR
> in e1000_msix_other() and steer clear of the errata. This also ensures that
> ICR will automatically be cleared on read, therefore we no longer need to
> clear bits explicitly.
> 
> Signed-off-by: Benjamin Poirier <bpoir...@suse.com>
> ---
>  drivers/net/ethernet/intel/e1000e/defines.h | 21
> -
>  drivers/net/ethernet/intel/e1000e/netdev.c  | 11 ---
>  2 files changed, 24 insertions(+), 8 deletions(-)
> 

Tested-by: Aaron Brown <aaron.f.br...@intel.com>

Re: [PATCH net-queue 3/3] e1000e: Avoid missed interrupts following ICR read.

2018-02-08 Thread Alexander Duyck 
On Wed, Feb 7, 2018 at 10:47 PM, Benjamin Poirier  wrote:
> The 82574 specification update errata 12 states that interrupts may be
> missed if ICR is read while INT_ASSERTED is not set. Avoid that problem by
> setting all bits related to events that can trigger the Other interrupt in
> IMS.
>
> The Other interrupt is raised for such events regardless of whether or not
> they are set in IMS. However, only when they are set is the INT_ASSERTED
> bit also set in ICR.
>
> By doing this, we ensure that INT_ASSERTED is always set when we read ICR
> in e1000_msix_other() and steer clear of the errata. This also ensures that
> ICR will automatically be cleared on read, therefore we no longer need to
> clear bits explicitly.
>
> Signed-off-by: Benjamin Poirier 

Acked-by: Alexander Duyck 

> ---
>  drivers/net/ethernet/intel/e1000e/defines.h | 21 -
>  drivers/net/ethernet/intel/e1000e/netdev.c  | 11 ---
>  2 files changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/e1000e/defines.h 
> b/drivers/net/ethernet/intel/e1000e/defines.h
> index afb7ebe20b24..824fd44e25f0 100644
> --- a/drivers/net/ethernet/intel/e1000e/defines.h
> +++ b/drivers/net/ethernet/intel/e1000e/defines.h
> @@ -400,6 +400,10 @@
>  #define E1000_ICR_RXDMT00x0010 /* Rx desc min. threshold (0) */
>  #define E1000_ICR_RXO   0x0040 /* Receiver Overrun */
>  #define E1000_ICR_RXT0  0x0080 /* Rx timer intr (ring 0) */
> +#define E1000_ICR_MDAC  0x0200 /* MDIO Access Complete */
> +#define E1000_ICR_SRPD  0x0001 /* Small Receive Packet Detected 
> */
> +#define E1000_ICR_ACK   0x0002 /* Receive ACK Frame Detected */
> +#define E1000_ICR_MNG   0x0004 /* Manageability Event Detected */
>  #define E1000_ICR_ECCER 0x0040 /* Uncorrectable ECC Error */
>  /* If this bit asserted, the driver should claim the interrupt */
>  #define E1000_ICR_INT_ASSERTED 0x8000
> @@ -407,7 +411,7 @@
>  #define E1000_ICR_RXQ1  0x0020 /* Rx Queue 1 Interrupt */
>  #define E1000_ICR_TXQ0  0x0040 /* Tx Queue 0 Interrupt */
>  #define E1000_ICR_TXQ1  0x0080 /* Tx Queue 1 Interrupt */
> -#define E1000_ICR_OTHER 0x0100 /* Other Interrupts */
> +#define E1000_ICR_OTHER 0x0100 /* Other Interrupt */
>
>  /* PBA ECC Register */
>  #define E1000_PBA_ECC_COUNTER_MASK  0xFFF0 /* ECC counter mask */
> @@ -431,12 +435,27 @@
> E1000_IMS_RXSEQ  |\
> E1000_IMS_LSC)
>
> +/* These are all of the events related to the OTHER interrupt.
> + */
> +#define IMS_OTHER_MASK ( \
> +   E1000_IMS_LSC  | \
> +   E1000_IMS_RXO  | \
> +   E1000_IMS_MDAC | \
> +   E1000_IMS_SRPD | \
> +   E1000_IMS_ACK  | \
> +   E1000_IMS_MNG)
> +
>  /* Interrupt Mask Set */
>  #define E1000_IMS_TXDW  E1000_ICR_TXDW  /* Transmit desc written 
> back */
>  #define E1000_IMS_LSC   E1000_ICR_LSC   /* Link Status Change */
>  #define E1000_IMS_RXSEQ E1000_ICR_RXSEQ /* Rx sequence error */
>  #define E1000_IMS_RXDMT0E1000_ICR_RXDMT0/* Rx desc min. threshold */
> +#define E1000_IMS_RXO   E1000_ICR_RXO   /* Receiver Overrun */
>  #define E1000_IMS_RXT0  E1000_ICR_RXT0  /* Rx timer intr */
> +#define E1000_IMS_MDAC  E1000_ICR_MDAC  /* MDIO Access Complete */
> +#define E1000_IMS_SRPD  E1000_ICR_SRPD  /* Small Receive Packet */
> +#define E1000_IMS_ACK   E1000_ICR_ACK   /* Receive ACK Frame 
> Detected */
> +#define E1000_IMS_MNG   E1000_ICR_MNG   /* Manageability Event */
>  #define E1000_IMS_ECCER E1000_ICR_ECCER /* Uncorrectable ECC Error */
>  #define E1000_IMS_RXQ0  E1000_ICR_RXQ0  /* Rx Queue 0 Interrupt */
>  #define E1000_IMS_RXQ1  E1000_ICR_RXQ1  /* Rx Queue 1 Interrupt */
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c 
> b/drivers/net/ethernet/intel/e1000e/netdev.c
> index 2c9609bee2ae..9fd4050a91ca 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -1914,16 +1914,12 @@ static irqreturn_t e1000_msix_other(int 
> __always_unused irq, void *data)
> struct net_device *netdev = data;
> struct e1000_adapter *adapter = netdev_priv(netdev);
> struct e1000_hw *hw = >hw;
> -   u32 icr;
> -
> -   icr = er32(ICR);
> -   ew32(ICR, E1000_ICR_OTHER);
> +   u32 icr = er32(ICR);
>
> if (icr & adapter->eiac_mask)
> ew32(ICS, (icr & adapter->eiac_mask));
>
> if (icr & E1000_ICR_LSC) {
> -   ew32(ICR, E1000_ICR_LSC);
> hw->mac.get_link_status = true;
> /* guard against interrupt when we're going down */
> if (!test_bit(__E1000_DOWN, >state))
> @@ -1931,7 +1927,7 @@ static irqreturn_t e1000_msix_other(int

[PATCH net-queue 3/3] e1000e: Avoid missed interrupts following ICR read.

2018-02-07 Thread Benjamin Poirier 
The 82574 specification update errata 12 states that interrupts may be
missed if ICR is read while INT_ASSERTED is not set. Avoid that problem by
setting all bits related to events that can trigger the Other interrupt in
IMS.

The Other interrupt is raised for such events regardless of whether or not
they are set in IMS. However, only when they are set is the INT_ASSERTED
bit also set in ICR.

By doing this, we ensure that INT_ASSERTED is always set when we read ICR
in e1000_msix_other() and steer clear of the errata. This also ensures that
ICR will automatically be cleared on read, therefore we no longer need to
clear bits explicitly.

Signed-off-by: Benjamin Poirier 
---
 drivers/net/ethernet/intel/e1000e/defines.h | 21 -
 drivers/net/ethernet/intel/e1000e/netdev.c  | 11 ---
 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/defines.h 
b/drivers/net/ethernet/intel/e1000e/defines.h
index afb7ebe20b24..824fd44e25f0 100644
--- a/drivers/net/ethernet/intel/e1000e/defines.h
+++ b/drivers/net/ethernet/intel/e1000e/defines.h
@@ -400,6 +400,10 @@
 #define E1000_ICR_RXDMT00x0010 /* Rx desc min. threshold (0) */
 #define E1000_ICR_RXO   0x0040 /* Receiver Overrun */
 #define E1000_ICR_RXT0  0x0080 /* Rx timer intr (ring 0) */
+#define E1000_ICR_MDAC  0x0200 /* MDIO Access Complete */
+#define E1000_ICR_SRPD  0x0001 /* Small Receive Packet Detected */
+#define E1000_ICR_ACK   0x0002 /* Receive ACK Frame Detected */
+#define E1000_ICR_MNG   0x0004 /* Manageability Event Detected */
 #define E1000_ICR_ECCER 0x0040 /* Uncorrectable ECC Error */
 /* If this bit asserted, the driver should claim the interrupt */
 #define E1000_ICR_INT_ASSERTED 0x8000
@@ -407,7 +411,7 @@
 #define E1000_ICR_RXQ1  0x0020 /* Rx Queue 1 Interrupt */
 #define E1000_ICR_TXQ0  0x0040 /* Tx Queue 0 Interrupt */
 #define E1000_ICR_TXQ1  0x0080 /* Tx Queue 1 Interrupt */
-#define E1000_ICR_OTHER 0x0100 /* Other Interrupts */
+#define E1000_ICR_OTHER 0x0100 /* Other Interrupt */
 
 /* PBA ECC Register */
 #define E1000_PBA_ECC_COUNTER_MASK  0xFFF0 /* ECC counter mask */
@@ -431,12 +435,27 @@
E1000_IMS_RXSEQ  |\
E1000_IMS_LSC)
 
+/* These are all of the events related to the OTHER interrupt.
+ */
+#define IMS_OTHER_MASK ( \
+   E1000_IMS_LSC  | \
+   E1000_IMS_RXO  | \
+   E1000_IMS_MDAC | \
+   E1000_IMS_SRPD | \
+   E1000_IMS_ACK  | \
+   E1000_IMS_MNG)
+
 /* Interrupt Mask Set */
 #define E1000_IMS_TXDW  E1000_ICR_TXDW  /* Transmit desc written back 
*/
 #define E1000_IMS_LSC   E1000_ICR_LSC   /* Link Status Change */
 #define E1000_IMS_RXSEQ E1000_ICR_RXSEQ /* Rx sequence error */
 #define E1000_IMS_RXDMT0E1000_ICR_RXDMT0/* Rx desc min. threshold */
+#define E1000_IMS_RXO   E1000_ICR_RXO   /* Receiver Overrun */
 #define E1000_IMS_RXT0  E1000_ICR_RXT0  /* Rx timer intr */
+#define E1000_IMS_MDAC  E1000_ICR_MDAC  /* MDIO Access Complete */
+#define E1000_IMS_SRPD  E1000_ICR_SRPD  /* Small Receive Packet */
+#define E1000_IMS_ACK   E1000_ICR_ACK   /* Receive ACK Frame Detected 
*/
+#define E1000_IMS_MNG   E1000_ICR_MNG   /* Manageability Event */
 #define E1000_IMS_ECCER E1000_ICR_ECCER /* Uncorrectable ECC Error */
 #define E1000_IMS_RXQ0  E1000_ICR_RXQ0  /* Rx Queue 0 Interrupt */
 #define E1000_IMS_RXQ1  E1000_ICR_RXQ1  /* Rx Queue 1 Interrupt */
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c 
b/drivers/net/ethernet/intel/e1000e/netdev.c
index 2c9609bee2ae..9fd4050a91ca 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -1914,16 +1914,12 @@ static irqreturn_t e1000_msix_other(int __always_unused 
irq, void *data)
struct net_device *netdev = data;
struct e1000_adapter *adapter = netdev_priv(netdev);
struct e1000_hw *hw = >hw;
-   u32 icr;
-
-   icr = er32(ICR);
-   ew32(ICR, E1000_ICR_OTHER);
+   u32 icr = er32(ICR);
 
if (icr & adapter->eiac_mask)
ew32(ICS, (icr & adapter->eiac_mask));
 
if (icr & E1000_ICR_LSC) {
-   ew32(ICR, E1000_ICR_LSC);
hw->mac.get_link_status = true;
/* guard against interrupt when we're going down */
if (!test_bit(__E1000_DOWN, >state))
@@ -1931,7 +1927,7 @@ static irqreturn_t e1000_msix_other(int __always_unused 
irq, void *data)
}
 
if (!test_bit(__E1000_DOWN, >state))
-   ew32(IMS, E1000_IMS_OTHER);
+   ew32(IMS, E1000_IMS_OTHER | IMS_OTHER_MASK);
 
return IRQ_HANDLED;
 }
@@ -2258,7 +2254,8 @@ static void e1000_irq_enable(struct e1000_adapter 
*adapter)
 
if