Re: [PATCH net v6 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] 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