Re: [net-next, v4 02/10] bnge: add NTUPLE/ARFS VNIC

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 5

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_alloc_and_setup_vnic() (bnge_vnic.c) allocates the NTUPLE
  VNIC in firmware with bnge_hwrm_vnic_alloc() and then calls…
- [Medium] Any bnge_alloc_rfs_vnic() failure during open, including a
  transient HWRM or firmware error, makes bnge_init_chip() clear…
- [Low] At this commit, bnge_set_dflt_rfs() adds NETIF_F_NTUPLE to
  hw_features and features, and ndo_fix_features/ndo_set_features let…
- [Low] The reservation side and the allocation side use different
  conditions.
- [Low] On aRFS VNIC allocation failure, bnge_init_chip() writes directly
  to bn->netdev->features (&= ~NETIF_F_NTUPLE) from the ndo_open path.
- [Low] The patch adds the first runtime read-modify-write of bd->flags
  on the ndo_open path: 'bd->flags &= ~BNGE_EN_ARFS_CAP' in…
- [Low] bnge_arfs_capable() (bnge_resc.c) sends HWRM_FUNC_CFG through
  bnge_hwrm_reserve_rings() with only vnic and rss_ctx set, so…

--

bnge: add NTUPLE/ARFS VNIC

This commit adds a second VNIC for NTUPLE/aRFS. On aRFS-capable devices
it reserves the extra VNIC and RSS contexts and turns NETIF_F_NTUPLE on
by default. It also adds ndo_fix_features and ndo_set_features, so
NTUPLE can be changed only while the interface is down.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> index 9e64b1933c023..c6b9048586b36 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_netdev.c

[ ... ]

> @@ -2546,6 +2548,18 @@ static int bnge_init_chip(struct bnge_net *bn)
>  	if (rc)
>  		goto err_out;
>  
> +	if ((bn->priv_flags & BNGE_NET_EN_NTUPLE) && bnge_is_arfs_cap(bd)) {
> +		rc = bnge_alloc_rfs_vnic(bn);
> +		if (rc) {
> +			netdev_warn(bn->netdev,
> +				    "Failed to allocate aRFS VNIC (%d), disabling ARFS\n",
> +				    rc);
> +			bd->flags &= ~BNGE_EN_ARFS_CAP;

[Severity: Medium]
Does clearing BNGE_EN_ARFS_CAP here turn a transient firmware error into
a permanent loss of the feature?

The only code that sets this bit is in bnge_net_init_dflt_config(), and
that function is only reached from bnge_probe_one():

    if (bnge_arfs_capable(bd, false))
        bd->flags |= BNGE_EN_ARFS_CAP;

After one failed open, bnge_fix_features() drops NTUPLE from every later
request:

    } else if ((features & NETIF_F_NTUPLE) && !bnge_is_arfs_cap(bn->bd)) {
        features &= ~NETIF_F_NTUPLE;

hw_features still advertises NETIF_F_NTUPLE. So "ethtool -K <dev> ntuple
on" silently does nothing until the driver is reloaded.

This fallback also leaves bn->nr_vnics at 2, and vnic_info[1] may be only
half configured. Later in bnge_init_chip(), bnge_set_tpa() still walks
that VNIC. If HWRM_VNIC_TPA_CFG fails there, the code takes goto err_out.

Could that fail the same open this fallback is meant to save? That part
depends on how the firmware treats the half-configured VNIC.

[Severity: Low]
Can this read-modify-write of bd->flags race with
bnge_rdma_aux_device_add()?

bnge_probe_one() registers the netdev in bnge_netdev_alloc() before it
calls bnge_rdma_aux_device_add(). If auxiliary_device_add() fails, that
path does this with no lock held:

    auxiliary_device_uninit(aux_dev);
    bd->flags &= ~BNGE_EN_ROCE;

The update here runs under rtnl and the netdev instance lock, so the two
writers share no lock. Userspace can open the new netdev at the same
time. If both paths fail together, one of the two plain u64 updates can
be lost. For example, BNGE_EN_ARFS_CAP could reappear after open decided
to disable aRFS.

Before this patch, bd->flags was only written during probe or on that aux
path, so this race is new. The window is small and needs two failures at
the same time.

> +			bn->priv_flags &= ~BNGE_NET_EN_NTUPLE;
> +			bn->netdev->features &= ~NETIF_F_NTUPLE;

[Severity: Low]
Is it fine to write netdev->features directly from the ndo_open path?

Documentation/networking/netdev-features.rst says netdev->features
"should be changed only by network core or in error paths of
ndo_set_features callback".

The direct write skips the feature recalculation and the
NETDEV_FEAT_CHANGE notification. The netdev_update_features() call added
to bnge_close() then computes the same set, which is already cleared. So
no notification is ever sent for this on-to-off change.

Would calling netdev_update_features() here work instead? ndo_open
already holds rtnl and the instance lock.

> +		}
> +	}
> +

