Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH net v2 2/2] enic: require DEVCMD2 and set the V2 VF mailbox source
@ 2026-10-26 12:18 netdev-bot+sashiko
  0 siblings, 0 replies; 2+ messages in thread
From: netdev-bot+sashiko @ 2026-10-26 12:18 UTC (permalink / raw)
  To: satishkh; +Cc: netdev, sebaddel, aradhana, rfaucett, kuba

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] Only one place clears sriov_stats_dma_unresolved:
  enic_admin_qp_type_set() (enic_admin.c:416-417), and only when…
- [Low] The retention branches in enic_probe() (enic_main.c:3456-3465)
  and enic_remove() (enic_main.c:3530-3543) now trigger on…
- [Low] The VF's mailbox source identity is set only after the admin
  channel is already live.

--

enic: require DEVCMD2 and set the V2 VF mailbox source

This patch requires DEVCMD2 for V2 SR-IOV PF support and for V2 VF probe.
After opening the admin channel, the VF now queries CMD_SRIOV_STATS_GET to
get its logical VF index and uses that index as the mailbox source. The
patch also adds a per-call DEVCMD2 completion status, so an unresolved
statistics DMA target can be tracked and kept alive when probe fails.

> diff --git a/drivers/net/ethernet/cisco/enic/enic_admin.c b/drivers/net/ethernet/cisco/enic/enic_admin.c
> index 4325c6d9c30e3..4eb17e88d368e 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_admin.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_admin.c
> @@ -401,12 +401,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);

[Severity: Medium]
Is enic_admin_qp_type_set() meant to be the only place that clears
sriov_stats_dma_unresolved?

The new comment in struct enic says the target stays device-owned "until a
later serialized command completes". The commit message also relies on
command ordering for the fence. By that reasoning, any later blocking
DEVCMD2 command that completes should fence the statistics target too.

Consider this probe failure path for a V2 VF in enic_probe():

enic_dev_sriov_stats()
  CMD_SRIOV_STATS_GET posted, times out -> sriov_stats_dma_unresolved = true
goto err_out_admin_close
  enic_admin_channel_close()
    CMD_QP_TYPE_SET(QP_DISABLE) posted, times out -> flag stays set
    (WQ/RQ RUNNING checks pass, admin channel is released)
err_out_dev_close:
  vnic_dev_close()
    vnic_dev_cmd(CMD_CLOSE) -> _vnic_dev_cmd2(), status == NULL
err_out_vnic_unregister:
  enic_control_dma_unresolved() -> true

CMD_CLOSE is a blocking _CMDC command. If it completes, the same ordering
has fenced the statistics target. However, its return value is ignored and
its completion status is never seen, so the flag stays set.

Does this send probe into the "retaining resources until reboot" branch?
That branch keeps the netdev, vdev, BAR mappings and PCI regions, takes an
extra pci_dev reference and blocks reprobe until reboot, even though the
device can no longer write vdev->sriov_stats.

Could the CMD_CLOSE completion in vnic_dev_close(), in both enic_probe()
and enic_remove(), also clear the flag?

[ ... ]

> diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
> index 838b6dc837ee4..846221937ab10 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_main.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_main.c

[ ... ]

> @@ -3238,6 +3258,9 @@ static int enic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	 * unmasks the admin interrupt so an early completion is not dropped.
>  	 */
>  	if (enic_is_sriov_vf_v2(enic)) {
> +		u32 stats_ver;
> +		u16 vf_id;
> +
>  		enic_mbox_init(enic);
>  		err = enic_admin_channel_open(enic);
>  		if (err) {
> @@ -3245,6 +3268,23 @@ static int enic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  				"Failed to open admin channel: %d\n", err);
>  			goto err_out_dev_deinit;
>  		}
> +		err = enic_dev_sriov_stats(enic, &sriov_stats);
> +		if (err) {
> +			dev_err(dev, "Failed to get SR-IOV statistics: %d\n", err);
> +			goto err_out_admin_close;
> +		}
> +		/* Snapshot the coherent fields after the command's DMA barrier. */
> +		stats_ver = le32_to_cpu(sriov_stats->ver);
> +		vf_id = le16_to_cpu(sriov_stats->vf_index);
> +		if (stats_ver != VNIC_SRIOV_STATS_VERSION ||
> +		    vf_id == ENIC_MBOX_DST_PF) {
> +			dev_err(dev,
> +				"Invalid SR-IOV statistics version %u VF index %#x\n",
> +				stats_ver, vf_id);
> +			err = -EINVAL;
> +			goto err_out_admin_close;
> +		}
> +		WRITE_ONCE(enic->vf_id, vf_id);

