* [PATCH net v6 0/2] enic: retain admin DMA resources and identify V2 VFs
@ 2026-11-07 19:02 Satish Kharat
2026-11-07 19:02 ` [PATCH net v6 1/2] enic: retain admin-channel DMA until queues stop Satish Kharat
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Satish Kharat @ 2026-11-07 19:02 UTC (permalink / raw)
To: netdev
Cc: Sesidhar Baddela, Aravind Dhanasekaran, Reese Faucette,
Satish Kharat, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Fix premature freeing of admin-channel buffers and set the correct V2 VF
source ID in mailbox messages. This is v6 of the two-patch admin-only
series. The generic datapath quarantine proposal withdrawn in v2 remains
out of scope, including its page-pool/device lifetime problem.
The first patch releases the admin WQ/RQ/CQ allocation only after both
complete queue-disable helpers succeed. Give both queues a first attempt,
then retry once only a helper which timed out. The 500 ms polling bound
is local to admin shutdown, and both RQ disable/check passes are preserved.
Ordinary datapath and DEVCMD2 queue timeouts are unchanged. Admin receive
buffers are driver-owned mappings, not page-pool pages returned later
from the networking stack.
Keep PCI bus mastering enabled during runtime admin quarantine so command
transport and ordinary teardown can continue. An affected V2 VF is closed
and cannot reopen while its control channel is quarantined. Detach the
netdev under RTNL during removal, then perform the VF unregister handshake
and admin close after releasing RTNL. Skip admin reopen after a failed
datapath open. Clear local VF registration after channel replies have been
drained, including after a failed re-registration closes the channel.
Probe unwind and removal finish normal generic teardown and attempt
CMD_CLOSE before one final direct admin queue check. If that check fails,
retain the inert owner and unresolved DMA targets rather than free memory
the device might still reference. This intentionally leaves allocations
and a BAR reservation behind. It blocks ordinary reprobe while the PCI
device remains enumerated; PCI remove/rescan is not a supported recovery
method. A PCI-device reference keeps the reservation name valid, but does
not preserve IOMMU attachment or establish that DMA stopped. No IRQ, work
or other CPU callback may enter retained objects after module unload. DMA
debugging can report pending allocations. Reboot is the supported recovery.
The second patch requires DEVCMD2 for SR-IOV V2 and obtains the VF source
index from CMD_SRIOV_STATS_GET. A deployed async PF compares that source
against the CQ-derived sender. After QP enable, obtain and validate the ID
while receive interrupts remain masked. Publish send readiness before
unmasking, so queued notifications can be processed and acknowledged with
a valid source. Sends remain disabled until the ID is valid, using the
existing send gate. Notification handling does not rely on PF retransmission.
Keep the statistics target persistent and observe command completion
separately from firmware status. A later blocking QP or close result
fences earlier statistics DMA, including an error result. A pre-post
failure, posted timeout or NOWAIT posting does not establish completion.
This command ordering does not replace admin queue-stop proof.
Sashiko's feedback prompted this audit. The pre-existing failed-reset
reopen lifecycle and generic queue-disable error paths remain separate
follow-up work; this revision does not claim to fix them. Source review
and testing were assisted by LLM tools. The author is responsible for
the submitted changes.
Validation:
- V6 passed build and static checks at both patch boundaries. Sparse
reported the same existing diagnostics as the public base.
- The uninstrumented V6 candidate successfully bound, opened and closed
a V2 VF.
- A test-only overlay accepted statistics version 2 and rejected version 0
and VF index 0, which is reserved for the PF.
- A test-only overlay exercised admin quarantine during probe failure
and removal. Generic teardown and CMD_CLOSE completed before the final
queue check released the admin graph, and a clean reprobe succeeded.
Keeping the synthetic timeout through that check exercised terminal
retention. Guest reboot restored the original module and configuration,
after which traffic passed.
- Full functional and interoperability tests on V4 passed with Cisco VIC
PFs and V2 VFs, including a nonzero VF and all four combinations of the
development ENIC and deployed async PF/VF drivers. Traffic covered
bidirectional ICMP, TCP, UDP and jumbo frames.
- Tests covered station/address/filter and administrative-MAC policy
changes, concurrent address updates and reopen, and multiple module
reload, ring resize, interface reopen and VF destroy/recreate cycles.
- Earlier fault-injection tests covered admin queue retry, reset recovery,
queued PF notification delivery after VF-ID discovery and completed-close
statistics-buffer cleanup. They let real DMA complete; no test reproduced
a physically stuck queue.
The functional and interoperability tests included later VF/PF feature
patches. These two fixes alone do not enable in-tree PF SR-IOV support.
Physical device removal was not tested; a physical DMA-engine failure
was not reproduced.
Signed-off-by: Satish Kharat <satishkh@cisco.com>
---
Changes in v6:
- Accept statistics ABI version 1 or later because newer versions preserve
the version-1 prefix.
- Document that the statistics command is mandatory for V2 VF probe and
correct the reset and DMA-fence comments.
- Link to v5: https://patch.msgid.link/20261103-b4-enic-dma-vf-index-net-generic-first-v1-20261010-v5-0-2501d1223eff@cisco.com
Changes in v5:
- Remove the redundant mbox_tx_poisoned flag. The channel state owns the
allocation lifetime, while mbox_send_disabled gates new sends.
- Run the final direct admin queue recheck after generic teardown and
CMD_CLOSE in both probe unwind and removal.
- Keep all-ones MMIO reads and recoverable PCI channel-offline states
conservative; neither is accepted as queue-stop proof.
- Validate final release and terminal retention on the exact V5 tree.
- Link to v4: https://patch.msgid.link/20261101-b4-enic-dma-vf-index-net-generic-first-v1-20261010-v4-0-a7e209210453@cisco.com
Changes in v4:
- Clear vf_registered after closing the channel on both failed VF
re-registration paths; a late reply may have set it before the send
reported a timeout.
- Hold RTNL only while detaching the netdev during removal. Perform the VF
unregister handshake and admin close after releasing RTNL.
- Keep the valid pre-existing generic datapath repeated-stop problem as
separate follow-up work rather than adding a partial fix to this series.
- Link to v3: https://patch.msgid.link/20261027-b4-enic-dma-vf-index-net-generic-first-v1-20261010-v3-0-ba237ed71241@cisco.com
Changes in v3:
- Use observed blocking CMD_CLOSE completion, including an error result,
to fence earlier statistics DMA in probe unwind and removal.
- Obtain the VF source ID after QP enable but before receive interrupts
are unmasked, and permit sends before delivering queued notifications.
Use the existing send gate instead of a redundant invalid-ID check.
- Validate delayed notifications, discovery failures and completed-close
cleanup on a VF.
- Rerun the full functional suite and all four PF/VF driver pairings
with the v3 fixes in the later VF/PF feature compositions.
- Clear local VF registration after channel close in both reset workers.
- Correct comments and explain quarantine, recovery and retention limits.
- Link to v2: https://patch.msgid.link/20261022-b4-enic-dma-vf-index-net-generic-first-v1-20261010-v2-0-8db19b4a714e@cisco.com
Changes in v2:
- Withdraw v1 patch 1's generic datapath quarantine proposal. Rewrite v1
patch 2 as the first patch here, followed by the VF mailbox-source fix.
- Keep the admin buffers' lifetime separate from generic page pools and
notification teardown. Keep bus mastering enabled at runtime.
- Bound longer polling to the two admin queues, preserve both RQ passes,
and retry only a complete disable helper returning -ETIMEDOUT.
- Serialize remove/open and skip admin reopen after datapath-open failure.
- Observe DEVCMD2 transport completion independently of command success,
including a completed QP error, without treating pre-post errors or
NOWAIT posting as a fence.
- Clarify final remove recheck, terminal retention and reprobe limits.
- Link to v1: https://patch.msgid.link/20261016-b4-enic-dma-vf-index-net-generic-first-v1-20261010-v1-0-d27b55f69883@cisco.com
Cc: Andrew Lunn <andrew+netdev@lunn.ch>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@kernel.org>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
---
Satish Kharat (2):
enic: retain admin-channel DMA until queues stop
enic: require DEVCMD2 and set the V2 VF mailbox source
drivers/net/ethernet/cisco/enic/enic.h | 21 ++-
drivers/net/ethernet/cisco/enic/enic_admin.c | 240 ++++++++++++++++++++------
drivers/net/ethernet/cisco/enic/enic_admin.h | 4 +-
drivers/net/ethernet/cisco/enic/enic_dev.c | 44 +++++
drivers/net/ethernet/cisco/enic/enic_dev.h | 3 +
drivers/net/ethernet/cisco/enic/enic_main.c | 129 ++++++++++++--
drivers/net/ethernet/cisco/enic/enic_mbox.c | 12 +-
drivers/net/ethernet/cisco/enic/vnic_dev.c | 85 ++++++++-
drivers/net/ethernet/cisco/enic/vnic_dev.h | 15 ++
drivers/net/ethernet/cisco/enic/vnic_devcmd.h | 34 ++++
10 files changed, 512 insertions(+), 75 deletions(-)
---
base-commit: 0df412012a26389582b120844d098e1e961f10a6
change-id: 20261010-b4-enic-dma-vf-index-net-generic-first-v1-20261010-d9abf4510335
Best regards,
--
Satish Kharat <satishkh@cisco.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH net v6 1/2] enic: retain admin-channel DMA until queues stop
2026-11-07 19:02 [PATCH net v6 0/2] enic: retain admin DMA resources and identify V2 VFs Satish Kharat
@ 2026-11-07 19:02 ` Satish Kharat
2026-11-08 20:00 ` netdev-bot+sashiko
2026-11-07 19:02 ` [PATCH net v6 2/2] enic: require DEVCMD2 and set the V2 VF mailbox source Satish Kharat
2026-11-09 20:26 ` [PATCH net v6 0/2] enic: retain admin DMA resources and identify V2 VFs Satish Kharat
2 siblings, 1 reply; 6+ messages in thread
From: Satish Kharat @ 2026-11-07 19:02 UTC (permalink / raw)
To: netdev
Cc: Sesidhar Baddela, Aravind Dhanasekaran, Reese Faucette,
Satish Kharat, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
Admin close and the partial-open unwind currently free the admin
WQ/RQ/CQ allocation even when disabling one of the queues times out. A
posted WQ message or an RQ buffer can then remain reachable by the
device after its mapping and allocation have been released.
Track the channel as DOWN, LIVE or QUARANTINED. Stop its IRQ and work,
block new sends and join an in-flight sender before queue shutdown. Give
both queues a first disable attempt, then retry only a complete helper
that timed out. Poll for up to 500 ms per pass locally to the admin
path, preserving the RQ's two disable/check passes. Ordinary datapath
and DEVCMD2 queue timeouts are unchanged.
RUNNING == 0 proves that queue DMA has stopped. A nonzero value leaves
ownership unresolved; it does not establish that DMA is active. An
all-ones MMIO read or a recoverable PCI channel-offline state is not
accepted as queue-stop proof. Release the admin graph only when both
complete disable helpers succeed. Retain the graph otherwise and refuse
to reopen it. Keep bus mastering enabled at runtime so command transport
and ordinary teardown can continue. A V2 VF with a quarantined control
channel is closed and cannot reopen. Reset callers must not reset or
reopen a retained admin graph, or reopen admin queues after a failed
datapath open. Clear local VF registration after draining channel
replies, including when recovery exits early. Clear it again after a
failed re-registration closes the channel because a late REGISTER_REPLY
may have set it before the WQ completion timed out.
Check that an internal datapath open still has a present device. Recheck
the running state under RTNL in reset workers. During removal, detach the
netdev under RTNL and then release RTNL before the VF unregister handshake
and admin close. The detach and present-device checks exclude a concurrent
new open without holding the global lock across protocol and queue waits.
Probe unwind and removal finish ordinary generic teardown and attempt
CMD_CLOSE before making one final direct admin queue check. If that check
still fails, clear bus mastering and disable the PCI function. Keep the
inert netdev, vnic_dev, admin allocations and control targets rather than
freeing device-owned memory. Admin buffers use driver-owned mappings,
never page pools or pages returned later from sockets. This patch does
not repair the separate generic datapath queue-disable error path.
Keep the BAR reservation to block an ordinary rebind while the PCI
device remains enumerated. Use the PCI device name for the reservation
and hold a PCI-device reference so the owner-name pointer remains valid
after module unload. That reference does not preserve the IOMMU
attachment or prove DMA stopped. No IRQ, work or other CPU callback may
use the retained objects. DMA debugging can report the deliberately
pending allocations; reboot is the supported terminal recovery.
Fixes: 3258931d4052 ("enic: add admin channel open and close for SR-IOV")
Signed-off-by: Satish Kharat <satishkh@cisco.com>
---
drivers/net/ethernet/cisco/enic/enic.h | 15 ++-
drivers/net/ethernet/cisco/enic/enic_admin.c | 188 +++++++++++++++++++++------
drivers/net/ethernet/cisco/enic/enic_admin.h | 4 +-
drivers/net/ethernet/cisco/enic/enic_main.c | 104 +++++++++++++--
4 files changed, 249 insertions(+), 62 deletions(-)
diff --git a/drivers/net/ethernet/cisco/enic/enic.h b/drivers/net/ethernet/cisco/enic/enic.h
index 7a509a056990..ebe617e752c7 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_admin_chan_state {
+ ENIC_ADMIN_CHAN_DOWN,
+ ENIC_ADMIN_CHAN_LIVE,
+ ENIC_ADMIN_CHAN_QUARANTINED,
+};
+
/* Per-instance private data structure */
struct enic {
struct net_device *netdev;
@@ -298,12 +304,9 @@ 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.
- */
- bool admin_chan_up;
- /* set on send timeout; cleared on channel re-open */
+ /* QUARANTINED retains admin DMA until a final teardown recheck. */
+ enum enic_admin_chan_state admin_chan_state;
+ /* Blocks sends throughout close, partial open and timeout recovery. */
bool mbox_send_disabled;
struct vnic_wq admin_wq;
struct vnic_rq admin_rq;
diff --git a/drivers/net/ethernet/cisco/enic/enic_admin.c b/drivers/net/ethernet/cisco/enic/enic_admin.c
index 61c82b48044d..8a063c563a41 100644
--- a/drivers/net/ethernet/cisco/enic/enic_admin.c
+++ b/drivers/net/ethernet/cisco/enic/enic_admin.c
@@ -5,6 +5,7 @@
#include <linux/netdevice.h>
#include <linux/dma-mapping.h>
#include <linux/interrupt.h>
+#include <linux/iopoll.h>
#include "vnic_dev.h"
#include "vnic_wq.h"
@@ -530,11 +531,17 @@ 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_ADMIN_CHAN_DOWN) {
+ netdev_err(enic->netdev,
+ "Refusing to open non-down admin channel (%u)\n",
+ READ_ONCE(enic->admin_chan_state));
+ return -EBUSY;
+ }
/* 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 +574,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_ADMIN_CHAN_LIVE);
err = enic_admin_rq_fill(enic, GFP_KERNEL);
if (err) {
@@ -600,60 +608,51 @@ 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 int enic_admin_wq_disable(struct enic *enic)
{
- int err;
+ struct vnic_wq *wq = &enic->admin_wq;
+ u32 running;
- if (!enic->has_admin_channel)
- return;
-
- /* 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;
-
- WRITE_ONCE(enic->mbox_send_disabled, true);
-
- netdev_dbg(enic->netdev, "admin channel close\n");
+ iowrite32(0, &wq->ctrl->enable);
+ return read_poll_timeout(ioread32, running, !running, 10, 500000,
+ false, &wq->ctrl->running);
+}
- 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);
+static int enic_admin_rq_disable(struct enic *enic)
+{
+ struct vnic_rq *rq = &enic->admin_rq;
+ u32 running;
+ int err;
+ int i;
- enic_admin_qp_type_set(enic, QP_DISABLE);
+ /* Preserve the two disable/check passes used for the RQ mini-cache. */
+ for (i = 0; i < 2; i++) {
+ iowrite32(0, &rq->ctrl->enable);
+ err = read_poll_timeout(ioread32, running, !running, 10, 500000,
+ false, &rq->ctrl->running);
+ if (err)
+ return err;
+ }
- err = vnic_wq_disable(&enic->admin_wq);
- if (err)
- netdev_warn(enic->netdev,
- "Failed to disable admin WQ: %d\n", err);
- err = vnic_rq_disable(&enic->admin_rq);
- if (err)
- netdev_warn(enic->netdev,
- "Failed to disable admin RQ: %d\n", err);
+ return 0;
+}
+static void enic_admin_channel_release(struct enic *enic)
+{
vnic_wq_clean(&enic->admin_wq, enic_admin_wq_buf_clean);
enic_admin_rq_drain(enic);
vnic_cq_clean(&enic->admin_cq[0]);
@@ -662,6 +661,111 @@ void enic_admin_channel_close(struct enic *enic)
enic->admin_rq_handler = NULL;
enic_admin_free_resources(enic);
+ WRITE_ONCE(enic->admin_chan_state, ENIC_ADMIN_CHAN_DOWN);
+}
+
+int enic_admin_channel_close(struct enic *enic)
+{
+ int qp_err;
+ int rq_err;
+ int wq_err;
- enic->admin_chan_up = false;
+ if (!enic->has_admin_channel)
+ return 0;
+ if (READ_ONCE(enic->admin_chan_state) == ENIC_ADMIN_CHAN_DOWN)
+ return 0;
+ if (READ_ONCE(enic->admin_chan_state) == ENIC_ADMIN_CHAN_QUARANTINED)
+ return -EBUSY;
+
+ WRITE_ONCE(enic->mbox_send_disabled, true);
+
+ if (READ_ONCE(enic->admin_chan_state) == ENIC_ADMIN_CHAN_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);
+ }
+
+ /* 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);
+
+ wq_err = enic_admin_wq_disable(enic);
+ rq_err = enic_admin_rq_disable(enic);
+
+ /* Run both complete disable helpers once before retrying only a helper
+ * which timed out.
+ */
+ if (wq_err == -ETIMEDOUT)
+ wq_err = enic_admin_wq_disable(enic);
+ if (rq_err == -ETIMEDOUT)
+ rq_err = enic_admin_rq_disable(enic);
+
+ if (wq_err)
+ netdev_warn(enic->netdev,
+ "Failed to disable admin WQ after retry: %d\n",
+ wq_err);
+ if (rq_err)
+ netdev_warn(enic->netdev,
+ "Failed to disable admin RQ after retry: %d\n",
+ rq_err);
+
+ /* 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_ADMIN_CHAN_QUARANTINED);
+ mutex_unlock(&enic->mbox_lock);
+ /* Retained targets remain valid for late DMA. Keep bus mastering
+ * enabled for the independent datapath and command transport.
+ */
+ return wq_err ?: rq_err;
+ }
+
+ enic_admin_channel_release(enic);
+ mutex_unlock(&enic->mbox_lock);
+
+ /* Firmware rejection does not undermine the WQ/RQ proof. */
+ return qp_err;
+}
+
+int enic_admin_channel_final_recheck(struct enic *enic)
+{
+ int rq_err;
+ int wq_err;
+
+ if (READ_ONCE(enic->admin_chan_state) != ENIC_ADMIN_CHAN_QUARANTINED)
+ return 0;
+
+ /* Channel close already detached every CPU entry point and attempted
+ * QP_DISABLE. This is the final direct check of queue ownership.
+ */
+ mutex_lock(&enic->mbox_lock);
+ wq_err = enic_admin_wq_disable(enic);
+ rq_err = enic_admin_rq_disable(enic);
+ if (wq_err || rq_err) {
+ mutex_unlock(&enic->mbox_lock);
+ return wq_err ?: rq_err;
+ }
+
+ enic_admin_channel_release(enic);
+ mutex_unlock(&enic->mbox_lock);
+ netdev_info(enic->netdev,
+ "Admin queues stopped during final teardown; releasing DMA graph\n");
+
+ return 0;
+}
+
+bool enic_admin_channel_is_quarantined(const struct enic *enic)
+{
+ return READ_ONCE(enic->admin_chan_state) ==
+ ENIC_ADMIN_CHAN_QUARANTINED;
}
diff --git a/drivers/net/ethernet/cisco/enic/enic_admin.h b/drivers/net/ethernet/cisco/enic/enic_admin.h
index 62c80220b0ca..1fe747c3dd35 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_final_recheck(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 6b5824f45a14..899e0b1e14be 100644
--- a/drivers/net/ethernet/cisco/enic/enic_main.c
+++ b/drivers/net/ethernet/cisco/enic/enic_main.c
@@ -1736,6 +1736,13 @@ static int enic_open(struct net_device *netdev)
.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV,
};
+ /* Internal reopen paths also bypass the networking core's presence check. */
+ if (!netif_device_present(netdev))
+ return -ENODEV;
+ if (enic_is_sriov_vf_v2(enic) &&
+ enic_admin_channel_is_quarantined(enic))
+ return -EBUSY;
+
err = enic_request_intr(enic);
if (err) {
netdev_err(netdev, "Unable to request irq.\n");
@@ -2206,6 +2213,9 @@ static void enic_admin_chan_reopen(struct enic *enic)
{
int err;
+ if (enic_admin_channel_is_quarantined(enic))
+ return;
+
/* Install the MBOX receive handler and clear pending reply state before
* opening the channel, so the handler is in place before the admin
* interrupt is unmasked and no early completion is dropped. Keep the
@@ -2237,6 +2247,7 @@ static void enic_admin_chan_reopen(struct enic *enic)
"MBOX capability check after reset failed: %d\n",
err);
enic_admin_channel_close(enic);
+ WRITE_ONCE(enic->vf_registered, false);
return;
}
err = enic_mbox_vf_register(enic);
@@ -2245,6 +2256,7 @@ static void enic_admin_chan_reopen(struct enic *enic)
"MBOX VF re-registration after reset failed: %d\n",
err);
enic_admin_channel_close(enic);
+ WRITE_ONCE(enic->vf_registered, false);
}
} else {
/* The link came back up during enic_open() above while MBOX
@@ -2264,17 +2276,27 @@ static void enic_reset(struct work_struct *work)
rtnl_lock();
+ if (!netif_running(enic->netdev))
+ goto unlock;
+
/* 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.
+ /* Close the V2 admin/MBOX channel before the soft reset. Release its
+ * DMA resources only after both queues report stopped; otherwise
+ * quarantine the channel and abandon the reset.
*/
- if (enic_has_admin_chan(enic))
+ if (enic_has_admin_chan(enic)) {
enic_admin_channel_close(enic);
+ /* Channel close has drained registration replies. */
+ if (enic_is_sriov_vf_v2(enic))
+ WRITE_ONCE(enic->vf_registered, false);
+ }
+ if (enic_admin_channel_is_quarantined(enic)) {
+ /* Core close excludes netpoll and clears __LINK_STATE_START first. */
+ dev_close(enic->netdev);
+ goto reset_out;
+ }
enic_stop(enic->netdev);
if (enic_is_sriov_vf_v2(enic))
@@ -2287,7 +2309,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 (enic_open(enic->netdev))
+ goto reset_out;
/* Re-establish the admin/MBOX channel after the data path is back up.
* It was fully torn down by enic_admin_channel_close() above;
@@ -2296,12 +2319,17 @@ static void enic_reset(struct work_struct *work)
*/
if (enic_has_admin_chan(enic))
enic_admin_chan_reopen(enic);
+ if (enic_is_sriov_vf_v2(enic) &&
+ enic_admin_channel_is_quarantined(enic))
+ dev_close(enic->netdev);
+reset_out:
/* Allow infiniband to fiddle with the device again */
enic_set_api_busy(enic, false);
call_netdevice_notifiers(NETDEV_REBOOT, enic->netdev);
+unlock:
rtnl_unlock();
}
@@ -2311,15 +2339,26 @@ static void enic_tx_hang_reset(struct work_struct *work)
rtnl_lock();
+ if (!netif_running(enic->netdev))
+ 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.
+ /* Close the V2 admin/MBOX channel before the hang reset. Release its
+ * DMA resources only after both queues report stopped; otherwise
+ * quarantine the channel and abandon the reset.
*/
- if (enic_has_admin_chan(enic))
+ if (enic_has_admin_chan(enic)) {
enic_admin_channel_close(enic);
+ /* Channel close has drained registration replies. */
+ if (enic_is_sriov_vf_v2(enic))
+ WRITE_ONCE(enic->vf_registered, false);
+ }
+ if (enic_admin_channel_is_quarantined(enic)) {
+ dev_close(enic->netdev);
+ goto reset_out;
+ }
enic_dev_hang_notify(enic);
enic_stop(enic->netdev);
@@ -2333,7 +2372,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 (enic_open(enic->netdev))
+ goto reset_out;
/* Re-establish the admin/MBOX channel after the data path is back up.
* It was fully torn down by enic_admin_channel_close() above;
@@ -2342,12 +2382,17 @@ static void enic_tx_hang_reset(struct work_struct *work)
*/
if (enic_has_admin_chan(enic))
enic_admin_chan_reopen(enic);
+ if (enic_is_sriov_vf_v2(enic) &&
+ enic_admin_channel_is_quarantined(enic))
+ dev_close(enic->netdev);
+reset_out:
/* Allow infiniband to fiddle with the device again */
enic_set_api_busy(enic, false);
call_netdevice_notifiers(NETDEV_REBOOT, enic->netdev);
+unlock:
rtnl_unlock();
}
@@ -3036,7 +3081,8 @@ 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);
+ /* A terminal admin allocation keeps the reservation after module unload. */
+ err = pci_request_regions(pdev, dev_name(dev));
if (err) {
dev_err(dev, "Cannot request PCI regions, aborting\n");
goto err_out_disable_device;
@@ -3364,6 +3410,8 @@ static int enic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
enic_dev_deinit(enic);
err_out_dev_close:
vnic_dev_close(enic->vdev);
+ if (enic_admin_channel_is_quarantined(enic))
+ enic_admin_channel_final_recheck(enic);
err_out_disable_sriov:
kfree(enic->pp);
err_out_disable_sriov_pp:
@@ -3374,6 +3422,17 @@ static int enic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
}
#endif
err_out_vnic_unregister:
+ if (enic_admin_channel_is_quarantined(enic)) {
+ /* Normal generic cleanup and CMD_CLOSE have already run. Retain
+ * only the inert admin/control owners and their PCI reservation.
+ */
+ pci_dev_get(pdev);
+ pci_clear_master(pdev);
+ pci_disable_device(pdev);
+ pci_set_drvdata(pdev, NULL);
+ dev_err(dev, "Control DMA unresolved; retaining resources until reboot\n");
+ return err;
+ }
vnic_dev_unregister(enic->vdev);
err_out_iounmap:
enic_iounmap(enic);
@@ -3397,6 +3456,9 @@ static void enic_remove(struct pci_dev *pdev)
disable_work_sync(&enic->reset);
disable_work_sync(&enic->tx_hang_reset);
disable_work_sync(&enic->change_mtu_work);
+ rtnl_lock();
+ netif_device_detach(netdev);
+ rtnl_unlock();
/* Close the admin channel and unregister from the PF before
* unregister_netdev() to prevent a late PF notification from
@@ -3431,7 +3493,23 @@ static void enic_remove(struct pci_dev *pdev)
#endif
enic_dev_deinit(enic);
vnic_dev_close(enic->vdev);
+ if (enic_admin_channel_is_quarantined(enic))
+ enic_admin_channel_final_recheck(enic);
kfree(enic->pp);
+ if (enic_admin_channel_is_quarantined(enic)) {
+ /* All generic queues and CPU entry points followed ordinary
+ * teardown. No admin buffer can return from the network stack.
+ * Pin the PCI object for the reservation's owner-name pointer;
+ * this reference does not fence DMA or preserve IOMMU state.
+ */
+ pci_dev_get(pdev);
+ pci_clear_master(pdev);
+ pci_disable_device(pdev);
+ pci_set_drvdata(pdev, NULL);
+ dev_err(&pdev->dev,
+ "Control DMA unresolved; retaining resources until reboot\n");
+ return;
+ }
vnic_dev_unregister(enic->vdev);
enic_iounmap(enic);
pci_release_regions(pdev);
--
2.43.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH net v6 2/2] enic: require DEVCMD2 and set the V2 VF mailbox source
2026-11-07 19:02 [PATCH net v6 0/2] enic: retain admin DMA resources and identify V2 VFs Satish Kharat
2026-11-07 19:02 ` [PATCH net v6 1/2] enic: retain admin-channel DMA until queues stop Satish Kharat
@ 2026-11-07 19:02 ` Satish Kharat
2026-11-08 20:00 ` netdev-bot+sashiko
2026-11-09 20:26 ` [PATCH net v6 0/2] enic: retain admin DMA resources and identify V2 VFs Satish Kharat
2 siblings, 1 reply; 6+ messages in thread
From: Satish Kharat @ 2026-11-07 19:02 UTC (permalink / raw)
To: netdev
Cc: Sesidhar Baddela, Aravind Dhanasekaran, Reese Faucette,
Satish Kharat, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
V2 VFs currently transmit mailbox source zero. A deployed async PF
checks that field against the sender identity obtained from CQ metadata,
so VFs other than VF zero cannot register with that PF.
Require DEVCMD2 for V2 PF support and V2 VF probe, preserving the
DEVCMD1 fallback for ordinary PF Ethernet, V1 and usNIC. Enable the
admin QP, then query CMD_SRIOV_STATS_GET while its receive interrupt is
still masked. The version-1 statistics prefix is stable, so accept that
version or later. Reject VF index 0 because it identifies the PF. Publish
the returned index as the mailbox source. Permit sends before unmasking the
interrupt so queued notifications can be processed and acknowledged
with a valid source. Sends remain disabled until the ID is valid,
without a separate unknown-ID send check or reliance on PF
retransmission. Query failures use the partial-open close path and its
existing ownership checks.
CMD_SRIOV_STATS_GET is mandatory for V2 VF probe. Firmware which does
not implement it fails probe because the mailbox source ID cannot
otherwise be determined safely. Do not fall back to source zero, which
would identify only VF zero correctly.
Keep the coherent statistics target for the vNIC lifetime. Extend the
DEVCMD2 transport with an optional per-call observation of NOT_POSTED,
POSTED or COMPLETED. A posted statistics command without a result leaves
its target unresolved. A later blocking QP or close command result
fences that target through command ordering, including a firmware error
result. Observe close completion in both probe unwind and removal so a
completed close does not leave the statistics target unnecessarily
retained. Pre-submission errors, posted timeouts and NOWAIT posting do
not establish completion. This keeps transport completion separate from
command success and admin WQ/RQ queue-stop proof.
On an unresolved probe failure retain the inert control owner and target
through final containment instead of freeing memory which the command
may still write. Refuse reuse of an unresolved statistics target.
Fixes: 1f0c856b5963 ("enic: add MBOX core send and receive for admin channel")
Signed-off-by: Satish Kharat <satishkh@cisco.com>
---
drivers/net/ethernet/cisco/enic/enic.h | 6 ++
drivers/net/ethernet/cisco/enic/enic_admin.c | 52 ++++++++++++++--
drivers/net/ethernet/cisco/enic/enic_dev.c | 44 ++++++++++++++
drivers/net/ethernet/cisco/enic/enic_dev.h | 3 +
drivers/net/ethernet/cisco/enic/enic_main.c | 29 +++++++--
drivers/net/ethernet/cisco/enic/enic_mbox.c | 12 +++-
drivers/net/ethernet/cisco/enic/vnic_dev.c | 85 ++++++++++++++++++++++++++-
drivers/net/ethernet/cisco/enic/vnic_dev.h | 15 +++++
drivers/net/ethernet/cisco/enic/vnic_devcmd.h | 34 +++++++++++
9 files changed, 265 insertions(+), 15 deletions(-)
diff --git a/drivers/net/ethernet/cisco/enic/enic.h b/drivers/net/ethernet/cisco/enic/enic.h
index ebe617e752c7..a4f9f58c1adf 100644
--- a/drivers/net/ethernet/cisco/enic/enic.h
+++ b/drivers/net/ethernet/cisco/enic/enic.h
@@ -271,6 +271,12 @@ struct enic {
u16 num_vfs;
enum enic_vf_type vf_type;
bool vf_registered;
+ u16 vf_id;
+ /* A statistics-command timeout leaves its persistent coherent target
+ * device-owned until completion of a blocking QP or close command is
+ * observed.
+ */
+ bool sriov_stats_dma_unresolved;
u32 pf_cap_version;
unsigned int enable_count;
spinlock_t enic_api_lock;
diff --git a/drivers/net/ethernet/cisco/enic/enic_admin.c b/drivers/net/ethernet/cisco/enic/enic_admin.c
index 8a063c563a41..d4f3f58a0ec2 100644
--- a/drivers/net/ethernet/cisco/enic/enic_admin.c
+++ b/drivers/net/ethernet/cisco/enic/enic_admin.c
@@ -16,6 +16,7 @@
#include "vnic_devcmd.h"
#include "enic.h"
#include "enic_admin.h"
+#include "enic_dev.h"
#include "cq_desc.h"
#include "cq_enet_desc.h"
#include "wq_enet_desc.h"
@@ -401,12 +402,23 @@ static void enic_admin_teardown_intr(struct enic *enic)
static int enic_admin_qp_type_set(struct enic *enic, u32 enable)
{
+ enum vnic_devcmd2_status status;
u64 a0 = QP_TYPE_ADMIN, a1 = enable;
int wait = 1000;
int err;
spin_lock_bh(&enic->devcmd_lock);
- err = vnic_dev_cmd(enic->vdev, CMD_QP_TYPE_SET, &a0, &a1, wait);
+ if (vnic_dev_uses_devcmd2(enic->vdev)) {
+ err = vnic_dev_cmd2_status(enic->vdev, CMD_QP_TYPE_SET,
+ &a0, &a1, wait, &status);
+ /* Ordered completion fences the earlier statistics DMA target,
+ * including an error result. Admin queues still need RUNNING == 0.
+ */
+ if (status == VNIC_DEVCMD2_COMPLETED)
+ WRITE_ONCE(enic->sriov_stats_dma_unresolved, false);
+ } else {
+ err = vnic_dev_cmd(enic->vdev, CMD_QP_TYPE_SET, &a0, &a1, wait);
+ }
spin_unlock_bh(&enic->devcmd_lock);
return err;
@@ -542,6 +554,8 @@ int enic_admin_channel_open(struct enic *enic)
READ_ONCE(enic->admin_chan_state));
return -EBUSY;
}
+ if (READ_ONCE(enic->sriov_stats_dma_unresolved))
+ return -EBUSY;
/* 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
@@ -590,15 +604,41 @@ int enic_admin_channel_open(struct enic *enic)
goto disable_queues;
}
- vnic_intr_unmask(&enic->admin_intr);
+ if (enic_is_sriov_vf_v2(enic) &&
+ READ_ONCE(enic->vf_id) == ENIC_MBOX_DST_PF) {
+ struct vnic_sriov_stats *stats;
+ u32 version;
+ u16 vf_id;
+
+ /* Keep received notifications queued until their ACK can use
+ * the VF source ID. The statistics query needs QP_ENABLE first.
+ */
+ err = enic_dev_sriov_stats(enic, &stats);
+ if (err) {
+ netdev_err(enic->netdev,
+ "Failed to get SR-IOV statistics: %d\n", err);
+ goto disable_queues;
+ }
+ version = le32_to_cpu(stats->ver);
+ vf_id = le16_to_cpu(stats->vf_index);
+ /* Later versions preserve the version-1 prefix used here. */
+ if (version < VNIC_SRIOV_STATS_VERSION ||
+ vf_id == ENIC_MBOX_DST_PF) {
+ netdev_err(enic->netdev,
+ "Invalid SR-IOV statistics version %u VF index %#x\n",
+ version, vf_id);
+ err = -EINVAL;
+ goto disable_queues;
+ }
+ WRITE_ONCE(enic->vf_id, vf_id);
+ }
/* Only now that the admin WQ/RQ/CQ and interrupt are fully allocated,
- * programmed and enabled is it safe to allow MBOX sends. Clearing this
- * earlier opened a window where a concurrent sender (e.g. link-notify
- * work scheduled by a post-reset link-up) could call enic_mbox_send_msg()
- * against a not-yet-allocated admin_wq and crash.
+ * programmed and enabled is it safe to allow MBOX sends. Publish send
+ * readiness before unmasking so queued notifications can be acknowledged.
*/
WRITE_ONCE(enic->mbox_send_disabled, false);
+ vnic_intr_unmask(&enic->admin_intr);
netdev_dbg(enic->netdev,
"admin channel open: intr=%u wq_avail=%u rq_avail=%u cq0_color=%u cq1_color=%u\n",
diff --git a/drivers/net/ethernet/cisco/enic/enic_dev.c b/drivers/net/ethernet/cisco/enic/enic_dev.c
index 659787f73cf1..7d5298e6df02 100644
--- a/drivers/net/ethernet/cisco/enic/enic_dev.c
+++ b/drivers/net/ethernet/cisco/enic/enic_dev.c
@@ -171,6 +171,50 @@ int enic_dev_intr_coal_timer_info(struct enic *enic)
return err;
}
+int enic_dev_close(struct enic *enic)
+{
+ enum vnic_devcmd2_status status;
+ u64 a0 = 0, a1 = 0;
+ int err;
+
+ spin_lock_bh(&enic->devcmd_lock);
+ if (vnic_dev_uses_devcmd2(enic->vdev)) {
+ err = vnic_dev_cmd2_status(enic->vdev, CMD_CLOSE,
+ &a0, &a1, 1000, &status);
+ /* A close result also fences earlier statistics DMA, including
+ * an error result. Admin queues still need RUNNING == 0.
+ */
+ if (status == VNIC_DEVCMD2_COMPLETED)
+ WRITE_ONCE(enic->sriov_stats_dma_unresolved, false);
+ } else {
+ err = vnic_dev_close(enic->vdev);
+ }
+ spin_unlock_bh(&enic->devcmd_lock);
+
+ return err;
+}
+
+int enic_dev_sriov_stats(struct enic *enic,
+ struct vnic_sriov_stats **stats)
+{
+ enum vnic_devcmd2_status status;
+ int err;
+
+ *stats = NULL;
+ spin_lock_bh(&enic->devcmd_lock);
+ if (READ_ONCE(enic->sriov_stats_dma_unresolved)) {
+ err = -EBUSY;
+ goto unlock;
+ }
+ err = vnic_dev_sriov_stats(enic->vdev, stats, &status);
+ if (status == VNIC_DEVCMD2_POSTED)
+ WRITE_ONCE(enic->sriov_stats_dma_unresolved, true);
+unlock:
+ spin_unlock_bh(&enic->devcmd_lock);
+
+ return err;
+}
+
/* rtnl lock is held */
int enic_vlan_rx_add_vid(struct net_device *netdev, __be16 proto, u16 vid)
{
diff --git a/drivers/net/ethernet/cisco/enic/enic_dev.h b/drivers/net/ethernet/cisco/enic/enic_dev.h
index 698d0cb02064..eed1674ff30f 100644
--- a/drivers/net/ethernet/cisco/enic/enic_dev.h
+++ b/drivers/net/ethernet/cisco/enic/enic_dev.h
@@ -40,6 +40,9 @@ int enic_dev_set_ig_vlan_rewrite_mode(struct enic *enic);
int enic_dev_enable(struct enic *enic);
int enic_dev_disable(struct enic *enic);
int enic_dev_intr_coal_timer_info(struct enic *enic);
+int enic_dev_close(struct enic *enic);
+int enic_dev_sriov_stats(struct enic *enic,
+ struct vnic_sriov_stats **stats);
int enic_dev_status_to_errno(int devcmd_status);
#endif /* _ENIC_DEV_H_ */
diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
index 899e0b1e14be..6d49d7105a0d 100644
--- a/drivers/net/ethernet/cisco/enic/enic_main.c
+++ b/drivers/net/ethernet/cisco/enic/enic_main.c
@@ -2203,6 +2203,12 @@ static bool enic_has_admin_chan(struct enic *enic)
(enic_sriov_enabled(enic) && enic->vf_type == ENIC_VF_TYPE_V2);
}
+static bool enic_control_dma_unresolved(struct enic *enic)
+{
+ return enic_admin_channel_is_quarantined(enic) ||
+ READ_ONCE(enic->sriov_stats_dma_unresolved);
+}
+
/* Re-establish the admin/MBOX channel after a reset has re-created the data
* path. Mirrors the relevant part of the probe / SR-IOV-enable sequence:
* reinitialise MBOX and reopen the channel, then for a VF re-run the PF
@@ -2886,6 +2892,12 @@ static void enic_sriov_detect_vf_type(struct enic *enic)
if (enic->vf_type != ENIC_VF_TYPE_V2)
return;
+ if (!vnic_dev_uses_devcmd2(enic->vdev)) {
+ dev_warn(&pdev->dev,
+ "SR-IOV V2 requires DEVCMD2; disabling V2 support\n");
+ enic->vf_type = ENIC_VF_TYPE_NONE;
+ return;
+ }
/* A successful command means firmware recognizes
* VIC_FEATURE_SRIOV; supported_versions is available
@@ -3070,6 +3082,7 @@ static int enic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
enic = netdev_priv(netdev);
enic->netdev = netdev;
enic->pdev = pdev;
+ WRITE_ONCE(enic->vf_id, ENIC_MBOX_DST_PF);
spin_lock_init(&enic->vf_link_state_lock);
/* Setup PCI resources
@@ -3137,6 +3150,12 @@ static int enic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
if (err)
goto err_out_vnic_unregister;
+ if (enic_is_sriov_vf_v2(enic) &&
+ !vnic_dev_uses_devcmd2(enic->vdev)) {
+ dev_err(dev, "SR-IOV V2 VF requires DEVCMD2\n");
+ err = -EOPNOTSUPP;
+ goto err_out_vnic_unregister;
+ }
#ifdef CONFIG_PCI_IOV
enic_sriov_detect_vf_type(enic);
@@ -3265,6 +3284,8 @@ static int enic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
"MBOX VF registration failed: %d\n", err);
goto err_out_admin_close;
}
+ dev_info(dev, "V2 VF registered, VF index %u\n",
+ READ_ONCE(enic->vf_id));
}
netif_set_real_num_tx_queues(netdev, enic->wq_count);
@@ -3409,7 +3430,7 @@ 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);
+ enic_dev_close(enic);
if (enic_admin_channel_is_quarantined(enic))
enic_admin_channel_final_recheck(enic);
err_out_disable_sriov:
@@ -3422,7 +3443,7 @@ static int enic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
}
#endif
err_out_vnic_unregister:
- if (enic_admin_channel_is_quarantined(enic)) {
+ if (enic_control_dma_unresolved(enic)) {
/* Normal generic cleanup and CMD_CLOSE have already run. Retain
* only the inert admin/control owners and their PCI reservation.
*/
@@ -3492,11 +3513,11 @@ static void enic_remove(struct pci_dev *pdev)
}
#endif
enic_dev_deinit(enic);
- vnic_dev_close(enic->vdev);
+ enic_dev_close(enic);
if (enic_admin_channel_is_quarantined(enic))
enic_admin_channel_final_recheck(enic);
kfree(enic->pp);
- if (enic_admin_channel_is_quarantined(enic)) {
+ if (enic_control_dma_unresolved(enic)) {
/* All generic queues and CPU entry points followed ordinary
* teardown. No admin buffer can return from the network stack.
* Pin the PCI object for the reservation's owner-name pointer;
diff --git a/drivers/net/ethernet/cisco/enic/enic_mbox.c b/drivers/net/ethernet/cisco/enic/enic_mbox.c
index 5c93ca49552a..a1f8138d0bbd 100644
--- a/drivers/net/ethernet/cisco/enic/enic_mbox.c
+++ b/drivers/net/ethernet/cisco/enic/enic_mbox.c
@@ -18,10 +18,17 @@
#define ENIC_MBOX_POLL_TIMEOUT_US 5000000
#define ENIC_MBOX_POLL_INTERVAL_US 100
-static void enic_mbox_fill_hdr(struct enic_mbox_hdr *hdr, u8 msg_type,
+static void enic_mbox_fill_hdr(struct enic *enic, struct enic_mbox_hdr *hdr,
+ u8 msg_type,
u16 dst_vnic_id, u16 msg_len, u64 msg_num)
{
+ u16 src_vnic_id = ENIC_MBOX_DST_PF;
+
+ if (enic_is_sriov_vf_v2(enic))
+ src_vnic_id = READ_ONCE(enic->vf_id);
+
memset(hdr, 0, sizeof(*hdr));
+ hdr->src_vnic_id = cpu_to_le16(src_vnic_id);
hdr->dst_vnic_id = cpu_to_le16(dst_vnic_id);
hdr->msg_type = msg_type;
hdr->msg_len = cpu_to_le16(msg_len);
@@ -85,7 +92,8 @@ static int enic_mbox_send_msg_id(struct enic *enic, u8 msg_type,
reply_expected = true;
}
- enic_mbox_fill_hdr(buf, msg_type, dst_vnic_id, total_len, msg_num);
+ enic_mbox_fill_hdr(enic, buf, msg_type, dst_vnic_id, total_len,
+ msg_num);
if (payload_len) {
void *dst = buf + sizeof(struct enic_mbox_hdr);
diff --git a/drivers/net/ethernet/cisco/enic/vnic_dev.c b/drivers/net/ethernet/cisco/enic/vnic_dev.c
index c8d657e97094..92eec1455ceb 100644
--- a/drivers/net/ethernet/cisco/enic/vnic_dev.c
+++ b/drivers/net/ethernet/cisco/enic/vnic_dev.c
@@ -287,8 +287,9 @@ static int _vnic_dev_cmd(struct vnic_dev *vdev, enum vnic_devcmd_cmd cmd,
return -ETIMEDOUT;
}
-static int _vnic_dev_cmd2(struct vnic_dev *vdev, enum vnic_devcmd_cmd cmd,
- int wait)
+static int _vnic_dev_cmd2_status(struct vnic_dev *vdev,
+ enum vnic_devcmd_cmd cmd, int wait,
+ enum vnic_devcmd2_status *status)
{
struct devcmd2_controller *dc2c = vdev->devcmd2;
struct devcmd2_result *result;
@@ -298,6 +299,9 @@ static int _vnic_dev_cmd2(struct vnic_dev *vdev, enum vnic_devcmd_cmd cmd,
u32 fetch_index, new_posted;
u32 posted = dc2c->posted;
+ if (status)
+ *status = VNIC_DEVCMD2_NOT_POSTED;
+
fetch_index = ioread32(&dc2c->wq_ctrl->fetch_index);
if (fetch_index == 0xFFFFFFFF)
@@ -326,6 +330,8 @@ static int _vnic_dev_cmd2(struct vnic_dev *vdev, enum vnic_devcmd_cmd cmd,
wmb();
iowrite32(new_posted, &dc2c->wq_ctrl->posted_index);
dc2c->posted = new_posted;
+ if (status)
+ *status = VNIC_DEVCMD2_POSTED;
if (dc2c->cmd_ring[posted].flags & DEVCMD2_FNORESULT)
return 0;
@@ -340,7 +346,10 @@ static int _vnic_dev_cmd2(struct vnic_dev *vdev, enum vnic_devcmd_cmd cmd,
}
for (delay = 0; delay < wait; delay++) {
- if (result->color == color) {
+ if (READ_ONCE(result->color) == color) {
+ dma_rmb();
+ if (status)
+ *status = VNIC_DEVCMD2_COMPLETED;
if (result->error) {
err = result->error;
if (err != ERR_ECMDUNKNOWN ||
@@ -363,6 +372,31 @@ static int _vnic_dev_cmd2(struct vnic_dev *vdev, enum vnic_devcmd_cmd cmd,
return -ETIMEDOUT;
}
+static int _vnic_dev_cmd2(struct vnic_dev *vdev, enum vnic_devcmd_cmd cmd,
+ int wait)
+{
+ return _vnic_dev_cmd2_status(vdev, cmd, wait, NULL);
+}
+
+int vnic_dev_cmd2_status(struct vnic_dev *vdev, enum vnic_devcmd_cmd cmd,
+ u64 *a0, u64 *a1, int wait,
+ enum vnic_devcmd2_status *status)
+{
+ int err;
+
+ *status = VNIC_DEVCMD2_NOT_POSTED;
+ if (!vdev->devcmd2 || vdev->proxy != PROXY_NONE)
+ return -EOPNOTSUPP;
+
+ memset(vdev->args, 0, sizeof(vdev->args));
+ vdev->args[0] = *a0;
+ vdev->args[1] = *a1;
+ err = _vnic_dev_cmd2_status(vdev, cmd, wait, status);
+ *a0 = vdev->args[0];
+ *a1 = vdev->args[1];
+ return err;
+}
+
static int vnic_dev_init_devcmd1(struct vnic_dev *vdev)
{
vdev->devcmd = vnic_dev_get_res(vdev, RES_TYPE_DEVCMD, 0);
@@ -1046,6 +1080,11 @@ void vnic_dev_unregister(struct vnic_dev *vdev)
dma_free_coherent(&vdev->pdev->dev,
sizeof(struct vnic_stats),
vdev->stats, vdev->stats_pa);
+ if (vdev->sriov_stats)
+ dma_free_coherent(&vdev->pdev->dev,
+ sizeof(struct vnic_sriov_stats),
+ vdev->sriov_stats,
+ vdev->sriov_stats_pa);
if (vdev->fw_info)
dma_free_coherent(&vdev->pdev->dev,
sizeof(struct vnic_devcmd_fw_info),
@@ -1088,6 +1127,11 @@ struct pci_dev *vnic_dev_get_pdev(struct vnic_dev *vdev)
}
EXPORT_SYMBOL(vnic_dev_get_pdev);
+bool vnic_dev_uses_devcmd2(struct vnic_dev *vdev)
+{
+ return vdev->devcmd2;
+}
+
int vnic_devcmd_init(struct vnic_dev *vdev)
{
void __iomem *res;
@@ -1295,3 +1339,38 @@ int vnic_dev_capable_rss_hash_type(struct vnic_dev *vdev, u8 *rss_hash_type)
return 0;
}
+
+int vnic_dev_sriov_stats(struct vnic_dev *vdev,
+ struct vnic_sriov_stats **stats,
+ enum vnic_devcmd2_status *status)
+{
+ u64 a0, a1;
+ int err;
+
+ *stats = NULL;
+ *status = VNIC_DEVCMD2_NOT_POSTED;
+ if (!vdev->sriov_stats) {
+ vdev->sriov_stats =
+ dma_alloc_coherent(&vdev->pdev->dev,
+ sizeof(*vdev->sriov_stats),
+ &vdev->sriov_stats_pa,
+ GFP_ATOMIC);
+ if (!vdev->sriov_stats)
+ return -ENOMEM;
+ }
+
+ memset(vdev->sriov_stats, 0, sizeof(*vdev->sriov_stats));
+ a0 = vdev->sriov_stats_pa;
+ a1 = sizeof(*vdev->sriov_stats);
+ err = vnic_dev_cmd2_status(vdev, CMD_SRIOV_STATS_GET, &a0, &a1,
+ 1000, status);
+ if (!err) {
+ /* Order CPU reads from the coherent target after command
+ * completion.
+ */
+ dma_rmb();
+ *stats = vdev->sriov_stats;
+ }
+
+ return err;
+}
diff --git a/drivers/net/ethernet/cisco/enic/vnic_dev.h b/drivers/net/ethernet/cisco/enic/vnic_dev.h
index 7fdd8c661c99..6a4d7ce3b636 100644
--- a/drivers/net/ethernet/cisco/enic/vnic_dev.h
+++ b/drivers/net/ethernet/cisco/enic/vnic_dev.h
@@ -93,6 +93,8 @@ struct vnic_dev {
dma_addr_t linkstatus_pa;
struct vnic_stats *stats;
dma_addr_t stats_pa;
+ struct vnic_sriov_stats *sriov_stats;
+ dma_addr_t sriov_stats_pa;
struct vnic_devcmd_fw_info *fw_info;
dma_addr_t fw_info_pa;
enum vnic_proxy_type proxy;
@@ -106,6 +108,12 @@ struct vnic_dev {
struct vnic_stats;
+enum vnic_devcmd2_status {
+ VNIC_DEVCMD2_NOT_POSTED,
+ VNIC_DEVCMD2_POSTED,
+ VNIC_DEVCMD2_COMPLETED,
+};
+
void *vnic_dev_priv(struct vnic_dev *vdev);
unsigned int vnic_dev_get_res_count(struct vnic_dev *vdev,
enum vnic_res_type type);
@@ -118,6 +126,9 @@ void vnic_dev_free_desc_ring(struct vnic_dev *vdev,
struct vnic_dev_ring *ring);
int vnic_dev_cmd(struct vnic_dev *vdev, enum vnic_devcmd_cmd cmd,
u64 *a0, u64 *a1, int wait);
+int vnic_dev_cmd2_status(struct vnic_dev *vdev, enum vnic_devcmd_cmd cmd,
+ u64 *a0, u64 *a1, int wait,
+ enum vnic_devcmd2_status *status);
void vnic_dev_cmd_proxy_by_index_start(struct vnic_dev *vdev, u16 index);
void vnic_dev_cmd_proxy_end(struct vnic_dev *vdev);
int vnic_dev_fw_info(struct vnic_dev *vdev,
@@ -163,6 +174,7 @@ struct vnic_dev *vnic_dev_register(struct vnic_dev *vdev,
void *priv, struct pci_dev *pdev, struct vnic_dev_bar *bar,
unsigned int num_bars);
struct pci_dev *vnic_dev_get_pdev(struct vnic_dev *vdev);
+bool vnic_dev_uses_devcmd2(struct vnic_dev *vdev);
int vnic_dev_init_prov2(struct vnic_dev *vdev, u8 *buf, u32 len);
int vnic_dev_enable2(struct vnic_dev *vdev, int active);
int vnic_dev_enable2_done(struct vnic_dev *vdev, int *status);
@@ -177,5 +189,8 @@ int vnic_dev_overlay_offload_cfg(struct vnic_dev *vdev, u8 overlay,
int vnic_dev_get_supported_feature_ver(struct vnic_dev *vdev, u8 feature,
u64 *supported_versions, u64 *a1);
int vnic_dev_capable_rss_hash_type(struct vnic_dev *vdev, u8 *rss_hash_type);
+int vnic_dev_sriov_stats(struct vnic_dev *vdev,
+ struct vnic_sriov_stats **stats,
+ enum vnic_devcmd2_status *status);
#endif /* _VNIC_DEV_H_ */
diff --git a/drivers/net/ethernet/cisco/enic/vnic_devcmd.h b/drivers/net/ethernet/cisco/enic/vnic_devcmd.h
index 90ca06691ebd..9f397df88c50 100644
--- a/drivers/net/ethernet/cisco/enic/vnic_devcmd.h
+++ b/drivers/net/ethernet/cisco/enic/vnic_devcmd.h
@@ -461,6 +461,14 @@ enum vnic_devcmd_cmd {
* in: (u32) a1 = enable (1) / disable (0)
*/
CMD_QP_TYPE_SET = _CMDC(_CMD_DIR_WRITE, _CMD_VTYPE_ENET, 97),
+
+ /*
+ * Get SR-IOV capabilities, resource usage, and this function's logical
+ * VF index.
+ * in: (u64) a0 = DMA address of struct vnic_sriov_stats
+ * in: (u32) a1 = length of the buffer
+ */
+ CMD_SRIOV_STATS_GET = _CMDC(_CMD_DIR_WRITE, _CMD_VTYPE_ENET, 98),
};
#define QP_TYPE_ADMIN 0
@@ -468,6 +476,32 @@ enum vnic_devcmd_cmd {
#define QP_ENABLE 1
#define QP_DISABLE 0
+#define VNIC_SRIOV_STATS_VERSION 1
+
+struct vnic_sriov_stats {
+ __le32 ver;
+ u8 sriov_vlan_membership_cap;
+ u8 sriov_vlan_membership_enabled;
+ u8 sriov_rss_vf_full_cap;
+ u8 sriov_host_rx_stats;
+ __le32 ig_classifier0_tcam_cfg;
+ __le32 ig_classifier0_tcam_free;
+ __le32 eg_classifier0_tcam_cfg;
+ __le32 eg_classifier0_tcam_free;
+ __le32 ig_classifier1_tcam_cfg;
+ __le32 ig_classifier1_tcam_free;
+ __le32 eg_classifier1_tcam_cfg;
+ __le32 eg_classifier1_tcam_free;
+ __le32 sriov_ig_flow_table_cfg;
+ __le32 sriov_ig_flow_table_free;
+ __le32 sriov_eg_flow_table_cfg;
+ __le32 sriov_eg_flow_table_free;
+ u8 admin_qp_ready[32];
+ __le16 vf_index;
+ __le16 reserved1;
+ __le32 reserved2[256 - 23];
+};
+
/* CMD_ENABLE2 flags */
#define CMD_ENABLE2_STANDBY 0x0
#define CMD_ENABLE2_ACTIVE 0x1
--
2.43.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net v6 1/2] enic: retain admin-channel DMA until queues stop
2026-11-07 19:02 ` [PATCH net v6 1/2] enic: retain admin-channel DMA until queues stop Satish Kharat
@ 2026-11-08 20:00 ` netdev-bot+sashiko
0 siblings, 0 replies; 6+ messages in thread
From: netdev-bot+sashiko @ 2026-11-08 20:00 UTC (permalink / raw)
To: satishkh
Cc: netdev, sebaddel, aradhana, rfaucett, andrew+netdev, davem,
edumazet, kuba, pabeni
Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.
Critical: 0 · High: 0 (1) · Medium: 1 · Low: 0
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] Surprise removal of a V2 VF now permanently retains the
driver's objects, and each removal cycle leaks a new set.
Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
edits the exact lines involved and leaves it in place.
--
enic: retain admin-channel DMA until queues stop
This commit tracks the SR-IOV admin channel as DOWN, LIVE or QUARANTINED.
The admin WQ/RQ/CQ DMA allocations are freed only after both queues report
RUNNING == 0; otherwise the channel is quarantined, reset workers close the
netdev, and probe unwind and removal keep the netdev, vnic_dev and admin
allocations until reboot. It also checks the enic_open() return value in
the reset workers and detaches the netdev during removal.
> diff --git a/drivers/net/ethernet/cisco/enic/enic_admin.c b/drivers/net/ethernet/cisco/enic/enic_admin.c
> index 61c82b48044d0..8a063c563a41e 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_admin.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_admin.c
[ ... ]
> @@ -600,60 +608,51 @@ int enic_admin_channel_open(struct enic *enic)
[ ... ]
> +static int enic_admin_wq_disable(struct enic *enic)
> {
> - int err;
> + struct vnic_wq *wq = &enic->admin_wq;
> + u32 running;
>
[ ... ]
> + iowrite32(0, &wq->ctrl->enable);
> + return read_poll_timeout(ioread32, running, !running, 10, 500000,
> + false, &wq->ctrl->running);
> +}
[Severity: Medium]
What happens here after a surprise removal of the VF, for example through
pciehp or when the VF is pulled out from under a guest? MMIO reads then
return 0xffffffff, so !running never becomes true and read_poll_timeout()
returns -ETIMEDOUT on every pass. enic_admin_rq_disable() behaves the same
way.
On removal the sequence looks like this:
enic_remove()
enic_admin_channel_close()
enic_admin_qp_type_set(QP_DISABLE) fails fast
enic_admin_wq_disable()/rq_disable() time out, retried once
admin_chan_state = ENIC_ADMIN_CHAN_QUARANTINED
unregister_netdev(), enic_dev_deinit(), vnic_dev_close()
enic_admin_channel_final_recheck() times out again
quarantine branch:
pci_dev_get(pdev);
pci_clear_master(pdev);
pci_disable_device(pdev);
pci_set_drvdata(pdev, NULL);
...
return;
That return skips vnic_dev_unregister(), enic_iounmap(),
pci_release_regions() and free_netdev(). Does every surprise removal of a
V2 VF now keep the following until reboot?
- the netdev
- the vnic_dev and its coherent buffers
- the admin rings and RQ buffers
- a pinned pci_dev
Removal also gets a few seconds of extra polling. Before this patch, all
of these were freed.
The kept BAR reservation also doesn't seem to block a re-added device.
pci_destroy_dev()->pci_free_resources() detaches the old resource from the
iomem tree. A device re-enumerated in the same slot could then probe a new
enic instance and leak another set of objects on its next removal.
The probe unwind at err_out_vnic_unregister behaves the same way if the
device disappears during probe.
The commit message says:
An all-ones MMIO read or a recoverable PCI channel-offline state is not
accepted as queue-stop proof.
However, nothing in drivers/net/ethernet/cisco/enic/ checks
pci_dev_is_disconnected(), pci_channel_offline() or
pci_channel_io_perm_failure. Should a permanently removed device, which
can no longer bus-master, count as stopped so that normal teardown runs?
The later commit "enic: require DEVCMD2 and set the V2 VF mailbox source"
changes the retention check to enic_control_dma_unresolved(), but it does
not change this behaviour.
[ ... ]
> diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
> index 6b5824f45a14c..899e0b1e14be2 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_main.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_main.c
[ ... ]
> @@ -2264,17 +2276,27 @@ static void enic_reset(struct work_struct *work)
[ ... ]
> + if (enic_admin_channel_is_quarantined(enic)) {
> + /* Core close excludes netpoll and clears __LINK_STATE_START first. */
> + dev_close(enic->netdev);
> + goto reset_out;
> + }
>
> enic_stop(enic->netdev);
> if (enic_is_sriov_vf_v2(enic))
> @@ -2287,7 +2309,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 (enic_open(enic->netdev))
> + goto reset_out;
[Severity: High]
This isn't a bug introduced by this patch, but this patch changes these
exact lines. If enic_open() fails here, the netdev keeps IFF_UP and
__LINK_STATE_START. By then, enic_stop() above has already disabled every
NAPI, freed the IRQs and destroyed the page pools.
enic_open() can fail in any of these:
- enic_request_intr()
- enic_dev_notify_set()
- page_pool_create()
- the RQ fill
- enic_dev_enable()
Each of those failure paths leaves NAPI disabled.
A later dev_close(), or unregister_netdev() from enic_remove(), then runs
enic_stop() a second time:
enic_stop()
napi_disable()
napi_disable_locked()
while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
usleep_range(20, 200);
val = READ_ONCE(n->state);
}
The first disable left those bits set. Could this loop spin forever while
holding rtnl_lock and the netdev instance lock? Even if the loop exited,
enic_free_intr() would call free_irq() on vectors whose msix[i].requested
flag was never cleared.
Should the netdev be closed with dev_close() on this failure, as the
quarantined case just above already does? The same applies to
enic_open() in enic_tx_hang_reset().
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261107-b4-enic-dma-vf-index-net-generic-first-v1-20261010-v6-0-155d64f802e9%40cisco.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v6 2/2] enic: require DEVCMD2 and set the V2 VF mailbox source
2026-11-07 19:02 ` [PATCH net v6 2/2] enic: require DEVCMD2 and set the V2 VF mailbox source Satish Kharat
@ 2026-11-08 20:00 ` netdev-bot+sashiko
0 siblings, 0 replies; 6+ messages in thread
From: netdev-bot+sashiko @ 2026-11-08 20:00 UTC (permalink / raw)
To: satishkh
Cc: netdev, sebaddel, aradhana, rfaucett, andrew+netdev, davem,
edumazet, kuba, pabeni
Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 · 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).
- [Medium] The commit message and the code disagree about how the
firmware VF index from CMD_SRIOV_STATS_GET is encoded, and that…
- [Low] enic_mbox_fill_hdr() now always sets an explicit src_vnic_id.
- [Low] The patch changes the DEVCMD2 completion poll for every command,
not only the new status-observing callers.
--
enic: require DEVCMD2 and set the V2 VF mailbox source
This patch makes DEVCMD2 mandatory for V2 PF support and V2 VF probe. At
admin channel open, a V2 VF queries CMD_SRIOV_STATS_GET to learn its VF
index and publishes that index as the mailbox source ID. The DEVCMD2
transport also gains an optional NOT_POSTED/POSTED/COMPLETED status, used to
track whether the coherent statistics target can still be written by the
device.
> diff --git a/drivers/net/ethernet/cisco/enic/enic_admin.c b/drivers/net/ethernet/cisco/enic/enic_admin.c
> index 8a063c563a41e..d4f3f58a0ec2d 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_admin.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_admin.c
[ ... ]
> @@ -590,15 +604,41 @@ int enic_admin_channel_open(struct enic *enic)
> goto disable_queues;
> }
>
> - vnic_intr_unmask(&enic->admin_intr);
> + if (enic_is_sriov_vf_v2(enic) &&
> + READ_ONCE(enic->vf_id) == ENIC_MBOX_DST_PF) {
> + struct vnic_sriov_stats *stats;
> + u32 version;
> + u16 vf_id;
> +
> + /* Keep received notifications queued until their ACK can use
> + * the VF source ID. The statistics query needs QP_ENABLE first.
> + */
> + err = enic_dev_sriov_stats(enic, &stats);
> + if (err) {
> + netdev_err(enic->netdev,
> + "Failed to get SR-IOV statistics: %d\n", err);
> + goto disable_queues;
> + }
> + version = le32_to_cpu(stats->ver);
> + vf_id = le16_to_cpu(stats->vf_index);
> + /* Later versions preserve the version-1 prefix used here. */
> + if (version < VNIC_SRIOV_STATS_VERSION ||
> + vf_id == ENIC_MBOX_DST_PF) {
> + netdev_err(enic->netdev,
> + "Invalid SR-IOV statistics version %u VF index %#x\n",
> + version, vf_id);
> + err = -EINVAL;
> + goto disable_queues;
> + }
> + WRITE_ONCE(enic->vf_id, vf_id);
[Severity: Medium]
The commit message says:
"Reject VF index 0 because it identifies the PF."
This check compares vf_id with ENIC_MBOX_DST_PF, and enic_mbox.h defines
that as 0xFFFF:
#define ENIC_MBOX_DST_PF 0xFFFF
So a vf_index of 0 passes the check. The raw value is then stored in
enic->vf_id, and enic_mbox_fill_hdr() sends it unchanged as src_vnic_id.
Which encoding does CMD_SRIOV_STATS_GET use for vf_index?
The rest of the driver treats mailbox IDs as 0-based.
enic_admin_rq_cq_service() derives the sender identity from the CQ VLAN:
hdr->src_vnic_id = (sender_vlan == 0) ?
cpu_to_le16(ENIC_MBOX_DST_PF) :
cpu_to_le16(sender_vlan - 1);
enic_mbox_send_msg_id() routes with vlan_tag = dst_vnic_id + 1. The
commit message also says source zero "would identify only VF zero
correctly", which points the same way.
Suppose firmware reports 0 for the PF and 1-based indices for the VFs, as
the commit message implies. Wouldn't every V2 VF then publish a source one
higher than the identity the PF derives from the CQ VLAN? That would break
registration for every VF, including VF zero, which works today.
If vf_index is 0-based, the code is right and the commit message is not.
Could the base of vf_index be documented in the CMD_SRIOV_STATS_GET comment
in vnic_devcmd.h? Then either this check or the commit message could be
adjusted to match.
> + }
>
> /* Only now that the admin WQ/RQ/CQ and interrupt are fully allocated,
[ ... ]
> diff --git a/drivers/net/ethernet/cisco/enic/enic_mbox.c b/drivers/net/ethernet/cisco/enic/enic_mbox.c
> index 5c93ca49552ac..a1f8138d0bbd8 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_mbox.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_mbox.c
> @@ -18,10 +18,17 @@
> #define ENIC_MBOX_POLL_TIMEOUT_US 5000000
> #define ENIC_MBOX_POLL_INTERVAL_US 100
>
> -static void enic_mbox_fill_hdr(struct enic_mbox_hdr *hdr, u8 msg_type,
> +static void enic_mbox_fill_hdr(struct enic *enic, struct enic_mbox_hdr *hdr,
> + u8 msg_type,
> u16 dst_vnic_id, u16 msg_len, u64 msg_num)
> {
> + u16 src_vnic_id = ENIC_MBOX_DST_PF;
> +
> + if (enic_is_sriov_vf_v2(enic))
> + src_vnic_id = READ_ONCE(enic->vf_id);
> +
> memset(hdr, 0, sizeof(*hdr));
> + hdr->src_vnic_id = cpu_to_le16(src_vnic_id);
[Severity: Low]
This also changes the PF side. Before this patch, src_vnic_id kept the 0
from the memset. Now any sender that is not a V2 VF writes 0xFFFF.
That changes the on-wire source of every PF-originated message, including
PF_LINK_STATE_NOTIF and the CAPABILITY, REGISTER and UNREGISTER replies.
The commit message only describes setting the V2 VF mailbox source.
The in-tree VF overwrites src_vnic_id from the CQ VLAN on receive, and
0xFFFF matches the PF identity, so this looks harmless here.
This patch exists to interoperate with deployed peers that check the header
source field. Could the commit message mention this PF-side change?
> hdr->dst_vnic_id = cpu_to_le16(dst_vnic_id);
> hdr->msg_type = msg_type;
> hdr->msg_len = cpu_to_le16(msg_len);
[ ... ]
> diff --git a/drivers/net/ethernet/cisco/enic/vnic_dev.c b/drivers/net/ethernet/cisco/enic/vnic_dev.c
> index c8d657e97094e..92eec1455ceb8 100644
> --- a/drivers/net/ethernet/cisco/enic/vnic_dev.c
> +++ b/drivers/net/ethernet/cisco/enic/vnic_dev.c
[ ... ]
> @@ -340,7 +346,10 @@ static int _vnic_dev_cmd2(struct vnic_dev *vdev, enum vnic_devcmd_cmd cmd,
> }
>
> for (delay = 0; delay < wait; delay++) {
> - if (result->color == color) {
> + if (READ_ONCE(result->color) == color) {
> + dma_rmb();
[Severity: Low]
This isn't a bug, but this poll loop is shared by every DEVCMD2 command.
_vnic_dev_cmd2() is now a wrapper around _vnic_dev_cmd2_status(), so the
READ_ONCE() and dma_rmb() ordering of result->color against result->error
and result->results applies to all existing DEVCMD2 users.
The commit message describes the transport change only as "an optional
per-call observation of NOT_POSTED, POSTED or COMPLETED".
Should this ordering fix be stated in the commit message, or split into its
own patch, so it is visible for review and stable backporting?
> + if (status)
> + *status = VNIC_DEVCMD2_COMPLETED;
> if (result->error) {
> err = result->error;
> if (err != ERR_ECMDUNKNOWN ||
> @@ -363,6 +372,31 @@ static int _vnic_dev_cmd2(struct vnic_dev *vdev, enum vnic_devcmd_cmd cmd,
> return -ETIMEDOUT;
> }
>
> +static int _vnic_dev_cmd2(struct vnic_dev *vdev, enum vnic_devcmd_cmd cmd,
> + int wait)
> +{
> + return _vnic_dev_cmd2_status(vdev, cmd, wait, NULL);
> +}
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261107-b4-enic-dma-vf-index-net-generic-first-v1-20261010-v6-0-155d64f802e9%40cisco.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net v6 0/2] enic: retain admin DMA resources and identify V2 VFs
2026-11-07 19:02 [PATCH net v6 0/2] enic: retain admin DMA resources and identify V2 VFs Satish Kharat
2026-11-07 19:02 ` [PATCH net v6 1/2] enic: retain admin-channel DMA until queues stop Satish Kharat
2026-11-07 19:02 ` [PATCH net v6 2/2] enic: require DEVCMD2 and set the V2 VF mailbox source Satish Kharat
@ 2026-11-09 20:26 ` Satish Kharat
2 siblings, 0 replies; 6+ messages in thread
From: Satish Kharat @ 2026-11-09 20:26 UTC (permalink / raw)
To: netdev
Cc: netdev-bot+sashiko, sebaddel, aradhana, rfaucett, andrew+netdev,
davem, edumazet, kuba, pabeni
Thanks for the review.
For patch 1, I am keeping the conservative surprise-removal behavior.
An all-ones MMIO read or PCI disconnect state establishes that the queue
register is no longer readable, but it does not provide the queue-stop
proof used by this patch for previously issued DMA. The driver therefore
releases the admin graph only after directly observing RUNNING == 0. If
surprise removal makes that observation impossible, the graph and PCI
reservation can remain retained until reboot. V7 will state this
limitation more clearly; the code is unchanged.
The reset-worker source comments in V6 already describe both outcomes:
resources are released only after both queues report stopped; otherwise
the channel is quarantined and the reset is abandoned.
The failed internal reopen followed by a repeated datapath stop is a
valid pre-existing lifecycle issue. It is separate from admin DMA
ownership, so I will address it in a separate datapath lifecycle fix
rather than add a partial fix here.
For patch 2, the code implements the deployed identity contract, but the
V6 wording incorrectly described it. Firmware vf_index and mailbox
src_vnic_id use 0xffff for the PF and zero-based values for VFs. Hardware
CQ sender metadata uses 0 for the PF and vf_index + 1 for a VF; receive
handling converts that value to the corresponding mailbox source ID. V7
will correct the cover letter and commit message.
The 0xffff PF mailbox source is intentional and matches the deployed
protocol; V7 will document the change from the previous zero-filled
field. V7 will also document that READ_ONCE() and dma_rmb() order the
common DEVCMD2 result path before device-written result fields are read.
V7 changes public text only. Its code and stable patch IDs are identical
to V6. I will post V7 shortly.
pw-bot: cr
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-11-09 20:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-11-07 19:02 [PATCH net v6 0/2] enic: retain admin DMA resources and identify V2 VFs Satish Kharat
2026-11-07 19:02 ` [PATCH net v6 1/2] enic: retain admin-channel DMA until queues stop Satish Kharat
2026-11-08 20:00 ` netdev-bot+sashiko
2026-11-07 19:02 ` [PATCH net v6 2/2] enic: require DEVCMD2 and set the V2 VF mailbox source Satish Kharat
2026-11-08 20:00 ` netdev-bot+sashiko
2026-11-09 20:26 ` [PATCH net v6 0/2] enic: retain admin DMA resources and identify V2 VFs Satish Kharat
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox