Module Name: src Committed By: knakahara Date: Tue Nov 17 04:50:29 UTC 2020
Modified Files: src/sys/dev/pci/ixgbe: ixgbe.c ixgbe.h Log Message: 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. To generate a diff of this commit: cvs rdiff -u -r1.259 -r1.260 src/sys/dev/pci/ixgbe/ixgbe.c cvs rdiff -u -r1.71 -r1.72 src/sys/dev/pci/ixgbe/ixgbe.h Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
Modified files: Index: src/sys/dev/pci/ixgbe/ixgbe.c diff -u src/sys/dev/pci/ixgbe/ixgbe.c:1.259 src/sys/dev/pci/ixgbe/ixgbe.c:1.260 --- src/sys/dev/pci/ixgbe/ixgbe.c:1.259 Fri Nov 13 05:53:36 2020 +++ src/sys/dev/pci/ixgbe/ixgbe.c Tue Nov 17 04:50:29 2020 @@ -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 @@ ixgbe_attach(device_t parent, device_t d 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 @@ err_out: 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 @@ static void 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 @@ ixgbe_config_link(struct adapter *adapte 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 @@ ixgbe_msix_admin(void *arg) 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_detach(device_t dev, int flags) 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 @@ ixgbe_handle_timer(struct work *wk, void 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 @@ out: * 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 @@ ixgbe_handle_admin(struct work *wk, void 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 @@ ixgbe_handle_admin(struct work *wk, void */ 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); - } -#endif + if ((task_requests & IXGBE_REQUEST_TASK_MBX) != 0) { + ixgbe_handle_mbx(adapter); } - if ((adapter->task_requests & IXGBE_REQUEST_TASK_NEED_ACKINTR) != 0) { - atomic_and_32(&adapter->task_requests, - ~IXGBE_REQUEST_TASK_NEED_ACKINTR); +#endif + 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 @@ ixgbe_legacy_irq(void *arg) 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; } Index: src/sys/dev/pci/ixgbe/ixgbe.h diff -u src/sys/dev/pci/ixgbe/ixgbe.h:1.71 src/sys/dev/pci/ixgbe/ixgbe.h:1.72 --- src/sys/dev/pci/ixgbe/ixgbe.h:1.71 Mon Aug 31 11:19:54 2020 +++ src/sys/dev/pci/ixgbe/ixgbe.h Tue Nov 17 04:50:29 2020 @@ -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 adapter { 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;