[PATCH net 2/3] enic: retain admin-channel DMA until queues stop

[PATCH net 2/3] enic: retain admin-channel DMA until queues stop

[Satish Kharat]

A timed-out admin WQ send leaves ownership of its descriptor and mapped
message unresolved. Independently, an admin RQ disable can time out
without observing RUNNING == 0, leaving DMA ownership of its posted
receive buffers and associated completion state unresolved. Channel close
currently logs WQ and RQ disable failures, then cleans and frees the full
WQ/RQ/CQ allocation graph.

Track the admin channel as DOWN, LIVE, or QUARANTINED. Block new
mailbox sends, stop its CPU callbacks, and take mbox_lock to wait for any
in-flight sender before disabling the queues. Release the graph only after
the WQ disable succeeds and both passes of the RQ disable succeed. The
preceding datapath fix gives every RUNNING poll a driver-chosen 500 ms
bound and preserves the two RQ passes required for its mini-cache. Run
each queue's complete disable helper once, then retry once only a helper
that timed out. Quarantine the graph if either retry still times out.

A mailbox send timeout alone is not terminal. Its mapping remains
attached to the live WQ, and a later clean WQ/RQ stop supplies
the ownership proof needed to release the graph. Only failure of a
queue-disable RUNNING check makes the admin graph quarantined.

Keep the admin and generic datapath ownership graphs independent. If
the admin graph is quarantined, stop the generic datapath and release
its queue resources only if all of its work and receive queues stop.
Likewise, a quarantined datapath does not prevent release of an admin
graph whose queues both stop. Do not partially release an individual graph
when only some of its queues stop. Retain the shared netdev, vnic_dev, BAR
mappings, PCI reservations, and any unresolved vNIC DMA target whenever
either complete graph remains quarantined.

Soft reset, hang reset, and PCI function reset do not provide an
independent ownership fence for a queue whose stop could not be
confirmed. Clear bus mastering to contain the function, detach software
entry points, and retain each quarantined graph during runtime. Clearing
bus mastering does not cancel DMA already in progress or prevent the final
remove-time
RUNNING recheck from observing a queue which subsequently drains.

Apply the ownership rules to partial open, reset, SR-IOV disable,
probe unwind, and remove. A reset worker encountering terminal admin
quarantine only converges the independent datapath to a stopped state;
it does not issue a firmware reset or reopen either graph.

At remove, after all CPU entry points have been detached, make one final
direct disable/RUNNING check of the quarantined admin WQ and RQ. Release
the complete admin graph if both queues have now stopped. Otherwise
retain it with the shared owner objects until reboot.

The issue was found while reviewing the V2 VF receive-control series
after its mailbox timeout path deliberately retained a DMA mapping. The
audit showed that admin-channel close could nevertheless free its owner
graph after an unconfirmed queue stop.

Fixes: 3258931d4052 ("enic: add admin channel open and close for SR-IOV")
Assisted-by: LLM
Signed-off-by: Satish Kharat <satishkh@cisco.com>
Reviewed-by: Aravind Dhanasekaran <aradhana@cisco.com>
Reviewed-by: Reese Faucette <rfaucett@cisco.com>
---
 drivers/net/ethernet/cisco/enic/enic.h       |  13 +-
 drivers/net/ethernet/cisco/enic/enic_admin.c | 171 +++++++++++++++++++-------
 drivers/net/ethernet/cisco/enic/enic_admin.h |   4 +-
 drivers/net/ethernet/cisco/enic/enic_main.c  | 175 ++++++++++++++++++++-------
 drivers/net/ethernet/cisco/enic/enic_mbox.c  |   1 +
 5 files changed, 274 insertions(+), 90 deletions(-)

