Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <20180208064714.6042-3-bpoirier@suse.com>
References: <20180208064714.6042-1-bpoirier@suse.com> <20180208064714.6042-3-bpoirier@suse.com>
From:   Alexander Duyck <alexander.duyck@gmail.com>
Date:   Thu, 8 Feb 2018 06:24:07 -0800
Message-ID: <CAKgT0Ud3v1zOFRJ2PT4Nq9EXSvpcKtR-YoLm87DYyMUgyQBYKA@mail.gmail.com>
Subject: Re: [PATCH net-queue 3/3] e1000e: Avoid missed interrupts following
 ICR read.
To:     Benjamin Poirier <bpoirier@suse.com>
Cc:     Jeff Kirsher <jeffrey.t.kirsher@intel.com>,
        intel-wired-lan <intel-wired-lan@lists.osuosl.org>,
        Netdev <netdev@vger.kernel.org>, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Feb 7, 2018 at 10:47 PM, Benjamin Poirier <bpoirier@suse.com> 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 <bpoirier@suse.com>

Acked-by: Alexander Duyck <alexander.h.duyck@intel.com>

> ---
>  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_RXDMT0        0x00000010 /* Rx desc min. threshold (0) */
>  #define E1000_ICR_RXO           0x00000040 /* Receiver Overrun */
>  #define E1000_ICR_RXT0          0x00000080 /* Rx timer intr (ring 0) */
> +#define E1000_ICR_MDAC          0x00000200 /* MDIO Access Complete */
> +#define E1000_ICR_SRPD          0x00010000 /* Small Receive Packet Detected */
> +#define E1000_ICR_ACK           0x00020000 /* Receive ACK Frame Detected */
> +#define E1000_ICR_MNG           0x00040000 /* Manageability Event Detected */
>  #define E1000_ICR_ECCER         0x00400000 /* Uncorrectable ECC Error */
>  /* If this bit asserted, the driver should claim the interrupt */
>  #define E1000_ICR_INT_ASSERTED 0x80000000
> @@ -407,7 +411,7 @@
>  #define E1000_ICR_RXQ1          0x00200000 /* Rx Queue 1 Interrupt */
>  #define E1000_ICR_TXQ0          0x00400000 /* Tx Queue 0 Interrupt */
>  #define E1000_ICR_TXQ1          0x00800000 /* Tx Queue 1 Interrupt */
> -#define E1000_ICR_OTHER         0x01000000 /* Other Interrupts */
> +#define E1000_ICR_OTHER         0x01000000 /* Other Interrupt */
>
>  /* PBA ECC Register */
>  #define E1000_PBA_ECC_COUNTER_MASK  0xFFF00000 /* 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_RXDMT0    E1000_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 = &adapter->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, &adapter->state))
> @@ -1931,7 +1927,7 @@ static irqreturn_t e1000_msix_other(int __always_unused irq, void *data)
>         }
>
>         if (!test_bit(__E1000_DOWN, &adapter->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 (adapter->msix_entries) {
>                 ew32(EIAC_82574, adapter->eiac_mask & E1000_EIAC_MASK_82574);
> -               ew32(IMS, adapter->eiac_mask | E1000_IMS_OTHER | E1000_IMS_LSC);
> +               ew32(IMS, adapter->eiac_mask | E1000_IMS_OTHER |
> +                    IMS_OTHER_MASK);
>         } else if (hw->mac.type >= e1000_pch_lpt) {
>                 ew32(IMS, IMS_ENABLE_MASK | E1000_IMS_ECCER);
>         } else {
> --
> 2.16.1
>