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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · 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] The fence for the statistics-command DMA is cleared only when
  the command succeeds, not whenever it completes.

--

enic: require DEVCMD2 and set the V2 VF mailbox source

This commit has V2 VFs query CMD_SRIOV_STATS_GET after opening the admin
channel. The returned logical VF index becomes the mailbox src_vnic_id,
and SR-IOV V2 now requires DEVCMD2. A timeout of the statistics command
is tracked with sriov_stats_dma_unresolved. A later CMD_QP_TYPE_SET is
expected to fence it, and if nothing does, the probe resources are kept
until reboot.

> diff --git a/drivers/net/ethernet/cisco/enic/enic.h b/drivers/net/ethernet/cisco/enic/enic.h
> index cf2dee8fa78ff..886f3cba543dc 100644
> --- a/drivers/net/ethernet/cisco/enic/enic.h
> +++ b/drivers/net/ethernet/cisco/enic/enic.h
> @@ -273,6 +273,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;
>  	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 a5c08cbcb3391..2f6688a73b41a 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_admin.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_admin.c
> @@ -406,6 +406,12 @@ static int enic_admin_qp_type_set(struct enic *enic, u32 enable)
>  
>  	spin_lock_bh(&enic->devcmd_lock);
>  	err = vnic_dev_cmd(enic->vdev, CMD_QP_TYPE_SET, &a0, &a1, wait);
> +	/* DEVCMD2 runs one command at a time for a vNIC. Completion of this
> +	 * blocking command proves that earlier statistics-command DMA has
> +	 * finished. Admin WQ/RQ ownership still requires RUNNING == 0.
> +	 */
> +	if (!err)
> +		WRITE_ONCE(enic->sriov_stats_dma_unresolved, false);

[Severity: Medium]
Should sriov_stats_dma_unresolved be cleared whenever CMD_QP_TYPE_SET
completes, not only when it returns 0?

The comment above, the enic.h field comment ("until a later serialized
command completes") and the commit message all treat completion as the
fence. On the DEVCMD2 path, _vnic_dev_cmd2() returns -result->error only
after firmware has written this command's result slot:

drivers/net/ethernet/cisco/enic/vnic_dev.c:_vnic_dev_cmd2() {
    ...
	for (delay = 0; delay < wait; delay++) {
		if (result->color == color) {
			if (result->error) {
				err = result->error;
				...
				return -err;
			}
    ...
}

Only -ETIMEDOUT leaves completion unknown after the command is posted.
-ENODEV and -EBUSY are returned before anything is posted.

In enic_probe(), a timeout in enic_dev_sriov_stats() sets the flag, and
the unwind goes to err_out_admin_close. There, enic_admin_channel_close()
calls enic_admin_qp_type_set(QP_DISABLE). If firmware completes that
command with an error status, the flag stays set. This happens even
though enic_admin_channel_close() treats the rejection as harmless:

	/* Firmware rejection does not undermine the WQ/RQ proof. */
	return qp_err;

At err_out_admin_fence, enic_dma_quarantined() still returns true. Probe
then calls enic_terminal_dma_quarantine() and returns early, skipping
enic_dev_deinit(), vnic_dev_close(), vnic_dev_unregister(),
enic_iounmap(), pci_release_regions() and free_netdev().

Would that keep the netdev, vdev, coherent buffers, BAR mappings and PCI
regions held until reboot, and prevent the VF from being probed again?

If clearing only on success is intended, could the comments and the
commit message say so?

>  	spin_unlock_bh(&enic->devcmd_lock);
>  
>  	return err;

[ ... ]

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