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 - Fix locking problem with optics module i...



details:   https://anonhg.NetBSD.org/src/rev/78830794f5a2
branches:  trunk
changeset: 1007046:78830794f5a2
user:      thorpej <thorpej%NetBSD.org@localhost>
date:      Tue Feb 04 19:42:55 2020 +0000

description:
- Fix locking problem with optics module interrupts: ifmedia_add()
  may block on memory allocation, and so it cannot be safely done from
  a softint nor can it be done while holding a spin lock.  Fix this by
  using a workqueue rather than a softint, and hold the IFNET_LOCK
  across the entire handler, and the CORE_LOCK only across the code that
  needs to serialize access to the hardware state.
- Use ifmedia_fini().

Tested in a variety of devices by msaitoh@.  (Thanks!)

diffstat:

 sys/dev/pci/ixgbe/ixgbe.c |  66 +++++++++++++++++++++++++++++++++++++---------
 sys/dev/pci/ixgbe/ixgbe.h |   7 +++-
 2 files changed, 57 insertions(+), 16 deletions(-)

diffs (227 lines):

diff -r 2369b22f1c40 -r 78830794f5a2 sys/dev/pci/ixgbe/ixgbe.c
--- a/sys/dev/pci/ixgbe/ixgbe.c Tue Feb 04 18:36:16 2020 +0000
+++ b/sys/dev/pci/ixgbe/ixgbe.c Tue Feb 04 19:42:55 2020 +0000
@@ -1,4 +1,4 @@
-/* $NetBSD: ixgbe.c,v 1.222 2020/02/01 12:55:22 thorpej Exp $ */
+/* $NetBSD: ixgbe.c,v 1.223 2020/02/04 19:42:55 thorpej Exp $ */
 
 /******************************************************************************
 
@@ -267,10 +267,11 @@
 /* Software interrupts for deferred work */
 static void    ixgbe_handle_que(void *);
 static void    ixgbe_handle_link(void *);
-static void    ixgbe_handle_msf(void *);
 static void    ixgbe_handle_mod(void *);
 static void    ixgbe_handle_phy(void *);
 
+static void    ixgbe_handle_msf(struct work *, void *);
+
 /* Workqueue handler for deferred work */
 static void    ixgbe_handle_que_work(struct work *, void *);
 
@@ -410,10 +411,12 @@
 #define IXGBE_CALLOUT_FLAGS    CALLOUT_MPSAFE
 #define IXGBE_SOFTINFT_FLAGS   SOFTINT_MPSAFE
 #define IXGBE_WORKQUEUE_FLAGS  WQ_PERCPU | WQ_MPSAFE
+#define IXGBE_TASKLET_WQ_FLAGS WQ_MPSAFE
 #else
 #define IXGBE_CALLOUT_FLAGS    0
 #define IXGBE_SOFTINFT_FLAGS   0
 #define IXGBE_WORKQUEUE_FLAGS  WQ_PERCPU
+#define IXGBE_TASKLET_WQ_FLAGS 0
 #endif
 #define IXGBE_WORKQUEUE_PRI PRI_SOFTNET
 
@@ -1099,8 +1102,6 @@
            ixgbe_handle_link, adapter);
        adapter->mod_si = softint_establish(SOFTINT_NET | IXGBE_SOFTINFT_FLAGS,
            ixgbe_handle_mod, adapter);
-       adapter->msf_si = softint_establish(SOFTINT_NET | IXGBE_SOFTINFT_FLAGS,
-           ixgbe_handle_msf, adapter);
        adapter->phy_si = softint_establish(SOFTINT_NET | IXGBE_SOFTINFT_FLAGS,
            ixgbe_handle_phy, adapter);
        if (adapter->feat_en & IXGBE_FEATURE_FDIR)
