Source-Changes-HG archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

[src/trunk]: src/sys/dev/pci/ixgbe Add new spin mutex to avoid race between i...



details:   https://anonhg.NetBSD.org/src/rev/71808a972381
branches:  trunk
changeset: 957103:71808a972381
user:      knakahara <knakahara%NetBSD.org@localhost>
date:      Tue Nov 17 04:50:29 2020 +0000

description:
Add new spin mutex to avoid race between ixgbe_msix_admin() and ixgbe_handle_admin().

At first, it seems "IXGBE_WRITE_REG(hw, IXGBE_EIMC, IXGBE_EIMC_OTHER)"
cannot stop interrupts, because 31th bit is reserved for 82598, 82599,
X540 and X550.  So, the current following design
    (1) ixgbe_msix_admin() disables interrupts
    (2) ixgbe_msix_admin() calls workqueue_enqueue() for ixgbe_handle_admin()
    (3) ixgbe_handle_admin() does interrupt processing
    (4) after ixgbe_handle_admin() has done all interrupt processings,
        ixgbe_handle_admin() enables interrupts
does not work correctly, that is, interrupts can be lost while
ixgbe_handle_admin() is running.

To fix that, add new spin mutex(adapter->admmin_mtx) which protects
atomically the following two members.
    - adapter->admin_pending
    - adapter->task_requests

The unnecessary "IXGBE_WRITE_REG(hw, IXGBE_EIMC, IXGBE_EIMC_OTHER)"
code will be removed later.

Reviewed and tested by hikaru@n.o and msaitoh@n.o, thanks.

diffstat:

 sys/dev/pci/ixgbe/ixgbe.c |  109 +++++++++++++++++++++++----------------------
 sys/dev/pci/ixgbe/ixgbe.h |    8 ++-
 2 files changed, 62 insertions(+), 55 deletions(-)

diffs (224 lines):

diff -r 6d8b922e2d01 -r 71808a972381 sys/dev/pci/ixgbe/ixgbe.c
--- a/sys/dev/pci/ixgbe/ixgbe.c Tue Nov 17 03:22:33 2020 +0000
+++ b/sys/dev/pci/ixgbe/ixgbe.c Tue Nov 17 04:50:29 2020 +0000
@@ -1,4 +1,4 @@
-/* $NetBSD: ixgbe.c,v 1.259 2020/11/13 05:53:36 msaitoh Exp $ */
+/* $NetBSD: ixgbe.c,v 1.260 2020/11/17 04:50:29 knakahara Exp $ */
 
 /******************************************************************************
 
@@ -1136,6 +1136,7 @@
                goto err_late;
 
        /* Tasklets for Link, SFP, Multispeed Fiber and Flow Director */
+       mutex_init(&(adapter)->admin_mtx, MUTEX_DEFAULT, IPL_NET);
        snprintf(wqname, sizeof(wqname), "%s-admin", device_xname(dev));
        error = workqueue_create(&adapter->admin_wq, wqname,
            ixgbe_handle_admin, adapter, IXGBE_WORKQUEUE_PRI, IPL_NET,
@@ -1283,6 +1284,7 @@
        ixgbe_free_pci_resources(adapter);
        if (adapter->mta != NULL)
                free(adapter->mta, M_DEVBUF);
+       mutex_destroy(&(adapter)->admin_mtx); /* XXX appropriate order? */
        IXGBE_CORE_LOCK_DESTROY(adapter);
 
        return;
@@ -1538,10 +1540,13 @@
 ixgbe_schedule_admin_tasklet(struct adapter *adapter)
 {
 
+       KASSERT(mutex_owned(&adapter->admin_mtx));
+
        if (__predict_true(adapter->osdep.detaching == false)) {
-               if (atomic_cas_uint(&adapter->admin_pending, 0, 1) == 0)
+               if (adapter->admin_pending == 0)
                        workqueue_enqueue(adapter->admin_wq,
                            &adapter->admin_wc, NULL);
+               adapter->admin_pending = 1;
        }
 }
 
@@ -1564,8 +1569,11 @@
                        task_requests |= IXGBE_REQUEST_TASK_MSF;
                }
                task_requests |= IXGBE_REQUEST_TASK_MOD;