[Severity: Low]
Can a mailbox message go out with src_vnic_id = 0xFFFF in this window?

enic->vf_id starts as ENIC_MBOX_DST_PF. Before enic_dev_sriov_stats() runs,
enic_admin_channel_open() has already unmasked admin_intr and cleared
mbox_send_disabled. The statistics query can then busy-wait for up to
1000 x udelay(100) while holding only devcmd_lock.

A PF_LINK_STATE_NOTIF can arrive during that wait, for example from a PF
that still has this VF registered because an earlier driver instance did
not unregister cleanly. enic_mbox_vf_process_msg() has no vf_registered
check, so on another CPU:

admin_msg_work
  enic_mbox_recv_handler()
    enic_mbox_vf_handle_link_state()
      enic_mbox_send_reply(enic, ENIC_MBOX_PF_LINK_STATE_ACK, ...)
        enic_mbox_fill_hdr()
          src_vnic_id = READ_ONCE(enic->vf_id);   /* still 0xFFFF */

enic_mbox_send_reply() takes mbox_lock, not devcmd_lock, so it is not
serialized against the statistics query.

The commit message says the index is queried "before the first VF
request", but that does not cover replies and ACKs. Would a PF that checks
src_vnic_id against the CQ metadata sender reject this ACK?

Should replies be held off until enic->vf_id has been set?

>  		err = enic_mbox_vf_capability_check(enic);

[ ... ]

> @@ -3412,7 +3453,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.
>  		 */

[Severity: Low]
This isn't a bug, but enic_control_dma_unresolved() now also covers
sriov_stats_dma_unresolved. Both this retention branch in enic_probe() and
the one in enic_remove() still print:

    "Admin DMA unresolved; retaining resources until reboot\n"

When only the statistics flag is set, enic_admin_channel_close() has
already released the admin channel and admin_chan_state is DOWN. The memory
actually being kept is vdev->sriov_stats.

Could the message also name the SR-IOV statistics target, so it points at
the right resource?

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

