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

[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: 2 · Low: 1 (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] The commit's central claim "Treat the PF administrative MAC as
  authoritative" is not enforced.
- [Medium] Unaligned memory accesses in ether_addr_equal and
  ether_addr_copy
- [Low] enic_vf_station_sync_reset() only clears ha->sync_cnt for the
  station address; at the target commit enic_open() calls it two…

Pre-existing issues:
- [Low] enic_reset_addr_lists() takes netif_addr_lock_bh() only around
  __dev_uc_unsync()/__dev_mc_unsync() and resets the driver-side mirrors…

--

enic: manage V2 VF station and administrative MAC

This adds V2 VF station-address management over the mailbox (delete plus add
in one compound request), keeps that address out of netdev secondary-unicast
synchronization, and adds a delayed work item plus a post-registration
refresh so the PF administrative MAC drives the VF operational address.

A few questions on the new code below.

> diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
> index 1c60e7a666aeb..16412c29e9321 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 isn't a bug introduced by this patch, but should the new critical
section in enic_reset_addr_lists() also cover the counter reset?

The address-list lock is dropped before enic->mc_count, enic->uc_count and
enic->flags are cleared, and those same fields are updated from the
ndo_set_rx_mode sync callbacks under netif_addr_lock. The baseline took no
address-list lock here at all, so this is pre-existing, and for V2 VFs at
the end of the series every writer of these counters runs under RTNL
(netif_rx_mode_run() asserts the ops-compat lock before invoking
enic_set_vf_rx_mode), so the reset does not appear reachable concurrently
today. Would extending the lock to the counter reset make the intended
invariant explicit?

> @@ -1065,6 +1070,456 @@ static int enic_set_mac_addr(struct net_device *netdev, char *addr)
>  	return 0;
>  }
>  

[ ... ]

> +static int enic_vf_keep_nonstation_sync(struct net_device *netdev,
> +					const u8 *addr)
> +{
> +	struct enic *enic = netdev_priv(netdev);
> +
> +	if (!ether_addr_equal(addr, enic->vf_station_addr))
> +		return -ENOENT;
> +	if (WARN_ON_ONCE(!enic->uc_count))
> +		return 0;
> +	enic->uc_count--;
> +
> +	return 0;
> +}
> +
> +static void enic_vf_station_sync_reset(struct enic *enic)
> +{
> +	if (!enic->vf_station_addr_valid)
> +		return;
> +
> +	/* The PF keys its MAC ledger by address. A station entry is also the
> +	 * receive filter for that address and must not retain a second core
> +	 * synchronization reference which could later delete the station.
> +	 */
> +	netif_addr_lock_bh(enic->netdev);
> +	__dev_uc_unsync(enic->netdev, enic_vf_keep_nonstation_sync);
> +	netif_addr_unlock_bh(enic->netdev);
> +}

[Severity: Low]
Does enic_vf_station_sync_reset() actually keep the station address out of
core unicast synchronization at this point in the series?

It only clears ha->sync_cnt for the matching entry; the entry itself stays
on netdev->uc. __hw_addr_sync_dev() re-syncs every entry whose sync_cnt is
zero, so the next __dev_uc_sync() recreates the reference, after which a
core unsync can call enic_uc_unsync() -> enic_dev_del_addr() on the station
address while enic->vf_station_addr_valid is still true.

In enic_open() the reset is two statements before enic_set_rx_mode():

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

    enic_set_rx_mode(netdev);

Worth noting this looks harmless by the end of the series: a later patch
("enic: configure V2 VF receive mode over mailbox") gives V2 VFs their own
ops table with only .ndo_set_rx_mode_async, guards the legacy
enic_set_rx_mode() call with if (!enic_is_sriov_vf_v2(enic)), and the async
collector skips any address equal to enic->vf_station_addr in both its add
and delete loops. Is the intent that this helper is only a stopgap until
that patch lands, and if so could it be squashed or the ordering noted?

[ ... ]