[ ... ]

> @@ -3233,6 +3285,18 @@ static void bnge_init_ring_params(struct bnge_net *bn)
>  	bn->netdev->cfg->hds_thresh = max(BNGE_DEFAULT_RX_COPYBREAK, rx_size);
>  }
>  
> +static void bnge_set_dflt_rfs(struct bnge_net *bn)
> +{
> +	bn->netdev->hw_features &= ~NETIF_F_NTUPLE;
> +	bn->netdev->features &= ~NETIF_F_NTUPLE;
> +	bn->priv_flags &= ~BNGE_NET_EN_NTUPLE;
> +	if (bnge_is_arfs_cap(bn->bd)) {
> +		bn->netdev->hw_features |= NETIF_F_NTUPLE;
> +		bn->netdev->features |= NETIF_F_NTUPLE;
> +		bn->priv_flags |= BNGE_NET_EN_NTUPLE;
> +	}
> +}

[Severity: Low]
With this patch, NETIF_F_NTUPLE is advertised and on by default, and it
can be toggled through ndo_fix_features and ndo_set_features. Is the code
behind the feature there yet?

At this commit:

- bnge_netdev_ops has no ndo_rx_flow_steer.
- rx_cpu_rmap is never allocated.
- bnge_ethtool_ops has no get_rxnfc or set_rxnfc.

So "ntuple-filters: on" is reported and "ethtool -N" returns -EOPNOTSUPP.
An extra firmware VNIC and extra RSS contexts are also reserved for a
feature that does nothing yet.

Later patches in the series fill this in:

- "bnge: add NTUPLE filter support in ethtool" adds get_rxnfc and
  set_rxnfc.
- "bnge: add aRFS flow steering ndo support" adds ndo_rx_flow_steer.
- "bnge: add cpu_rmap support for IRQ affinity" allocates rx_cpu_rmap
  after that.

Since rx_cpu_rmap only shows up after ndo_rx_flow_steer exists, nothing
can crash in the meantime. Would it make sense to advertise
NETIF_F_NTUPLE only once that code is in place?

[ ... ]

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_resc.c b/drivers/net/ethernet/broadcom/bnge/bnge_resc.c
> index 69a894b52485e..c7024e75f9257 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_resc.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_resc.c

[ ... ]

> @@ -190,11 +200,18 @@ int bnge_cal_nr_rss_ctxs(u16 rx_rings)
>  static u16 bnge_get_total_rss_ctxs(struct bnge_dev *bd,
>  				   struct bnge_hw_rings *hwr)
>  {
> -	return bnge_cal_nr_rss_ctxs(hwr->grp);
> +	u16 rss_ctx = bnge_cal_nr_rss_ctxs(hwr->grp);
> +
> +	rss_ctx *= hwr->vnic;
> +
> +	return rss_ctx;
>  }
>  
>  static u16 bnge_get_total_vnics(struct bnge_dev *bd)
>  {
> +	if (bnge_is_arfs_cap(bd))
> +		return 2;
> +
>  	return 1;
>  }

[Severity: Low]
Should this check BNGE_NET_EN_NTUPLE rather than BNGE_EN_ARFS_CAP?

The reservation side keys on the capability. On capable devices,
bnge_reserve_rings() and bnge_need_reserve_rings() always ask for 2
VNICs, and bnge_get_total_rss_ctxs() doubles the RSS context count.

The allocation side keys on the flag the user controls:

bnge_alloc_vnics():
    if (bn->priv_flags & BNGE_NET_EN_NTUPLE)
        num_vnics++;

