13359 mlxcx_update_link_state can race against mlxcx_register_mac

13370 mlxcx_intr_n doing redundant check on mleqe_event_type
Reviewed by: Robert Mustacchi <rm@fingolfin.org>
Reviewed by: Andy Fiddaman <andy@omnios.org>
Reviewed by: Paul Winder <paul@winder.uk.net>
Approved by: Dan McDonald <danmcd@joyent.com>

4 files changed, 81 insertions, 17 deletions

diff --git a/usr/src/uts/common/io/mlxcx/mlxcx.c b/usr/src/uts/common/io/mlxcx/mlxcx.c
index 9aae5244de..f74d093b9c 100644
--- a/usr/src/uts/common/io/mlxcx/mlxcx.c
+++ b/usr/src/uts/common/io/mlxcx/mlxcx.c
@@ -10,7 +10,7 @@
  */
 
 /*
- * Copyright 2020, The University of Queensland
+ * Copyright 2021, The University of Queensland
  * Copyright (c) 2018, Joyent, Inc.
  * Copyright 2020 RackTop Systems, Inc.
  */
@@ -2365,12 +2365,28 @@ mlxcx_setup_eq(mlxcx_t *mlxp, uint_t vec, uint64_t events)
 		return (B_FALSE);
 	}
 	mleq->mleq_state |= MLXCX_EQ_INTR_ENABLED;
+	mleq->mleq_state |= MLXCX_EQ_ATTACHING;
 	mlxcx_arm_eq(mlxp, mleq);
 	mutex_exit(&mleq->mleq_mtx);
 
 	return (B_TRUE);
 }
 
