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;
 

Reply via email to