Re: [net PATCH 03/13] fbnic: Keep the all-multi MACDA entry out of the address lists

[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 rewritten layout comment in
  drivers/net/ethernet/meta/fbnic/fbnic_rpc.h says the reserved entry…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but a BMC
  multicast address can be silently dropped when the host fills the…
- [Low] This problem wasn't introduced by this patch, but the comments on
  the multicast scan loops in __fbnic_mc_sync…

--

fbnic: Keep the all-multi MACDA entry out of the address lists

The MACDA entry just below mac_addr_boundary is now kept for the BMC
all-multi wildcard. Before this, either the unicast or the multicast list
could take that entry, and the BMC rule then matched a host address. The
unicast scan now starts at mac_addr_boundary, and the multicast sync and
unsync scans start one entry below it.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
> index dc86b51f35830..0be71e6fd3d4e 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c

[ ... ]

> @@ -507,9 +508,10 @@ struct fbnic_mac_addr *__fbnic_mc_sync(struct fbnic_dev *fbd,
>  
>  	/* Scan from middle of list to top, filling top down.
>  	 * Skip over the address reserved for the BMC MAC and
> -	 * exclude index 0 as that belongs to the broadcast address
> +	 * exclude index 0 as that belongs to the broadcast address.
> +	 * The entry below the boundary is reserved for all-multi.
>  	 */

[Severity: Low]
This is a pre-existing issue, but the comment says the scan excludes
"index 0 as that belongs to the broadcast address". Broadcast is at
FBNIC_RPC_TCAM_MACDA_BROADCAST_IDX (4), and index 0 is
FBNIC_RPC_TCAM_MACDA_BMC_ADDR_IDX. The loop stops at:

	--i > FBNIC_RPC_TCAM_MACDA_BROADCAST_IDX

Since this comment is being edited anyway, could the index be fixed too?

fbnic_mc_unsync() in fbnic_netdev.c has the same text. This patch changes
the start of that loop, but its comment does not get the new reservation
sentence.

> -	for (i = fbd->mac_addr_boundary;
> +	for (i = fbd->mac_addr_boundary - 1;
>  	     --i > FBNIC_RPC_TCAM_MACDA_BROADCAST_IDX;) {
>  		struct fbnic_mac_addr *mac_addr = &fbd->mac_addr[i];
>  

[Severity: Medium]
This isn't a bug introduced by this patch, but the shared multicast
region is now 5..22. If the host fills it, can a BMC multicast address
get dropped?

When the first scan finds no free entry, the second scan in
__fbnic_mc_sync() walks BMC indices 3..1 and only matches entries that
already exist:

	while (--i) {
		...
		/* Only move on if we find a match */
		if (!ether_addr_equal(mac_addr->value.addr8, addr))
			continue;

So __fbnic_mc_sync() returns NULL even when BMC slots 1-3 are free.
fbnic_bmc_rpc_init() then logs the failure and skips the address:

		if (!mac_addr) {
			netdev_err(fbd->netdev,
				   "No slot for BMC MAC address[%d]\n", j);
			continue;
		}

By this point fbnic_bmc_rpc_check() has already cleared
FBNIC_FW_CAP_F_BMC_TCAM_REINIT, and nothing schedules a retry. If the
host addresses are already synced, __fbnic_set_rx_mode() never sees
-ENOSPC, so neither the host nor the BMC falls back to all-multi. With BMC
all-multi off, the BMC address stays unprogrammed until the next firmware
reinit.

This patch lowers the trigger from 19 host multicast entries to 18.

Later in the series, "fbnic: Rebuild the BMC rules from scratch on every
reinit" adds fbnic_bmc_mc_reserved(). That only keeps a copy that is
already in the BMC region, so a new, distinct BMC multicast address would
still fail.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.h b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.h
> index 3d4925b2ac75d..679ba0a1058d9 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.h
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.h
> @@ -156,9 +156,9 @@ enum {
>   * always go top-down with unicast, and bottom-up with multicast so that
>   * there should be free-space in the middle between the two.
>   *
> - * The entry at MADCA_DEFAULT_BOUNDARY is a special case as it can be used
> - * for the ALL MULTI address if the list is full, or the BMC has requested
> - * it.
> + * The entry just below the boundary is reserved for the ALL MULTI address,
> + * used if the list is full, or the BMC has requested it. Neither the unicast
> + * nor the multicast list allocates it.
>   */

[Severity: Low]
Is "used if the list is full, or the BMC has requested it" accurate? The
only place that fills this entry is fbnic_bmc_rpc_all_multi_config(), and
it does so only when BMC all-multi is on:

	mac_addr = &fbd->mac_addr[fbd->mac_addr_boundary - 1];
	if (fbnic_bmc_all_multi(fbd)) {

If the host multicast list is full and BMC all-multi is off,
fbnic_promisc_sync() puts the wildcard at PROMISC_IDX instead:

	mac_addr = &fbd->mac_addr[FBNIC_RPC_TCAM_MACDA_PROMISC_IDX];
	...
	} else if (mc_promisc && !fbnic_bmc_all_multi(fbd)) {

That agrees with the comment a few lines further down:

/* Reserved for use to record Multicast promisc, or Promiscuous */
#define FBNIC_RPC_TCAM_MACDA_PROMISC_IDX	31

The host only shares this entry, through FBNIC_MAC_ADDR_T_ALLMULTI, when
the BMC has already asked for all-multi. Otherwise the entry is unused.

Could the comment say "and" instead of "or", so it does not contradict
the PROMISC_IDX comment?

>  #define FBNIC_RPC_TCAM_MACDA_MULTICAST_IDX	5
>  #define FBNIC_RPC_TCAM_MACDA_DEFAULT_BOUNDARY	24

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/179322311834.40667.5383039334126572422.stgit%40ahduyck-xeon-server.home.arpa