Re: [net-next, v4 03/10] bnge: add helper functions for multi RSS contexts

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 3 · 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] bnge_get_total_vnics()
  (drivers/net/ethernet/broadcom/bnge/bnge_resc.c) adds bn->num_rss_ctx…
- [Medium] At this commit, bnge_modify_rss() writes a user context's
  indirection table into ethtool_rxfh_context_indir(ctx), but…
- [Medium] For a user context, bnge_modify_rss() copies tbl_size entries
  and then zero-pads up to bd->rss_indir_tbl_entries (512) into…

--

bnge: add helper functions for multi RSS contexts

This commit adds helpers to create, modify, restore and delete
user-defined RSS contexts in the bnge driver, with one VNIC per context.
It also adds a num_rss_ctx counter to struct bnge_net and includes it in
the VNIC count that bnge_get_total_vnics() returns.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_resc.c b/drivers/net/ethernet/broadcom/bnge/bnge_resc.c
> index c7024e75f9257..e0744e59c6aea 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_resc.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_resc.c
> @@ -209,8 +209,16 @@ static u16 bnge_get_total_rss_ctxs(struct bnge_dev *bd,
>  
>  static u16 bnge_get_total_vnics(struct bnge_dev *bd)
>  {
> -	if (bnge_is_arfs_cap(bd))
> -		return 2;
> +	if (bnge_is_arfs_cap(bd)) {
> +		u16 vnics = 2;
> +
> +		if (bd->netdev) {
> +			struct bnge_net *bn = netdev_priv(bd->netdev);
> +
> +			vnics += bn->num_rss_ctx;
> +		}
> +		return vnics;
> +	}
>  
>  	return 1;
>  }

[Severity: Medium]
bn->num_rss_ctx is only counted while bnge_is_arfs_cap(bd) is true. Can
that flag be cleared while user RSS contexts still exist?

bnge_init_chip() clears it when the aRFS VNIC allocation fails, and it
does not check netdev->ethtool->rss_ctx first:

drivers/net/ethernet/broadcom/bnge/bnge_netdev.c:bnge_init_chip() {
    ...
	if ((bn->priv_flags & BNGE_NET_EN_NTUPLE) && bnge_is_arfs_cap(bd)) {
		rc = bnge_alloc_rfs_vnic(bn);
		if (rc) {
			...
			bd->flags &= ~BNGE_EN_ARFS_CAP;
			bn->priv_flags &= ~BNGE_NET_EN_NTUPLE;
			bn->netdev->features &= ~NETIF_F_NTUPLE;
		}
	}
    ...
}

After this, bnge_get_total_vnics() returns 1. bnge_need_reserve_rings()
and bnge_reserve_rings() then ask firmware for one VNIC and one VNIC's
worth of RSS contexts. Each remaining user context still needs its own
VNIC.

On the next bnge_open_core(), bnge_reserve_rings() runs before
bnge_hwrm_realloc_rss_ctx_vnic(). Could the context VNIC allocations
fail at that point? Each context would then be torn down through
bnge_del_one_rss_ctx() and ethtool_rxfh_context_lost(). I haven't
confirmed that firmware enforces the lower reservation on later VNIC
allocs, so this step is partly speculative.

Also, bnge_init_chip() clears BNGE_NET_EN_NTUPLE directly. That bypasses
the -EBUSY check that later patches in the series add to
bnge_set_features(). That check stops NTUPLE from being disabled while
bn->num_rss_ctx is non-zero.

The capability is only set again in bnge_net_init_dflt_config(). While
it is clear, bnge_fix_features() strips NETIF_F_NTUPLE. So the remaining
contexts can't be modified, because bnge_rxfh_context_check() requires
BNGE_NET_EN_NTUPLE. Their VNICs also drop out of resource accounting.
This is still the case at the end of the series.

Should num_rss_ctx be counted whether or not aRFS is available? Or
should the aRFS fallback in bnge_init_chip() take existing contexts into
account?

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_vnic.c b/drivers/net/ethernet/broadcom/bnge/bnge_vnic.c
> index d98a6196c859c..a44971a7c74d3 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_vnic.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_vnic.c
> @@ -129,3 +129,116 @@ int bnge_alloc_rfs_vnic(struct bnge_net *bn)
>  	vnic = &bn->vnic_info[BNGE_VNIC_NTUPLE];
>  	return bnge_alloc_and_setup_vnic(bn, vnic, bn->bd->rx_nr_rings);
>  }
> +
> +void bnge_modify_rss(struct bnge_net *bn, struct ethtool_rxfh_context *ctx,
> +		     struct bnge_rss_ctx *rss_ctx,
> +		     const struct ethtool_rxfh_param *rxfh)
> +{

[ ... ]

> +	if (rxfh->indir) {
> +		u32 i, pad, tbl_size = bnge_get_rxfh_indir_size(bd);
> +		u32 *indir_tbl = bd->rss_indir_tbl;
> +
> +		if (rss_ctx)
> +			indir_tbl = ethtool_rxfh_context_indir(ctx);

[Severity: Medium]
At this commit, is anything reading the per-context table written here?

For non-NTUPLE VNICs, bnge_fill_hw_rss_tbl() only reads the default
table:

    j = bd->rss_indir_tbl[i];

struct bnge_vnic_info also has no link back to the ctx. Used as they are
in this patch, wouldn't bnge_hwrm_realloc_rss_ctx_vnic()->bnge_setup_vnic()
program the default table into a context VNIC?

The helpers have no callers yet. The next patch, "bnge: add ethtool
support to manage RSS contexts", adds BNGE_VNIC_RSSCTX_FLAG and a
vnic->rss_ctx back-pointer. It also changes bnge_fill_hw_rss_tbl() to
read ethtool_rxfh_context_indir(vnic->rss_ctx). Could the reader side
move into this patch so these helpers are consistent on their own?

> +		for (i = 0; i < tbl_size; i++)
> +			indir_tbl[i] = rxfh->indir[i];
> +		pad = bd->rss_indir_tbl_entries - tbl_size;
> +		if (pad)
> +			memset(&indir_tbl[i], 0, pad * sizeof(*indir_tbl));

[Severity: Medium]
Can this memset() overflow the ctx indirection table when rss_ctx is
non-NULL?

The pad runs up to bd->rss_indir_tbl_entries, which
bnge_alloc_rss_indir_tbl() sets to BNGE_MAX_RSS_TABLE_ENTRIES. That is
the device-wide capacity, not the size of the ctx allocation.

ethtool_rxfh_ctx_alloc() sizes the ctx indirection area as
max(indir_size, ops->rxfh_indir_space). bnge doesn't set rxfh_indir_space
at this commit. The pad would then write past the indirection table,
into the key area and beyond the end of the kzalloc'd ctx.

This can't be reached yet because bnge_modify_rss() has no caller. Later
in the series the pad is guarded with "if (pad && !rss_ctx)", and
bnge_ethtool_ops sets .rxfh_indir_space = BNGE_MAX_RSS_TABLE_ENTRIES.
Could those changes be folded into this patch?

> +	}
> +}

[ ... ]

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