-               atomic_or_32(&adapter->task_requests, task_requests);
+
+               mutex_enter(&adapter->admin_mtx);
+               adapter->task_requests |= task_requests;
                ixgbe_schedule_admin_tasklet(adapter);
+               mutex_exit(&adapter->admin_mtx);
        } else {
                struct ifmedia  *ifm = &adapter->media;
 
@@ -3206,8 +3214,11 @@
        if (task_requests != 0) {
                /* Re-enabling other interrupts is done in the admin task */
                task_requests |= IXGBE_REQUEST_TASK_NEED_ACKINTR;
-               atomic_or_32(&adapter->task_requests, task_requests);
+
+               mutex_enter(&adapter->admin_mtx);
+               adapter->task_requests |= task_requests;
                ixgbe_schedule_admin_tasklet(adapter);
+               mutex_exit(&adapter->admin_mtx);
        } else {
                /* Re-enable other interrupts */
                IXGBE_WRITE_REG(hw, IXGBE_EIMS, IXGBE_EIMS_OTHER);
@@ -3758,6 +3769,7 @@
        ixgbe_free_queues(adapter);
        free(adapter->mta, M_DEVBUF);
 
+       mutex_destroy(&adapter->admin_mtx); /* XXX appropriate order? */
        IXGBE_CORE_LOCK_DESTROY(adapter);
 
        return (0);
@@ -4522,9 +4534,10 @@
                                sched_mod_task = true;
                }
                if (sched_mod_task) {
-                       atomic_or_32(&adapter->task_requests,
-                           IXGBE_REQUEST_TASK_MOD);
+                       mutex_enter(&adapter->admin_mtx);
+                       adapter->task_requests |= IXGBE_REQUEST_TASK_MOD;
                        ixgbe_schedule_admin_tasklet(adapter);
+                       mutex_exit(&adapter->admin_mtx);
                }
        }
 
@@ -4733,8 +4746,11 @@
         * MSF. At least, calling ixgbe_handle_msf on 82598 DA makes the link
         * flap because the function calls setup_link().
         */
-       if (hw->mac.type != ixgbe_mac_82598EB)
-               atomic_or_32(&adapter->task_requests, IXGBE_REQUEST_TASK_MSF);
+       if (hw->mac.type != ixgbe_mac_82598EB) {
+               mutex_enter(&adapter->admin_mtx);
+               adapter->task_requests |= IXGBE_REQUEST_TASK_MSF;
+               mutex_exit(&adapter->admin_mtx);
+       }
 
        /*
         * Don't call ixgbe_schedule_admin_tasklet() because we are on
@@ -4794,7 +4810,13 @@
        struct adapter  *adapter = context;
        struct ifnet    *ifp = adapter->ifp;
        struct ixgbe_hw *hw = &adapter->hw;
-       u32             req;
+       u32             task_requests;
+
+       mutex_enter(&adapter->admin_mtx);
+       adapter->admin_pending = 0;
+       task_requests = adapter->task_requests;
+       adapter->task_requests = 0;
+       mutex_exit(&adapter->admin_mtx);
 
        /*
         * Hold the IFNET_LOCK across this entire call.  This will
@@ -4805,52 +4827,27 @@
         */
        IFNET_LOCK(ifp);
        IXGBE_CORE_LOCK(adapter);
