ena: Share ena_global_lock between driver instances

In order to use `ena_global_lock` in sysctl context, it must be kept
outside the driver instance's software context, as sysctls can be called
before attach and after detach, leading to lock use before sx_init and
after sx_destroy otherwise.
Solve this issue by turning `ena_global_lock` into a file scope
variable, shared between all instances of the driver and associated
sysctl context, and in turn initialized/destroyed in dedicated
SYSINIT/SYSUNINIT functions.
As a side effect, this change also fixes existing race in the reset
routine, when simultaneously accessing sysctl exposed properties.

Obtained from: Semihalf
MFC after: 2 weeks
Sponsored by: Amazon, Inc.

4 files changed, 99 insertions, 58 deletions

diff --git a/sys/dev/ena/ena.c b/sys/dev/ena/ena.c
index 7dff59043bb2..c1b770926b0f 100644
--- a/sys/dev/ena/ena.c
+++ b/sys/dev/ena/ena.c
@@ -181,6 +181,8 @@ static ena_vendor_info_t ena_vendor_info_array[] = {
     { 0, 0, 0 }
 };
 
+struct sx ena_global_lock;
+
 /*
  * Contains pointers to event handlers, e.g. link state chage.
  */
@@ -1127,8 +1129,6 @@ ena_update_buf_ring_size(struct ena_adapter *adapter,
 	int rc = 0;
 	bool dev_was_up;
 
-	ENA_LOCK_LOCK(adapter);
-
 	old_buf_ring_size = adapter->buf_ring_size;
 	adapter->buf_ring_size = new_buf_ring_size;
 
@@ -1163,8 +1163,6 @@ ena_update_buf_ring_size(struct ena_adapter *adapter,
 		}
 	}
 
-	ENA_LOCK_UNLOCK(adapter);
-
 	return (rc);
 }
 
@@ -1176,8 +1174,6 @@ ena_update_queue_size(struct ena_adapter *adapter, uint32_t new_tx_size,
 	int rc = 0;
 	bool dev_was_up;
 
-	ENA_LOCK_LOCK(adapter);
-
 	old_tx_size = adapter->requested_tx_ring_size;
 	old_rx_size = adapter->requested_rx_ring_size;
 	adapter->requested_tx_ring_size = new_tx_size;
@@ -1218,8 +1214,6 @@ ena_update_queue_size(struct ena_adapter *adapter, uint32_t new_tx_size,
 		}
 	}
 
-	ENA_LOCK_UNLOCK(adapter);
-
 	return (rc);
 }
 
@@ -1242,8 +1236,6 @@ ena_update_io_queue_nb(struct ena_adapter *adapter, uint32_t new_num)
 	int rc = 0;
 	bool dev_was_up;
 
-	ENA_LOCK_LOCK(adapter);
-
 	dev_was_up = ENA_FLAG_ISSET(ENA_FLAG_DEV_UP, adapter);
 	old_num = adapter->num_io_queues;
 	ena_down(adapter);
@@ -1273,8 +1265,6 @@ ena_update_io_queue_nb(struct ena_adapter *adapter, uint32_t new_num)
 		}
 	}
 
-	ENA_LOCK_UNLOCK(adapter);
-
 	return (rc);
 }
 
