From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 9F1AE31F983; Thu, 7 May 2026 16:59:23 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=192.198.163.12 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778173167; cv=none; b=cnHD7Hk7eciFRnhAAX9S+GA4q51p04FmnEZicPlEGQD/vEkuR1X703OSVKGsgpw9//SX5nBBiuVPikHOfu1xinf9awvcjuJL+ttO/CZrAGx46nbCzyii2H9hBg7NVMG07SFxFXd8WPHSbgMAgZSR1VVoRYEeOHr1nSqhLWFH9/s= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778173167; c=relaxed/simple; bh=pazXxEd7A1cd1sWGeu/YHbddxLF5r07GBKYjKOW2Pzc=; h=Message-ID:Date:MIME-Version:Subject:From:To:Cc:References: In-Reply-To:Content-Type; b=Pippnx6hyCVA2bzwIVT1egb35y5UJB5caFDhuqbXZgeGpJlJz6BsTKC+nhBEurlLhcHpEZGTGN9jScY0FllAmUXmKJbyAzjPx+5HivEx7FEGhVfdSQ9EPqDoTvqcX0gbmOc2wilEAM1jDGqWZUpjt0vPHb9BswmmeNV2HL7/gr4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com; spf=pass smtp.mailfrom=linux.intel.com; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b=mLZ+/sT4; arc=none smtp.client-ip=192.198.163.12 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b="mLZ+/sT4" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1778173163; x=1809709163; h=message-id:date:mime-version:subject:from:to:cc: references:in-reply-to:content-transfer-encoding; bh=pazXxEd7A1cd1sWGeu/YHbddxLF5r07GBKYjKOW2Pzc=; b=mLZ+/sT4r6BRCyRVo8iiS89p4A832B5LkLnGF/VNpITpd45T8YBJavrr imw5kTRI94dRoujppDiM1R5bSsy6/rAmIwR+ITF76F7vIr6eHoMOl18/e Vhd2r+eXBpO+0qiieBtOZR2GsVQJLU/MaevK6aqo1ClWyYyDaWI2n9o6/ SX7w3Q0oNY37WcxgGWDYzrBF2moWrXx2zngg5ASNh0JB0z52iCFqRJDLL /ZSnw/OuG7IL7ZjE/vAZ85SYGRTTuNv/wzjDqgmsYeIufbzMHKMuUrYCs vZFF9xqVhq5k8Dl4196h+uorJaN7DuyA6sKz9gFFoASoobDYCMEryQYtO Q==; X-CSE-ConnectionGUID: c8rJnQjfR9ux+o3jqGUflw== X-CSE-MsgGUID: N6aEvWlET5ywoTap9Jpaww== X-IronPort-AV: E=McAfee;i="6800,10657,11779"; a="82975389" X-IronPort-AV: E=Sophos;i="6.23,221,1770624000"; d="scan'208";a="82975389" Received: from fmviesa002.fm.intel.com ([10.60.135.142]) by fmvoesa106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 May 2026 09:59:18 -0700 X-CSE-ConnectionGUID: A/1g4vxpRaeXQt2eYX0VBw== X-CSE-MsgGUID: 1zqVMEuqTBCtM0Sus+wYDw== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.23,221,1770624000"; d="scan'208";a="259952125" Received: from mszycik-mobl1.ger.corp.intel.com (HELO [10.246.20.168]) ([10.246.20.168]) by fmviesa002-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 May 2026 09:59:14 -0700 Message-ID: <3abb7d8f-82eb-46e8-8243-fc6f596ab84c@linux.intel.com> Date: Thu, 7 May 2026 18:59:11 +0200 Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH net 09/13] ice: fix setting RSS VSI hash for E830 From: Marcin Szycik To: Jacob Keller , Przemek Kitszel , Andrew Lunn , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Piotr Kwapulinski , Aleksandr Loktionov , Arkadiusz Kubalewski , Maciej Fijalkowski , Joshua Hay , Madhu Chittim , Willem de Bruijn , Dave Ertman , Ivan Vecera , Grzegorz Nitka Cc: netdev@vger.kernel.org, stable@vger.kernel.org References: <20260504-jk-iwl-net-2026-05-04-v1-0-a222a88bd962@intel.com> <20260504-jk-iwl-net-2026-05-04-v1-9-a222a88bd962@intel.com> <068a5266-4721-4496-b027-3b32da3e02ea@intel.com> <56e52628-d029-4919-95dd-aa6da13f3b08@linux.intel.com> Content-Language: en-US In-Reply-To: <56e52628-d029-4919-95dd-aa6da13f3b08@linux.intel.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit 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 >>> >>> 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 >>> Reviewed-by: Przemek Kitszel >>> Signed-off-by: Marcin Szycik >>> Signed-off-by: Jacob Keller >>> --- >>> 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. 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 > >> Someone from the ice team should look into this and determine whether or >> not its valid.