Re: [PATCH net-next v6 06/10] bnge: remove refcount from L2 filter

[netdev-bot+sashiko]

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 commit message says the patch only removes the L2 filter
  refcount, but the patch also changes what bnge_alloc_l2_filter()…

--

bnge: remove refcount from L2 filter

This patch removes the refcount from struct bnge_l2_filter. From now on,
only the context that created a filter will delete it. The lookup helper
is folded into one function that asserts the netdev instance lock is
held, and bnge_alloc_l2_filter() now returns -EEXIST for a duplicate key.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_filter.c b/drivers/net/ethernet/broadcom/bnge/bnge_filter.c
> index a8bb441ebef9a..6316f75ca36ea 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_filter.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_filter.c

[ ... ]

> @@ -75,9 +63,10 @@ static struct bnge_l2_filter *bnge_alloc_l2_filter(struct bnge_net *bn,
>  
>  	idx = jhash2(&key->filter_key, BNGE_L2_KEY_SIZE, bn->hash_seed) &
>  	      BNGE_L2_FLTR_HASH_MASK;
> +
>  	fltr = bnge_lookup_l2_filter(bn, key, idx);
>  	if (fltr)
> -		return fltr;
> +		return ERR_PTR(-EEXIST);

[Severity: Low]
This changes what bnge_alloc_l2_filter() does, and the commit message
doesn't mention it. Could the changelog describe this?

Before this patch, a duplicate dst MAC/VLAN key returned the existing
filter with an extra reference, so the caller shared it. Now a duplicate
key is a hard error.

bnge_hwrm_set_vnic_filter() passes the error up to both of its callers.
In bnge_cfg_rx_mode() it gets treated like any other HWRM failure:

	rc = bnge_hwrm_set_vnic_filter(bn, 0, i, vnic->uc_list + off);
	if (rc) {
		...
			netdev_err(dev, "HWRM vnic filter failure rc: %d\n",
				   rc);
		vnic->uc_filter_count = i;
		return rc;
	}

In bnge_init_chip() it would make open fail on the dev_addr filter.

As far as I can tell, nothing can request a duplicate key at this
commit:

  - the unicast address list is already de-duplicated
  - dev_addr is skipped when uc_list is filled
  - there is no ndo_set_mac_address
  - bnge_hwrm_clear_vnic_filter() removes every tracked filter on close

So this shouldn't change behaviour yet. A later patch in the series also
seems to depend on it on purpose, since it adds explicit -EEXIST handling
in bnge_cfg_rx_mode() for addresses "Already delivered by a user L2
rule".

The commit message only describes removing the refcount, though, so
could it also mention that duplicate keys now return -EEXIST instead of
being shared?

>  
>  	fltr = kzalloc_obj(*fltr, gfp);
>  	if (!fltr)

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106093524.838341-1-vikas.gupta%40broadcom.com