@@ -2029,7 +2019,7 @@ ena_up(struct ena_adapter *adapter)
 {
 	int rc = 0;
 
-	ENA_LOCK_ASSERT(adapter);
+	ENA_LOCK_ASSERT();
 
 	if (unlikely(device_is_attached(adapter->pdev) == 0)) {
 		ena_log(adapter->pdev, ERR, "device is not attached!\n");
@@ -2148,13 +2138,13 @@ ena_media_status(if_t ifp, struct ifmediareq *ifmr)
 	struct ena_adapter *adapter = if_getsoftc(ifp);
 	ena_log(adapter->pdev, DBG, "Media status update\n");
 
-	ENA_LOCK_LOCK(adapter);
+	ENA_LOCK_LOCK();
 
 	ifmr->ifm_status = IFM_AVALID;
 	ifmr->ifm_active = IFM_ETHER;
 
 	if (!ENA_FLAG_ISSET(ENA_FLAG_LINK_UP, adapter)) {
-		ENA_LOCK_UNLOCK(adapter);
+		ENA_LOCK_UNLOCK();
 		ena_log(adapter->pdev, INFO, "Link is down\n");
 		return;
 	}
@@ -2162,7 +2152,7 @@ ena_media_status(if_t ifp, struct ifmediareq *ifmr)
 	ifmr->ifm_status |= IFM_ACTIVE;
 	ifmr->ifm_active |= IFM_UNKNOWN | IFM_FDX;
 
-	ENA_LOCK_UNLOCK(adapter);
+	ENA_LOCK_UNLOCK();
 }
 
 static void
@@ -2171,9 +2161,9 @@ ena_init(void *arg)
 	struct ena_adapter *adapter = (struct ena_adapter *)arg;
 
 	if (!ENA_FLAG_ISSET(ENA_FLAG_DEV_UP, adapter)) {
-		ENA_LOCK_LOCK(adapter);
+		ENA_LOCK_LOCK();
 		ena_up(adapter);
-		ENA_LOCK_UNLOCK(adapter);
+		ENA_LOCK_UNLOCK();
 	}
 }
 
@@ -2195,13 +2185,13 @@ ena_ioctl(if_t ifp, u_long command, caddr_t data)
 	case SIOCSIFMTU:
 		if (ifp->if_mtu == ifr->ifr_mtu)
 			break;
-		ENA_LOCK_LOCK(adapter);
+		ENA_LOCK_LOCK();
 		ena_down(adapter);
 
 		ena_change_mtu(ifp, ifr->ifr_mtu);
 
 		rc = ena_up(adapter);
-		ENA_LOCK_UNLOCK(adapter);
+		ENA_LOCK_UNLOCK();
 		break;
 
 	case SIOCSIFFLAGS:
@@ -2213,15 +2203,15 @@ ena_ioctl(if_t ifp, u_long command, caddr_t data)
 					    "ioctl promisc/allmulti\n");
 				}
 			} else {
-				ENA_LOCK_LOCK(adapter);
+				ENA_LOCK_LOCK();
 				rc = ena_up(adapter);
-				ENA_LOCK_UNLOCK(adapter);
+				ENA_LOCK_UNLOCK();
 			}
 		} else {
 			if ((if_getdrvflags(ifp) & IFF_DRV_RUNNING) != 0) {
-				ENA_LOCK_LOCK(adapter);
+				ENA_LOCK_LOCK();
 				ena_down(adapter);
-				ENA_LOCK_UNLOCK(adapter);
+				ENA_LOCK_UNLOCK();
 			}
 		}
 		break;
@@ -2246,10 +2236,10 @@ ena_ioctl(if_t ifp, u_long command, caddr_t data)
 
 			if ((reinit != 0) &&
 			    ((if_getdrvflags(ifp) & IFF_DRV_RUNNING) != 0)) {
-				ENA_LOCK_LOCK(adapter);
+				ENA_LOCK_LOCK();
 				ena_down(adapter);
 				rc = ena_up(adapter);
-				ENA_LOCK_UNLOCK(adapter);
+				ENA_LOCK_UNLOCK();
 			}
 		}
 