+static void
+mlxcx_eq_set_attached(mlxcx_t *mlxp)
+{
+	uint_t vec;
+	mlxcx_event_queue_t *mleq;
+
+	for (vec = 0; vec < mlxp->mlx_intr_count; ++vec) {
+		mleq = &mlxp->mlx_eqs[vec];
+
+		mutex_enter(&mleq->mleq_mtx);
+		mleq->mleq_state &= ~MLXCX_EQ_ATTACHING;
+		mutex_exit(&mleq->mleq_mtx);
+	}
+}
+
 static boolean_t
 mlxcx_setup_async_eqs(mlxcx_t *mlxp)
 {
@@ -2764,7 +2780,10 @@ mlxcx_attach(dev_info_t *dip, ddi_attach_cmd_t cmd)
 	 * Set up asynchronous event queue which handles control type events
 	 * like PAGE_REQUEST and CMD completion events.
 	 *
-	 * This will enable and arm the interrupt on EQ 0.
+	 * This will enable and arm the interrupt on EQ 0. Note that only page
+	 * reqs and cmd completions will be handled until we call
+	 * mlxcx_eq_set_attached further down (this way we don't need an extra
+	 * set of locks over the mlxcx_t sub-structs not allocated yet)
 	 */
 	if (!mlxcx_setup_async_eqs(mlxp)) {
 		goto err;
@@ -2891,6 +2910,12 @@ mlxcx_attach(dev_info_t *dip, ddi_attach_cmd_t cmd)
 	}
 	mlxp->mlx_attach |= MLXCX_ATTACH_MAC_HDL;
 
+	/*
+	 * This tells the interrupt handlers they can start processing events
+	 * other than cmd completions and page requests.
+	 */
+	mlxcx_eq_set_attached(mlxp);
+
 	return (DDI_SUCCESS);
 
 err:
diff --git a/usr/src/uts/common/io/mlxcx/mlxcx.h b/usr/src/uts/common/io/mlxcx/mlxcx.h
index e28fe89806..68da65765f 100644
--- a/usr/src/uts/common/io/mlxcx/mlxcx.h
+++ b/usr/src/uts/common/io/mlxcx/mlxcx.h
@@ -10,7 +10,7 @@
  */
 
 /*
- * Copyright 2020, The University of Queensland
+ * Copyright 2021, The University of Queensland
  * Copyright (c) 2018, Joyent, Inc.
  * Copyright 2020 RackTop Systems, Inc.
  */
@@ -318,6 +318,7 @@ typedef enum {
 	MLXCX_EQ_INTR_ENABLED	= 1 << 5,	/* ddi_intr_enable()'d */
 	MLXCX_EQ_INTR_ACTIVE	= 1 << 6,	/* 'rupt handler running */
 	MLXCX_EQ_INTR_QUIESCE	= 1 << 7,	/* 'rupt handler to quiesce */
+	MLXCX_EQ_ATTACHING	= 1 << 8,	/* mlxcx_attach still running */
 } mlxcx_eventq_state_t;
 
 typedef struct mlxcx_bf {
diff --git a/usr/src/uts/common/io/mlxcx/mlxcx_gld.c b/usr/src/uts/common/io/mlxcx/mlxcx_gld.c
index 941eb0f9e7..2c41f4ddeb 100644
--- a/usr/src/uts/common/io/mlxcx/mlxcx_gld.c
+++ b/usr/src/uts/common/io/mlxcx/mlxcx_gld.c
@@ -10,7 +10,7 @@
  */
 
 /*
- * Copyright (c) 2020, the University of Queensland
+ * Copyright (c) 2021, the University of Queensland
  * Copyright 2020 RackTop Systems, Inc.
  */
 
@@ -1493,6 +1493,8 @@ mlxcx_register_mac(mlxcx_t *mlxp)
 	VERIFY3U(mlxp->mlx_nports, ==, 1);
 	port = &mlxp->mlx_ports[0];
 
+	mutex_enter(&port->mlp_mtx);
+
 	mac->m_type_ident = MAC_PLUGIN_IDENT_ETHER;
 	mac->m_driver = mlxp;
 	mac->m_dip = mlxp->mlx_dip;
@@ -1510,6 +1512,8 @@ mlxcx_register_mac(mlxcx_t *mlxp)
 	}
 	mac_free(mac);
 
+	mutex_exit(&port->mlp_mtx);
+
 	mlxcx_update_link_state(mlxp, port);
 
 	return (ret == 0);
diff --git a/usr/src/uts/common/io/mlxcx/mlxcx_intr.c b/usr/src/uts/common/io/mlxcx/mlxcx_intr.c
index 53ea4d683e..e2f5141171 100644
--- a/usr/src/uts/common/io/mlxcx/mlxcx_intr.c
+++ b/usr/src/uts/common/io/mlxcx/mlxcx_intr.c
@@ -10,7 +10,7 @@
  */
 
 /*
- * Copyright (c) 2020, the University of Queensland
+ * Copyright (c) 2021, the University of Queensland
  * Copyright 2020 RackTop Systems, Inc.
  * Copyright 2020 OmniOS Community Edition (OmniOSce) Association.
  */
@@ -422,7 +422,9 @@ mlxcx_update_link_state(mlxcx_t *mlxp, mlxcx_port_t *port)
 	default:
 		ls = LINK_STATE_UNKNOWN;
 	}
-	mac_link_update(mlxp->mlx_mac_hdl, ls);
+
+	if (mlxp->mlx_mac_hdl != NULL)
+		mac_link_update(mlxp->mlx_mac_hdl, ls);
 
 	mutex_exit(&port->mlp_mtx);
 }
@@ -762,10 +764,16 @@ mlxcx_intr_async(caddr_t arg, caddr_t arg2)
 		DTRACE_PROBE2(event, mlxcx_t *, mlxp, mlxcx_eventq_ent_t *,
 		    ent);
 