> +/* CMD_GET_MAC_ADDR is the PF-owned policy store. Re-read it after REGISTER so
> + * a lost notification cannot leave the VF using stale administrative policy.
> + */
> +static int enic_vf_admin_mac_refresh(struct enic *enic)
> +{

[ ... ]

> +		policy_changed = !ether_addr_equal(previous_policy, policy);
> +		apply_policy = pending || policy_changed ||
> +			!is_valid_ether_addr(enic->netdev->dev_addr);
> +		if (!apply_policy) {
> +			ether_addr_copy(selected, enic->netdev->dev_addr);

[Severity: Medium]
Why does vf_station_addr get __aligned(2) in this same patch while
the new MAC buffers next to it do not?

vf_admin_mac and vf_admin_mac_random_addr are plain u8[ETH_ALEN] and
rely purely on the packing of the preceding bool fields
(vf_station_addr_valid, vf_link_running, ...) to land on an even
offset. Both are passed to ether_addr_copy() and ether_addr_equal()
in enic_vf_admin_mac_notify(), enic_vf_admin_mac_cache_selected(),
enic_vf_admin_mac_work() and enic_vf_admin_mac_refresh(), and on
builds without CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS those helpers
cast to u16 * and do three 16-bit accesses. On architectures that
trap unaligned accesses that is a fault, elsewhere it is a silent
fixup cost. The same applies to the on-stack buffers policy,
selected, previous_policy and random_addr, which sit among bool and
u8 locals in enic_vf_admin_mac_work() and enic_vf_admin_mac_refresh()
and are handed to the same helpers.

The explicit annotation on vf_station_addr suggests the author was
already aware of the requirement, so the omission on the neighbouring
fields looks accidental. Should __aligned(2) be applied consistently
to the new struct fields and the local address buffers, or is the
intent that only vf_station_addr needed it for some reason that is
not obvious from the patch?

[Severity: Medium]
Is the commit message claim "Treat the PF administrative MAC as
authoritative" enforced on this path?

enic_vf_admin_mac_refresh() compares the freshly read policy against the
cached previous_policy, not against the operational netdev->dev_addr. When
the PF policy value has not changed and dev_addr is valid, apply_policy is
false and selected is taken from dev_addr, so an address that disagrees
with the policy is preserved rather than corrected.

Combined with the V2 branch of enic_set_mac_address_dynamic() below, can a
locally installed address survive every later registration and reconnect?

[ ... ]

> @@ -1072,6 +1527,45 @@ static int enic_set_mac_address_dynamic(struct net_device *netdev, void *p)
>  	char *addr = saddr->sa_data;
>  	int err;
>  
> +	if (enic_is_sriov_vf_v2(enic)) {
> +		if (!is_valid_ether_addr(addr))
> +			return -EADDRNOTAVAIL;
> +		if (ether_addr_equal(addr, netdev->dev_addr))
> +			return 0;
> +
> +		if (enic->vf_datapath_open &&
> +		    !READ_ONCE(enic->vf_registered))
> +			return -ENODEV;
> +
> +		/* An internal reset keeps IFF_UP set while its failed reopen leaves
> +		 * the datapath closed. Cache the requested address in that state; the
> +		 * next successful open installs it together with the datapath.
> +		 */
> +		if (!enic->vf_datapath_open) {
> +			err = enic_set_mac_addr(netdev, addr);
> +			if (!err)
> +				enic_vf_admin_mac_cache_selected(enic, addr);
> +			return err;
> +		}

[Severity: Medium]
Should this branch also check the requested address against
enic->vf_admin_mac?

With the datapath closed, the requested address is installed purely locally
via enic_set_mac_addr() with no mailbox request, so no PF policy check
happens at all. A nonzero administrative policy is then silently overridden:

  ip link set <vf> down      (__enic_stop() clears vf_datapath_open)
  ip link set <vf> address <C>

On the next internal reset or reconnect, enic_vf_admin_mac_refresh() sees
previous_policy equal to the fetched policy and a valid dev_addr, so
apply_policy is false and <C> is kept. The following enic_open() then calls
enic_vf_station_addr_replace(netdev->dev_addr) and fails with -EACCES
("Failed to register VF station address"), leaving the interface unable to
come up until the policy address is restored by hand.

Note that a repeated PF notification does still correct this, since
enic_vf_admin_mac_notify() re-arms on !enic->vf_admin_mac_pending, but that
requires the PF to notify again. Would gating this branch on the cached
policy, or making apply_policy compare policy against dev_addr, match the
stated intent better?

[ ... ]

> @@ -1807,6 +2302,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)) {
> +		if (!READ_ONCE(enic->vf_registered)) {
> +			netdev_err(netdev, "VF is not registered with its PF\n");
> +			err = -ENODEV;
> +			goto err_out_disable_wq;
> +		}
> +
> +		err = enic_vf_station_addr_replace(enic, netdev->dev_addr);
> +		if (err) {
> +			netdev_err(netdev,
> +				   "Failed to register VF station address: %d\n",
> +				   err);
> +			goto err_out_disable_wq;
> +		}
> +		enic_vf_station_addr_set(enic, netdev->dev_addr);
> +		enic_vf_station_sync_reset(enic);
> +		vf_mac_added = true;
> +	}
> +
>  	enic_set_rx_mode(netdev);
>  
>  	netif_tx_wake_all_queues(netdev);

[ ... ]

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