[PATCH net 1/3] enic: retain datapath DMA resources until queues stop

[PATCH net 1/3] enic: retain datapath DMA resources until queues stop

[Satish Kharat]

enic_stop() returns immediately when the first WQ or RQ disable times
out.  The networking core does not treat ndo_stop() as fallible. A
later open can therefore reuse the still-allocated datapath even though
hardware ownership of one of its rings was never disproved. The same
problem occurs while unwinding a partially opened datapath.

The queue-disable helpers can return -ETIMEDOUT without observing
RUNNING == 0. Firmware defines RUNNING == 0 as conclusive evidence that
queue DMA has stopped. A nonzero RUNNING value does not establish that
DMA is active, but it leaves DMA ownership unresolved. The existing error
path can nevertheless release DMA-backed resources without obtaining the
required ownership proof.

The existing WQ loop waits about 10 ms, while the RQ uses a CPU-dependent
tight loop. Replace both with read_poll_timeout() and a driver-chosen
500 ms bound. Keep the two complete RQ disable passes required
to flush its hardware mini-cache.

Try every WQ and RQ disable and retain the first error. Release the
datapath only when every enabled queue reports RUNNING == 0. Although an
individual queue which reports zero has stopped, the queues, completion
queues, interrupts, mappings, page pools, and notification allocation
form one datapath lifetime graph. If any queue times out, retain that
complete graph instead of partially reclaiming it.

This complete-graph retention is deliberately conservative. It can retain
resources belonging to queues which stopped successfully, but avoids
adding partial teardown and ownership states to this correctness fix.
Per-queue reclamation of proven-stopped packet buffers can be implemented
as a separate follow-up and is not required for DMA safety.

Ring resizing cannot rely on dev_close() to report an ndo_stop()
failure. Stop the datapath explicitly before dev_close(), propagate a
queue-disable error, and reject ring reconfiguration while the datapath
is quarantined so retained DMA resources cannot be freed through the
ethtool path.

Track the datapath state so an internal reset cannot make a later
ndo_stop() disable NAPI or tear down the same graph twice. Do not issue
an internal reset or reopen the datapath when queue stop could not be
confirmed and DMA ownership remains unresolved. A clean internal open
unwind remains retryable; if stop cannot be confirmed for a queue during
that unwind, quarantine the datapath.

These transitions apply to every ENIC device because the reset workers,
ndo_open(), ndo_stop(), and their resource graphs are shared by PFs and
both VF generations.

A persistent RUNNING value means the driver cannot prove that queue DMA
has stopped. Do not reset or reclaim a quarantined datapath during
runtime. Detach the netdev and clear bus mastering to contain the
function. Clearing bus mastering blocks new device-initiated PCI
transactions; it does not cancel DMA which is already in progress or
prevent a queue from draining to RUNNING == 0.

At device removal, make one final direct disable/RUNNING check of every
queue. Release the complete graph only if every queue reports RUNNING == 0.
Do not partially release queues which report zero if any peer remains
nonzero.
If ownership is still unresolved, detach every remaining CPU entry point
and intentionally retain the netdev, BAR mappings and reservations,
rings, mappings, page pools, and buffer metadata until reboot. The final
recheck remains meaningful after bus mastering was cleared because a
queue may subsequently report RUNNING == 0, but the bus-master state is
not proof that already-issued DMA completed. Leaking one failed device
instance is preferable to risking a DMA use-after-free.

The BAR reservation may therefore outlive driver unbind and module
unload while the PCI device remains enumerated. The resource subsystem
stores the supplied owner-name pointer rather than copying it. Request
the regions using the PCI device name instead of DRV_NAME so the pointer
does not refer to module rodata after unload.

A PCI driver's remove callback cannot reject driver unbind. With DMA API
debugging enabled, the unbind notification therefore reports the
deliberately retained DMA allocations. The retained BAR reservation
prevents a normal ENIC rebind, so reboot is the supported recovery from
terminal quarantine.

The early-return behavior dates to the original driver. An automated
Sashiko review of the V2 SR-IOV reset paths prompted the audit of
admin-channel reset error exits which uncovered it. Sashiko prompted the
investigation; the defect and its correction were established through
source analysis of the queue-disable and resource-release paths. Fix the
generic datapath first so the following admin-channel correction does not
create an unsafe intermediate kernel.

Fixes: 01f2e4ead2c5 ("enic: add Cisco 10G Ethernet NIC driver")
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         |  18 ++
 drivers/net/ethernet/cisco/enic/enic_ethtool.c |  10 +-
 drivers/net/ethernet/cisco/enic/enic_main.c    | 322 ++++++++++++++++++++-----
 drivers/net/ethernet/cisco/enic/vnic_rq.c      |  18 +-
 drivers/net/ethernet/cisco/enic/vnic_wq.c      |  18 +-
 5 files changed, 312 insertions(+), 74 deletions(-)

diff --git a/drivers/net/ethernet/cisco/enic/enic.h b/drivers/net/ethernet/cisco/enic/enic.h
index 7a509a056990..992ac43d1b0a 100644
--- a/drivers/net/ethernet/cisco/enic/enic.h
+++ b/drivers/net/ethernet/cisco/enic/enic.h
@@ -238,6 +238,12 @@ enum enic_vf_type {
 	ENIC_VF_TYPE_V2,
 };
 
