Re: [PATCH net-next v6 08/10] bnge: add NTUPLE filter support in ethtool

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 4 · Low: 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] bnge_cfg_rx_mode()
  (drivers/net/ethernet/broadcom/bnge/bnge_netdev.c) used to program at…
- [Medium] bnge_cfg_rx_mode() decides from the shared L2 pool whether to
  program unicast filters or fall back to promiscuous mode.
- [Medium] bnge_srxclsrldel()
  (drivers/net/ethernet/broadcom/bnge/bnge_ethtool.c) ignores the return…
- [Medium] bnge_hwrm_l2_filter_alloc() now sets
  CFA_L2_FILTER_ALLOC_REQ_FLAGS_TRAFFIC_L2 for user ETHER_FLOW ring…
- [Low] Two existing ethtool operations gain new -EBUSY failures, and the
  commit message mentions neither.

--

bnge: add NTUPLE filter support in ethtool

This adds ethtool rxnfc operations to manage NTUPLE and ETHER_FLOW filters
in bnge. User filters stay in software across down/up and are programmed
again on open. The unicast address filters and the user ETHER_FLOW rules
now share the L2 filter pool. BNGE_MAX_UC_ADDRS is removed, and
l2_filters[] and uc_list are sized by BNGE_MAX_L2_FLTRS.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_ethtool.c b/drivers/net/ethernet/broadcom/bnge/bnge_ethtool.c
> index c370686e2b38d..440e0c206aeca 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_ethtool.c

[ ... ]

> @@ -999,6 +1001,12 @@ static int bnge_set_rxfh_fields(struct net_device *dev,
>  		return -EBUSY;
>  	}
>  
> +	if (bnge_has_usr_ntp_fltr(bn)) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Delete user ntuple rules before changing the RSS hash fields");
> +		return -EBUSY;
> +	}
> +
>  	bd->rss_hash_cfg = rss_hash_cfg;
>  
>  	return 0;
> @@ -1060,6 +1068,12 @@ static int bnge_set_rxfh(struct net_device *dev,
>  		return -EBUSY;
>  	}
>  
> +	if (rxfh->key && bnge_has_usr_ntp_fltr(bn)) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Delete user ntuple rules before changing the RSS hash key");
> +		return -EBUSY;
> +	}
> +

[Severity: Low]
This isn't a bug, but the commit message doesn't mention that two existing
ethtool operations can now fail with -EBUSY.

While a user ntuple rule exists, bnge_set_rxfh_fields() rejects any hash
field change and bnge_set_rxfh() rejects any RSS hash key change. The only
explanation is the comment above bnge_has_usr_ntp_fltr(). It says rules
are indexed by a hash of the RSS key and hash fields and are never
rehashed.

So a user who adds rules with ethtool -N and later runs ethtool -X ... hkey
or changes rx-flow-hash will now get EBUSY. Could the commit log mention
this restriction?

[ ... ]

