From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (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 DFEB21E493 for ; Fri, 31 May 2024 13:18:06 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1717161487; cv=none; b=ucQtI1fJ72lOwj7UWcduuTJ55+EATHuwF31K91XqCQrHtumwyxUg1VQJ4/izPiGb/H9fJx7Gj+TdZVrlJ4rHwF4IhfZWnzfUDWr25b5Am35XUe+bbog7c6YtrR13Yp4pe114rJ1MDyRU4QUTGD/A4XRfNTc41KrpHBIHRuY/BGw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1717161487; c=relaxed/simple; bh=ccYr377cEwPSuGbW2X6NZwvsaM0OkIN44A0NOv2OZn8=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=Ab4lOsO+NLjSK5aNP1Q72tk2CVQ1AxjXshqJk7fkIu4OeeodAmlDyoDg7jCArnfl1BZKQG8sCqLYPXTIeDAdOPR3gpjzA2agKntLYWAWj6LtjHgzDNJhvM3tprS3fTHcI9fGdbG60URO/YnuQdEhmUEFF0VFALmZkPvhvBkn+34= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=R/pLSpKP; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="R/pLSpKP" Received: by smtp.kernel.org (Postfix) with ESMTPSA id E1E00C116B1; Fri, 31 May 2024 13:18:04 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1717161486; bh=ccYr377cEwPSuGbW2X6NZwvsaM0OkIN44A0NOv2OZn8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=R/pLSpKPnORuFK/WmMuBa6OLi/EtMkBm6++1+VFf0oA9z1XGksZttKmjKbC1zzq2A qzLVbmk1/s5H6oPo84MubkpqVYP897+HVjmyRtbnj86Nhei3a/yzlHCm/8py2lzzoF vo2clPGXRN/ufeC1lZkPlzkhuU0RhKS1XtIlCVRy0i0/JRKCmWofPKQkn8b0OFyO5m ZD+/dpW8rSX7GLKLrq7op236QOYSBxU8Zbol5dpakx5zkFH8VwFxJsFDhsRW2MjSdS mpwPZoG08imKqbUGiTr6mUPML8N9H/buFTAfftYgt1IaQPpFnG7skWNkyBE+T9Y6YE 9f+fDBVW9HCMA== Date: Fri, 31 May 2024 14:18:02 +0100 From: Simon Horman To: Ahmed Zaki Cc: intel-wired-lan@lists.osuosl.org, netdev@vger.kernel.org, jacob.e.keller@intel.com, anthony.l.nguyen@intel.com, Junfeng Guo , Marcin Szycik Subject: Re: [PATCH iwl-next v2 11/13] ice: enable FDIR filters from raw binary patterns for VFs Message-ID: <20240531131802.GG123401@kernel.org> References: <20240527185810.3077299-1-ahmed.zaki@intel.com> <20240527185810.3077299-12-ahmed.zaki@intel.com> Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240527185810.3077299-12-ahmed.zaki@intel.com> On Mon, May 27, 2024 at 12:58:08PM -0600, Ahmed Zaki wrote: > From: Junfeng Guo > > Enable VFs to create FDIR filters from raw binary patterns. > The corresponding processes for raw flow are added in the > Parse / Create / Destroy stages. > > Reviewed-by: Marcin Szycik > Signed-off-by: Junfeng Guo > Co-developed-by: Ahmed Zaki > Signed-off-by: Ahmed Zaki ... > diff --git a/drivers/net/ethernet/intel/ice/ice_flow.c b/drivers/net/ethernet/intel/ice/ice_flow.c ... > +/** > + * ice_flow_set_parser_prof - Set flow profile based on the parsed profile info > + * @hw: pointer to the HW struct > + * @dest_vsi: dest VSI > + * @fdir_vsi: fdir programming VSI > + * @prof: stores parsed profile info from raw flow > + * @blk: classification blk > + */ > +int > +ice_flow_set_parser_prof(struct ice_hw *hw, u16 dest_vsi, u16 fdir_vsi, > + struct ice_parser_profile *prof, enum ice_block blk) > +{ > + u64 id = find_first_bit(prof->ptypes, ICE_FLOW_PTYPE_MAX); > + struct ice_flow_prof_params *params __free(kfree); > + u8 fv_words = hw->blk[blk].es.fvw; > + int status; > + int i, idx; > + > + params = kzalloc(sizeof(*params), GFP_KERNEL); > + if (!params) > + return -ENOMEM; params seems to be leaked when this function returns below, in both error and non-error cases. > + > + for (i = 0; i < ICE_MAX_FV_WORDS; i++) { > + params->es[i].prot_id = ICE_PROT_INVALID; > + params->es[i].off = ICE_FV_OFFSET_INVAL; > + } > + > + for (i = 0; i < prof->fv_num; i++) { > + if (hw->blk[blk].es.reverse) > + idx = fv_words - i - 1; > + else > + idx = i; > + params->es[idx].prot_id = prof->fv[i].proto_id; > + params->es[idx].off = prof->fv[i].offset; > + params->mask[idx] = (((prof->fv[i].msk) << BITS_PER_BYTE) & > + HI_BYTE_IN_WORD) | > + (((prof->fv[i].msk) >> BITS_PER_BYTE) & > + LO_BYTE_IN_WORD); > + } > + > + switch (prof->flags) { > + case FLAG_GTPU_DW: > + params->attr = ice_attr_gtpu_down; > + params->attr_cnt = ARRAY_SIZE(ice_attr_gtpu_down); > + break; > + case FLAG_GTPU_UP: > + params->attr = ice_attr_gtpu_up; > + params->attr_cnt = ARRAY_SIZE(ice_attr_gtpu_up); > + break; > + default: > + if (prof->flags_msk & FLAG_GTPU_MSK) { > + params->attr = ice_attr_gtpu_session; > + params->attr_cnt = ARRAY_SIZE(ice_attr_gtpu_session); > + } > + break; > + } > + > + status = ice_add_prof(hw, blk, id, (u8 *)prof->ptypes, > + params->attr, params->attr_cnt, > + params->es, params->mask, false, false); > + if (status) > + return status; > + > + status = ice_flow_assoc_fdir_prof(hw, blk, dest_vsi, fdir_vsi, id); > + if (status) > + ice_rem_prof(hw, blk, id); > + > + return status; > +} ... > diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c b/drivers/net/ethernet/intel/ice/ice_vf_lib.c > index 5635e9da2212..9138f7783da0 100644 > --- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c > +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c > @@ -1,8 +1,8 @@ > // SPDX-License-Identifier: GPL-2.0 > /* Copyright (C) 2022, Intel Corporation. */ > > -#include "ice_vf_lib_private.h" > #include "ice.h" > +#include "ice_vf_lib_private.h" > #include "ice_lib.h" > #include "ice_fltr.h" > #include "ice_virtchnl_allowlist.h" To me tweaking the order of includes seems to indicate that something isn't quite right. Is there some sort of dependency loop being juggled here? > diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.h b/drivers/net/ethernet/intel/ice/ice_vf_lib.h > index fec16919ec19..be4266899690 100644 > --- a/drivers/net/ethernet/intel/ice/ice_vf_lib.h > +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.h > @@ -12,6 +12,7 @@ > #include > #include > #include "ice_type.h" > +#include "ice_flow.h" > #include "ice_virtchnl_fdir.h" > #include "ice_vsi_vlan_ops.h" > ... > diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c b/drivers/net/ethernet/intel/ice/ice_virtchnl.c > index 1c6ce0c4ed4e..886869648c91 100644 > --- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c > +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c > @@ -1,9 +1,9 @@ > // SPDX-License-Identifier: GPL-2.0 > /* Copyright (C) 2022, Intel Corporation. */ > > +#include "ice.h" > #include "ice_virtchnl.h" > #include "ice_vf_lib_private.h" > -#include "ice.h" > #include "ice_base.h" > #include "ice_lib.h" > #include "ice_fltr.h" ... > @@ -784,6 +798,106 @@ ice_vc_fdir_config_input_set(struct ice_vf *vf, struct virtchnl_fdir_add *fltr, > return ret; > } > > +/** > + * ice_vc_fdir_is_raw_flow > + * @proto: virtchnl protocol headers > + * > + * Check if the FDIR rule is raw flow (protocol agnostic flow) or not. > + * Note that common FDIR rule must have non-zero proto->count. > + * Thus, we choose the tunnel_level and count of proto as the indicators. > + * If both tunnel_level and count of proto are zeros, this FDIR rule will > + * be regarded as raw flow. > + * > + * Returns wheater headers describe raw flow or not. > + */ > +static bool > +ice_vc_fdir_is_raw_flow(struct virtchnl_proto_hdrs *proto) > +{ > + return (proto->tunnel_level == 0 && proto->count == 0); nit: Parentheses are not needed here. Likewise elsewhere. > + > +/** > + * ice_vc_fdir_parse_raw > + * @vf: pointer to the VF info > + * @proto: virtchnl protocol headers > + * @conf: FDIR configuration for each filter > + * > + * Parse the virtual channel filter's raw flow and store it in @conf > + * > + * Return: 0 on success, and other on error. > + */ > +static int > +ice_vc_fdir_parse_raw(struct ice_vf *vf, > + struct virtchnl_proto_hdrs *proto, > + struct virtchnl_fdir_fltr_conf *conf) > +{ > + u8 *pkt_buf, *msk_buf __free(kfree); > + struct ice_parser_result rslt; > + struct ice_pf *pf = vf->pf; > + struct ice_parser *psr; > + int status = -ENOMEM; > + struct ice_hw *hw; > + u16 udp_port = 0; > + > + pkt_buf = kzalloc(proto->raw.pkt_len, GFP_KERNEL); > + msk_buf = kzalloc(proto->raw.pkt_len, GFP_KERNEL); msk_buf appears to be leaked both in when this function returns for both error and non-error cases. > + if (!pkt_buf || !msk_buf) > + goto err_mem_alloc; > + > + memcpy(pkt_buf, proto->raw.spec, proto->raw.pkt_len); > + memcpy(msk_buf, proto->raw.mask, proto->raw.pkt_len); > + > + hw = &pf->hw; > + > + /* Get raw profile info via Parser Lib */ > + psr = ice_parser_create(hw); > + if (IS_ERR(psr)) { > + status = PTR_ERR(psr); > + goto err_mem_alloc; > + } > + > + ice_parser_dvm_set(psr, ice_is_dvm_ena(hw)); > + > + if (ice_get_open_tunnel_port(hw, &udp_port, TNL_VXLAN)) > + ice_parser_vxlan_tunnel_set(psr, udp_port, true); > + > + status = ice_parser_run(psr, pkt_buf, proto->raw.pkt_len, &rslt); > + if (status) > + goto err_parser_destroy; > + > + if (hw->debug_mask & ICE_DBG_PARSER) > + ice_parser_result_dump(hw, &rslt); > + > + conf->prof = kzalloc(sizeof(*conf->prof), GFP_KERNEL); > + if (!conf->prof) > + goto err_parser_destroy; > + > + status = ice_parser_profile_init(&rslt, pkt_buf, msk_buf, > + proto->raw.pkt_len, ICE_BLK_FD, > + conf->prof); > + if (status) > + goto err_parser_profile_init; > + > + if (hw->debug_mask & ICE_DBG_PARSER) > + ice_parser_profile_dump(hw, conf->prof); > + > + /* Store raw flow info into @conf */ > + conf->pkt_len = proto->raw.pkt_len; > + conf->pkt_buf = pkt_buf; > + conf->parser_ena = true; > + > + ice_parser_destroy(psr); > + return 0; > + > +err_parser_profile_init: > + kfree(conf->prof); > +err_parser_destroy: > + ice_parser_destroy(psr); > +err_mem_alloc: > + kfree(pkt_buf); > + return status; > +} ...