Re: [PATCH net-next v6 02/10] bnge: add NTUPLE/ARFS VNIC

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0

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] This patch makes the optional NTUPLE/aRFS VNIC a hard probe
  requirement, and nothing degrades gracefully when firmware grants too…

--

bnge: add NTUPLE/ARFS VNIC

This patch adds a second VNIC for NTUPLE/aRFS. The VNIC is always
allocated, so the function now needs two VNICs and twice the RSS
contexts. NTUPLE is turned on by default at netdev allocation time, and
bnge_rings_ok() now fails the reservation when fewer than two VNICs, or
too few RSS contexts for both, are granted.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_resc.c b/drivers/net/ethernet/broadcom/bnge/bnge_resc.c
> index 21f55ea056f4f..4f4c0a2f40bd2 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_resc.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_resc.c

[ ... ]

>  static u16 bnge_get_total_vnics(struct bnge_dev *bd)
>  {
> -	return 1;
> +	return BNGE_BASE_VNICS;
>  }

[ ... ]

> @@ -222,8 +226,12 @@ static void bnge_copy_reserved_rings(struct bnge_dev *bd,
>  
>  static bool bnge_rings_ok(struct bnge_hw_rings *hwr)
>  {
> -	return hwr->tx && hwr->rx && hwr->nq && hwr->grp && hwr->vnic &&
> -	       hwr->stat && hwr->cmpl;
> +	/* The default and the NTUPLE VNICs each need a full set of RSS
> +	 * contexts for the final number of ring groups.
> +	 */
> +	return hwr->tx && hwr->rx && hwr->nq && hwr->grp &&
> +	       hwr->vnic >= BNGE_BASE_VNICS && hwr->stat && hwr->cmpl &&
> +	       hwr->rss_ctx >= bnge_cal_nr_rss_ctxs(hwr->grp) * BNGE_BASE_VNICS;
>  }

[ ... ]

> @@ -327,6 +338,10 @@ int bnge_reserve_rings(struct bnge_dev *bd)
>  	bd->rx_nr_rings = rx_rings;
>  	bd->nq_nr_rings = hwr.nq;
>  
> +	/* One ring group per rx ring, so rss_ctx is checked against the final
> +	 * rx ring count.
> +	 */
> +	hwr.grp = rx_rings;
>  	if (!bnge_rings_ok(&hwr))
>  		return -ENOMEM;

[Severity: Medium]
Can a partial grant of RSS contexts or VNICs now stop the device from
probing?

bnge_get_total_vnics() always returns BNGE_BASE_VNICS, whatever the
NETIF_F_NTUPLE / BNGE_NET_EN_NTUPLE state. bnge_rings_ok() now requires
vnic >= 2 and rss_ctx >= 2 * bnge_cal_nr_rss_ctxs(grp). Before this
patch it only needed a non-zero vnic and did not check rss_ctx.

Earlier in bnge_reserve_rings(), rx_rings is trimmed by hwr.grp, by the
stat/nq limits and by bnge_adjust_rings():

	rx_rings = min_t(u16, rx_rings, hwr.grp);

However, rx_rings is never trimmed to what the granted resv_rsscos_ctxs
can cover. There is also no fallback that drops the NTUPLE VNIC.

The default ring count from _bnge_get_max_rings() also ignores
max_rsscos_ctxs and max_vnics:

	*max_rx = min_t(int, *max_rx, BNGE_MAX_RSS_TABLE_ENTRIES);

For example, with 100 default rx rings, bnge_cal_nr_rss_ctxs(100) is 2,
so 4 contexts are needed. If firmware grants 2 or 3, this worked before
the patch (one VNIC, rss_ctx not checked). Now the path is:

bnge_net_init_dflt_rings()
  bnge_reserve_rings()
    bnge_rings_ok() returns false
    return -ENOMEM

bnge_net_init_dflt_rings() then sets the tx/rx ring counts to 0 and
returns the error, so probe fails. With rx_rings trimmed to 64, only 2
contexts would have been needed.

A function that gets only one VNIC would also stop probing, even though
NTUPLE is optional and can be turned off.

The commit message says:

    If it grants fewer, bnge_reserve_rings() fails with -ENOMEM and the
    device does not probe, as it would for any other missing ring
    resource.

Is this accurate? The other resources are trimmed to a partial grant
and only fail when the grant is zero. A partial RSS context shortfall
fails outright.

For comparison, bnxt requests the NTUPLE VNIC only when RFS is enabled.
It also clears NTUPLE through bnxt_rfs_capable() when resources are
short. Would something similar work here? Another option would be to
trim rx_rings to what the granted RSS contexts can cover.

This also looks unchanged at the end of the series. The later
bnge_rss_ctx_capable() only covers additional user RSS contexts, and
bnge_rings_ok() and _bnge_get_max_rings() are not changed.

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