@@ -2404,7 +2394,7 @@ ena_down(struct ena_adapter *adapter)
 {
 	int rc;
 
-	ENA_LOCK_ASSERT(adapter);
+	ENA_LOCK_ASSERT();
 
 	if (!ENA_FLAG_ISSET(ENA_FLAG_DEV_UP, adapter))
 		return;
@@ -3400,12 +3390,12 @@ ena_reset_task(void *arg, int pending)
 {
 	struct ena_adapter *adapter = (struct ena_adapter *)arg;
 
-	ENA_LOCK_LOCK(adapter);
+	ENA_LOCK_LOCK();
 	if (likely(ENA_FLAG_ISSET(ENA_FLAG_TRIGGER_RESET, adapter))) {
 		ena_destroy_device(adapter, false);
 		ena_restore_device(adapter);
 	}
-	ENA_LOCK_UNLOCK(adapter);
+	ENA_LOCK_UNLOCK();
 }
 
 /**
@@ -3434,8 +3424,6 @@ ena_attach(device_t pdev)
 	adapter = device_get_softc(pdev);
 	adapter->pdev = pdev;
 
-	ENA_LOCK_INIT(adapter);
-
 	/*
 	 * Set up the timer service - driver is responsible for avoiding
 	 * concurrency, as the callout won't be using any locking inside.
@@ -3677,19 +3665,19 @@ ena_detach(device_t pdev)
 	ether_ifdetach(adapter->ifp);
 
 	/* Stop timer service */
-	ENA_LOCK_LOCK(adapter);
+	ENA_LOCK_LOCK();
 	callout_drain(&adapter->timer_service);
-	ENA_LOCK_UNLOCK(adapter);
+	ENA_LOCK_UNLOCK();
 
 	/* Release reset task */
 	while (taskqueue_cancel(adapter->reset_tq, &adapter->reset_task, NULL))
 		taskqueue_drain(adapter->reset_tq, &adapter->reset_task);
 	taskqueue_free(adapter->reset_tq);
 
-	ENA_LOCK_LOCK(adapter);
+	ENA_LOCK_LOCK();
 	ena_down(adapter);
 	ena_destroy_device(adapter, true);
-	ENA_LOCK_UNLOCK(adapter);
+	ENA_LOCK_UNLOCK();
 
 	/* Restore unregistered sysctl queue nodes. */
 	ena_sysctl_update_queue_node_nb(adapter, adapter->num_io_queues,
@@ -3723,8 +3711,6 @@ ena_detach(device_t pdev)
 
 	ena_com_delete_host_info(ena_dev);
 
-	ENA_LOCK_DESTROY(adapter);
-
 	if_free(adapter->ifp);
 
 	free(ena_dev->bus, M_DEVBUF);
@@ -3790,6 +3776,20 @@ static void ena_notification(void *adapter_data,
 	}
 }
 
+static void
+ena_lock_init(void *arg)
+{
+	ENA_LOCK_INIT();
+}
+SYSINIT(ena_lock_init, SI_SUB_LOCK, SI_ORDER_FIRST, ena_lock_init, NULL);
+
+static void
+ena_lock_uninit(void *arg)
+{
+	ENA_LOCK_DESTROY();
+}
+SYSUNINIT(ena_lock_uninit, SI_SUB_LOCK, SI_ORDER_FIRST, ena_lock_uninit, NULL);
+
 /**
  * This handler will called for unknown event group or unimplemented handlers
  **/
diff --git a/sys/dev/ena/ena.h b/sys/dev/ena/ena.h
index c7e952f5b571..0e85cb39b001 100644
--- a/sys/dev/ena/ena.h
+++ b/sys/dev/ena/ena.h
@@ -401,8 +401,6 @@ struct ena_adapter {
 	struct resource *msix;
 	int msix_rid;
 
-	struct sx global_lock;
-
 	/* MSI-X */
 	struct msix_entry *msix_entries;
 	int msix_vecs;
@@ -482,17 +480,18 @@ struct ena_adapter {
 #define ENA_RING_MTX_ASSERT(_ring)		\
 	mtx_assert(&(_ring)->ring_mtx, MA_OWNED)
 
-#define ENA_LOCK_INIT(adapter)			\
-	sx_init(&(adapter)->global_lock, "ENA global lock")
-#define ENA_LOCK_DESTROY(adapter)	sx_destroy(&(adapter)->global_lock)
-#define ENA_LOCK_LOCK(adapter)		sx_xlock(&(adapter)->global_lock)
-#define ENA_LOCK_UNLOCK(adapter)	sx_unlock(&(adapter)->global_lock)
-#define ENA_LOCK_ASSERT(adapter)		\
-	sx_assert(&(adapter)->global_lock, SA_XLOCKED)
+#define ENA_LOCK_INIT()					\
+	sx_init(&ena_global_lock,	"ENA global lock")
+#define ENA_LOCK_DESTROY()		sx_destroy(&ena_global_lock)
+#define ENA_LOCK_LOCK()			sx_xlock(&ena_global_lock)
+#define ENA_LOCK_UNLOCK()		sx_unlock(&ena_global_lock)
+#define ENA_LOCK_ASSERT()		sx_assert(&ena_global_lock, SA_XLOCKED)
 
 #define clamp_t(type, _x, min, max)	min_t(type, max_t(type, _x, min), max)
 #define clamp_val(val, lo, hi)		clamp_t(__typeof(val), val, lo, hi)
 
+extern struct sx ena_global_lock;
+
 static inline int ena_mbuf_count(struct mbuf *mbuf)
 {
 	int count = 1;
diff --git a/sys/dev/ena/ena_netmap.c b/sys/dev/ena/ena_netmap.c
index 74cdea6c34fa..1525b1efd954 100644
--- a/sys/dev/ena/ena_netmap.c
+++ b/sys/dev/ena/ena_netmap.c
@@ -278,7 +278,7 @@ ena_netmap_reg(struct netmap_adapter *na, int onoff)
 	enum txrx t;
 	int rc, i;
 
-	ENA_LOCK_LOCK(adapter);
+	ENA_LOCK_LOCK();
 	ENA_FLAG_CLEAR_ATOMIC(ENA_FLAG_TRIGGER_RESET, adapter);
 	ena_down(adapter);
 
@@ -315,7 +315,7 @@ ena_netmap_reg(struct netmap_adapter *na, int onoff)
 		ENA_FLAG_SET_ATOMIC(ENA_FLAG_DEV_UP_BEFORE_RESET, adapter);
 		rc = ena_restore_device(adapter);
 	}
-	ENA_LOCK_UNLOCK(adapter);
+	ENA_LOCK_UNLOCK();
 
 	return (rc);
 }
diff --git a/sys/dev/ena/ena_sysctl.c b/sys/dev/ena/ena_sysctl.c
index c9a5419811a8..cc8dff4af0c0 100644
--- a/sys/dev/ena/ena_sysctl.c
+++ b/sys/dev/ena/ena_sysctl.c
@@ -442,6 +442,12 @@ ena_sysctl_buf_ring_size(SYSCTL_HANDLER_ARGS)
 	uint32_t val;
 	int error;
 
+	ENA_LOCK_LOCK();
+	if (unlikely(!ENA_FLAG_ISSET(ENA_FLAG_DEVICE_RUNNING, adapter))) {
+		error = EINVAL;
+		goto unlock;
+	}
+
 	val = 0;
 	error = sysctl_wire_old_buffer(req, sizeof(val));
 	if (error == 0) {
@@ -449,13 +455,14 @@ ena_sysctl_buf_ring_size(SYSCTL_HANDLER_ARGS)
 		error = sysctl_handle_32(oidp, &val, 0, req);
 	}
 	if (error != 0 || req->newptr == NULL)
-		return (error);
+		goto unlock;
 
 	if (!powerof2(val) || val == 0) {
 		ena_log(adapter->pdev, ERR,
 		    "Requested new Tx buffer ring size (%u) is not a power of 2\n",
 		    val);
-		return (EINVAL);
+		error = EINVAL;
+		goto unlock;
 	}
 
 	if (val != adapter->buf_ring_size) {
@@ -470,6 +477,9 @@ ena_sysctl_buf_ring_size(SYSCTL_HANDLER_ARGS)
 		    adapter->buf_ring_size);
 	}
 
+unlock:
+	ENA_LOCK_UNLOCK();
+
 	return (error);
 }
 