diff --git a/drivers/net/ethernet/cisco/enic/enic.h b/drivers/net/ethernet/cisco/enic/enic.h
index 992ac43d1b0a..cf2dee8fa78f 100644
--- a/drivers/net/ethernet/cisco/enic/enic.h
+++ b/drivers/net/ethernet/cisco/enic/enic.h
@@ -306,13 +306,16 @@ struct enic {
 
 	/* Admin channel resources for SR-IOV MBOX */
 	bool has_admin_channel;
-	/* true only while the admin WQ/RQ/CQ are allocated and enabled; gates
-	 * enic_admin_channel_close() so it is a no-op after a failed (re)open
-	 * left the resources freed.
+	/* DOWN has no allocated admin DMA resources. QUARANTINED retains the
+	 * complete graph after a WQ or RQ failed to stop.
 	 */
-	bool admin_chan_up;
-	/* set on send timeout; cleared on channel re-open */
+	enum enic_dma_state admin_chan_state;
+	/* Blocks sends while the channel is closed or awaiting recovery. */
 	bool mbox_send_disabled;
+	/* A send timeout leaves a descriptor hardware-owned until both queues
+	 * report RUNNING == 0.
+	 */
+	bool mbox_tx_poisoned;
 	struct vnic_wq admin_wq;
 	struct vnic_rq admin_rq;
 	struct vnic_cq admin_cq[2];
diff --git a/drivers/net/ethernet/cisco/enic/enic_admin.c b/drivers/net/ethernet/cisco/enic/enic_admin.c
index 61c82b48044d..a5c08cbcb339 100644
--- a/drivers/net/ethernet/cisco/enic/enic_admin.c
+++ b/drivers/net/ethernet/cisco/enic/enic_admin.c
@@ -530,11 +530,22 @@ static void enic_admin_msg_drain(struct enic *enic)
 
 int enic_admin_channel_open(struct enic *enic)
 {
+	int close_err;
 	int err;
 
 	if (!enic->has_admin_channel)
 		return -ENODEV;
-
+	if (READ_ONCE(enic->admin_chan_state) != ENIC_DMA_DOWN) {
+		netdev_err(enic->netdev,
+			   "Refusing to open non-down admin channel (%u)\n",
+			   READ_ONCE(enic->admin_chan_state));
+		return -EBUSY;
+	}
+	if (READ_ONCE(enic->mbox_tx_poisoned)) {
+		netdev_err(enic->netdev,
+			   "Refusing to reopen poisoned admin channel\n");
+		return -EIO;
+	}
 	/* Keep MBOX sends disabled for the entire open sequence.  It is
 	 * cleared only after every resource is allocated and enabled below,
 	 * so any early error return here leaves sends disabled and a
@@ -567,6 +578,7 @@ int enic_admin_channel_open(struct enic *enic)
 
 	vnic_wq_enable(&enic->admin_wq);
 	vnic_rq_enable(&enic->admin_rq);
+	WRITE_ONCE(enic->admin_chan_state, ENIC_DMA_LIVE);
 
 	err = enic_admin_rq_fill(enic, GFP_KERNEL);
 	if (err) {
@@ -600,68 +612,141 @@ int enic_admin_channel_open(struct enic *enic)
 		   enic->admin_cq[0].last_color,
 		   enic->admin_cq[1].last_color);
 
-	enic->admin_chan_up = true;
-
 	return 0;
 
 disable_queues:
-	enic_admin_teardown_intr(enic);
-	enic_admin_qp_type_set(enic, QP_DISABLE);
-	if (vnic_wq_disable(&enic->admin_wq))
-		netdev_warn(enic->netdev, "Failed to disable admin WQ\n");
-	if (vnic_rq_disable(&enic->admin_rq))
-		netdev_warn(enic->netdev, "Failed to disable admin RQ\n");
-	cancel_work_sync(&enic->admin_msg_work);
-	enic_admin_msg_drain(enic);
-	enic_admin_rq_drain(enic);
+	close_err = enic_admin_channel_close(enic);
+	if (close_err)
+		netdev_err(enic->netdev,
+			   "Failed to close partial admin channel: %d\n",
+			   close_err);
+	return err;
 free_resources:
 	enic_admin_free_resources(enic);
 	return err;
 }
 
-void enic_admin_channel_close(struct enic *enic)
+static void enic_admin_channel_release(struct enic *enic)
 {
-	int err;
+	vnic_wq_clean(&enic->admin_wq, enic_admin_wq_buf_clean);
+	enic_admin_rq_drain(enic);
+	vnic_cq_clean(&enic->admin_cq[0]);
+	vnic_cq_clean(&enic->admin_cq[1]);
+	vnic_intr_clean(&enic->admin_intr);
 
-	if (!enic->has_admin_channel)
-		return;
+	enic->admin_rq_handler = NULL;
+	enic_admin_free_resources(enic);
+	WRITE_ONCE(enic->admin_chan_state, ENIC_DMA_DOWN);
+}
 
-	/* Nothing to tear down if the channel was never (re)opened, e.g. a
-	 * failed enic_admin_channel_open() in probe or in the reset path;
-	 * otherwise the disable/clean calls below dereference freed resources.
-	 */
-	if (!enic->admin_chan_up)
-		return;
+int enic_admin_channel_close(struct enic *enic)
+{
+	int qp_err;
+	int rq_err;
+	int wq_err;
+
+	if (!enic->has_admin_channel)
+		return 0;
+	if (READ_ONCE(enic->admin_chan_state) == ENIC_DMA_DOWN)
+		return 0;
+	if (READ_ONCE(enic->admin_chan_state) == ENIC_DMA_QUARANTINED)
+		return -EBUSY;
 
 	WRITE_ONCE(enic->mbox_send_disabled, true);
 
-	netdev_dbg(enic->netdev, "admin channel close\n");
+	if (READ_ONCE(enic->admin_chan_state) == ENIC_DMA_LIVE) {
+		netdev_dbg(enic->netdev, "admin channel quiesce\n");
 
-	vnic_intr_mask(&enic->admin_intr);
-	enic_admin_teardown_intr(enic);
-	cancel_work_sync(&enic->link_notify_work);
-	cancel_work_sync(&enic->admin_msg_work);
-	enic_admin_msg_drain(enic);
+		vnic_intr_mask(&enic->admin_intr);
+		enic_admin_teardown_intr(enic);
+		cancel_work_sync(&enic->link_notify_work);
+		cancel_work_sync(&enic->admin_msg_work);
+		enic_admin_msg_drain(enic);
+	}
 
-	enic_admin_qp_type_set(enic, QP_DISABLE);
+	/* A sender may have passed the gate before it was raised.  Wait only
+	 * after cancelling workers which can themselves wait for mbox_lock.
+	 */
+	mutex_lock(&enic->mbox_lock);
+	qp_err = enic_admin_qp_type_set(enic, QP_DISABLE);
+	if (qp_err)
+		netdev_warn(enic->netdev,
+			    "Failed to disable admin QP type: %d\n", qp_err);
 
-	err = vnic_wq_disable(&enic->admin_wq);
-	if (err)
+	wq_err = vnic_wq_disable(&enic->admin_wq);
+	rq_err = vnic_rq_disable(&enic->admin_rq);
+
+	/* Run both complete disable helpers once before retrying only a helper
+	 * which timed out.
+	 */
+	if (wq_err)
+		wq_err = vnic_wq_disable(&enic->admin_wq);
+	if (rq_err)
+		rq_err = vnic_rq_disable(&enic->admin_rq);
+
+	if (wq_err)
 		netdev_warn(enic->netdev,
-			    "Failed to disable admin WQ: %d\n", err);
-	err = vnic_rq_disable(&enic->admin_rq);
-	if (err)
+			    "Failed to disable admin WQ after retry: %d\n",
+			    wq_err);
+	if (rq_err)
 		netdev_warn(enic->netdev,
-			    "Failed to disable admin RQ: %d\n", err);
+			    "Failed to disable admin RQ after retry: %d\n",
+			    rq_err);
 
-	vnic_wq_clean(&enic->admin_wq, enic_admin_wq_buf_clean);
-	enic_admin_rq_drain(enic);
-	vnic_cq_clean(&enic->admin_cq[0]);
-	vnic_cq_clean(&enic->admin_cq[1]);
-	vnic_intr_clean(&enic->admin_intr);
+	/* QP_DISABLE does not report every low-level stop failure.  The direct
+	 * WQ/RQ RUNNING observations own the admin allocation graph.
+	 */
+	if (wq_err || rq_err) {
+		WRITE_ONCE(enic->admin_chan_state, ENIC_DMA_QUARANTINED);
+		WRITE_ONCE(enic->mbox_tx_poisoned, true);
+		mutex_unlock(&enic->mbox_lock);
+		netif_device_detach(enic->netdev);
+		netif_carrier_off(enic->netdev);
+		/* Stop new PCIe requests without treating bus-master disable as
+		 * proof that already-issued DMA has completed.
+		 */
+		pci_clear_master(enic->pdev);
+		return wq_err ?: rq_err;
+	}
 
-	enic->admin_rq_handler = NULL;
-	enic_admin_free_resources(enic);
+	enic_admin_channel_release(enic);
+	WRITE_ONCE(enic->mbox_tx_poisoned, false);
+	mutex_unlock(&enic->mbox_lock);
+
+	/* Firmware rejection does not undermine the WQ/RQ proof. */
+	return qp_err;
+}
+
+int enic_admin_channel_recheck_on_remove(struct enic *enic)
+{
+	int rq_err;
+	int wq_err;
+
+	if (READ_ONCE(enic->admin_chan_state) != ENIC_DMA_QUARANTINED)
+		return 0;
+
+	/* Channel close already detached every CPU entry point and attempted
+	 * QP_DISABLE.  Removal is the last direct check of queue ownership.
+	 */
+	mutex_lock(&enic->mbox_lock);
+	wq_err = vnic_wq_disable(&enic->admin_wq);
+	rq_err = vnic_rq_disable(&enic->admin_rq);
+	if (wq_err || rq_err) {
+		mutex_unlock(&enic->mbox_lock);
+		return wq_err ?: rq_err;
+	}
 
-	enic->admin_chan_up = false;
+	enic_admin_channel_release(enic);
+	WRITE_ONCE(enic->mbox_tx_poisoned, false);
+	mutex_unlock(&enic->mbox_lock);
+	netdev_info(enic->netdev,
+		    "Admin queues stopped at remove; releasing DMA graph\n");
+
+	return 0;
+}
+
+bool enic_admin_channel_is_quarantined(const struct enic *enic)
+{
+	return READ_ONCE(enic->admin_chan_state) ==
+		ENIC_DMA_QUARANTINED;
 }
