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/1eb2dc4282a3
branches: trunk
changeset: 946124:1eb2dc4282a3
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 e86b5c650447 -r 1eb2dc4282a3 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 e86b5c650447 -r 1eb2dc4282a3 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