Re: [PATCH net v2 2/2] enic: require DEVCMD2 and set the V2 VF mailbox source

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.

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

[ ... ]

[ ... ]

[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);

[ ... ]

[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