-       /*
-        * Clear the admin_pending flag before reading task_requests to avoid
-        * missfiring workqueue though setting task_request.
-        * Hmm, ixgbe_schedule_admin_tasklet() can extra-fire though
-        * task_requests are done by prior workqueue, but it is harmless.
-        */
-       atomic_store_relaxed(&adapter->admin_pending, 0);
-       while ((req =
-               (adapter->task_requests & ~IXGBE_REQUEST_TASK_NEED_ACKINTR))
-           != 0) {
-               if ((req & IXGBE_REQUEST_TASK_LSC) != 0) {
-                       ixgbe_handle_link(adapter);
-                       atomic_and_32(&adapter->task_requests,
-                           ~IXGBE_REQUEST_TASK_LSC);
-               }
-               if ((req & IXGBE_REQUEST_TASK_MOD) != 0) {
-                       ixgbe_handle_mod(adapter);
-                       atomic_and_32(&adapter->task_requests,
-                           ~IXGBE_REQUEST_TASK_MOD);
-               }
-               if ((req & IXGBE_REQUEST_TASK_MSF) != 0) {
-                       ixgbe_handle_msf(adapter);
-                       atomic_and_32(&adapter->task_requests,
-                           ~IXGBE_REQUEST_TASK_MSF);
-               }
-               if ((req & IXGBE_REQUEST_TASK_PHY) != 0) {
-                       ixgbe_handle_phy(adapter);
-                       atomic_and_32(&adapter->task_requests,
-                           ~IXGBE_REQUEST_TASK_PHY);
-               }
-               if ((req & IXGBE_REQUEST_TASK_FDIR) != 0) {
-                       ixgbe_reinit_fdir(adapter);
-                       atomic_and_32(&adapter->task_requests,
-                           ~IXGBE_REQUEST_TASK_FDIR);
-               }
+       if ((task_requests & IXGBE_REQUEST_TASK_LSC) != 0) {
+               ixgbe_handle_link(adapter);
+       }
+       if ((task_requests & IXGBE_REQUEST_TASK_MOD) != 0) {
+               ixgbe_handle_mod(adapter);
+       }
+       if ((task_requests & IXGBE_REQUEST_TASK_MSF) != 0) {
+               ixgbe_handle_msf(adapter);
+       }
+       if ((task_requests & IXGBE_REQUEST_TASK_PHY) != 0) {
+               ixgbe_handle_phy(adapter);
+       }
+       if ((task_requests & IXGBE_REQUEST_TASK_FDIR) != 0) {
+               ixgbe_reinit_fdir(adapter);
+       }
 #if 0 /* notyet */
-               if ((req & IXGBE_REQUEST_TASK_MBX) != 0) {
-                       ixgbe_handle_mbx(adapter);
-                       atomic_and_32(&adapter->task_requests,
-                           ~IXGBE_REQUEST_TASK_MBX);
-               }
+       if ((task_requests & IXGBE_REQUEST_TASK_MBX) != 0) {
+               ixgbe_handle_mbx(adapter);
+       }
 #endif
-       }
-       if ((adapter->task_requests & IXGBE_REQUEST_TASK_NEED_ACKINTR) != 0) {
-               atomic_and_32(&adapter->task_requests,
-                   ~IXGBE_REQUEST_TASK_NEED_ACKINTR);
+       if ((task_requests & IXGBE_REQUEST_TASK_NEED_ACKINTR) != 0) {
                if ((adapter->feat_en & IXGBE_FEATURE_MSIX) != 0) {
                        /* Re-enable other interrupts */
                        IXGBE_WRITE_REG(hw, IXGBE_EIMS, IXGBE_EIMS_OTHER);
@@ -5260,8 +5257,12 @@
        if (task_requests != 0) {
                /* Re-enabling other interrupts is done in the admin task */
                task_requests |= IXGBE_REQUEST_TASK_NEED_ACKINTR;
-               atomic_or_32(&adapter->task_requests, task_requests);
+
+               mutex_enter(&adapter->admin_mtx);
+               adapter->task_requests |= task_requests;
                ixgbe_schedule_admin_tasklet(adapter);
+               mutex_exit(&adapter->admin_mtx);
+
                reenable_intr = false;
        }
 
diff -r 6d8b922e2d01 -r 71808a972381 sys/dev/pci/ixgbe/ixgbe.h
--- a/sys/dev/pci/ixgbe/ixgbe.h Tue Nov 17 03:22:33 2020 +0000
+++ b/sys/dev/pci/ixgbe/ixgbe.h Tue Nov 17 04:50:29 2020 +0000
@@ -1,4 +1,4 @@
-/* $NetBSD: ixgbe.h,v 1.71 2020/08/31 11:19:54 msaitoh Exp $ */
+/* $NetBSD: ixgbe.h,v 1.72 2020/11/17 04:50:29 knakahara Exp $ */
 
 /******************************************************************************
   SPDX-License-Identifier: BSD-3-Clause
@@ -523,6 +523,12 @@
        struct work             admin_wc;
        u_int                   admin_pending;
        volatile u32            task_requests;
+       kmutex_t                admin_mtx; /* lock for admin_pending, task_request */
+                                          /*
+                                           * Don't acquire this mutex while
+                                           * holding rx_mtx or tx_mtx, and
+                                           * vice versa.
+                                           */
 
        bool                    txrx_use_workqueue;
 



Home | Main Index | Thread Index | Old Index