After "ethtool -K <dev> ntuple off" with the interface down, the next
open still reserves 2 VNICs and twice the RSS contexts in firmware, but
uses only one VNIC. For comparison, bnxt_get_total_vnics() keys on the
enabled flag BNXT_FLAG_RFS.

[ ... ]

> @@ -563,6 +580,44 @@ static int bnge_alloc_rss_indir_tbl(struct bnge_dev *bd)
>  	return 0;
>  }
>  
> +/* If runtime conditions support RFS */
> +bool bnge_arfs_capable(struct bnge_dev *bd, bool new_rss_ctx)
> +{

[ ... ]

> +	if (hwr.vnic <= bd->hw_resc.resv_vnics &&
> +	    hwr.rss_ctx <= bd->hw_resc.resv_rsscos_ctxs)
> +		return true;
> +
> +	bnge_hwrm_reserve_rings(bd, &hwr);
> +	if (hwr.vnic <= bd->hw_resc.resv_vnics &&
> +	    hwr.rss_ctx <= bd->hw_resc.resv_rsscos_ctxs)
> +		return true;
> +
> +	dev_warn(bd->dev, "Unable to reserve resources to support NTUPLE filters\n");
> +	return false;
> +}

[Severity: Low]
If firmware grants only part of this request, does anything release the
extra resources?

bnge_hwrm_reserve_rings() is called with only vnic and rss_ctx set, and
its return value is ignored. Suppose firmware grants fewer than 2 VNICs
but all the RSS contexts. This function then warns and returns false,
and the partial grant stays in place.

bnxt_rfs_capable() re-reserves with vnic = 1 on this path. Here,
bnge_need_reserve_rings() checks resv_vnics but never resv_rsscos_ctxs:

    if (hw_resc->resv_rx_rings != rx || hw_resc->resv_cp_rings != cprs ||
        hw_resc->resv_vnics != vnic || hw_resc->resv_stat_ctxs != stat)

So the extra RSS contexts seem to stay reserved to this function
indefinitely.

[ ... ]

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_vnic.c b/drivers/net/ethernet/broadcom/bnge/bnge_vnic.c
> index 3a6c8f0a59541..d98a6196c859c 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_vnic.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_vnic.c

[ ... ]

> +static int bnge_alloc_and_setup_vnic(struct bnge_net *bn,
> +				     struct bnge_vnic_info *vnic,
> +				     u16 rx_rings)
> +{
> +	int rc;
> +
> +	rc = bnge_hwrm_vnic_alloc(bn->bd, vnic, rx_rings);
> +	if (rc) {
> +		netdev_err(bn->netdev, "hwrm vnic %u alloc failure rc: %d\n",
> +			   vnic->vnic_id, rc);
> +		return rc;
> +	}
> +
> +	/* If bnge_setup_vnic() fails, the VNIC allocated above is not freed
> +	 * here; the caller (bnge_init_chip) unwinds via its err_out path.
> +	 */
> +	return bnge_setup_vnic(bn, vnic);
> +}

[Severity: Medium]
Is this comment accurate? In the bnge_init_chip() hunk above, a failure
from bnge_alloc_rfs_vnic() only logs a warning and clears
BNGE_EN_ARFS_CAP, BNGE_NET_EN_NTUPLE and NETIF_F_NTUPLE. Then it carries
on without jumping to err_out.

Suppose bnge_hwrm_vnic_alloc() succeeds and bnge_setup_vnic() fails
partway. The firmware VNIC and any RSS contexts already allocated by
bnge_hwrm_vnic_ctx_alloc() then stay in firmware while the interface is
up.

bn->nr_vnics stays 2, so bnge_set_tpa() still sends HWRM_VNIC_TPA_CFG to
the half-configured vnic_info[1]:

    for (i = 0; i < bn->nr_vnics; i++) {
        rc = bnge_hwrm_vnic_set_tpa(bn->bd, &bn->vnic_info[i], tpa_flags);

bnge_hwrm_vnic_set_tpa() skips a VNIC only when its fw_vnic_id is
INVALID_HW_RING_ID, and this VNIC has a valid ID.

Meanwhile, bnge_get_total_vnics() now returns 1, while firmware holds 2
VNICs and their contexts. bnge_clear_vnic() frees them at close, so they
are not held forever.

Should this function free the VNIC when setup fails, or should
bnge_init_chip() unwind it?

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