@@ -480,6 +490,12 @@ ena_sysctl_rx_queue_size(SYSCTL_HANDLER_ARGS)
 	uint32_t val;
 	int error;
 
+	ENA_LOCK_LOCK();
+	if (unlikely(!ENA_FLAG_ISSET(ENA_FLAG_DEVICE_RUNNING, adapter))) {
+		error = EINVAL;
+		goto unlock;
+	}
+
 	val = 0;
 	error = sysctl_wire_old_buffer(req, sizeof(val));
 	if (error == 0) {
@@ -487,13 +503,14 @@ ena_sysctl_rx_queue_size(SYSCTL_HANDLER_ARGS)
 		error = sysctl_handle_32(oidp, &val, 0, req);
 	}
 	if (error != 0 || req->newptr == NULL)
-		return (error);
+		goto unlock;
 
 	if  (val < ENA_MIN_RING_SIZE || val > adapter->max_rx_ring_size) {
 		ena_log(adapter->pdev, ERR,
 		    "Requested new Rx queue size (%u) is out of range: [%u, %u]\n",
 		    val, ENA_MIN_RING_SIZE, adapter->max_rx_ring_size);
-		return (EINVAL);
+		error = EINVAL;
+		goto unlock;
 	}
 
 	/* Check if the parameter is power of 2 */
