Re: [iwl-next,v1] ice: implement symmetric RSS hash configuration

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
ice: implement symmetric RSS hash configuration

The driver advertises symmetric RSS support via supported_input_xfrm
but ice_set_rxfh() ignored the input_xfrm parameter, making it
impossible to configure symmetric hashing. This patch fixes
ice_set_rxfh() to check the input_xfrm parameter and configure
symmetric hashing appropriately, modifies ice_set_rss_vsi_ctx() to use
vsi->rss_hfunc instead of hardcoding values, and updates
ice_get_rxfh_fields() to report symmetric field configurations.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> index c6bc29cfb8e6..6dc37f96b57e 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> @@ -3037,8 +3037,10 @@ ice_get_rxfh_fields(struct net_device *netdev, struct ethtool_rxfh_fields *nfc)
>
>  	hash_flds = ice_get_rss_cfg(&pf->hw, vsi->idx, hdrs, &symm);
>  	if (hash_flds == ICE_HASH_INVALID) {
> -		dev_dbg(dev, "No hash fields found for the given header type, vsi num = %d\n",
> +		/* Provide default symmetric hash fields when no config exists */
> +		dev_dbg(dev, "No RSS config for this flow, using symmetric defaults, vsi num = %d\n",
>  			vsi->vsi_num);
> +		nfc->data = RXH_IP_SRC | RXH_IP_DST | RXH_L4_B_0_1 | RXH_L4_B_2_3;
>  		return 0;
>  	}
>
> @@ -3067,6 +3069,7 @@ ice_get_rxfh_fields(struct net_device *netdev, struct ethtool_rxfh_fields *nfc)
>  	    hash_flds & ICE_FLOW_HASH_FLD_GTPU_DWN_TEID)
>  		nfc->data |= (u64)RXH_GTP_TEID;
>
> +	nfc->data = RXH_IP_SRC | RXH_IP_DST | RXH_L4_B_0_1 | RXH_L4_B_2_3;
                  ^^^^^^^

Does this unconditional assignment at the end of ice_get_rxfh_fields()
overwrite the actual hardware configuration that was just computed?

The hash fields are carefully decoded from the hardware configuration
returned by ice_get_rss_cfg() and built up in nfc->data through the series
of bitwise OR operations. Then this line uses assignment (=) rather than
bitwise OR (|=), replacing all those computed values with a fixed set of
hash fields.

This would make the entire per-field computation from the hardware state
become dead code. The function would always report the same fixed hash
fields (RXH_IP_SRC | RXH_IP_DST | RXH_L4_B_0_1 | RXH_L4_B_2_3) regardless
of what is actually configured in hardware. GTP TEID hash fields would
never be reported even when configured.

Was this intended to be an additional |= operation, or should this line
be removed entirely?

> @@ -3667,7 +3670,6 @@ ice_set_rxfh(struct net_device *netdev, struct ethtool_rxfh_param *rxfh,
>  	     struct netlink_ext_ack *extack)
>  {
>  	struct ice_netdev_priv *np = netdev_priv(netdev);
> -	u8 hfunc = ICE_AQ_VSI_Q_OPT_RSS_HASH_TPLZ;
>  	struct ice_vsi *vsi = np->vsi;
>  	struct ice_pf *pf = vsi->back;
>  	struct device *dev;

[ ... ]

> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
> index d921269e1fe7..31f2757cf7fa 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> @@ -1155,7 +1155,7 @@ static void ice_set_fd_vsi_ctx(struct ice_vsi_ctx *ctxt, struct ice_vsi *vsi)
>   * @ctxt: the VSI context being set
>   * @vsi: the VSI being configured
>   */
> -static void ice_set_rss_vsi_ctx(struct ice_vsi_ctx *ctxt, struct ice_vsi *vsi)
> +void ice_set_rss_vsi_ctx(struct ice_vsi_ctx *ctxt, struct ice_vsi *vsi)
>  {
>  	u8 lut_type, hash_type;
>  	struct device *dev;
> @@ -1181,7 +1181,9 @@ static void ice_set_rss_vsi_ctx(struct ice_vsi_ctx *ctxt, struct ice_vsi *vsi)
>  		return;
>  	}
>
> -	hash_type = ICE_AQ_VSI_Q_OPT_RSS_HASH_TPLZ;
> +        if (!vsi->rss_hfunc)
> +		vsi->rss_hfunc = ICE_AQ_VSI_Q_OPT_RSS_HASH_TPLZ;
> +	hash_type = vsi->rss_hfunc;
>  	vsi->rss_hfunc = hash_type;
>
>  	ctxt->info.q_opt_rss =

[ ... ]

> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.h b/drivers/net/ethernet/intel/ice/ice_lib.h
> index 49454d98dcfe..29ba335dcc54 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lib.h
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.h
> @@ -46,6 +46,7 @@ void ice_vsi_delete(struct ice_vsi *vsi);
>  int ice_vsi_cfg_tc(struct ice_vsi *vsi, u8 ena_tc);
>
>  int ice_vsi_cfg_rss_lut_key(struct ice_vsi *vsi);
> +void ice_set_rss_vsi_ctx(struct ice_vsi_ctx *ctxt, struct ice_vsi *vsi);
>
>  void ice_vsi_cfg_netdev_tc(struct ice_vsi *vsi, u8 ena_tc);
>