-----Original Message-----
From: Jakub Kicinski <kuba@kernel.org>
Sent: Tuesday, October 21, 2025 3:23 AM
To: Keller, Jacob E <jacob.e.keller@intel.com>
Cc: Jiri Pirko <jiri@resnulli.us>; David S. Miller
[off-list ref]; Eric Dumazet [off-list ref]; Paolo Abeni
[off-list ref]; Simon Horman [off-list ref]; Jonathan Corbet
[off-list ref]; Nguyen, Anthony L [off-list ref];
Kitszel, Przemyslaw [off-list ref]; Andrew Lunn
[off-list ref]; Lobakin, Aleksander
[off-list ref]; netdev@vger.kernel.org; linux-
doc@vger.kernel.org; linux-kernel@vger.kernel.org; Loktionov,
Aleksandr [off-list ref]; Nowlin, Dan
[off-list ref]; Wang, Jie1X [off-list ref]; Junfeng
Guo [off-list ref]; Zhang, Qi Z [off-list ref]; Ting
Xu [off-list ref]; Romanowski, Rafal [off-list ref]
Subject: Re: [PATCH net-next v2 04/14] ice: add virtchnl and VF
context support for GTP RSS

On Thu, 16 Oct 2025 23:08:33 -0700 Jacob Keller wrote:

quoted

 void ice_parser_destroy(struct ice_parser *psr)  {
+	if (!psr)
+		return;

questionable

But it potentially simplifies cleanup code in different functions (no need for additional if (psr) in functions).
Do you insist to move it from here?

quoted

+	{VIRTCHNL_PROTO_HDR_L2TPV2,
+		FIELD_SELECTOR(VIRTCHNL_PROTO_HDR_L2TPV2_SESS_ID),
+		BIT_ULL(ICE_FLOW_FIELD_IDX_L2TPV2_SESS_ID)},
+	{VIRTCHNL_PROTO_HDR_L2TPV2,
+		FIELD_SELECTOR(VIRTCHNL_PROTO_HDR_L2TPV2_LEN_SESS_ID),
+		BIT_ULL(ICE_FLOW_FIELD_IDX_L2TPV2_LEN_SESS_ID)},

consider moving out all the static data and define changes to a
separate commit for ease of review?

Ok, will do my best. Patch #2 is big, but everything is related to the context.

quoted

 };

+static enum virtchnl_status_code
+ice_vc_rss_hash_update(struct ice_hw *hw, struct ice_vsi *vsi, u8
+hash_type) {
+	enum virtchnl_status_code v_ret = VIRTCHNL_STATUS_SUCCESS;
+	struct ice_vsi_ctx *ctx;
+	int ret;
+
+	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return VIRTCHNL_STATUS_ERR_NO_MEMORY;

I feel like the VIRTCHNL_* error codes are spreading, we've been over
this.

I can understand your feelings.
But it's:
 - static function (only used within this file)
 - only called from virtchnl handler functions
 - "vc" (virtchnl) in its name indicating it's part of the virtchnl layer
 - not exposed in any headers
I think this is appropriate because it's part of the virtchnl abstraction layer and only used within virtchnl handlers.

quoted

+static int
+ice_hash_remove(struct ice_vf *vf, struct ice_rss_hash_cfg *cfg) {
+	int ret;
+
+	ret = ice_hash_moveout(vf, cfg);
+	if (ret && (ret != -ENOENT))

spurious brackets

Yep, have to fix.

quoted

+/**
+ * ice_add_rss_cfg_pre_ip - Pre-process IP-layer RSS configuration
+ * @vf: VF pointer
+ * @ctx: IP L4 hash context (ESP/UDP-ESP/AH/PFCP and UDP/TCP/SCTP)
+ *
+ * Remove covered/recorded IP RSS configurations prior to adding a

new one.

quoted

+ *
+ * Return: 0 on success; negative error code on failure.
+ */
+static int
+ice_add_rss_cfg_pre_ip(struct ice_vf *vf, struct ice_vf_hash_ip_ctx
+*ctx) {
+	int i, ret;
+
+	for (i = 1; i < ICE_HASH_IP_CTX_MAX; i++)
+		if (ice_is_hash_cfg_valid(&ctx->ctx[i])) {
+			ret = ice_hash_remove(vf, &ctx->ctx[i]);
+

spurious new line

Will fix it.

quoted

+			if (ret)
+				return ret;
+		}
+
+	return 0;
+}

quoted

+static int
+ice_parse_raw_rss_pattern(struct ice_vf *vf, struct

virtchnl_proto_hdrs *proto,

quoted

+			  struct ice_rss_raw_cfg *raw_cfg) {
+	struct ice_parser_result pkt_parsed;
+	struct ice_hw *hw = &vf->pf->hw;
+	struct ice_parser_profile prof;
+	u16 pkt_len;
+	struct ice_parser *psr;
+	u8 *pkt_buf, *msk_buf;
+	int ret = 0;
+
+	pkt_len = proto->raw.pkt_len;
+	if (!pkt_len)
+		return -EINVAL;
+	if (pkt_len > VIRTCHNL_MAX_SIZE_RAW_PACKET)
+		pkt_len = VIRTCHNL_MAX_SIZE_RAW_PACKET;
+
+	pkt_buf = kzalloc(pkt_len, GFP_KERNEL);
+	msk_buf = kzalloc(pkt_len, GFP_KERNEL);
+	if (!pkt_buf || !msk_buf) {
+		ret = -ENOMEM;
+		goto free_alloc;
+	}
+
+	memcpy(pkt_buf, proto->raw.spec, pkt_len);
+	memcpy(msk_buf, proto->raw.mask, pkt_len);
+
+	psr = ice_parser_create(hw);
+	if (IS_ERR(psr)) {
+		ret = PTR_ERR(psr);
+		goto parser_destroy;

goto free_alloc, surely, parser creation has failed, there's nothing
to destroy

Agree, easy fix.
 

quoted

+	}
+
+	ret = ice_parser_run(psr, pkt_buf, pkt_len, &pkt_parsed);
+	if (ret)
+		goto parser_destroy;
+
+	ret = ice_parser_profile_init(&pkt_parsed, pkt_buf, msk_buf,
+				      pkt_len, ICE_BLK_RSS, &prof);
+	if (ret)
+		goto parser_destroy;
+
+	memcpy(&raw_cfg->prof, &prof, sizeof(prof));
+
+parser_destroy:
+	ice_parser_destroy(psr);
+free_alloc:
+	kfree(pkt_buf);
+	kfree(msk_buf);
+	return ret;
+}

Thank you for reviewing
Alex