> @@ -1243,6 +1257,611 @@ static int bnge_remove_rxfh_context(struct net_device *dev,

[ ... ]

> +static int bnge_srxclsrldel(struct bnge_net *bn, struct ethtool_rxnfc *cmd)
> +{
> +	struct ethtool_rx_flow_spec *fs = &cmd->fs;
> +	struct bnge_filter_base *fltr_base;
> +	struct bnge_ntuple_filter *fltr;
> +	u32 id = fs->location;
> +
> +	rcu_read_lock();
> +	fltr_base = bnge_get_one_fltr_rcu(bn, bn->l2_fltr_hash_tbl,
> +					  BNGE_L2_FLTR_HASH_SIZE, id, 0);
> +	if (fltr_base) {
> +		struct bnge_l2_filter *l2_fltr;
> +
> +		l2_fltr = container_of(fltr_base, struct bnge_l2_filter, base);
> +		rcu_read_unlock();
> +		/* The firmware filter is gone once the interface is closed. */
> +		if (test_bit(BNGE_FLTR_VALID, &l2_fltr->base.state))
> +			bnge_hwrm_l2_filter_free(bn->bd, l2_fltr);
> +		bnge_del_usr_l2_filter(bn, l2_fltr);
> +		return 0;
> +	}

[ ... ]

> +	rcu_read_unlock();
> +	bnge_hwrm_cfa_ntuple_filter_free(bn->bd, fltr);
> +	bnge_del_ntp_filter_rcu(bn, fltr);
> +	return 0;
> +}

[Severity: Medium]
What happens in bnge_srxclsrldel() if bnge_hwrm_l2_filter_free() or
bnge_hwrm_cfa_ntuple_filter_free() fails?

Both can fail through bnge_hwrm_req_send(). Examples are -EAGAIN for
HWRM_ERR_CODE_BUSY or HOT_RESET_PROGRESS, -EIO for unknown errors, and
timeouts. bnge_hwrm_cfa_ntuple_filter_free() resets filter_id only on
success.

In both branches the return value is ignored. The software entry is freed
by bnge_del_l2_filter_rcu() or bnge_del_ntp_filter_rcu(), and userspace
gets 0. The rule disappears from ETHTOOL_GRXCLSRLALL, but the firmware
filter would still be installed and still steering or dropping traffic.

The only record of that filter_id is gone, so nothing later can free it.
bnge_hwrm_clear_usr_l2_fltrs() walks only usr_fltr_list, and
bnge_hwrm_clear_vnic_filter() walks only vnic->l2_filters[].

For an ETHER_FLOW rule, bnge_del_usr_l2_filter() then programs a new
unicast filter for the same MAC on the default VNIC right away. The
orphaned RING_DST filter may still be in firmware at that point.

Should the error be returned and the software entry kept? bnxt_srxclsrldel()
in bnxt_en has the same pattern.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_filter.c b/drivers/net/ethernet/broadcom/bnge/bnge_filter.c
> index 3ebb3cae4829b..6cf6531a27231 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_filter.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_filter.c
> @@ -584,3 +584,130 @@ int bnge_hwrm_set_vnic_filter(struct bnge_net *bn, u16 vnic_id, u16 idx,

[ ... ]

> +void bnge_del_usr_l2_filter(struct bnge_net *bn, struct bnge_l2_filter *fltr)
> +{

[ ... ]

> +	vnic = &bn->vnic_info[BNGE_VNIC_DEFAULT];
> +	for (i = 1, off = 0; i < vnic->uc_filter_count; i++, off += ETH_ALEN) {
> +		if (vnic->l2_filters[i] ||
> +		    !ether_addr_equal(vnic->uc_list + off, mac))
> +			continue;
> +		if (bnge_hwrm_set_vnic_filter(bn, 0, i, vnic->uc_list + off))
> +			netdev_warn(bn->netdev,
> +				    "failed to restore uc filter %pM\n", mac);
> +		break;
> +	}
> +}

[Severity: Medium]
Is a failed restore in bnge_del_usr_l2_filter() ever retried?

If bnge_hwrm_set_vnic_filter() fails here, even with a transient -EAGAIN
from HWRM_ERR_CODE_BUSY, the driver only prints a warning.
vnic->l2_filters[i] stays NULL. bnge_uc_list_updated() compares only the
address lists against vnic->uc_list:

    if (!ether_addr_equal(ha->addr, vnic->uc_list + off))
        return true;

Later rx_mode events and the core retry timer therefore see
uc_update == false and never reprogram that slot. Unicast traffic to the
MAC (a macvlan, for example) would be dropped until the address list
changes, unless the device is promiscuous for some other reason.

The commit message does say "the address is left without a filter until
its address list changes". Elsewhere the driver retries on -EAGAIN, but a
firmware-busy error here is never retried. The open path also reaches this
code: bnge_cfg_one_usr_fltr() calls it when a user L2 rule cannot be
restored.

A second stale state is not mentioned. Suppose bnge_cfg_rx_mode() chose
promiscuous mode earlier because uc_count > l2_free. Then uc_filter_count
is 1 and this loop does nothing. Deleting user rules frees L2 slots, but
nothing runs the promiscuous decision again. The VNIC stays promiscuous
until some unrelated rx_mode event.

Would it make sense to schedule an rx_mode update after a user L2 rule is
deleted?

[ ... ]

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_hwrm_lib.c b/drivers/net/ethernet/broadcom/bnge/bnge_hwrm_lib.c
> index 33cbdd6d8276f..bb62333514841 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_hwrm_lib.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_hwrm_lib.c
> @@ -906,15 +906,18 @@ int bnge_hwrm_l2_filter_alloc(struct bnge_dev *bd, struct bnge_l2_filter *fltr)

[ ... ]

> -	req->flags = cpu_to_le32(CFA_L2_FILTER_ALLOC_REQ_FLAGS_PATH_RX);
> -
> -	req->flags |= cpu_to_le32(CFA_L2_FILTER_ALLOC_REQ_FLAGS_OUTERMOST);
> +	flags = CFA_L2_FILTER_ALLOC_REQ_FLAGS_PATH_RX |
> +		CFA_L2_FILTER_ALLOC_REQ_FLAGS_OUTERMOST;
> +	if (BNGE_L2_FLTR_IS_NTP_MAC(fltr))
> +		flags |= CFA_L2_FILTER_ALLOC_REQ_FLAGS_TRAFFIC_L2;
> +	req->flags = cpu_to_le32(flags);

[Severity: Medium]
Can RoCE traffic for a MAC end up with no L2 filter at all?

User ETHER_FLOW ring rules now get TRAFFIC_L2. The unicast and default
filters from bnge_hwrm_set_vnic_filter() keep traffic type 0
(CFA_L2_FILTER_ALLOC_REQ_FLAGS_TRAFFIC_NO_ROCE_L2).

When a user rule for MAC X already exists, bnge_alloc_l2_filter() returns
-EEXIST from the software hash lookup. bnge_cfg_rx_mode() then installs no
firmware filter for that address:

    if (rc == -EEXIST) {
        /* Already delivered by a user L2 rule */
        vnic->l2_filters[i] = NULL;
        continue;
    }

In the HWRM spec, type 0 exists for old drivers and covers both RoCE and
L2 traffic, while TRAFFIC_L2 matches L2 traffic only. If that reading is
right, no firmware L2 filter covers RoCE frames to MAC X while the rule
exists. One example is a RoCE GID on a macvlan with the rule's MAC, with
the bnge RoCE auxiliary device present.

The commit message only talks about L2 delivery to the rule's ring.
bnxt_en uses the same HWRM interface and never sets TRAFFIC_L2 on its L2
filters, so this split looks new to bnge.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> index ff6573b5e60ec..94406d5ecb71e 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c

[ ... ]

> @@ -2079,7 +2082,12 @@ static int bnge_cfg_rx_mode(struct bnge_net *bn, struct netdev_hw_addr_list *uc,
>  	netdev_hw_addr_list_for_each(ha, uc)
>  		if (!ether_addr_equal(ha->addr, dev->dev_addr))
>  			uc_count++;
> -	if (uc_count > BNGE_MAX_UC_ADDRS - 1) {
> +	/* The unicast addresses and the user ETHER_FLOW rules share the L2
> +	 * filters. Receive the addresses that do not fit in promiscuous mode.
> +	 */
> +	l2_free = BNGE_MAX_L2_FLTRS -
> +		  bitmap_weight(bn->l2_fltr_bmap, BNGE_MAX_L2_FLTRS);
> +	if (uc_count > BNGE_MAX_L2_FLTRS - 1 || uc_count > l2_free) {
>  		vnic->rx_mask |= CFA_L2_SET_RX_MASK_REQ_MASK_PROMISCUOUS;
>  	} else {
>  		netdev_hw_addr_list_for_each(ha, uc) {
> @@ -2095,6 +2103,11 @@ static int bnge_cfg_rx_mode(struct bnge_net *bn, struct netdev_hw_addr_list *uc,
>  
>  	for (i = 1, off = 0; i < vnic->uc_filter_count; i++, off += ETH_ALEN) {
>  		rc = bnge_hwrm_set_vnic_filter(bn, 0, i, vnic->uc_list + off);
> +		if (rc == -EEXIST) {
> +			/* Already delivered by a user L2 rule */
> +			vnic->l2_filters[i] = NULL;
> +			continue;
> +		}
>  		if (rc) {
>  			if (rc == -EAGAIN)
>  				netdev_warn(dev, "FW busy while setting vnic filter, will retry\n");

[Severity: Medium]
What happens in bnge_cfg_rx_mode() when firmware runs out of L2 filters?

Before this patch, at most 3 secondary unicast addresses were programmed
(BNGE_MAX_UC_ADDRS - 1). Anything beyond that fell back to promiscuous
mode. Now up to 127 can be programmed, but both bounds come only from the
software l2_fltr_bmap. bnge_hwrm_func_resc_qcaps() reads
hw_resc->max_l2_ctxs, but nothing uses it, and the CFA L2 table is shared
across functions.

Suppose firmware rejects a filter partway through this loop with an error
other than -EEXIST, for example HWRM_ERR_CODE_RESOURCE_ALLOC_ERROR, which
maps to -ENOSPC. The function then returns before skip_uc:

            vnic->uc_filter_count = i;
            return rc;

PROMISCUOUS is never set and the rx mask is not reprogrammed. The core
netif_rx_mode retry gives up after 4 attempts. After that, the remaining
addresses have neither a filter nor promiscuous mode, so their traffic
would be dropped silently. With the old 3-address cap this path was
practically unreachable.

Should a failure here fall back to promiscuous mode, or should the limit
take firmware capacity into account?

The commit message also says the driver uses promiscuous mode "as it
already does for more than BNGE_MAX_L2_FLTRS - 1 addresses". The old
threshold was actually BNGE_MAX_UC_ADDRS - 1, which is 3.

This change has two side effects. Every unicast list change now frees and
re-allocates up to 127 firmware filters through HWRM instead of 3. Also,
every struct bnge_vnic_info grows from 4 to 128 l2_filters[] pointers,
including the copy embedded in each struct bnge_rss_ctx, even though only
the default VNIC uses the array.

[ ... ]

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