diff --git a/drivers/net/ethernet/cisco/enic/enic_admin.h b/drivers/net/ethernet/cisco/enic/enic_admin.h
index 62c80220b0ca..5945bbb2e6d4 100644
--- a/drivers/net/ethernet/cisco/enic/enic_admin.h
+++ b/drivers/net/ethernet/cisco/enic/enic_admin.h
@@ -20,7 +20,9 @@ struct enic_admin_msg {
 };
 
 int enic_admin_channel_open(struct enic *enic);
-void enic_admin_channel_close(struct enic *enic);
+int enic_admin_channel_close(struct enic *enic);
+int enic_admin_channel_recheck_on_remove(struct enic *enic);
+bool enic_admin_channel_is_quarantined(const struct enic *enic);
 unsigned int enic_admin_wq_cq_service(struct enic *enic);
 unsigned int enic_admin_rq_cq_service(struct enic *enic);
 
diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
index 5a205ea3031b..06d76648b420 100644
--- a/drivers/net/ethernet/cisco/enic/enic_main.c
+++ b/drivers/net/ethernet/cisco/enic/enic_main.c
@@ -71,6 +71,7 @@
 #define PCI_DEVICE_ID_CISCO_VIC_ENET_VF_V2   0x02b7  /* enet SRIOV V2 VF */
 #define PCI_DEVICE_ID_CISCO_VIC_ENET_VF_USNIC 0x00cf /* enet USNIC VF */
 
+static bool enic_dma_quarantined(struct enic *enic);
 static void enic_terminal_dma_quarantine(struct enic *enic);
 
 /* Supported devices */
@@ -1729,11 +1730,15 @@ static void enic_notify_timer_start(struct enic *enic)
 	}
 }
 
-static void enic_datapath_release(struct enic *enic)
+static void enic_datapath_release(struct enic *enic, bool release_notify)
 {
 	unsigned int i;
 
-	enic_dev_notify_unset(enic);
+	/* The notification allocation is a separate vNIC DMA target.  Keep it
+	 * with the shared lifetime envelope if another DMA graph is quarantined.
+	 */
+	if (release_notify)
+		enic_dev_notify_unset(enic);
 
 	enic_unset_affinity_hint(enic);
 	enic_free_intr(enic);
@@ -1899,7 +1904,7 @@ static int enic_open(struct net_device *netdev)
 		enic_datapath_quarantine(enic);
 		return err;
 	}
-	enic_datapath_release(enic);
+	enic_datapath_release(enic, true);
 
 	return err;
 
@@ -1974,7 +1979,8 @@ static int enic_stop(struct net_device *netdev)
 		return disable_err;
 	}
 
