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

[Marcin Szycik <marcin.szycik@linux.intel.com>]

On 06.05.2026 23:06, Jacob Keller wrote:
> 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?

Yes.

>> Could this cause the firmware to silently revert the RSS hash function to its
>> previous configuration during the next ice_update_vsi() call?

No, because the context object passed to ice_update_vsi() only sets
ctx->info.valid_sections for the sections it wants to update, so unrelated
values are not updated in HW.

Looking at other ice_update_vsi() calls, most of the times the context object is
being allocated, not taken from cache. It's not immediately clear to me what
purpose does hw->vsi_ctx[] serve - it only appears to be used in
ice_cfg_rdma_fltr() (correct me if I'm wrong), where options from the cached
context are being read to fill the unchanged fields in the updated section.
This seems to be the equivalent of keeping track of context values in vsi->info,
which is what almost all ice_update_vsi() callers do.
If I had to guess, I'd say hw->vsi_ctx[] could probably be removed and vsi->info
used instead, but maybe I'm missing something.

TLDR I think this is just old, inconsistent code that could be improved, but it
needs some investigation. Until we don't have a clear signal that there's a bug,
I wouldn't touch it.

>> 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?

This looks valid.

Thanks,
Marcin

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