@@ -1108,13 +1109,23 @@
                    softint_establish(SOFTINT_NET | IXGBE_SOFTINFT_FLAGS,
                        ixgbe_reinit_fdir, adapter);
        if ((adapter->link_si == NULL) || (adapter->mod_si == NULL)
-           || (adapter->msf_si == NULL) || (adapter->phy_si == NULL)
+           || (adapter->phy_si == NULL)
            || ((adapter->feat_en & IXGBE_FEATURE_FDIR)
                && (adapter->fdir_si == NULL))) {
                aprint_error_dev(dev,
                    "could not establish software interrupts ()\n");
                goto err_out;
        }
+       char wqname[MAXCOMLEN];
+       snprintf(wqname, sizeof(wqname), "%s-msf", device_xname(dev));
+       error = workqueue_create(&adapter->msf_wq, wqname,
+           ixgbe_handle_msf, adapter, IXGBE_WORKQUEUE_PRI, IPL_NET,
+           IXGBE_TASKLET_WQ_FLAGS);
+       if (error) {
+               aprint_error_dev(dev,
+                   "could not create MSF workqueue (%d)\n", error);
+               goto err_out;
+       }
 
        error = ixgbe_start_hw(hw);
        switch (error) {
@@ -1510,7 +1521,9 @@
                if (hw->phy.multispeed_fiber) {
                        ixgbe_enable_tx_laser(hw);
                        kpreempt_disable();
-                       softint_schedule(adapter->msf_si);
+                       if (adapter->schedule_wqs_ok)
+                               workqueue_enqueue(adapter->msf_wq,
+                                   &adapter->msf_wc, NULL);
                        kpreempt_enable();
                }
                kpreempt_disable();
@@ -3088,7 +3101,9 @@
                    (eicr & IXGBE_EICR_GPI_SDP1_BY_MAC(hw))) {
                        IXGBE_WRITE_REG(hw, IXGBE_EICR,
                            IXGBE_EICR_GPI_SDP1_BY_MAC(hw));
-                       softint_schedule(adapter->msf_si);
+                       if (adapter->schedule_wqs_ok)
+                               workqueue_enqueue(adapter->msf_wq,
+                                   &adapter->msf_wc, NULL);
                }
        }
 
@@ -3500,9 +3515,9 @@
                softint_disestablish(adapter->mod_si);
                adapter->mod_si = NULL;
        }
-       if (adapter->msf_si != NULL) {
-               softint_disestablish(adapter->msf_si);
-               adapter->msf_si = NULL;
+       if (adapter->msf_wq != NULL) {
+               workqueue_destroy(adapter->msf_wq);
+               adapter->msf_wq = NULL;
        }
        if (adapter->phy_si != NULL) {
                softint_disestablish(adapter->phy_si);
@@ -3593,6 +3608,7 @@
        bus_generic_detach(dev);
 #endif
        if_detach(adapter->ifp);
+       ifmedia_fini(&adapter->media);
        if_percpuq_destroy(adapter->ipq);
 
        sysctl_teardown(&adapter->sysctllog);
@@ -4129,6 +4145,9 @@
        /* Now inform the stack we're ready */
        ifp->if_flags |= IFF_RUNNING;
 
+       /* OK to schedule workqueues. */
+       adapter->schedule_wqs_ok = true;
+
        return;
 } /* ixgbe_init_locked */
 
@@ -4657,7 +4676,8 @@
                        goto out;
                }
        }
-       softint_schedule(adapter->msf_si);
+       if (adapter->schedule_wqs_ok)
+               workqueue_enqueue(adapter->msf_wq, &adapter->msf_wc, NULL);
 out:
        IXGBE_CORE_UNLOCK(adapter);
 } /* ixgbe_handle_mod */
@@ -4667,13 +4687,23 @@
  * ixgbe_handle_msf - Tasklet for MSF (multispeed fiber) interrupts
  ************************************************************************/
 static void