-	enic_datapath_release(enic);
+	enic_datapath_release(enic,
+			      !enic_admin_channel_is_quarantined(enic));
 
 	return 0;
 }
@@ -2328,18 +2334,30 @@ static void enic_admin_chan_reopen(struct enic *enic)
 	if (enic_is_sriov_vf_v2(enic)) {
 		err = enic_mbox_vf_capability_check(enic);
 		if (err) {
+			int close_err;
+
 			netdev_err(enic->netdev,
 				   "MBOX capability check after reset failed: %d\n",
 				   err);
-			enic_admin_channel_close(enic);
+			close_err = enic_admin_channel_close(enic);
+			if (close_err)
+				netdev_err(enic->netdev,
+					   "Admin close after capability failure failed: %d\n",
+					   close_err);
 			return;
 		}
 		err = enic_mbox_vf_register(enic);
 		if (err) {
+			int close_err;
+
 			netdev_err(enic->netdev,
 				   "MBOX VF re-registration after reset failed: %d\n",
 				   err);
-			enic_admin_channel_close(enic);
+			close_err = enic_admin_channel_close(enic);
+			if (close_err)
+				netdev_err(enic->netdev,
+					   "Admin close after registration failure failed: %d\n",
+					   close_err);
 		}
 	} else {
 		/* The link came back up during enic_open() above while MBOX
@@ -2355,9 +2373,14 @@ static void enic_reset(struct work_struct *work)
 	struct enic *enic = container_of(work, struct enic, reset);
 	int stop_err = 0;
 	bool reopen;
+	int err;
 
-	if (enic_datapath_is_quarantined(enic) ||
-	    !netif_running(enic->netdev))
+	if (enic_datapath_is_quarantined(enic))
+		return;
+	if (enic_admin_channel_is_quarantined(enic) &&
+	    READ_ONCE(enic->datapath_state) != ENIC_DMA_LIVE)
+		return;
+	if (!netif_running(enic->netdev))
 		return;
 
 	rtnl_lock();
@@ -2368,22 +2391,27 @@ static void enic_reset(struct work_struct *work)
 	/* Stop any activity from infiniband */
 	enic_set_api_busy(enic, true);
 
-	/* Fully tear down the V2 admin/MBOX channel before the soft reset.
-	 * The reset wipes all hardware queues including the admin WQ/RQ;
-	 * closing first tells firmware to stop the admin QP (so it no longer
-	 * DMAs from the about-to-be-reset rings) and frees the admin resources
-	 * so they are cleanly re-allocated afterwards.
+	/* Stop the V2 admin/MBOX graph before the soft reset.  A queue timeout
+	 * quarantines that graph permanently; continue only far enough to stop
+	 * and, when possible, release the independent datapath graph.
 	 */
-	if (enic_has_admin_chan(enic))
-		enic_admin_channel_close(enic);
+	if (enic_has_admin_chan(enic)) {
+		err = enic_admin_channel_close(enic);
+		if (err && enic_admin_channel_is_quarantined(enic))
+			netdev_err(enic->netdev,
+				   "Admin queue did not stop: %d\n", err);
+		else if (err)
+			netdev_warn(enic->netdev,
+				    "Admin QP disable returned: %d\n", err);
+	}
 
 	if (reopen)
 		stop_err = enic_stop(enic->netdev);
 	if (enic_is_sriov_vf_v2(enic))
 		enic_mbox_vf_link_state_reset(enic);
-	if (enic_datapath_is_quarantined(enic)) {
+	if (enic_dma_quarantined(enic)) {
 		netdev_err(enic->netdev,
-			   "Datapath queue did not stop: %d; DMA resources retained until reboot\n",
+			   "Queue DMA ownership unresolved (datapath stop %d); resources retained until reboot\n",
 			   stop_err);
 		goto reset_out;
 	}
@@ -2398,13 +2426,17 @@ static void enic_reset(struct work_struct *work)
 	if (reopen)
 		enic_open(enic->netdev);
 
-	/* Re-establish the admin/MBOX channel after the data path is back up.
-	 * It was fully torn down by enic_admin_channel_close() above;
-	 * enic_admin_chan_reopen() reopens it and, for a PF re-pushes link
-	 * state, or for a VF re-runs the probe-time PF handshake.
+	/* Re-establish the admin/MBOX channel after its clean stop.  For a PF,
+	 * re-push link state; for a VF, rerun the probe-time PF handshake.
 	 */
 	if (enic_has_admin_chan(enic))
 		enic_admin_chan_reopen(enic);
+	if (enic_admin_channel_is_quarantined(enic)) {
+		stop_err = enic_stop(enic->netdev);
+		netdev_err(enic->netdev,
+			   "Admin queue did not stop after reopen failure (datapath stop %d); resources retained until reboot\n",
+			   stop_err);
+	}
 
 reset_out:
 	/* Allow infiniband to fiddle with the device again */
@@ -2422,30 +2454,39 @@ static void enic_tx_hang_reset(struct work_struct *work)
 	struct enic *enic = container_of(work, struct enic, tx_hang_reset);
 	int stop_err = 0;
 	bool reopen;
+	int err;
 
 	rtnl_lock();
 	reopen = netif_running(enic->netdev);
-	if (!reopen || enic_datapath_is_quarantined(enic))
+	if (enic_datapath_is_quarantined(enic) ||
+	    (enic_admin_channel_is_quarantined(enic) &&
+	     READ_ONCE(enic->datapath_state) != ENIC_DMA_LIVE) || !reopen)
 		goto unlock;
 
 	/* Stop any activity from infiniband */
 	enic_set_api_busy(enic, true);
 
-	/* Fully tear down the V2 admin/MBOX channel before the hang reset, for
-	 * the same reason as the soft reset path: stop the admin QP and free
-	 * the admin resources before the hardware queues are wiped.
+	/* Stop the V2 admin/MBOX graph before the hang reset.  As in the soft
+	 * reset path, a queue timeout is terminal for that graph.
 	 */
-	if (enic_has_admin_chan(enic))
-		enic_admin_channel_close(enic);
+	if (enic_has_admin_chan(enic)) {
+		err = enic_admin_channel_close(enic);
+		if (err && enic_admin_channel_is_quarantined(enic))
+			netdev_err(enic->netdev,
+				   "Admin queue did not stop: %d\n", err);
+		else if (err)
+			netdev_warn(enic->netdev,
+				    "Admin QP disable returned: %d\n", err);
+	}
 
 	enic_dev_hang_notify(enic);
 	if (reopen)
 		stop_err = enic_stop(enic->netdev);
 	if (enic_is_sriov_vf_v2(enic))
 		enic_mbox_vf_link_state_reset(enic);
-	if (enic_datapath_is_quarantined(enic)) {
+	if (enic_dma_quarantined(enic)) {
 		netdev_err(enic->netdev,
-			   "Datapath queue did not stop: %d; DMA resources retained until reboot\n",
+			   "Queue DMA ownership unresolved (datapath stop %d); resources retained until reboot\n",
 			   stop_err);
 		goto hang_reset_out;
 	}
@@ -2460,13 +2501,17 @@ static void enic_tx_hang_reset(struct work_struct *work)
 	if (reopen)
 		enic_open(enic->netdev);
 
-	/* Re-establish the admin/MBOX channel after the data path is back up.
-	 * It was fully torn down by enic_admin_channel_close() above;
-	 * enic_admin_chan_reopen() reopens it and, for a PF re-pushes link
-	 * state, or for a VF re-runs the probe-time PF handshake.
+	/* Re-establish the admin/MBOX channel after its clean stop.  For a PF,
+	 * re-push link state; for a VF, rerun the probe-time PF handshake.
 	 */
 	if (enic_has_admin_chan(enic))
 		enic_admin_chan_reopen(enic);
+	if (enic_admin_channel_is_quarantined(enic)) {
+		stop_err = enic_stop(enic->netdev);
+		netdev_err(enic->netdev,
+			   "Admin queue did not stop after reopen failure (datapath stop %d); resources retained until reboot\n",
+			   stop_err);
+	}
 
 hang_reset_out:
 	/* Allow infiniband to fiddle with the device again */
@@ -2998,6 +3043,7 @@ static void enic_sriov_detect_vf_type(struct enic *enic)
 static int __maybe_unused
 enic_sriov_v2_enable(struct enic *enic, int num_vfs)
 {
+	int close_err;
 	int err;
 
 	if (!enic->has_admin_channel) {
@@ -3037,15 +3083,26 @@ enic_sriov_v2_enable(struct enic *enic, int num_vfs)
 
 close_admin:
 	enic->num_vfs = 0;
-	enic_admin_channel_close(enic);
+	close_err = enic_admin_channel_close(enic);
+	if (close_err)
+		netdev_err(enic->netdev,
+			   "Failed to close admin channel: %d\n", close_err);
 free_vf_state:
 	kfree(enic->vf_state);
 	enic->vf_state = NULL;
+	/* The reset worker only converges the independent datapath to a stopped
+	 * state after terminal admin-queue failure; it does not issue a reset.
+	 */
+	if (enic_admin_channel_is_quarantined(enic) &&
+	    netif_running(enic->netdev))
+		schedule_work(&enic->reset);
 	return err;
 }
 
 static void enic_sriov_v2_disable(struct enic *enic)
 {
+	int close_err;
+
 	/* Stop new VF link-state broadcasts before tearing down vf_state.
 	 * Clearing ENIC_SRIOV_ENABLED makes enic_link_check() (called from
 	 * the notify timer/ISR) skip the VF notify path, and cancelling
@@ -3056,10 +3113,16 @@ static void enic_sriov_v2_disable(struct enic *enic)
 	cancel_work_sync(&enic->link_notify_work);
 
 	pci_disable_sriov(enic->pdev);
-	enic_admin_channel_close(enic);
+	close_err = enic_admin_channel_close(enic);
+	if (close_err)
+		netdev_err(enic->netdev,
+			   "Failed to close admin channel: %d\n", close_err);
 	kfree(enic->vf_state);
 	enic->vf_state = NULL;
 	enic->num_vfs = 0;
+	if (enic_admin_channel_is_quarantined(enic) &&
+	    netif_running(enic->netdev))
+		schedule_work(&enic->reset);
 }
 
 /*
@@ -3346,7 +3409,7 @@ static int enic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		if (err) {
 			dev_err(dev,
 				"Failed to open admin channel: %d\n", err);
-			goto err_out_dev_deinit;
+			goto err_out_admin_fence;
 		}
 		err = enic_mbox_vf_capability_check(enic);
 		if (err) {
@@ -3491,6 +3554,8 @@ static int enic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 err_out_admin_close:
 	if (enic_is_sriov_vf_v2(enic)) {
+		int close_err;
+
 		if (READ_ONCE(enic->vf_registered)) {
 			int unreg_err = enic_mbox_vf_unregister(enic);
 
@@ -3499,12 +3564,24 @@ static int enic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 					    "Failed to unregister from PF: %d\n",
 					    unreg_err);
 		}
-		enic_admin_channel_close(enic);
+		close_err = enic_admin_channel_close(enic);
+		if (close_err)
+			netdev_err(netdev,
+				   "Failed to close admin channel: %d\n",
+				   close_err);
+	}
+err_out_admin_fence:
+	if (enic_dma_quarantined(enic)) {
+		dev_err(dev,
+			"Queue DMA ownership unresolved; retaining probe resources until reboot\n");
+		enic_terminal_dma_quarantine(enic);
+		pci_disable_device(pdev);
+		return err;
 	}
-err_out_dev_deinit:
 	enic_dev_deinit(enic);
 err_out_dev_close:
 	vnic_dev_close(enic->vdev);
+	goto err_out_disable_sriov;
 err_out_disable_sriov:
 	kfree(enic->pp);
 err_out_disable_sriov_pp:
@@ -3528,6 +3605,12 @@ static int enic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	return err;
 }
 
+static bool enic_dma_quarantined(struct enic *enic)
+{
+	return enic_datapath_is_quarantined(enic) ||
+	       enic_admin_channel_is_quarantined(enic);
+}
+
 static int enic_datapath_recheck_on_remove(struct enic *enic)
 {
 	int disable_err = 0;
@@ -3555,7 +3638,8 @@ static int enic_datapath_recheck_on_remove(struct enic *enic)
 
 	dev_info(&enic->pdev->dev,
 		 "All datapath queues stopped at remove; releasing DMA graph\n");
-	enic_datapath_release(enic);
+	enic_datapath_release(enic,
+			      !enic_admin_channel_is_quarantined(enic));
 
 	return 0;
 }
@@ -3568,6 +3652,7 @@ static void enic_terminal_dma_quarantine(struct enic *enic)
 	 * ordinary stop path can return before freeing IRQs when a queue does
 	 * not stop, so each ownership-aware operation must be idempotent.
 	 */
+	WRITE_ONCE(enic->mbox_send_disabled, true);
 	for (i = 0; i < enic->intr_count; i++) {
 		vnic_intr_mask(&enic->intr[i]);
 		(void)vnic_intr_masked(&enic->intr[i]);
@@ -3604,6 +3689,8 @@ static void enic_remove(struct pci_dev *pdev)
 		 * touching a netdev that is being torn down.
 		 */
 		if (enic_is_sriov_vf_v2(enic)) {
+			int close_err;
+
 			if (READ_ONCE(enic->vf_registered)) {
 				int unreg_err = enic_mbox_vf_unregister(enic);
 
@@ -3612,7 +3699,11 @@ static void enic_remove(struct pci_dev *pdev)
 						    "Failed to unregister from PF: %d\n",
 						    unreg_err);
 			}
-			enic_admin_channel_close(enic);
+			close_err = enic_admin_channel_close(enic);
+			if (close_err)
+				netdev_warn(netdev,
+					    "Admin close returned: %d\n",
+					    close_err);
 		}
 
 		unregister_netdev(netdev);
@@ -3630,9 +3721,11 @@ static void enic_remove(struct pci_dev *pdev)
 				pci_disable_sriov(pdev);
 		}
 #endif
+		if (enic_admin_channel_is_quarantined(enic))
+			enic_admin_channel_recheck_on_remove(enic);
 		if (enic_datapath_is_quarantined(enic))
 			enic_datapath_recheck_on_remove(enic);
-		if (enic_datapath_is_quarantined(enic)) {
+		if (enic_dma_quarantined(enic)) {
 			dev_err(&pdev->dev,
 				"Queue DMA ownership unresolved; retaining resources until reboot\n");
 			enic_terminal_dma_quarantine(enic);
diff --git a/drivers/net/ethernet/cisco/enic/enic_mbox.c b/drivers/net/ethernet/cisco/enic/enic_mbox.c
index 5c93ca49552a..e106ee7d411d 100644
--- a/drivers/net/ethernet/cisco/enic/enic_mbox.c
+++ b/drivers/net/ethernet/cisco/enic/enic_mbox.c
@@ -150,6 +150,7 @@ static int enic_mbox_send_msg_id(struct enic *enic, u8 msg_type,
 		 * Mark the channel unusable so no further sends are attempted.
 		 */
 		WRITE_ONCE(enic->mbox_send_disabled, true);
+		WRITE_ONCE(enic->mbox_tx_poisoned, true);
 	}
 
 	netdev_dbg(enic->netdev,

-- 
2.43.0

Re: [PATCH net 2/3] enic: retain admin-channel DMA until queues stop

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 8 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 4 · Low: 4

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The notify DMA target that the patch now keeps while the admin
  graph is quarantined is never unregistered.
- [Medium] After admin quarantine, enic_admin_channel_close() calls
  pci_clear_master() (enic_admin.c:708).
- [Medium] enic_reset() and enic_tx_hang_reset() reopen the admin channel
  without checking whether the enic_open() just before it quarantined…
- [Medium] The admin quarantine path calls pci_clear_master() on the
  whole PCI function (enic_admin.c:708) while the generic datapath WQ/RQ…
- [Low] In the rewritten enic_probe() unwind, `goto
  err_out_disable_sriov;` sits on the line directly before the…
- [Low] Claim: enic_open() has no admin-quarantine gate, so
  enic_change_mtu_work -> _enic_change_mtu() -> enic_open() could re-…
- [Low] The three callers of the newly parameterised
  enic_datapath_release() apply different rules.
- [Low] enic_stop() decides whether to keep the shared notify target from
  a lockless READ_ONCE of admin_chan_state (enic_datapath_release(enic,…

--

enic: retain admin-channel DMA until queues stop

This patch tracks the enic admin channel as DOWN, LIVE or QUARANTINED. The
admin WQ/RQ/CQ graph is released only after both queue disables observe
RUNNING == 0. On a disable timeout the graph is kept and bus mastering is
cleared, and the reset, SR-IOV, probe and remove paths are updated to hold
quarantined graphs until a final recheck at remove time.

> diff --git a/drivers/net/ethernet/cisco/enic/enic_admin.c b/drivers/net/ethernet/cisco/enic/enic_admin.c
> index 61c82b48044d0..a5c08cbcb3391 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_admin.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_admin.c

[ ... ]

> @@ -600,68 +612,141 @@ int enic_admin_channel_open(struct enic *enic)

[ ... ]

> +	/* QP_DISABLE does not report every low-level stop failure.  The direct
> +	 * WQ/RQ RUNNING observations own the admin allocation graph.
> +	 */
> +	if (wq_err || rq_err) {
> +		WRITE_ONCE(enic->admin_chan_state, ENIC_DMA_QUARANTINED);
> +		WRITE_ONCE(enic->mbox_tx_poisoned, true);
> +		mutex_unlock(&enic->mbox_lock);
> +		netif_device_detach(enic->netdev);
> +		netif_carrier_off(enic->netdev);
> +		/* Stop new PCIe requests without treating bus-master disable as
> +		 * proof that already-issued DMA has completed.
> +		 */
> +		pci_clear_master(enic->pdev);

[Severity: Medium]
pci_clear_master() here applies to the whole PCI function, and the generic
datapath WQs and RQs may still be enabled. Can a healthy datapath still
drain after this?

In enic_reset() and enic_tx_hang_reset() the admin close runs before
enic_stop(). In a V2 VF enic_remove() it runs before
unregister_netdev()->enic_stop().

enic_stop() then calls vnic_wq_disable() and vnic_rq_disable(), which poll
ctrl->running for 500 ms. With bus mastering cleared, the function can no
longer do the descriptor fetches and completion writes needed to retire
in-flight work. If those polls time out, enic_datapath_quarantine() keeps
the datapath until reboot. That seems to contradict the commit message:

  "Keep the admin and generic datapath ownership graphs independent."

The same question applies to enic_admin_channel_recheck_on_remove(). It
polls the admin WQ/RQ after bus mastering has already been removed. The
commit message says clearing bus mastering does not "prevent the final
remove-time RUNNING recheck from observing a queue which subsequently
drains". Is this behaviour of the VIC queue engine documented anywhere?

> +		return wq_err ?: rq_err;
> +	}

[ ... ]

> diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
> index 5a205ea3031b9..06d76648b420c 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_main.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_main.c

[ ... ]

> @@ -1729,11 +1730,15 @@ static void enic_notify_timer_start(struct enic *enic)
>  	}
>  }
>  
> -static void enic_datapath_release(struct enic *enic)
> +static void enic_datapath_release(struct enic *enic, bool release_notify)
>  {
>  	unsigned int i;
>  
> -	enic_dev_notify_unset(enic);
> +	/* The notification allocation is a separate vNIC DMA target.  Keep it
> +	 * with the shared lifetime envelope if another DMA graph is quarantined.
> +	 */
> +	if (release_notify)
> +		enic_dev_notify_unset(enic);

[Severity: Medium]
Once this unset is skipped, does any later path ever do it?

enic_stop() can run while the admin graph is quarantined. Two examples:

  enic_reset()->enic_admin_channel_close() times out, then enic_stop()

  a V2 VF enic_remove()->enic_admin_channel_close() times out, then
  unregister_netdev()->enic_stop()

In both cases release_notify is false. datapath_state becomes
ENIC_DMA_DOWN while vdev->notify is still registered with firmware through
CMD_NOTIFY.

Later in enic_remove(), enic_admin_channel_recheck_on_remove() can succeed
and set admin_chan_state to ENIC_DMA_DOWN. enic_datapath_recheck_on_remove()
is skipped because the datapath is not quarantined, so no caller passes
release_notify=true. enic_dma_quarantined() is then false and the full
teardown runs:

enic_remove()
  enic_dev_deinit()
  vnic_dev_close()
  vnic_dev_unregister()
    dma_free_coherent(..., vdev->notify, vdev->notify_pa)

vnic_dev_unregister() frees the buffer without the CMD_NOTIFY retraction,
which only vnic_dev_notify_unsetcmd() issues. Can this free a coherent
buffer whose bus address firmware still holds as its notify target?

Bus mastering was cleared at quarantine. For this to cause corruption, the
stale registration would have to survive until a later bind, where
enic_probe() calls pci_set_master() before notify is set up again.

The kept block also makes any later enic_open() fail in
vnic_dev_notify_set() with "notify block ... still allocated".

[ ... ]

> @@ -1899,7 +1904,7 @@ static int enic_open(struct net_device *netdev)
>  		enic_datapath_quarantine(enic);
>  		return err;
>  	}
> -	enic_datapath_release(enic);
> +	enic_datapath_release(enic, true);

[Severity: Low]
Should this follow the same rule as enic_stop() and
enic_datapath_recheck_on_remove()? Both pass
!enic_admin_channel_is_quarantined(enic).

The comment in enic_datapath_release() says notify is kept "if another DMA
graph is quarantined".

For a V2 VF, enic_remove() calls enic_admin_channel_close() before
unregister_netdev() and without rtnl. At the same time, enic_open() can be
running under rtnl from a user "ip link set up".

If the admin close quarantines and clears bus mastering mid-open,
enic_dev_enable() can time out. This unwind then releases notify with
release_notify=true and issues a CMD_NOTIFY unset that cannot reach
firmware.

[Severity: Low]
Separately, enic_open() only checks enic_datapath_is_quarantined() before
it proceeds. Can enic_change_mtu_work->_enic_change_mtu()->enic_open()
reach this function after the admin graph has been quarantined and bus
mastering cleared?

In the final state of the series this does not seem to reach ring
enablement. enic_dev_notify_set() fails first, either with -EINVAL from the
kept notify block or with a DEVCMD2 timeout once "enic: require DEVCMD2 and
set the V2 VF mailbox source" makes DEVCMD2 mandatory.

With only this patch applied, though, a devcmd1 V2 VF could get past
CMD_NOTIFY and re-enable the datapath rings on a contained function. Would
an explicit enic_admin_channel_is_quarantined() check here be clearer?

[ ... ]

> @@ -1974,7 +1979,8 @@ static int enic_stop(struct net_device *netdev)
>  		return disable_err;
>  	}
>  
> -	enic_datapath_release(enic);
> +	enic_datapath_release(enic,
> +			      !enic_admin_channel_is_quarantined(enic));

[Severity: Low]
Is this lockless read of admin_chan_state stable against a concurrent
enic_admin_channel_close()?

enic_stop() holds only rtnl here. In enic_remove() for a V2 VF,
enic_admin_channel_close() runs before unregister_netdev() without rtnl. It
publishes ENIC_DMA_QUARANTINED under mbox_lock only:

CPU0 (ip link set down)          CPU1 (enic_remove)
enic_stop()
  reads "not quarantined"
                                 enic_admin_channel_close()
                                   WRITE_ONCE(enic->admin_chan_state,
                                              ENIC_DMA_QUARANTINED)
                                   pci_clear_master()
  enic_datapath_release(enic, true)
    enic_dev_notify_unset()

This releases the notify target while another graph is quarantined, which
release_notify is meant to prevent.

[ ... ]

> @@ -2398,13 +2426,17 @@ static void enic_reset(struct work_struct *work)
>  	if (reopen)
>  		enic_open(enic->netdev);
>  

[ ... ]

>  	if (enic_has_admin_chan(enic))
>  		enic_admin_chan_reopen(enic);

[Severity: Medium]
Should enic_admin_chan_reopen() be skipped when the enic_open() just above
has quarantined the datapath?

The return value of enic_open() is ignored. enic_open() can enable RQs,
fail later, and then time out disabling an RQ. In that case
enic_datapath_quarantine() sets mbox_send_disabled, detaches the netdev and
clears bus mastering.

The new checks in enic_admin_channel_open() test only admin_chan_state and
mbox_tx_poisoned (and, later in the series, sriov_stats_dma_unresolved).
The reopen therefore still allocates admin resources, requests the admin
IRQ, enables the admin WQ/RQ, fills the admin RQ with freshly mapped
buffers and sends QP_ENABLE.

On a devcmd1 device QP_ENABLE succeeds and mbox_send_disabled is cleared
again, which undoes enic_datapath_quarantine(). With DEVCMD2, QP_ENABLE
times out and the partial-open close polls admin queues that cannot
complete DMA. That could quarantine the admin graph too.

enic_tx_hang_reset() has the same sequence.

> +	if (enic_admin_channel_is_quarantined(enic)) {
> +		stop_err = enic_stop(enic->netdev);
> +		netdev_err(enic->netdev,
> +			   "Admin queue did not stop after reopen failure (datapath stop %d); resources retained until reboot\n",
> +			   stop_err);
> +	}

[ ... ]

> @@ -2422,30 +2454,39 @@ static void enic_tx_hang_reset(struct work_struct *work)

[ ... ]

> -	if (enic_has_admin_chan(enic))
> -		enic_admin_channel_close(enic);
> +	if (enic_has_admin_chan(enic)) {
> +		err = enic_admin_channel_close(enic);
> +		if (err && enic_admin_channel_is_quarantined(enic))
> +			netdev_err(enic->netdev,
> +				   "Admin queue did not stop: %d\n", err);
> +		else if (err)
> +			netdev_warn(enic->netdev,
> +				    "Admin QP disable returned: %d\n", err);
> +	}
>  
>  	enic_dev_hang_notify(enic);
>  	if (reopen)
>  		stop_err = enic_stop(enic->netdev);

[Severity: Medium]
If enic_admin_channel_close() has just quarantined and called
pci_clear_master(), can the devcmds that follow still be delivered?

Later in the series ("enic: require DEVCMD2 and set the V2 VF mailbox
source") DEVCMD2 becomes mandatory for admin-channel functions.
_vnic_dev_cmd2() writes into the host-memory cmd_ring, bumps posted_index,
and then busy-polls result->color. Only a device DMA write can update
result->color.

With bus mastering off, each of these commands would spin for its full
timeout under devcmd_lock with BH disabled, then return -ETIMEDOUT:

enic_tx_hang_reset()
  enic_dev_hang_notify()          HANG_NOTIFY
  enic_stop()
    enic_rfs_flw_tbl_free()       filter deletes
    enic_dev_disable()            CMD_DISABLE

enic_reset()->enic_stop() runs into the same thing.

dc2c->posted, next_result and color keep advancing while the hardware
fetch_index does not. Once the ring fills, commands fail with -EBUSY.

At remove, after a successful admin recheck, the CMD_NOTIFY unset,
enic_dev_deinit() and vnic_dev_close() are lost the same way. Then
vnic_dev_deinit_devcmd2() frees the results ring before it disables a
DEVCMD2 WQ that still holds unfetched descriptors. Does this leave firmware
with the vNIC open and a stale results-ring address?

[ ... ]

> @@ -3499,12 +3564,24 @@ static int enic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)

[ ... ]

> -err_out_dev_deinit:
>  	enic_dev_deinit(enic);
>  err_out_dev_close:
>  	vnic_dev_close(enic->vdev);
> +	goto err_out_disable_sriov;
>  err_out_disable_sriov:

[Severity: Low]
This isn't a bug, but this goto jumps to the label on the very next line.
Was it meant to be removed?

>  	kfree(enic->pp);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261016-b4-enic-dma-vf-index-net-generic-first-v1-20261010-v1-0-d27b55f69883%40cisco.com