Re: [PATCH net-next v2 5/6] enic: manage V2 VF station and administrative MAC

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 3 (1)

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] At this commit, enic_remove() runs
  enic_vf_admin_mac_quiesce(), VF_UNREGISTER and…
- [Low] At this commit, enic_vf_station_sync_reset() drops the core sync
  reference for the uc entry that matches vf_station_addr only once.
- [Low] The comment in enic_remove() says "VF_UNREGISTER is the protocol
  teardown operation: the PF removes all VF-requested configuration,…
- [Low] At this commit, a runtime station change does not reprogram an
  old station address A that is still wanted as a secondary unicast…

Pre-existing issues:
- [Low] enic_reset_addr_lists() now holds netif_addr_lock_bh() only
  around __dev_uc_unsync()/__dev_mc_unsync().

--

enic: manage V2 VF station and administrative MAC

V2 VFs now manage the station MAC through a mailbox DEL+ADD replace
request. The address is installed on open and removed on an ordinary
stop. A delayed worker applies PF administrative MAC notifications, the
admin MAC policy is re-read after every VF registration, and the worker
is quiesced and rearmed around teardown and reset.

> diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
> index 28738bd14fa7c..2132bfa9c8d38 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_main.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_main.c

[ ... ]

> @@ -1040,8 +1043,10 @@ void enic_reset_addr_lists(struct enic *enic)
>  {
>  	struct net_device *netdev = enic->netdev;
>  
> +	netif_addr_lock_bh(netdev);
>  	__dev_uc_unsync(netdev, NULL);
>  	__dev_mc_unsync(netdev, NULL);
> +	netif_addr_unlock_bh(netdev);
>  
>  	enic->mc_count = 0;
>  	enic->uc_count = 0;

[Severity: Low]
This is a pre-existing issue, and this patch did not introduce it. The
baseline did both the unsync and the counter reset with no lock at all,
so this change only narrows the window.

The addr lock now covers only the two unsync calls. mc_count, uc_count
and flags are still reset after netif_addr_unlock_bh().

Devices that use the legacy ndo_set_rx_mode path update these counters
under addr_list_lock only:

enic_set_rx_mode()
  __dev_uc_sync(netdev, enic_uc_sync, enic_uc_unsync);
    enic_uc_sync()
      enic->uc_count++;

Can an rx_mode run between the unlock and the counter reset leave
uc_count or mc_count out of step with the core sync state? If uc_count
underflows, the ENIC_*_PERFECT_FILTERS limit check would no longer work.
Could the counter resets move inside the locked section?

For V2 VFs, the last patch in the series ("enic: configure V2 VF receive
mode over mailbox") switches to ndo_set_rx_mode_async.
netif_rx_mode_run() is serialized by RTNL, and so are the callers of
enic_reset_addr_lists().

[ ... ]

> @@ -1072,6 +1532,52 @@ static int enic_set_mac_address_dynamic(struct net_device *netdev, void *p)

[ ... ]

> +		/* Keep the old software address visible until the complete station
> +		 * replacement proves convergence.
> +		 */
> +		err = enic_vf_station_addr_replace(enic, addr);
> +		if (err)
> +			return err;
> +
> +		enic_vf_station_sync_reset(enic);
> +		err = enic_set_mac_addr(netdev, addr);
> +		if (!err) {
> +			enic_vf_station_addr_set(enic, addr);
> +			enic_vf_station_sync_reset(enic);
> +			enic_vf_admin_mac_cache_selected(enic, addr);
> +		}
> +
> +		return err;
> +	}

[Severity: Low]
At this commit, if the old station address A is also in the uc list as a
secondary address, does anything reprogram A after the station moves?

enic_vf_station_addr_replace() deletes A at the PF, and
enic_vf_station_sync_reset() unsyncs A's core entry. No rx_mode update
is scheduled afterwards, so A would not be added back as a secondary
filter.

The nonzero-policy path in enic_vf_admin_mac_work() has a similar gap:

enic_vf_admin_mac_work() {
    ...
	} else if (enic->vf_station_addr_valid &&
		   !ether_addr_equal(enic->vf_station_addr, selected)) {
		mutated = true;
		err = enic_vf_station_addr_del(enic);
    ...
	changed = !ether_addr_equal(enic->netdev->dev_addr, selected);
	enic_vf_station_sync_reset(enic);
    ...
}

enic_vf_station_addr_del() clears vf_station_addr_valid on success. The
first enic_vf_station_sync_reset() then returns early, and a re-synced
entry for A can keep sync_cnt=1 while the PF no longer has a filter for
A.

Could receive for A be lost until the next rx_mode event?

The last patch in the series, "enic: configure V2 VF receive mode over
mailbox", fixes this:

  - It adds netif_rx_mode_schedule_update() here and after the admin-MAC
    update in enic_vf_admin_mac_work().
  - enic_vf_collect_mac_ops() skips the station address, so A has
    sync_cnt==0 while it is the station.

Would it make sense to schedule the rx_mode update in this patch too, so
that intermediate commits don't have this behaviour?

[ ... ]

> @@ -1807,6 +2314,25 @@ static int enic_open(struct net_device *netdev)
>  	if (!enic_is_dynamic(enic) && !enic_is_sriov_vf(enic))
>  		enic_dev_add_station_addr(enic);
>  
> +	if (enic_is_sriov_vf_v2(enic)) {

[ ... ]

> +		enic_vf_station_addr_set(enic, netdev->dev_addr);
> +		enic_vf_station_sync_reset(enic);
> +		vf_mac_added = true;
> +	}
> +
>  	enic_set_rx_mode(netdev);

[Severity: Low]
enic_vf_station_sync_reset() drops the core sync reference for the
station entry only once. At this commit V2 VFs still use
enic_set_rx_mode(), which runs on the very next line, and enic_uc_sync()
does not exclude vf_station_addr:

enic_uc_sync() {
    ...
	enic_dev_add_addr(enic, uc_addr);
	enic->uc_count++;
    ...
}

Won't this re-sync the station entry right away?

If that uc entry is removed later, enic_uc_unsync() would call
enic_dev_del_addr() for the station address. It is not clear whether
that devcmd reaches the PF's address-keyed ledger for V2 VFs. The last
patch says V2 VFs cannot program receive filters directly.

The last patch in the series, "enic: configure V2 VF receive mode over
mailbox", resolves this:

  - V2 VFs move to enic_set_vf_rx_mode() via ndo_set_rx_mode_async.
  - enic_vf_collect_mac_ops() and enic_vf_addr_list_count() skip
    vf_station_addr.
  - enic_open() calls enic_set_rx_mode() only for non-V2 devices.

[ ... ]

> @@ -3542,9 +4106,13 @@ static void enic_remove(struct pci_dev *pdev)
>  
>  		/* Close the admin channel and unregister from the PF before
>  		 * unregister_netdev() to prevent a late PF notification from
> -		 * touching a netdev that is being torn down.
> +		 * touching a netdev that is being torn down. VF_UNREGISTER is the
> +		 * protocol teardown operation: the PF removes all VF-requested
> +		 * configuration, including the station address, before replying.
>  		 */

[Severity: Low]
The VF now relies on this contract to skip the station DEL_MAC on
remove and on internal reset (__enic_stop(netdev, false)).

The in-tree PF side doesn't do this yet. enic_mbox_pf_handle_unregister()
only does:

	enic->vf_state[vf_id].registered = false;

and replies. enic_mbox_pf_process_msg() has no case for
ENIC_MBOX_VF_ADD_DEL_MAC_REQUEST or ENIC_MBOX_PF_SET_ADMIN_MAC_ACK, so
both fall through to -EOPNOTSUPP.

This can't be reached today. enic_driver has no .sriov_configure, and
the comment above enic_sriov_configure() says the V2 helpers are
"defined but not yet wired". The earlier patch "enic: validate V2 VF
configuration replies" also says PF-side MAC handling must be added
before that callback is wired.

Should this comment say it describes the PF protocol contract rather
than the in-tree PF? That would remind the PF activation series to
implement station cleanup in the unregister handler.

>  		if (enic_is_sriov_vf_v2(enic)) {
> +			enic_vf_admin_mac_quiesce(enic);
> +
>  			if (READ_ONCE(enic->vf_registered)) {
>  				int unreg_err = enic_mbox_vf_unregister(enic);

[Severity: Medium]
At this commit, enic_vf_admin_mac_quiesce(), VF_UNREGISTER and
enic_admin_channel_close() all run while the netdev is still registered
and attached, and RTNL is not held. Can a concurrent ndo_open undo this
teardown?

Under RTNL, enic_open() can see !admin_chan_up, !vf_registered or
reconnect_required and reopen the channel:

	if (enic->admin_chan_up)
		enic_admin_channel_close(enic);
	err = enic_admin_chan_reopen(enic);

That path re-registers with the PF and then rearms the worker:

enic_open()
  enic_admin_chan_reopen()
    ... admin channel open, VF register ...
    enic_vf_admin_mac_rearm()
      enic->vf_admin_mac_work_enabled = true;

unregister_netdev() -> enic_stop() does not close the admin channel.
Could the admin resources and the admin IRQ then outlive
free_netdev()?

The last patch in the series, "enic: configure V2 VF receive mode over
mailbox", closes this window:

  - It adds rtnl_lock(); netif_device_detach(netdev); rtnl_unlock();
    after the quiesce, so __dev_open() returns -ENODEV once the device
    is detached.
  - enic_admin_channel_close() quiesces the worker again at entry.

Could the detach be moved into this patch so intermediate commits don't
carry the race?

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