Date:   Fri, 26 Jan 2018 08:50:42 -0800
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Benjamin Poirier <bpoirier@...e.com>
Cc:     Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
        intel-wired-lan <intel-wired-lan@...ts.osuosl.org>,
        Netdev <netdev@...r.kernel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] Partial revert "e1000e: Avoid receiver overrun
 interrupt bursts"

On Fri, Jan 26, 2018 at 1:12 AM, Benjamin Poirier <bpoirier@...e.com> wrote:
> This reverts commit 4aea7a5c5e940c1723add439f4088844cd26196d.
>
> We keep the fix for the first part of the problem (1) described in the log
> of that commit however we use the implementation of e1000_msix_other() from
> before commit 16ecba59bc33 ("e1000e: Do not read ICR in Other interrupt").
> We remove the fix for the second part of the problem (2).
>
> Bursts of "Other" interrupts may once again occur during rxo (receive
> overflow) traffic conditions. This is deemed acceptable in the interest of
> reverting driver code back to its previous state. The e1000e driver should
> be in "maintenance" mode and we want to avoid unforeseen fallout from
> changes that are not strictly necessary.
>
> Link: https://www.spinics.net/lists/netdev/msg480675.html
> Signed-off-by: Benjamin Poirier <bpoirier@...e.com>

Thanks for doing this.

Only a few minor tweaks I would recommend. Otherwise it looks good.

> ---
>  drivers/net/ethernet/intel/e1000e/defines.h |  1 -
>  drivers/net/ethernet/intel/e1000e/netdev.c  | 32 +++++++++++------------------
>  2 files changed, 12 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/e1000e/defines.h b/drivers/net/ethernet/intel/e1000e/defines.h
> index afb7ebe20b24..0641c0098738 100644
> --- a/drivers/net/ethernet/intel/e1000e/defines.h
> +++ b/drivers/net/ethernet/intel/e1000e/defines.h
> @@ -398,7 +398,6 @@
>  #define E1000_ICR_LSC           0x00000004 /* Link Status Change */
>  #define E1000_ICR_RXSEQ         0x00000008 /* Rx sequence error */
>  #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_ECCER         0x00400000 /* Uncorrectable ECC Error */
>  /* If this bit asserted, the driver should claim the interrupt */
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index 9f18d39bdc8f..398e940436f8 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -1914,30 +1914,23 @@ 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;
> -       bool enable = true;
> -
> -       icr = er32(ICR);
> -       if (icr & E1000_ICR_RXO) {
> -               ew32(ICR, E1000_ICR_RXO);
> -               enable = false;
> -               /* napi poll will re-enable Other, make sure it runs */
> -               if (napi_schedule_prep(&adapter->napi)) {
> -                       adapter->total_rx_bytes = 0;
> -                       adapter->total_rx_packets = 0;
> -                       __napi_schedule(&adapter->napi);
> -               }
> -       }
> -       if (icr & E1000_ICR_LSC) {
> -               ew32(ICR, E1000_ICR_LSC);
> +       u32 icr = er32(ICR);
> +
> +       if (icr & adapter->eiac_mask)
> +               ew32(ICS, (icr & adapter->eiac_mask));

I'm not sure about this bit as it includes queue interrupts if I am
not mistaken. What we should be focusing on are the bits that are not
auto-cleared so this should probably be icr & ~adapter->eiac_mask.
Actually what you might do is something like:
    icr &= ~adapter->eiac_mask;
    if (icr)
        ew32(ICS, icr);

Also a comment explaining that we have to clear the bits that are not
automatically cleared by other interrupt causes might be useful.

> +       if (icr & E1000_ICR_OTHER) {
> +               if (!(icr & E1000_ICR_LSC))
> +                       goto no_link_interrupt;

I wouldn't bother with the jump tag. Let's just make this one if
statement checking both OTHER && LSC.

>                 hw->mac.get_link_status = true;
>                 /* guard against interrupt when we're going down */
>                 if (!test_bit(__E1000_DOWN, &adapter->state))
>                         mod_timer(&adapter->watchdog_timer, jiffies + 1);
>         }
>
> -       if (enable && !test_bit(__E1000_DOWN, &adapter->state))
> -               ew32(IMS, E1000_IMS_OTHER);
> +no_link_interrupt:

If you just combine the if statement logic the code naturally flows to
this point anyway without the jump label being needed.

> +       if (!test_bit(__E1000_DOWN, &adapter->state))
> +               ew32(IMS, E1000_IMS_LSC | E1000_IMS_OTHER);
>
>         return IRQ_HANDLED;
>  }
> @@ -2707,8 +2700,7 @@ static int e1000e_poll(struct napi_struct *napi, int weight)
>                 napi_complete_done(napi, work_done);
>                 if (!test_bit(__E1000_DOWN, &adapter->state)) {
>                         if (adapter->msix_entries)
> -                               ew32(IMS, adapter->rx_ring->ims_val |
> -                                    E1000_IMS_OTHER);
> +                               ew32(IMS, adapter->rx_ring->ims_val);
>                         else
>                                 e1000_irq_enable(adapter);
>                 }
> --
> 2.15.1
>

Powered by blists - more mailing lists