@@ -501,7 +518,8 @@ ena_sysctl_rx_queue_size(SYSCTL_HANDLER_ARGS)
 		ena_log(adapter->pdev, ERR,
 		    "Requested new Rx queue size (%u) is not a power of 2\n",
 		    val);
-		return (EINVAL);
+		error = EINVAL;
+		goto unlock;
 	}
 
 	if (val != adapter->requested_rx_ring_size) {
@@ -517,6 +535,9 @@ ena_sysctl_rx_queue_size(SYSCTL_HANDLER_ARGS)
 		    adapter->requested_rx_ring_size);
 	}
 
+unlock:
+	ENA_LOCK_UNLOCK();
+
 	return (error);
 }
 
@@ -530,18 +551,25 @@ ena_sysctl_io_queues_nb(SYSCTL_HANDLER_ARGS)
 	uint32_t old_num_queues, tmp = 0;
 	int error;
 
+	ENA_LOCK_LOCK();
+	if (unlikely(!ENA_FLAG_ISSET(ENA_FLAG_DEVICE_RUNNING, adapter))) {
+		error = EINVAL;
+		goto unlock;
+	}
+
 	error = sysctl_wire_old_buffer(req, sizeof(tmp));
 	if (error == 0) {
 		tmp = adapter->num_io_queues;
 		error = sysctl_handle_int(oidp, &tmp, 0, req);
 	}
 	if (error != 0 || req->newptr == NULL)
-		return (error);
+		goto unlock;
 
 	if (tmp == 0) {
 		ena_log(adapter->pdev, ERR,
 		    "Requested number of IO queues is zero\n");
-		return (EINVAL);
+		error = EINVAL;
+		goto unlock;
 	}
 
 	/*
@@ -555,7 +583,8 @@ ena_sysctl_io_queues_nb(SYSCTL_HANDLER_ARGS)
 		ena_log(adapter->pdev, ERR,
 		    "Requested number of IO queues is higher than maximum "
 		    "allowed (%u)\n", adapter->msix_vecs - ENA_ADMIN_MSIX_VEC);
-		return (EINVAL);
+		error = EINVAL;
+		goto unlock;
 	}
 	if (tmp == adapter->num_io_queues) {
 		ena_log(adapter->pdev, ERR,
@@ -574,6 +603,9 @@ ena_sysctl_io_queues_nb(SYSCTL_HANDLER_ARGS)
 		ena_sysctl_update_queue_node_nb(adapter, old_num_queues, tmp);
 	}
 
+unlock:
+	ENA_LOCK_UNLOCK();
+
 	return (error);
 }
 
@@ -584,19 +616,26 @@ ena_sysctl_eni_metrics_interval(SYSCTL_HANDLER_ARGS)
 	uint16_t interval;
 	int error;
 
+	ENA_LOCK_LOCK();
+	if (unlikely(!ENA_FLAG_ISSET(ENA_FLAG_DEVICE_RUNNING, adapter))) {
+		error = EINVAL;
+		goto unlock;
+	}
+
 	error = sysctl_wire_old_buffer(req, sizeof(interval));
 	if (error == 0) {
 		interval = adapter->eni_metrics_sample_interval;
 		error = sysctl_handle_16(oidp, &interval, 0, req);
 	}
 	if (error != 0 || req->newptr == NULL)
-		return (error);
+		goto unlock;
 
 	if (interval > ENI_METRICS_MAX_SAMPLE_INTERVAL) {
 		ena_log(adapter->pdev, ERR,
 		    "ENI metrics update interval is out of range - maximum allowed value: %d seconds\n",
 		    ENI_METRICS_MAX_SAMPLE_INTERVAL);
-		return (EINVAL);
+		error = EINVAL;
+		goto unlock;
 	}
 
 	if (interval == 0) {
@@ -611,5 +650,8 @@ ena_sysctl_eni_metrics_interval(SYSCTL_HANDLER_ARGS)
 
 	adapter->eni_metrics_sample_interval = interval;
 
+unlock:
+	ENA_LOCK_UNLOCK();
+
 	return (0);
 }