Re: [PATCH net 09/13] ice: fix setting RSS VSI hash for E830

[Jacob Keller <jacob.e.keller@intel.com>]

On 5/4/2026 10:14 PM, Jacob Keller wrote:
> From: Marcin Szycik <marcin.szycik@linux.intel.com>
> 
> ice_set_rss_hfunc() performs a VSI update, in which it sets hashing
> function, leaving other VSI options unchanged. However, ::q_opt_flags is
> mistakenly set to the value of another field, instead of its original
> value, probably due to a typo. What happens next is hardware-dependent:
> 
> On E810, only the first bit is meaningful (see
> ICE_AQ_VSI_Q_OPT_PE_FLTR_EN) and can potentially end up in a different
> state than before VSI update.
> 
> On E830, some of the remaining bits are not reserved. Setting them
> to some unrelated values can cause the firmware to reject the update
> because of invalid settings, or worse - succeed.
> 
> Reproducer:
>   sudo ethtool -X $PF1 equal 8
> 
> Output in dmesg:
>   Failed to configure RSS hash for VSI 6, error -5
> 
> Fixes: 352e9bf23813 ("ice: enable symmetric-xor RSS for Toeplitz hash function")
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index 1d1947a7fe11..c52c465280f7 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -8046,7 +8046,7 @@ int ice_set_rss_hfunc(struct ice_vsi *vsi, u8 hfunc)
>  	ctx->info.q_opt_rss |=
>  		FIELD_PREP(ICE_AQ_VSI_Q_OPT_RSS_HASH_M, hfunc);
>  	ctx->info.q_opt_tc = vsi->info.q_opt_tc;
> -	ctx->info.q_opt_flags = vsi->info.q_opt_rss;
> +	ctx->info.q_opt_flags = vsi->info.q_opt_flags;
>  
>  	err = ice_update_vsi(hw, vsi->idx, ctx, NULL);
>  	if (err) {
> 

Sashiko complains about ice_set_rss_hfunc() but it is unrelated to this fix:

> While looking at this function, I noticed a pre-existing issue regarding the
> hardware cache. Does calling ice_update_vsi() with a local context leave the
> global hw->vsi_ctx[vsi->idx] out of sync?
> If ice_update_vsi() succeeds, vsi->info.q_opt_rss is updated, but
> hw->vsi_ctx[vsi->idx]->info.q_opt_rss is not.
> When an unrelated feature such as RDMA filtering is subsequently toggled via
> ice_cfg_rdma_fltr(), could it retrieve this stale cached context via
> ice_get_vsi_ctx() and copy the stale q_opt_rss value back into its command
> buffer?
> Could this cause the firmware to silently revert the RSS hash function to its
> previous configuration during the next ice_update_vsi() call?
> There also appears to be a pre-existing issue with state desynchronization
> when handling failures. After ice_update_vsi() succeeds, vsi->rss_hfunc
> is updated, but ice_set_rss_hfunc() then calls ice_set_rss_cfg_symm() at the
> end of the function:
> ice_set_rss_hfunc() {
>         ...
>         err = ice_update_vsi(hw, vsi->idx, ctx, NULL);
>         if (err) {
>                 ...
>         } else {
>                 vsi->info.q_opt_rss = ctx->info.q_opt_rss;
>                 vsi->rss_hfunc = hfunc;
>                 ...
>         }
>         ...
>         return ice_set_rss_cfg_symm(hw, vsi, symm);
> }
> If ice_set_rss_cfg_symm() fails, the error is returned but vsi->rss_hfunc
> remains updated. If the user retries the command, the early check:
> if (hfunc == vsi->rss_hfunc)
>         return 0;
> evaluates to true and returns success immediately.
> Could this skip the ice_set_rss_cfg_symm() retry entirely, leaving the
> hardware's flow director rules in a non-symmetric state permanently while
> falsely reporting success?

Someone from the ice team should look into this and determine whether or
not its valid.