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

[netdev-bot+sashiko@kernel.org]

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] V2 VF probe now depends on firmware support for the new devcmd
  CMD_SRIOV_STATS_GET (devcmd 98), and nothing checks for that support…
- [Low] enic_admin_channel_open() accepts the CMD_SRIOV_STATS_GET reply
  only when le32_to_cpu(stats->ver) == VNIC_SRIOV_STATS_VERSION (1).
- [Low] The comment on the new struct enic field
  sriov_stats_dma_unresolved says a CMD_SRIOV_STATS_GET timeout leaves…

--

enic: require DEVCMD2 and set the V2 VF mailbox source

This patch makes DEVCMD2 mandatory for V2 PF support and V2 VF probe. On
a V2 VF, enic_admin_channel_open() now issues CMD_SRIOV_STATS_GET and uses
the returned vf_index as the MBOX source ID. It also adds per-call DEVCMD2
completion tracking so that a statistics DMA target is retained after a
timeout.

> diff --git a/drivers/net/ethernet/cisco/enic/enic.h b/drivers/net/ethernet/cisco/enic/enic.h
> index ebe617e752c79..d1f9f2156e0b0 100644
> --- a/drivers/net/ethernet/cisco/enic/enic.h
> +++ b/drivers/net/ethernet/cisco/enic/enic.h
> @@ -271,6 +271,11 @@ struct enic {
>  	u16 num_vfs;
>  	enum enic_vf_type vf_type;
>  	bool vf_registered;
> +	u16 vf_id;
> +	/* A CMD_SRIOV_STATS_GET timeout leaves its persistent coherent target
> +	 * device-owned until a later serialized command completes.
> +	 */
> +	bool sriov_stats_dma_unresolved;

[Severity: Low]
This isn't a bug, but does this comment describe a broader release
condition than the code implements?

The flag is cleared in only two places: on a VNIC_DEVCMD2_COMPLETED result
from CMD_QP_TYPE_SET in enic_admin_qp_type_set(), and on one from
CMD_CLOSE in enic_dev_close(). Every other DEVCMD2 command goes through
vnic_dev_cmd() -> _vnic_dev_cmd2(), which passes a NULL status. Those
commands complete on the same ring but leave the flag set.

The commit message is more precise: "A later blocking QP or close command
result fences that target". Could the comment say the same thing? A later
change might otherwise expect an ordinary devcmd to clear the flag, and
then get -EBUSY from enic_admin_channel_open().

> diff --git a/drivers/net/ethernet/cisco/enic/enic_admin.c b/drivers/net/ethernet/cisco/enic/enic_admin.c
> index 8a063c563a41e..6fa27a755e3f3 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_admin.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_admin.c

[ ... ]

> @@ -590,15 +604,40 @@ 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;
> +		}

[Severity: Medium]
What happens on firmware that provides DEVCMD2 but does not implement
CMD_SRIOV_STATS_GET (devcmd 98)?

For a V2 VF, enic_probe() checks only vnic_dev_uses_devcmd2(), and nothing
checks for the new command before it is sent. vnic_dev.c already has
vnic_dev_capable(), which sends CMD_CAPABILITY for this purpose, but
vnic_dev_sriov_stats() does not call it.

If the firmware answers ERR_ECMDUNKNOWN, _vnic_dev_cmd2_status() exempts
only CMD_CAPABILITY:

			if (result->error) {
				err = result->error;
				if (err != ERR_ECMDUNKNOWN ||
				    cmd != CMD_CAPABILITY)
					vdev_neterr(vdev, "Error %d devcmd %d\n",
						    err, _CMD_N(cmd));
				return -err;
			}

So the driver logs an error and returns a failure. A timeout returns
-ETIMEDOUT. In both cases the path is:

enic_probe()
  enic_admin_channel_open()
    enic_dev_sriov_stats()
      vnic_dev_sriov_stats()
        vnic_dev_cmd2_status(CMD_SRIOV_STATS_GET)  -> error
    goto disable_queues
  goto err_out_dev_deinit

and the netdev is never registered.

Does this also break VF 0? Before this patch VF 0 could register with the
deployed PF, because its source ID of 0 matched.

The PF-side VIC_FEATURE_SRIOV check in enic_sriov_detect_vf_type() does not
cover VF probe. The patch carries a Fixes: tag and may be backported.
Would it be worth adding a vnic_dev_capable(CMD_SRIOV_STATS_GET) check, or
stating a minimum firmware version in the commit message?

> +		version = le32_to_cpu(stats->ver);
> +		vf_id = le16_to_cpu(stats->vf_index);
> +		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;
> +		}

[Severity: Low]
Is the exact match on VNIC_SRIOV_STATS_VERSION intended?

struct vnic_sriov_stats has a reserved1 field and a reserved2[256 - 23]
tail, which looks like space for fields added in later versions. This code
reads only ver and vf_index, and vf_index sits at a fixed offset. If
firmware raises ver above 1, every V2 VF would fail probe with -EINVAL.

The MBOX capability negotiation in enic_mbox_vf_capability_check() treats
the version as a minimum instead:

	if (version < ENIC_MBOX_CAP_VERSION_1) {

Would a "version < VNIC_SRIOV_STATS_VERSION" check work here too?

Also, the commit message says:

  Validate the returned version and zero-based VF index

The only check on vf_index, though, is that it is not the
ENIC_MBOX_DST_PF sentinel. Could the commit message say exactly what is
checked?

> +		WRITE_ONCE(enic->vf_id, vf_id);
> +	}

[ ... ]

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