-ixgbe_handle_msf(void *context)
+ixgbe_handle_msf(struct work *wk, void *context)
 {
        struct adapter  *adapter = context;
+       struct ifnet    *ifp = adapter->ifp;
        struct ixgbe_hw *hw = &adapter->hw;
        u32             autoneg;
        bool            negotiate;
 
+       /*
+        * Hold the IFNET_LOCK across this entire call.  This will
+        * prevent additional changes to adapter->phy_layer and
+        * and serialize calls to this tasklet.  We cannot hold the
+        * CORE_LOCK while calling into the ifmedia functions as
+        * they may block while allocating memory.
+        */
+       IFNET_LOCK(ifp);
+
        IXGBE_CORE_LOCK(adapter);
        ++adapter->msf_sicount.ev_count;
        /* get_supported_phy_layer will call hw->phy.ops.identify_sfp() */
@@ -4686,12 +4716,13 @@
                negotiate = 0;
        if (hw->mac.ops.setup_link)
                hw->mac.ops.setup_link(hw, autoneg, TRUE);
+       IXGBE_CORE_UNLOCK(adapter);
 
        /* Adjust media types shown in ifconfig */
        ifmedia_removeall(&adapter->media);
        ixgbe_add_media_types(adapter);
        ifmedia_set(&adapter->media, IFM_ETHER | IFM_AUTO);
-       IXGBE_CORE_UNLOCK(adapter);
+       IFNET_UNLOCK(ifp);
 } /* ixgbe_handle_msf */
 
 /************************************************************************
@@ -4723,6 +4754,8 @@
        IXGBE_CORE_LOCK(adapter);
        ixgbe_stop(adapter);
        IXGBE_CORE_UNLOCK(adapter);
+
+       workqueue_wait(adapter->msf_wq, &adapter->msf_wc);
 }
 
 /************************************************************************
@@ -4746,6 +4779,9 @@
        ixgbe_disable_intr(adapter);
        callout_stop(&adapter->timer);
 
+       /* Don't schedule workqueues. */
+       adapter->schedule_wqs_ok = false;
+
        /* Let the stack know...*/
        ifp->if_flags &= ~IFF_RUNNING;
 
@@ -5088,7 +5124,9 @@
                    (eicr & IXGBE_EICR_GPI_SDP1_BY_MAC(hw))) {
                        IXGBE_WRITE_REG(hw, IXGBE_EICR,
                            IXGBE_EICR_GPI_SDP1_BY_MAC(hw));
-                       softint_schedule(adapter->msf_si);
+                       if (adapter->schedule_wqs_ok)
+                               workqueue_enqueue(adapter->msf_wq,
+                                   &adapter->msf_wc, NULL);
                }
        }
 
diff -r 2369b22f1c40 -r 78830794f5a2 sys/dev/pci/ixgbe/ixgbe.h
--- a/sys/dev/pci/ixgbe/ixgbe.h Tue Feb 04 18:36:16 2020 +0000
+++ b/sys/dev/pci/ixgbe/ixgbe.h Tue Feb 04 19:42:55 2020 +0000
@@ -1,4 +1,4 @@
-/* $NetBSD: ixgbe.h,v 1.62 2020/01/21 14:55:55 msaitoh Exp $ */
+/* $NetBSD: ixgbe.h,v 1.63 2020/02/04 19:42:55 thorpej Exp $ */
 
 /******************************************************************************
   SPDX-License-Identifier: BSD-3-Clause
@@ -507,11 +507,14 @@
        /* Mbuf cluster size */
        u32                     rx_mbuf_sz;
 
+       bool                    schedule_wqs_ok;
+
        /* Support for pluggable optics */
        bool                    sfp_probe;
        void                    *link_si;  /* Link tasklet */
        void                    *mod_si;   /* SFP tasklet */
-       void                    *msf_si;   /* Multispeed Fiber */
+       struct workqueue        *msf_wq;   /* Multispeed Fiber */
+       struct work              msf_wc;
        void                    *mbx_si;   /* VF -> PF mailbox interrupt */
 
        /* Flow Director */



Home | Main Index | Thread Index | Old Index