+		/*
+		 * Handle events which can be processed while we're still in
+		 * mlxcx_attach(). Everything on the mlxcx_t which these events
+		 * use must be allocated and set up prior to the call to
+		 * mlxcx_setup_async_eqs().
+		 */
 		switch (ent->mleqe_event_type) {
 		case MLXCX_EVENT_CMD_COMPLETION:
 			mlxcx_cmd_completion(mlxp, ent);
-			break;
+			continue;
 		case MLXCX_EVENT_PAGE_REQUEST:
 			func = from_be16(ent->mleqe_page_request.
 			    mled_page_request_function_id);
@@ -783,7 +791,7 @@ mlxcx_intr_async(caddr_t arg, caddr_t arg2)
 				mutex_exit(&param->mla_mtx);
 				mlxcx_warn(mlxp, "Unexpected page request "
 				    "whilst another is pending");
-				break;
+				continue;
 			}
 			param->mla_pages.mlp_npages =
 			    (int32_t)from_be32(ent->mleqe_page_request.
@@ -795,7 +803,20 @@ mlxcx_intr_async(caddr_t arg, caddr_t arg2)
 
 			taskq_dispatch_ent(mlxp->mlx_async_tq, mlxcx_pages_task,
 			    param, 0, &param->mla_tqe);
-			break;
+			continue;
+		}
+
+		/*
+		 * All other events should be ignored while in attach.
+		 */
+		mutex_enter(&mleq->mleq_mtx);
+		if (mleq->mleq_state & MLXCX_EQ_ATTACHING) {
+			mutex_exit(&mleq->mleq_mtx);
+			continue;
+		}
+		mutex_exit(&mleq->mleq_mtx);
+
+		switch (ent->mleqe_event_type) {
 		case MLXCX_EVENT_PORT_STATE:
 			portn = get_bits8(
 			    ent->mleqe_port_state.mled_port_state_port_num,
@@ -1055,17 +1076,30 @@ mlxcx_intr_n(caddr_t arg, caddr_t arg2)
 	}
 	mleq->mleq_badintrs = 0;
 
+	mutex_enter(&mleq->mleq_mtx);
 	ASSERT(mleq->mleq_state & MLXCX_EQ_ARMED);
 	mleq->mleq_state &= ~MLXCX_EQ_ARMED;
+#if defined(DEBUG)
+	/*
+	 * If we're still in mlxcx_attach and an intr_n fired, something really
+	 * weird is going on. This shouldn't happen in the absence of a driver
+	 * or firmware bug, so in the interests of minimizing branches in this
+	 * function this check is under DEBUG.
+	 */
+	if (mleq->mleq_state & MLXCX_EQ_ATTACHING) {
+		mutex_exit(&mleq->mleq_mtx);
+		mlxcx_warn(mlxp, "intr_n (%u) fired during attach, disabling "
+		    "vector", mleq->mleq_intr_index);
+		mlxcx_fm_ereport(mlxp, DDI_FM_DEVICE_INVAL_STATE);
+		ddi_fm_service_impact(mlxp->mlx_dip, DDI_SERVICE_LOST);
+		(void) ddi_intr_disable(mlxp->mlx_intr_handles[
+		    mleq->mleq_intr_index]);
+		goto done;
+	}
+#endif
+	mutex_exit(&mleq->mleq_mtx);
 
 	for (; ent != NULL; ent = mlxcx_eq_next(mleq)) {
-		if (ent->mleqe_event_type != MLXCX_EVENT_COMPLETION) {
-			mlxcx_fm_ereport(mlxp, DDI_FM_DEVICE_INVAL_STATE);
-			ddi_fm_service_impact(mlxp->mlx_dip, DDI_SERVICE_LOST);
-			(void) ddi_intr_disable(mlxp->mlx_intr_handles[
-			    mleq->mleq_intr_index]);
-			goto done;
-		}
 		ASSERT3U(ent->mleqe_event_type, ==, MLXCX_EVENT_COMPLETION);
 
 		probe.mlcq_num =
@@ -1075,7 +1109,7 @@ mlxcx_intr_n(caddr_t arg, caddr_t arg2)
 		mutex_exit(&mleq->mleq_mtx);
 
 		if (mlcq == NULL)
-			continue;
+			goto update_eq;
 
 		mlwq = mlcq->mlcq_wq;