^ permalink raw reply	[flat|nested] 2+ messages in thread
* [PATCH net v2 0/2] enic: retain admin DMA resources and identify V2 VFs
@ 2026-10-22 11:56 Satish Kharat
  2026-10-22 11:57 ` [PATCH net v2 2/2] enic: require DEVCMD2 and set the V2 VF mailbox source Satish Kharat
  0 siblings, 1 reply; 2+ messages in thread
From: Satish Kharat @ 2026-10-22 11:56 UTC (permalink / raw)
  To: netdev
  Cc: Sesidhar Baddela, Aravind Dhanasekaran, Reese Faucette,
	Satish Kharat

Fix premature freeing of admin-channel buffers and set the correct V2 VF
source ID in mailbox messages. This revision replaces the three-patch v1
with two patches. The generic datapath quarantine proposal is withdrawn:
its page-pool/device lifetime and the underlying generic queue-disable
error path need separate work.

The first patch retains the admin WQ/RQ/CQ allocation when queue shutdown
does not establish RUNNING == 0. Both complete disable helpers must
succeed before the graph is released. 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; ordinary WQ/RQ and DEVCMD2 timeouts are
unchanged. Admin receive buffers are driver-owned mappings, not page-pool
pages which can return later from the networking stack.

Keep PCI bus mastering enabled during runtime admin quarantine so the
independent datapath and DEVCMD2 remain usable. Serialize removal against
open, do not reopen admin after a failed datapath open, and finish normal
generic teardown and required firmware commands before final containment.
A final direct admin queue recheck can release the retained graph. If it
cannot, retain the inert owner and DMA targets until reboot rather than
free memory the device might still reference.

Terminal retention intentionally leaves allocations and a BAR reservation
behind. The reservation prevents ordinary reprobe while the PCI device
remains enumerated; PCI remove/rescan is not a supported recovery method.
Hold the PCI object for the reservation name's lifetime, not as a DMA
fence or a way to preserve IOMMU state. No IRQ, work or other CPU callback
may enter the retained admin objects after module unload. DMA debugging
can report the deliberately pending allocations.

The second patch requires DEVCMD2 for SR-IOV V2 and obtains the VF's
logical mailbox source index from CMD_SRIOV_STATS_GET. This lets a nonzero
VF register with the deployed async PF, which checks the header against
the CQ-derived sender. Keep the statistics target persistent and observe
command posting/completion separately from firmware status. A later
blocking QP-command result fences the earlier target even when that
result reports an error. A pre-post failure, posted timeout or NOWAIT
posting does not establish completion.

Sashiko's v1 feedback prompted the lifetime and command-completion audit.
The fixes follow from the ownership paths and transport behavior; the
review tool's severity labels are not their justification. Source review
and testing were assisted by LLM tools. The author is responsible for the
submitted changes.

Validation:

- Tested Cisco VIC PFs and V2 VFs with all four combinations of in-tree
  and deployed async PF/VF drivers, including a nonzero VF. Traffic tests
  covered bidirectional ICMP, TCP, UDP and jumbo frames over ENIC.

- The full suite passed address and filter changes, administrative-MAC
  policy checks, and multiple cycles of module reload, ring resize,
  interface reopen and VF destroy/recreate.

- Test-only instrumentation checked admin queue retries, delayed queue
  shutdown, cleanup after partial open failures, statistics-command
  success and error replies, late replies and command-result ring wrap.

- Fault-injection tests verified admin quarantine and both removal paths:
  successful cleanup when the final queue check passed, and retention
  when it failed. Module unload preserved the BAR reservation and blocked
  reprobe. After a guest reboot, the driver loaded and traffic passed.
  Statistics-command failures requiring buffer retention were also tested.

- Statistics snapshots across traffic, address/filter changes, interface
  reopen and ring resizing showed a stable VF index and consistent
  resource counts. Free flow-table entries decreased after address
  additions and returned after deletion. Packet and byte counters
  increased consistently with TCP and UDP traffic.

The interoperability and full-suite tests included later VF/PF feature
patches. These two fixes alone do not enable in-tree PF SR-IOV support.

The fault-injection tests allowed the real hardware queues to stop;
they did not reproduce a physically stuck queue. Physical device
removal and IOMMU teardown were not tested.

Signed-off-by: Satish Kharat <satishkh@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

---
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        |  22 ++-
 drivers/net/ethernet/cisco/enic/enic_admin.c  | 211 ++++++++++++++++++++------
 drivers/net/ethernet/cisco/enic/enic_admin.h  |   4 +-
 drivers/net/ethernet/cisco/enic/enic_dev.c    |  21 +++
 drivers/net/ethernet/cisco/enic/enic_dev.h    |   2 +
 drivers/net/ethernet/cisco/enic/enic_main.c   | 115 +++++++++++++-
 drivers/net/ethernet/cisco/enic/enic_mbox.c   |  13 +-
 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, 464 insertions(+), 58 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] 2+ messages in thread

end of thread, other threads:[~2026-10-26 12:18 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-26 12:18 [PATCH net v2 2/2] enic: require DEVCMD2 and set the V2 VF mailbox source netdev-bot+sashiko
  -- strict thread matches above, loose matches on Subject: below --
2026-10-22 11:56 [PATCH net v2 0/2] enic: retain admin DMA resources and identify V2 VFs Satish Kharat
2026-10-22 11:57 ` [PATCH net v2 2/2] enic: require DEVCMD2 and set the V2 VF mailbox source Satish Kharat

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox