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

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

On 5/7/2026 9:59 AM, Marcin Szycik wrote:
> 
> 
> On 07.05.2026 13:47, Marcin Szycik wrote:
>>
>>
>> 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.
>>

Ok.

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

Makes sense. Its probably better to leave it as-is for now. Either way,
I think these shouldn't block the obvious fix.

>>>> 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.
> 
> On second thought, if we decide to rollback changes to VSI on ice_set_rss_cfg_symm()
> fail, we must call ice_update_vsi(), which then can also fail, still leaving us with
> hfunc programmed and symmetry not set. I'm not sure if it's worth adding rollback
> that can fail and still leave us with the original problem. User would just see 2 errors
> instead of 1.
>> Thanks,
>> Marcin
>>


Either way, its a pre-existing issue that could warrant an investigation
and a patch, not a reason to modify or delay this bug fix.

Thanks,
Jake


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