+enum enic_dma_state {
+	ENIC_DMA_DOWN,
+	ENIC_DMA_LIVE,
+	ENIC_DMA_QUARANTINED,
+};
+
 /* Per-instance private data structure */
 struct enic {
 	struct net_device *netdev;
@@ -251,6 +257,8 @@ struct enic {
 	struct work_struct change_mtu_work;
 	struct msix_entry *msix_entry;
 	struct enic_msix_entry *msix;
+	bool non_msix_intr_requested;
+	bool napi_added;
 	u32 msg_enable;
 	spinlock_t devcmd_lock;
 	u8 mac_addr[ETH_ALEN];
@@ -325,6 +333,10 @@ struct enic {
 	spinlock_t vf_link_state_lock;
 	enum enic_vf_link_state vf_link_state;
 	bool vf_link_running;
+	/* A queue-disable failure leaves DMA ownership unresolved.  Retain the
+	 * complete mapped datapath graph for the remainder of this boot.
+	 */
+	enum enic_dma_state datapath_state;
 
 	/* MBOX protocol state — mbox_lock serializes admin WQ sends */
 	struct mutex mbox_lock;
@@ -346,6 +358,11 @@ struct enic {
 	} *vf_state;
 };
 
+static inline bool enic_datapath_is_quarantined(struct enic *enic)
+{
+	return READ_ONCE(enic->datapath_state) == ENIC_DMA_QUARANTINED;
+}
+
 static inline struct net_device *vnic_get_netdev(struct vnic_dev *vdev)
 {
 	struct enic *enic = vdev->priv;
@@ -466,6 +483,7 @@ int enic_sriov_enabled(struct enic *enic);
 int enic_is_valid_vf(struct enic *enic, int vf);
 int enic_is_dynamic(struct enic *enic);
 int enic_is_sriov_vf_v2(struct enic *enic);
+int enic_close_for_reconfigure(struct net_device *netdev);
 void enic_set_ethtool_ops(struct net_device *netdev);
 int __enic_set_rsskey(struct enic *enic);
 void enic_ext_cq(struct enic *enic);
diff --git a/drivers/net/ethernet/cisco/enic/enic_ethtool.c b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
index 61541fb8db7b..18af3b73cee7 100644
--- a/drivers/net/ethernet/cisco/enic/enic_ethtool.c
+++ b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
@@ -381,6 +381,9 @@ static int enic_set_ringparam(struct net_device *netdev,
 	unsigned int tx_pending;
 	int err = 0;
 
+	if (enic_datapath_is_quarantined(enic))
+		return -EBUSY;
+
 	if (ring->rx_mini_max_pending || ring->rx_mini_pending) {
 		netdev_info(netdev,
 			    "modifying mini ring params is not supported");
@@ -407,8 +410,11 @@ static int enic_set_ringparam(struct net_device *netdev,
 			c->max_wq_ring);
 		return -EINVAL;
 	}
-	if (running)
-		dev_close(netdev);
+	if (running) {
+		err = enic_close_for_reconfigure(netdev);
+		if (err)
+			return err;
+	}
 	c->rq_desc_count =
 		ring->rx_pending & 0xffffffe0; /* must be aligned to groups of 32 */
 	c->wq_desc_count =
diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
index 6b5824f45a14..5a205ea3031b 100644
--- a/drivers/net/ethernet/cisco/enic/enic_main.c
+++ b/drivers/net/ethernet/cisco/enic/enic_main.c
@@ -71,6 +71,8 @@
 #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 void enic_terminal_dma_quarantine(struct enic *enic);
+
 /* Supported devices */
 static const struct pci_device_id enic_id_table[] = {
 	{ PCI_VDEVICE(CISCO, PCI_DEVICE_ID_CISCO_VIC_ENET) },
@@ -476,8 +478,7 @@ static void enic_notify_check(struct enic *enic)
 
 static irqreturn_t enic_isr_legacy(int irq, void *data)
 {
-	struct net_device *netdev = data;
-	struct enic *enic = netdev_priv(netdev);
+	struct enic *enic = data;
 	unsigned int io_intr = ENIC_LEGACY_IO_INTR;
 	unsigned int err_intr = ENIC_LEGACY_ERR_INTR;
 	unsigned int notify_intr = ENIC_LEGACY_NOTIFY_INTR;
@@ -1563,22 +1564,25 @@ static void enic_notify_timer(struct timer_list *t)
 
 static void enic_free_intr(struct enic *enic)
 {
-	struct net_device *netdev = enic->netdev;
 	unsigned int i;
 
 	enic_free_rx_cpu_rmap(enic);
 	switch (vnic_dev_get_intr_mode(enic->vdev)) {
 	case VNIC_DEV_INTR_MODE_INTX:
-		free_irq(enic->pdev->irq, netdev);
-		break;
 	case VNIC_DEV_INTR_MODE_MSI:
-		free_irq(enic->pdev->irq, enic);
+		if (enic->non_msix_intr_requested) {
+			free_irq(enic->pdev->irq, enic);
+			enic->non_msix_intr_requested = false;
+		}
 		break;
 	case VNIC_DEV_INTR_MODE_MSIX:
-		for (i = 0; i < enic->intr_count; i++)
-			if (enic->msix[i].requested)
+		for (i = 0; i < enic->intr_count; i++) {
+			if (enic->msix[i].requested) {
 				free_irq(enic->msix_entry[i].vector,
 					enic->msix[i].devid);
+				enic->msix[i].requested = false;
+			}
+		}
 		break;
 	default:
 		break;
@@ -1587,23 +1591,28 @@ static void enic_free_intr(struct enic *enic)
 
 static int enic_request_intr(struct enic *enic)
 {
+	enum vnic_dev_intr_mode intr_mode;
 	struct net_device *netdev = enic->netdev;
+	unsigned long irq_flags;
+	irq_handler_t handler;
 	unsigned int i, intr;
 	int err = 0;
 
 	enic_set_rx_cpu_rmap(enic);
-	switch (vnic_dev_get_intr_mode(enic->vdev)) {
+	intr_mode = vnic_dev_get_intr_mode(enic->vdev);
+	switch (intr_mode) {
 
 	case VNIC_DEV_INTR_MODE_INTX:
-
-		err = request_irq(enic->pdev->irq, enic_isr_legacy,
-			IRQF_SHARED, netdev->name, netdev);
-		break;
-
 	case VNIC_DEV_INTR_MODE_MSI:
-
-		err = request_irq(enic->pdev->irq, enic_isr_msi,
-			0, netdev->name, enic);
+		handler = intr_mode == VNIC_DEV_INTR_MODE_INTX ?
+			enic_isr_legacy : enic_isr_msi;
+		irq_flags = intr_mode == VNIC_DEV_INTR_MODE_INTX ?
+			IRQF_SHARED : 0;
+
+		err = request_irq(enic->pdev->irq, handler, irq_flags,
+				  netdev->name, enic);
+		if (!err)
+			enic->non_msix_intr_requested = true;
 		break;
 
 	case VNIC_DEV_INTR_MODE_MSIX:
@@ -1673,11 +1682,13 @@ static void enic_synchronize_irqs(struct enic *enic)
 	switch (vnic_dev_get_intr_mode(enic->vdev)) {
 	case VNIC_DEV_INTR_MODE_INTX:
 	case VNIC_DEV_INTR_MODE_MSI:
-		synchronize_irq(enic->pdev->irq);
+		if (enic->non_msix_intr_requested)
+			synchronize_irq(enic->pdev->irq);
 		break;
 	case VNIC_DEV_INTR_MODE_MSIX:
 		for (i = 0; i < enic->intr_count; i++)
-			synchronize_irq(enic->msix_entry[i].vector);
+			if (enic->msix[i].requested)
+				synchronize_irq(enic->msix_entry[i].vector);
 		break;
 	default:
 		break;
@@ -1718,11 +1729,51 @@ static void enic_notify_timer_start(struct enic *enic)
 	}
 }
 
+static void enic_datapath_release(struct enic *enic)
+{
+	unsigned int i;
+
+	enic_dev_notify_unset(enic);
+
+	enic_unset_affinity_hint(enic);
+	enic_free_intr(enic);
+
+	for (i = 0; i < enic->wq_count; i++)
+		vnic_wq_clean(&enic->wq[i].vwq, enic_free_wq_buf);
+	for (i = 0; i < enic->rq_count; i++) {
+		vnic_rq_clean(&enic->rq[i].vrq, enic_free_rq_buf);
+		if (enic->rq[i].pool) {
+			page_pool_destroy(enic->rq[i].pool);
+			enic->rq[i].pool = NULL;
+		}
+	}
+	for (i = 0; i < enic->cq_count; i++)
+		vnic_cq_clean(&enic->cq[i]);
+	for (i = 0; i < enic->intr_count; i++)
+		vnic_intr_clean(&enic->intr[i]);
+
+	WRITE_ONCE(enic->datapath_state, ENIC_DMA_DOWN);
+}
+
+static void enic_datapath_quarantine(struct enic *enic)
+{
+	WRITE_ONCE(enic->datapath_state, ENIC_DMA_QUARANTINED);
+	WRITE_ONCE(enic->mbox_send_disabled, true);
+	netif_device_detach(enic->netdev);
+	netif_carrier_off(enic->netdev);
+	/* Bus-master disable contains the failed function but does not return
+	 * ownership of DMA resources which the device may still reference.
+	 */
+	pci_clear_master(enic->pdev);
+}
+
 /* rtnl lock is held, process context */
 static int enic_open(struct net_device *netdev)
 {
 	struct enic *enic = netdev_priv(netdev);
+	unsigned int rqs_enabled = 0;
 	unsigned int i;
+	int disable_err = 0;
 	int err, ret;
 	unsigned int max_pkt_len = netdev->mtu + VLAN_ETH_HLEN;
 	struct page_pool_params pp_params = {
@@ -1736,6 +1787,11 @@ static int enic_open(struct net_device *netdev)
 		.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV,
 	};
 
+	if (enic_datapath_is_quarantined(enic)) {
+		netdev_err(netdev,
+			   "Datapath DMA remains quarantined until reboot\n");
+		return -EBUSY;
+	}
 	err = enic_request_intr(enic);
 	if (err) {
 		netdev_err(netdev, "Unable to request irq.\n");
@@ -1764,6 +1820,7 @@ static int enic_open(struct net_device *netdev)
 
 		/* enable rq before updating rq desc */
 		vnic_rq_enable(&enic->rq[i].vrq);
+		rqs_enabled++;
 		vnic_rq_fill(&enic->rq[i].vrq, enic_rq_alloc_buf);
 		/* Need at least one buffer on ring to get going */
 		if (vnic_rq_desc_used(&enic->rq[i].vrq) == 0) {
@@ -1802,6 +1859,7 @@ static int enic_open(struct net_device *netdev)
 	enic_rfs_timer_start(enic);
 	if (enic_is_sriov_vf_v2(enic))
 		enic_mbox_vf_link_state_set_running(enic, true);
+	WRITE_ONCE(enic->datapath_state, ENIC_DMA_LIVE);
 
 	return 0;
 
@@ -1814,18 +1872,37 @@ static int enic_open(struct net_device *netdev)
 	netif_tx_disable(netdev);
 	if (!enic_is_dynamic(enic) && !enic_is_sriov_vf(enic))
 		enic_dev_del_station_addr(enic);
-	for (i = 0; i < enic->wq_count; i++)
-		vnic_wq_disable(&enic->wq[i].vwq);
+	for (i = 0; i < enic->wq_count; i++) {
+		ret = vnic_wq_disable(&enic->wq[i].vwq);
+		if (ret)
+			netdev_err(netdev,
+				   "WQ %u did not stop during open unwind: %d\n",
+				   i, ret);
+		if (ret && !disable_err)
+			disable_err = ret;
+	}
 err_out_free_rq:
-	for (i = 0; i < enic->rq_count; i++) {
+	for (i = 0; i < rqs_enabled; i++) {
 		ret = vnic_rq_disable(&enic->rq[i].vrq);
-		if (!ret) {
-			vnic_rq_clean(&enic->rq[i].vrq, enic_free_rq_buf);
-			page_pool_destroy(enic->rq[i].pool);
-			enic->rq[i].pool = NULL;
-		}
+		if (ret)
+			netdev_err(netdev,
+				   "RQ %u did not stop during open unwind: %d\n",
+				   i, ret);
+		if (ret && !disable_err)
+			disable_err = ret;
+	}
+	if (disable_err) {
+		/* A partially stopped datapath remains a single DMA ownership
+		 * graph.  Keep every ring, mapping, page pool, interrupt, and the
+		 * notification allocation for the remainder of this boot.
+		 */
+		enic_datapath_quarantine(enic);
+		return err;
 	}
-	enic_dev_notify_unset(enic);
+	enic_datapath_release(enic);
+
+	return err;
+
 err_out_free_intr:
 	enic_unset_affinity_hint(enic);
 	enic_free_intr(enic);
@@ -1838,8 +1915,15 @@ static int enic_stop(struct net_device *netdev)
 {
 	struct enic *enic = netdev_priv(netdev);
 	unsigned int i;
+	int disable_err = 0;
 	int err;
 
+	/* An internal reset can leave IFF_UP set after software has already
+	 * stopped the datapath.  Never disable NAPI or release rings twice.
+	 */
+	if (READ_ONCE(enic->datapath_state) != ENIC_DMA_LIVE)
+		return 0;
+
 	for (i = 0; i < enic->intr_count; i++) {
 		vnic_intr_mask(&enic->intr[i]);
 		(void)vnic_intr_masked(&enic->intr[i]); /* flush write */
@@ -1870,33 +1954,44 @@ static int enic_stop(struct net_device *netdev)
 	for (i = 0; i < enic->wq_count; i++) {
 		err = vnic_wq_disable(&enic->wq[i].vwq);
 		if (err)
-			return err;
+			netdev_err(netdev, "WQ %u did not stop: %d\n", i, err);
+		if (err && !disable_err)
+			disable_err = err;
 	}
 	for (i = 0; i < enic->rq_count; i++) {
 		err = vnic_rq_disable(&enic->rq[i].vrq);
 		if (err)
-			return err;
+			netdev_err(netdev, "RQ %u did not stop: %d\n", i, err);
+		if (err && !disable_err)
+			disable_err = err;
 	}
 
-	enic_dev_notify_unset(enic);
-	enic_unset_affinity_hint(enic);
-	enic_free_intr(enic);
-
-	for (i = 0; i < enic->wq_count; i++)
-		vnic_wq_clean(&enic->wq[i].vwq, enic_free_wq_buf);
-	for (i = 0; i < enic->rq_count; i++) {
-		vnic_rq_clean(&enic->rq[i].vrq, enic_free_rq_buf);
-		page_pool_destroy(enic->rq[i].pool);
-		enic->rq[i].pool = NULL;
+	if (disable_err) {
+		/* Even queues which stopped belong to this datapath allocation
+		 * graph.  Retain the graph as a unit when any queue times out.
+		 */
+		enic_datapath_quarantine(enic);
+		return disable_err;
 	}
-	for (i = 0; i < enic->cq_count; i++)
-		vnic_cq_clean(&enic->cq[i]);
-	for (i = 0; i < enic->intr_count; i++)
-		vnic_intr_clean(&enic->intr[i]);
+
+	enic_datapath_release(enic);
 
 	return 0;
 }
 
+int enic_close_for_reconfigure(struct net_device *netdev)
+{
+	int err;
+
+	/* dev_close() cannot propagate an ndo_stop() error. */
+	err = enic_stop(netdev);
+	if (err)
+		return err;
+
+	dev_close(netdev);
+	return 0;
+}
+
 static int _enic_change_mtu(struct net_device *netdev, int new_mtu)
 {
 	bool running = netif_running(netdev);
@@ -1974,7 +2069,7 @@ static void enic_poll_controller(struct net_device *netdev)
 		enic_isr_msi(enic->pdev->irq, enic);
 		break;
 	case VNIC_DEV_INTR_MODE_INTX:
-		enic_isr_legacy(enic->pdev->irq, netdev);
+		enic_isr_legacy(enic->pdev->irq, enic);
 		break;
 	default:
 		break;
@@ -2258,11 +2353,17 @@ static void enic_admin_chan_reopen(struct enic *enic)
 static void enic_reset(struct work_struct *work)
 {
 	struct enic *enic = container_of(work, struct enic, reset);
+	int stop_err = 0;
+	bool reopen;
 
-	if (!netif_running(enic->netdev))
+	if (enic_datapath_is_quarantined(enic) ||
+	    !netif_running(enic->netdev))
 		return;
 
 	rtnl_lock();
+	reopen = netif_running(enic->netdev);
+	if (!reopen || enic_datapath_is_quarantined(enic))
+		goto unlock;
 
 	/* Stop any activity from infiniband */
 	enic_set_api_busy(enic, true);
@@ -2276,9 +2377,16 @@ static void enic_reset(struct work_struct *work)
 	if (enic_has_admin_chan(enic))
 		enic_admin_channel_close(enic);
 
-	enic_stop(enic->netdev);
+	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)) {
+		netdev_err(enic->netdev,
+			   "Datapath queue did not stop: %d; DMA resources retained until reboot\n",
+			   stop_err);
+		goto reset_out;
+	}
 
 	enic_dev_soft_reset(enic);
 	enic_reset_addr_lists(enic);
@@ -2287,7 +2395,8 @@ static void enic_reset(struct work_struct *work)
 	enic_dev_set_ig_vlan_rewrite_mode(enic);
 	enic_ext_cq(enic);
 
-	enic_open(enic->netdev);
+	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;
@@ -2297,19 +2406,27 @@ static void enic_reset(struct work_struct *work)
 	if (enic_has_admin_chan(enic))
 		enic_admin_chan_reopen(enic);
 
+reset_out:
 	/* Allow infiniband to fiddle with the device again */
 	enic_set_api_busy(enic, false);
 
-	call_netdevice_notifiers(NETDEV_REBOOT, enic->netdev);
+	if (reopen)
+		call_netdevice_notifiers(NETDEV_REBOOT, enic->netdev);
 
+unlock:
 	rtnl_unlock();
 }
 
 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;
 
 	rtnl_lock();
+	reopen = netif_running(enic->netdev);
+	if (!reopen || enic_datapath_is_quarantined(enic))
+		goto unlock;
 
 	/* Stop any activity from infiniband */
 	enic_set_api_busy(enic, true);
@@ -2322,9 +2439,16 @@ static void enic_tx_hang_reset(struct work_struct *work)
 		enic_admin_channel_close(enic);
 
 	enic_dev_hang_notify(enic);
-	enic_stop(enic->netdev);
+	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)) {
+		netdev_err(enic->netdev,
+			   "Datapath queue did not stop: %d; DMA resources retained until reboot\n",
+			   stop_err);
+		goto hang_reset_out;
+	}
 
 	enic_dev_hang_reset(enic);
 	enic_reset_addr_lists(enic);
@@ -2333,7 +2457,8 @@ static void enic_tx_hang_reset(struct work_struct *work)
 	enic_dev_set_ig_vlan_rewrite_mode(enic);
 	enic_ext_cq(enic);
 
-	enic_open(enic->netdev);
+	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;
@@ -2343,11 +2468,14 @@ static void enic_tx_hang_reset(struct work_struct *work)
 	if (enic_has_admin_chan(enic))
 		enic_admin_chan_reopen(enic);
 
+hang_reset_out:
 	/* Allow infiniband to fiddle with the device again */
 	enic_set_api_busy(enic, false);
 
-	call_netdevice_notifiers(NETDEV_REBOOT, enic->netdev);
+	if (reopen)
+		call_netdevice_notifiers(NETDEV_REBOOT, enic->netdev);
 
+unlock:
 	rtnl_unlock();
 }
 
@@ -2678,10 +2806,13 @@ static int enic_alloc_enic_resources(struct enic *enic)
 	return -ENOMEM;
 }
 
-static void enic_dev_deinit(struct enic *enic)
+static void enic_del_napi(struct enic *enic)
 {
 	unsigned int i;
 
+	if (!enic->napi_added)
+		return;
+
 	for (i = 0; i < enic->rq_count; i++)
 		__netif_napi_del(&enic->napi[i]);
 
@@ -2691,6 +2822,12 @@ static void enic_dev_deinit(struct enic *enic)
 
 	/* observe RCU grace period after __netif_napi_del() calls */
 	synchronize_net();
+	enic->napi_added = false;
+}
+
+static void enic_dev_deinit(struct enic *enic)
+{
+	enic_del_napi(enic);
 
 	enic_free_vnic_resources(enic);
 	enic_clear_intr_mode(enic);
@@ -2783,6 +2920,7 @@ static int enic_dev_init(struct enic *enic)
 				       enic_poll_msix_wq);
 		break;
 	}
+	enic->napi_added = true;
 
 	return 0;
 
@@ -3036,7 +3174,10 @@ static int enic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		goto err_out_free_netdev;
 	}
 
-	err = pci_request_regions(pdev, DRV_NAME);
+	/* Terminal DMA quarantine may retain the PCI regions across module
+	 * removal. Use the PCI device name because it outlives module rodata.
+	 */
+	err = pci_request_regions(pdev, dev_name(dev));
 	if (err) {
 		dev_err(dev, "Cannot request PCI regions, aborting\n");
 		goto err_out_disable_device;
@@ -3387,12 +3528,72 @@ static int enic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	return err;
 }
 
+static int enic_datapath_recheck_on_remove(struct enic *enic)
+{
+	int disable_err = 0;
+	unsigned int i;
+	int err;
+
+	if (!enic_datapath_is_quarantined(enic))
+		return 0;
+
+	/* Removal is the last opportunity to observe that every queue drained.
+	 * Keep the complete graph if even one queue still reports RUNNING.
+	 */
+	for (i = 0; i < enic->wq_count; i++) {
+		err = vnic_wq_disable(&enic->wq[i].vwq);
+		if (err && !disable_err)
+			disable_err = err;
+	}
+	for (i = 0; i < enic->rq_count; i++) {
+		err = vnic_rq_disable(&enic->rq[i].vrq);
+		if (err && !disable_err)
+			disable_err = err;
+	}
+	if (disable_err)
+		return disable_err;
+
+	dev_info(&enic->pdev->dev,
+		 "All datapath queues stopped at remove; releasing DMA graph\n");
+	enic_datapath_release(enic);
+
+	return 0;
+}
+
+static void enic_terminal_dma_quarantine(struct enic *enic)
+{
+	unsigned int i;
+
+	/* Detach every CPU callback before retaining the DMA owner graph.  The
+	 * ordinary stop path can return before freeing IRQs when a queue does
+	 * not stop, so each ownership-aware operation must be idempotent.
+	 */
+	for (i = 0; i < enic->intr_count; i++) {
+		vnic_intr_mask(&enic->intr[i]);
+		(void)vnic_intr_masked(&enic->intr[i]);
+	}
+	if (vnic_dev_get_intr_mode(enic->vdev) == VNIC_DEV_INTR_MODE_INTX)
+		pci_intx(enic->pdev, 0);
+	enic_synchronize_irqs(enic);
+	enic_unset_affinity_hint(enic);
+	enic_free_intr(enic);
+	enic_del_napi(enic);
+	enic_clear_intr_mode(enic);
+	enic_free_affinity_hint(enic);
+
+	/* Preserve netdev, vdev, BAR mappings, regions, rings, and their buffer
+	 * metadata because queue ownership remains unresolved until reboot.
+	 */
+	pci_clear_master(enic->pdev);
+}
+
 static void enic_remove(struct pci_dev *pdev)
 {
 	struct net_device *netdev = pci_get_drvdata(pdev);
 
 	if (netdev) {
 		struct enic *enic = netdev_priv(netdev);
+		bool retain_dma = false;
 
 		disable_work_sync(&enic->reset);
 		disable_work_sync(&enic->tx_hang_reset);
@@ -3429,14 +3630,25 @@ static void enic_remove(struct pci_dev *pdev)
 				pci_disable_sriov(pdev);
 		}
 #endif
+		if (enic_datapath_is_quarantined(enic))
+			enic_datapath_recheck_on_remove(enic);
+		if (enic_datapath_is_quarantined(enic)) {
+			dev_err(&pdev->dev,
+				"Queue DMA ownership unresolved; retaining resources until reboot\n");
+			enic_terminal_dma_quarantine(enic);
+			retain_dma = true;
+			goto disable_device;
+		}
 		enic_dev_deinit(enic);
 		vnic_dev_close(enic->vdev);
 		kfree(enic->pp);
 		vnic_dev_unregister(enic->vdev);
 		enic_iounmap(enic);
 		pci_release_regions(pdev);
+disable_device:
 		pci_disable_device(pdev);
-		free_netdev(netdev);
+		if (!retain_dma)
+			free_netdev(netdev);
 	}
 }
 
diff --git a/drivers/net/ethernet/cisco/enic/vnic_rq.c b/drivers/net/ethernet/cisco/enic/vnic_rq.c
index a662d9fd1199..f76de6432a86 100644
--- a/drivers/net/ethernet/cisco/enic/vnic_rq.c
+++ b/drivers/net/ethernet/cisco/enic/vnic_rq.c
@@ -6,9 +6,9 @@
 
 #include <linux/kernel.h>
 #include <linux/errno.h>
+#include <linux/iopoll.h>
 #include <linux/types.h>
 #include <linux/pci.h>
-#include <linux/delay.h>
 #include <linux/slab.h>
 
 #include "vnic_dev.h"
@@ -150,8 +150,9 @@ void vnic_rq_enable(struct vnic_rq *rq)
 
 int vnic_rq_disable(struct vnic_rq *rq)
 {
-	unsigned int wait;
 	struct vnic_dev *vdev = rq->vdev;
+	u32 running;
+	int err;
 	int i;
 
 	/* Due to a race condition with clearing RQ "mini-cache" in hw, we need
@@ -161,15 +162,16 @@ int vnic_rq_disable(struct vnic_rq *rq)
 	for (i = 0; i < 2; i++) {
 		iowrite32(0, &rq->ctrl->enable);
 
-		/* Wait for HW to ACK disable request */
-		for (wait = 20000; wait > 0; wait--)
-			if (!ioread32(&rq->ctrl->running))
-				break;
-		if (!wait) {
+		/* Poll for up to 500 ms for outstanding operations to drain.
+		 * Preserve both passes required for the RQ mini-cache.
+		 */
+		err = read_poll_timeout(ioread32, running, !running, 10, 500000,
+					false, &rq->ctrl->running);
+		if (err) {
 			vdev_neterr(vdev, "Failed to disable RQ[%d]\n",
 				    rq->index);
 
-			return -ETIMEDOUT;
+			return err;
 		}
 	}
 
diff --git a/drivers/net/ethernet/cisco/enic/vnic_wq.c b/drivers/net/ethernet/cisco/enic/vnic_wq.c
index 5a20bdc62141..3d189d6869dc 100644
--- a/drivers/net/ethernet/cisco/enic/vnic_wq.c
+++ b/drivers/net/ethernet/cisco/enic/vnic_wq.c
@@ -6,9 +6,9 @@
 
 #include <linux/kernel.h>
 #include <linux/errno.h>
+#include <linux/iopoll.h>
 #include <linux/types.h>
 #include <linux/pci.h>
-#include <linux/delay.h>
 #include <linux/slab.h>
 
 #include "vnic_dev.h"
@@ -170,21 +170,21 @@ void vnic_wq_enable(struct vnic_wq *wq)
 
 int vnic_wq_disable(struct vnic_wq *wq)
 {
-	unsigned int wait;
 	struct vnic_dev *vdev = wq->vdev;
+	u32 running;
+	int err;
 
 	iowrite32(0, &wq->ctrl->enable);
 
-	/* Wait for HW to ACK disable request */
-	for (wait = 0; wait < 1000; wait++) {
-		if (!(ioread32(&wq->ctrl->running)))
-			return 0;
-		udelay(10);
-	}
+	/* Poll for up to 500 ms for outstanding operations to drain. */
+	err = read_poll_timeout(ioread32, running, !running, 10, 500000,
+				false, &wq->ctrl->running);
+	if (!err)
+		return 0;
 
 	vdev_neterr(vdev, "Failed to disable WQ[%d]\n", wq->index);
 
-	return -ETIMEDOUT;
+	return err;
 }
 
 void vnic_wq_clean(struct vnic_wq *wq,

-- 
2.43.0

Re: [PATCH net 1/3] enic: retain datapath DMA resources until queues stop

[netdev-bot+sashiko]

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

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

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).

- [High] On terminal quarantine, enic_remove() skips
  enic_datapath_release(), the only caller of page_pool_destroy() on…
- [Medium] Each vnic_wq_disable() now sleep-polls for up to 500 ms; the
  old loop was 1000 x udelay(10), about 10 ms.
- [Medium] enic_datapath_quarantine() calls pci_clear_master() at runtime
  (enic_main.c:1767), but the driver keeps issuing firmware commands…
- [Medium] In enic_reset() and enic_tx_hang_reset(), quarantine is
  checked only after enic_stop().
- [Medium] enic_close_for_reconfigure() (enic_main.c:1982) calls
  enic_stop() directly before dev_close().
- [Low] The comment on enic->datapath_state in enic.h ('Retain the
  complete mapped datapath graph for the remainder of this boot') and…
- [Low] When enic_stop() or the enic_open() unwind quarantines the
  datapath, it returns before enic_datapath_release() →…

--

enic: retain datapath DMA resources until queues stop

The patch makes enic_stop() and the enic_open() unwind try every WQ/RQ
disable. If any queue fails to report RUNNING == 0, the whole datapath
graph is kept in a quarantined state and bus mastering is cleared.
It also switches the queue-disable polls to read_poll_timeout() with a
500 ms bound, and adds a remove-time recheck that either releases or
permanently retains the DMA graph.

> diff --git a/drivers/net/ethernet/cisco/enic/enic.h b/drivers/net/ethernet/cisco/enic/enic.h
> index 7a509a0569900..992ac43d1b0a3 100644
> --- a/drivers/net/ethernet/cisco/enic/enic.h
> +++ b/drivers/net/ethernet/cisco/enic/enic.h

[ ... ]

> @@ -325,6 +333,10 @@ struct enic {
>  	spinlock_t vf_link_state_lock;
>  	enum enic_vf_link_state vf_link_state;
>  	bool vf_link_running;
> +	/* A queue-disable failure leaves DMA ownership unresolved.  Retain the
> +	 * complete mapped datapath graph for the remainder of this boot.
> +	 */
> +	enum enic_dma_state datapath_state;

[Severity: Low]
Is this comment accurate? If every queue reports RUNNING == 0 at removal,
enic_datapath_recheck_on_remove() calls enic_datapath_release(). That
releases the graph and sets the state back to ENIC_DMA_DOWN, and
enic_remove() then does the normal teardown.

The comment in the enic_open() unwind ("for the remainder of this boot")
says the same thing. So do the "remains quarantined until reboot" and
"DMA resources retained until reboot" messages.

Could these mention the remove-time release path?

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

[ ... ]

> @@ -1718,11 +1729,51 @@ static void enic_notify_timer_start(struct enic *enic)

[ ... ]

> +static void enic_datapath_quarantine(struct enic *enic)
> +{
> +	WRITE_ONCE(enic->datapath_state, ENIC_DMA_QUARANTINED);
> +	WRITE_ONCE(enic->mbox_send_disabled, true);
> +	netif_device_detach(enic->netdev);
> +	netif_carrier_off(enic->netdev);
> +	/* Bus-master disable contains the failed function but does not return
> +	 * ownership of DMA resources which the device may still reference.
> +	 */
> +	pci_clear_master(enic->pdev);
> +}

[Severity: Medium]
Can devcmds still complete once bus mastering is cleared here? The netdev
is only detached, not unregistered, so ndo_get_stats64 still reaches the
firmware:

enic_get_stats()
  enic_dev_stats_dump()
    vnic_dev_stats_dump()
      vnic_dev_cmd()
        _vnic_dev_cmd2()

_vnic_dev_cmd2() writes the command into a host-memory ring. It then
polls result->color, which the device has to DMA-write. With BME off,
each call looks like it would busy-wait 1000 x udelay(100) under
spin_lock_bh(devcmd_lock) and then log "devcmd timed out".

The device's fetch_index would also stop advancing. After about 31
commands every call would then fail immediately with an unratelimited
"wq is full" error. Since /proc/net/dev and rtnetlink stats can be read
by unprivileged users, can this be used to flood the log?

There is a second case. When enic_datapath_recheck_on_remove() later
succeeds, the release path runs:

enic_datapath_release()
  enic_dev_notify_unset()
    vnic_dev_notify_unset()

vnic_dev_notify_unset() calls dma_free_coherent() on the notify buffer
before it sends CMD_NOTIFY, and that command cannot complete with BME
cleared. Does the firmware then keep the stale notify address? A rebind
calls pci_set_master() in enic_probe() before notify is set up again.
Could the firmware DMA into the freed buffer in that window?

[Severity: Low]
The runtime quarantine path does not free the IRQ or disable INTx. Only
enic_terminal_dma_quarantine() calls pci_intx(pdev, 0) and
enic_free_intr().

In INTx mode the line is IRQF_SHARED, so interrupts from a peer device
still run enic_isr_legacy():

	pba = vnic_intr_legacy_pba(enic->legacy_pba);
	if (!pba) {
		vnic_intr_unmask(&enic->intr[io_intr]);
		return IRQ_NONE;	/* not our interrupt */
	}

Does this unmask io_intr again after enic_stop() masked it?

If the PBA reports the notify bit, enic_notify_check()->enic_link_check()
can call netif_carrier_on() on the detached netdev, which undoes the
netif_carrier_off() above. It can also schedule link_notify_work, and
enic_mtu_check() can schedule change_mtu_work. None of these paths look
at datapath_state.

[ ... ]

> @@ -1870,33 +1954,44 @@ static int enic_stop(struct net_device *netdev)
>  	for (i = 0; i < enic->wq_count; i++) {
>  		err = vnic_wq_disable(&enic->wq[i].vwq);
>  		if (err)
> -			return err;
> +			netdev_err(netdev, "WQ %u did not stop: %d\n", i, err);
> +		if (err && !disable_err)
> +			disable_err = err;
>  	}
>  	for (i = 0; i < enic->rq_count; i++) {
>  		err = vnic_rq_disable(&enic->rq[i].vrq);
>  		if (err)
> -			return err;
> +			netdev_err(netdev, "RQ %u did not stop: %d\n", i, err);
> +		if (err && !disable_err)
> +			disable_err = err;
>  	}

[Severity: Medium]
How long can rtnl_mutex be held here when the device is wedged or
surprise-removed and the RUNNING registers read back as all-ones?

vnic_wq_disable() used to give up after about 10 ms. vnic_rq_disable()
used a tight 20000-read loop. Each now polls for up to 500 ms, and the
early return is gone. A full sweep could therefore take about
(wq_count + rq_count) * 500 ms. With ENIC_WQ_MAX and ENIC_RQ_MAX both
256, that is up to about 256 seconds.

The same sweep runs in the enic_open() unwind, and again in
enic_datapath_recheck_on_remove(). enic_stop() itself is called from
unregister_netdev() during remove, from set_ringparam via
enic_close_for_reconfigure(), from the change_mtu worker, and from both
reset workers. Could this trigger hung-task warnings or starve other
rtnl users for minutes?

Some callers ignore the return value but also get the longer bound:
vnic_wq_alloc()/vnic_rq_alloc(), the vnic_dev_init_devcmd2() error
unwind, vnic_dev_deinit_devcmd2(), and the admin channel close. The later
admin-channel patch also adds a retry around the admin close.

[ ... ]

> +int enic_close_for_reconfigure(struct net_device *netdev)
> +{
> +	int err;
> +
> +	/* dev_close() cannot propagate an ndo_stop() error. */
> +	err = enic_stop(netdev);
> +	if (err)
> +		return err;
> +
> +	dev_close(netdev);
> +	return 0;
> +}

[Severity: Medium]
enic_stop() now runs before dev_close() has called netpoll_poll_disable().
With netconsole active and the device in INTx mode, can another CPU be
in this path?

netpoll_poll_dev()
  enic_poll_controller()
    enic_isr_legacy()
      enic_notify_check()
        vnic_dev_notify_ready()

At the same time this CPU may be running:

enic_stop()
  enic_datapath_release()
    enic_dev_notify_unset()
      vnic_dev_notify_unset()

vnic_dev_notify_unset() calls dma_free_coherent() on vdev->notify but
only clears the pointer after the CMD_NOTIFY devcmd completes.
synchronize_irq() and napi_disable() do not cover handlers called directly
through ndo_poll_controller.

vnic_dev_notify_ready() checks vdev->notify and then memcpy()s from it,
looping while (csum != words[0]). Can it read the freed buffer, or even
spin forever if the freed memory holds a stable but mismatched checksum?

Before this patch, set_ringparam used dev_close(), which excluded netpoll.
The _enic_change_mtu() path already had the same pattern.

[ ... ]

> @@ -2287,7 +2395,8 @@ static void enic_reset(struct work_struct *work)
>  	enic_dev_set_ig_vlan_rewrite_mode(enic);
>  	enic_ext_cq(enic);
>  
> -	enic_open(enic->netdev);
> +	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;
> @@ -2297,19 +2406,27 @@ static void enic_reset(struct work_struct *work)
>  	if (enic_has_admin_chan(enic))
>  		enic_admin_chan_reopen(enic);

[Severity: Medium]
What happens if enic_open() fails here? For example, page_pool_create(),
the RQ fill or enic_dev_enable() fails, and the unwind then cannot stop a
queue. enic_open() calls enic_datapath_quarantine() and returns an error.
The worker checks neither the return value nor datapath_state before it
calls enic_admin_chan_reopen().

enic_admin_channel_open() allocates and enables the admin WQ/RQ, posts RX
buffers and issues QP_TYPE_SET. On success it does:

	WRITE_ONCE(enic->mbox_send_disabled, false);

Does this undo the mbox_send_disabled = true that
enic_datapath_quarantine() just set? It would then go on to the VF
capability check and registration, or the PF link_notify_work, on a
contained function.

In this tree a V2 device can still use DEVCMD1 over MMIO, so QP_TYPE_SET
can succeed with BME cleared. After the later "enic: require DEVCMD2 and
set the V2 VF mailbox source" commit, QP_TYPE_SET would probably time out
instead, and mbox_send_disabled would stay set. Even then, the worker
still enables admin DMA queues and posts buffers on a quarantined
function. It spends devcmd timeouts under rtnl, can push the admin graph
into quarantine too, and still fires NETDEV_REBOOT.

The enic_stop() failure case has a goto reset_out. Should there be a
similar check after enic_open()? enic_tx_hang_reset() has the same
sequence.

[ ... ]

> @@ -3429,14 +3630,25 @@ static void enic_remove(struct pci_dev *pdev)
>  				pci_disable_sriov(pdev);
>  		}
>  #endif
> +		if (enic_datapath_is_quarantined(enic))
> +			enic_datapath_recheck_on_remove(enic);
> +		if (enic_datapath_is_quarantined(enic)) {
> +			dev_err(&pdev->dev,
> +				"Queue DMA ownership unresolved; retaining resources until reboot\n");
> +			enic_terminal_dma_quarantine(enic);
> +			retain_dma = true;
> +			goto disable_device;
> +		}

[Severity: High]
Can the retained RX page pools outlive the struct pci_dev they point to?

enic_open() creates each pool with .dev = &enic->pdev->dev and
PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV. This branch skips
page_pool_destroy(). page_pool does not take a reference on the device.
The retained netdev does not pin the parent either, because
unregister_netdev()->device_del() drops its reference on the PCI parent.

The PCI device can then go away entirely, through surprise hot-unplug or
DPC (the all-ones reads make quarantine certain in that case), sysfs
remove, or VF removal when SR-IOV is disabled. After that, an in-flight
RX page still held by a socket or GRO is returned to the retained pool:

page_pool return path
  page_pool_dma_sync_for_device()
    dma_dev_need_sync(pool->p.dev)
  __page_pool_release_page_dma()
    dma_unmap_page_attrs(pool->p.dev, ...)

Does this dereference the freed struct device and its DMA ops or IOMMU
state? Normally page_pool_destroy()->page_pool_scrub() clears dma_sync and
unmaps the pages first, but that step is skipped here.

The commit message only claims safety "while the PCI device remains
enumerated". Should the retained graph hold a reference on the PCI
device?

-- 
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