Re: [PATCH net-next v6 04/10] bnge: add RXFH ethtool support

[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 adds bnge_set_rxfh_fields() (bnge_ethtool.c:856),
  which lets userspace change bd->rss_hash_cfg while the interface is…

--

bnge: add RXFH ethtool support

This commit adds ethtool RXFH operations to bnge for reading and
programming the RSS hash key, indirection table and hash fields. The set
operations are only allowed while the device is down. It also changes the
default RSS key to the system-wide netdev_rss_key_fill() key.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_ethtool.c b/drivers/net/ethernet/broadcom/bnge/bnge_ethtool.c
> index 2467e44de291a..878fe4888160f 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_ethtool.c

[ ... ]

> +static int bnge_set_rxfh_fields(struct net_device *dev,
> +				const struct ethtool_rxfh_fields *cmd,
> +				struct netlink_ext_ack *extack)
> +{

[ ... ]

> +	if (bd->rss_hash_cfg == rss_hash_cfg)
> +		return 0;
> +
> +	if (netif_running(dev)) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "RSS configuration can only be changed while the interface is down");
> +		return -EBUSY;
> +	}
> +
> +	bd->rss_hash_cfg = rss_hash_cfg;
> +
> +	return 0;

[Severity: Medium]
Could the hash fields set here be silently replaced at the next open on
firmware that advertises BNGE_RSS_CAP_RSS_HASH_TYPE_DELTA?

The new value is only stored in bd->rss_hash_cfg and 0 is returned at
once. Firmware first sees it on open, via
bnge_open_core()->bnge_init_chip()->bnge_setup_vnic()->
bnge_hwrm_vnic_set_rss(). That path always sends the full state, with no
INCLUDE/EXCLUDE flag:

drivers/net/ethernet/broadcom/bnge/bnge_hwrm_lib.c:__bnge_hwrm_vnic_set_rss() {
    ...
	req->hash_type = cpu_to_le32(bd->rss_hash_cfg);
    ...
}

Right after that, bnge_init_chip() reads the value back from firmware and
writes it over the stored one:

drivers/net/ethernet/broadcom/bnge/bnge_netdev.c:bnge_init_chip() {
    ...
	if (bd->rss_cap & BNGE_RSS_CAP_RSS_HASH_TYPE_DELTA)
		bnge_hwrm_update_rss_hash_cfg(bn);
    ...
}

drivers/net/ethernet/broadcom/bnge/bnge_hwrm_lib.c:bnge_hwrm_update_rss_hash_cfg() {
    ...
	if (!bnge_hwrm_req_send(bd, req))
		bd->rss_hash_cfg =
			le32_to_cpu(resp->hash_type) ?: bd->rss_hash_cfg;
    ...
}

bnxt uses the same HWRM interface, and it switches to delta semantics
whenever this capability is set:

  bnxt_set_dflt_rss_hash_type() sets rss_hash_delta = rss_hash_cfg
  bnxt_set_rxfh_fields() sets rss_hash_delta = old ^ new
  __bnxt_hwrm_vnic_set_rss() sends HASH_TYPE_INCLUDE/EXCLUDE

If DELTA-mode firmware does not apply a plain full-state hash_type as
requested, would the user's "ethtool -N <dev> rx-flow-hash ..." setting
be dropped at the next "ip link set up"? In that case there is no error
or extack warning, and bd->rss_hash_cfg quietly takes on firmware's
value.

Before this patch the full-state send and the readback already existed,
but nothing user-driven changed rss_hash_cfg. Later patches in the series
don't seem to add INCLUDE/EXCLUDE handling or a check of the readback
against the requested value.

Should __bnge_hwrm_vnic_set_rss() use INCLUDE/EXCLUDE when
BNGE_RSS_CAP_RSS_HASH_TYPE_DELTA is set? Alternatively, should the
readback be checked against the requested configuration?

> +
> +err_l3_shared:
> +	NL_SET_ERR_MSG_MOD(extack,
> +			   "2-tuple hashing is shared by all flows of an IP version, use ip4/ip6 to disable it");
> +	return -EINVAL;
> +}

[ ... ]

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