Re: [PATCH net-next 2/6] enic: serialize V2 VF mailbox requests

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch repairs merged code — the VF single-outstanding-reply
  mailbox state (`enic->mbox_expected_reply`,…

--

enic: serialize V2 VF mailbox requests

This adds a VF-only mutex (vf_mbox_request_lock) that is held across the
lifetime of a V2 VF mailbox request, from arming the expected reply until
the reply or the timeout has been consumed. The three VF request helpers
enic_mbox_vf_capability_check(), enic_mbox_vf_register() and
enic_mbox_vf_unregister() are converted to the new start/abort/finish
helpers.

One question below about the changelog.

>     enic: serialize V2 VF mailbox requests
>
>     The VF stores one expected reply and uses one completion for mailbox
>     requests. If two VF control paths issue requests at the same time, the
>     second request can replace the reply state for the first.

[Severity: Low]
Should this carry a Fixes: tag, and is the described trigger reachable at
this point in the series?

The state being protected (enic->mbox_expected_reply,
enic->mbox_expected_msg_num, enic->mbox_comp) and all three request
helpers already exist in the baseline tree; they came in with commit
72b65c94058e ("enic: add MBOX VF handlers for capability, register and
link state"), whose changelog relies on a single-outstanding-reply
invariant that nothing in the code enforces. Since this changelog
describes a live defect ("the second request can replace the reply state
for the first"), a Fixes: tag pointing at that commit would let
backporters and stable tooling find the affected releases.

On reachability, the VF request issuers present at this commit look
mutually exclusive:

- enic_probe() runs single threaded, before register_netdev().

- enic_admin_chan_reopen() is called from enic_reset() and
  enic_tx_hang_reset(), both of which run under rtnl_lock and return
  early on !netif_running().

- enic_remove() disables and syncs the reset work items before it issues
  the unregister:

    disable_work_sync(&enic->reset);
    disable_work_sync(&enic->tx_hang_reset);
    disable_work_sync(&enic->change_mtu_work);
    ...
    if (enic_is_sriov_vf_v2(enic)) {
            if (READ_ONCE(enic->vf_registered)) {
                    int unreg_err = enic_mbox_vf_unregister(enic);

The additional request sites that could actually overlap (the delayed VF
admin MAC work and the mailbox receive-mode path) appear only in later
patches of this series.

Would it be more accurate to either name the commit being fixed with a
Fixes: tag, or state that this is preparatory serialization for the
request sites added later in the series?

> diff --git a/drivers/net/ethernet/cisco/enic/enic_mbox.c b/drivers/net/ethernet/cisco/enic/enic_mbox.c
> index 5c93ca49552ac..b8a18d9682b2f 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_mbox.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_mbox.c
> @@ -218,6 +218,32 @@ static int enic_mbox_wait_reply(struct enic *enic, unsigned long timeout_ms)
>  	return err;
>  }
>  
> +static void enic_mbox_vf_request_start(struct enic *enic)
> +{
> +	mutex_lock(&enic->vf_mbox_request_lock);
> +	reinit_completion(&enic->mbox_comp);
> +	spin_lock_bh(&enic->mbox_state_lock);
> +	enic->mbox_expected_msg_num = 0;
> +	enic->mbox_expected_reply = 0;
> +	spin_unlock_bh(&enic->mbox_state_lock);
> +}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921-b4-enic-sriov-v2-vf-receive-control-v1-0-67a0a6e08d43%40cisco.com