[PATCH net-next 00/11][pull request] 100GbE Intel Wired LAN Driver Updates 2022-04-28

[PATCH net-next 00/11][pull request] 100GbE Intel Wired LAN Driver Updates 2022-04-28

[Tony Nguyen]

This series contains updates to ice driver only.

Amritha adds support for classid based queue forwarding.

Wan Jiabing converts an open coded min selection to min_t().

Maciej commonizes on a single find VSI function and removes the
duplicated implementation.

Wojciech adjusts the return value when exceeding ICE_MAX_CHAIN_WORDS to,
a more appropriate, -ENOSPC and allows for the error to be propagated.

Michal adds support for ndo_get_devlink_port().

Jake does some cleanup related to virtualization code. Mainly involving
function header comments and wording changes. NULL checks are added to
ice_get_vf_vsi() calls in order to prevent static analysis tools from
complaining that a NULL value could be dereferenced.

The following are changes since commit f3412b3879b4f7c4313b186b03940d4791345534:
  net: make sure net_rx_action() calls skb_defer_free_flush()
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/next-queue 100GbE

Amritha Nambiar (1):
  ice: Add support for classid based queue selection

Jacob Keller (6):
  ice: add newline to dev_dbg in ice_vf_fdir_dump_info
  ice: always check VF VSI pointer values
  ice: remove return value comment for ice_reset_all_vfs
  ice: fix wording in comment for ice_reset_vf
  ice: add a function comment for ice_cfg_mac_antispoof
  ice: remove period on argument description in ice_for_each_vf

Maciej Fijalkowski (1):
  ice: introduce common helper for retrieving VSI by vsi_num

Michal Swiatkowski (1):
  ice: get switch id on switchdev devices

Wan Jiabing (1):
  ice: use min_t() to make code cleaner in ice_gnss

Wojciech Drewek (1):
  ice: return ENOSPC when exceeding ICE_MAX_CHAIN_WORDS

 drivers/net/ethernet/intel/ice/ice.h          |  30 ++
 drivers/net/ethernet/intel/ice/ice_devlink.c  |  27 +-
 drivers/net/ethernet/intel/ice/ice_gnss.c     |   3 +-
 drivers/net/ethernet/intel/ice/ice_idc.c      |  15 -
 drivers/net/ethernet/intel/ice/ice_main.c     |  17 +-
 drivers/net/ethernet/intel/ice/ice_repr.c     |   7 +-
 drivers/net/ethernet/intel/ice/ice_sriov.c    |  32 ++-
 drivers/net/ethernet/intel/ice/ice_switch.c   |   5 +-
 drivers/net/ethernet/intel/ice/ice_tc_lib.c   | 272 ++++++++++++++----
 drivers/net/ethernet/intel/ice/ice_tc_lib.h   |  40 ++-
 drivers/net/ethernet/intel/ice/ice_vf_lib.c   |  43 ++-
 drivers/net/ethernet/intel/ice/ice_vf_lib.h   |   4 +-
 drivers/net/ethernet/intel/ice/ice_virtchnl.c |  27 +-
 .../ethernet/intel/ice/ice_virtchnl_fdir.c    |   9 +-
 14 files changed, 410 insertions(+), 121 deletions(-)

-- 
2.31.1

[PATCH net-next 01/11] ice: Add support for classid based queue selection

[Tony Nguyen]

From: Amritha Nambiar <amritha.nambiar@intel.com>

This patch uses TC flower filter's classid feature to support
forwarding packets to a device queue. Such filters with action
forward to queue will be the highest priority switch filter in
HW.
Example:
$ tc filter add dev ens4f0 protocol ip ingress flower\
  dst_ip 192.168.1.12 ip_proto tcp dst_port 5001\
  skip_sw classid ffff:0x5

The above command adds an ingress filter, the accepted packets
will be directed to queue 4. The major number represents the ingress
qdisc. The general rule is "classID's minor number - 1" upto max
queues supported. The queue number is in hex format.

Refactored ice_add_tc_flower_adv_fltr() to consolidate code with
action FWD_TO_VSI and FWD_TO QUEUE.

Signed-off-by: Amritha Nambiar <amritha.nambiar@intel.com>
Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
Tested-by: Bharathi Sreenivas <bharathi.sreenivas@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice.h        |  15 ++
 drivers/net/ethernet/intel/ice/ice_main.c   |   2 +-
 drivers/net/ethernet/intel/ice/ice_tc_lib.c | 271 +++++++++++++++-----
 drivers/net/ethernet/intel/ice/ice_tc_lib.h |  40 ++-
 4 files changed, 260 insertions(+), 68 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index 8ed3c9ab7ff7..517eb8bb0f9b 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -136,6 +136,21 @@
  */
 #define ICE_BW_KBPS_DIVISOR		125
 
+/* prio 5..7 can be used as advanced switch filter priority. Default recipes
+ * have prio 4 and below, hence prio value between 5..7 can be used as filter
+ * prio for advanced switch filter (advanced switch filter means it needs
+ * new recipe to be created to represent specified extraction sequence because
+ * default recipe extraction sequence does not represent custom extraction)
+ */
+#define ICE_SWITCH_FLTR_PRIO_QUEUE	7
+/* prio 6 is reserved for future use (e.g. switch filter with L3 fields +
+ * (Optional: IP TOS/TTL) + L4 fields + (optionally: TCP fields such as
+ * SYN/FIN/RST))
+ */
+#define ICE_SWITCH_FLTR_PRIO_RSVD	6
+#define ICE_SWITCH_FLTR_PRIO_VSI	5
+#define ICE_SWITCH_FLTR_PRIO_QGRP	ICE_SWITCH_FLTR_PRIO_VSI
+
 /* Macro for each VSI in a PF */
 #define ice_for_each_vsi(pf, i) \
 	for ((i) = 0; (i) < (pf)->num_alloc_vsi; (i)++)
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index fde839ef0613..0666a9105871 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -8108,7 +8108,7 @@ static void ice_rem_all_chnl_fltrs(struct ice_pf *pf)
 
 		rule.rid = fltr->rid;
 		rule.rule_id = fltr->rule_id;
-		rule.vsi_handle = fltr->dest_id;
+		rule.vsi_handle = fltr->dest_vsi_handle;
 		status = ice_rem_adv_rule_by_id(&pf->hw, &rule);
 		if (status) {
 			if (status == -ENOENT)
diff --git a/drivers/net/ethernet/intel/ice/ice_tc_lib.c b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
index 3acd9f921c44..e1e294a1654c 100644
--- a/drivers/net/ethernet/intel/ice/ice_tc_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
@@ -530,6 +530,121 @@ ice_eswitch_add_tc_fltr(struct ice_vsi *vsi, struct ice_tc_flower_fltr *fltr)
 	return ret;
 }
 
+/**
+ * ice_locate_vsi_using_queue - locate VSI using queue (forward to queue)
+ * @vsi: Pointer to VSI
+ * @tc_fltr: Pointer to tc_flower_filter
+ *
+ * Locate the VSI using specified "queue" (which is part of tc_fltr). When ADQ
+ * is not enabled, always return input VSI, otherwise locate corresponding
+ * VSI based on per channel "offset" and "qcount"
+ */
+static struct ice_vsi *
+ice_locate_vsi_using_queue(struct ice_vsi *vsi,
+			   struct ice_tc_flower_fltr *tc_fltr)
+{
+	int num_tc, tc;
+	int queue;
+
+	/* verify action is forward to queue */
+	if (tc_fltr->action.fltr_act != ICE_FWD_TO_Q)
+		return NULL;
+
+	/* if ADQ is not active, passed VSI is the candidate VSI */
+	if (!ice_is_adq_active(vsi->back))
+		return vsi;
+
+	/* now locate the VSI (it could still be main PF VSI or CHNL_VSI
+	 * depending upon "queue number")
+	 */
+	num_tc = vsi->mqprio_qopt.qopt.num_tc;
+	queue = tc_fltr->action.fwd.q.queue;
+
+	for (tc = 0; tc < num_tc; tc++) {
+		int offset = vsi->mqprio_qopt.qopt.offset[tc];
+		int qcount = vsi->mqprio_qopt.qopt.count[tc];
+
+		if (queue >= offset && queue < offset + qcount) {
+			/* for non-ADQ TCs, passed VSI is the candidate VSI */
+			if (tc < ICE_CHNL_START_TC)
+				return vsi;
+			else
+				return vsi->tc_map_vsi[tc];
+		}
+	}
+	return NULL;
+}
+
+static struct ice_rx_ring *
+ice_locate_rx_ring_using_queue(struct ice_vsi *vsi,
+			       struct ice_tc_flower_fltr *tc_fltr)
+{
+	u32 queue = tc_fltr->action.fwd.q.queue;
+
+	return queue < vsi->num_rxq ? vsi->rx_rings[queue] : NULL;
+}
+
+/**
+ * ice_tc_forward_action - Determine destination VSI and queue for the action
+ * @vsi: Pointer to VSI
+ * @tc_fltr: Pointer to TC flower filter structure
+ * @dest_vsi: Pointer to VSI ptr
+ *
+ * Validates the tc forward action and determines the destination VSI and queue
+ * for the forward action.
+ */
+static int
+ice_tc_forward_action(struct ice_vsi *vsi, struct ice_tc_flower_fltr *tc_fltr,
+		      struct ice_vsi **dest_vsi)
+{
+	struct ice_rx_ring *ring = NULL;
+	struct ice_vsi *ch_vsi = NULL;
+	struct ice_pf *pf = vsi->back;
+	struct device *dev;
+	u16 tc_class = 0;
+
+	dev = ice_pf_to_dev(pf);
+	*dest_vsi = NULL;
+
+	/* Get the destination VSI and/or destination queue and validate them */
+	tc_class = tc_fltr->action.fwd.tc.tc_class;
+	if (tc_class && tc_fltr->action.fltr_act == ICE_FWD_TO_VSI) {
+		/* Select the destination VSI */
+		if (tc_class < ICE_CHNL_START_TC) {
+			NL_SET_ERR_MSG_MOD(tc_fltr->extack,
+					   "Unable to add filter because of unsupported destination");
+			return -EOPNOTSUPP;
+		}
+		/* Locate ADQ VSI depending on hw_tc number */
+		ch_vsi = vsi->tc_map_vsi[tc_class];
+	} else if (tc_fltr->action.fltr_act == ICE_FWD_TO_Q) {
+		/* Locate the Rx queue using "action.fwd.q.queue" */
+		ring = ice_locate_rx_ring_using_queue(vsi, tc_fltr);
+		if (!ring) {
+			dev_err(dev, "Unable to locate Rx queue for action fwd_to_queue: %u\n",
+				tc_fltr->action.fwd.q.queue);
+			return -EINVAL;
+		}
+		/* Determine destination VSI even though forward action is
+		 * FWD_TO_QUEUE, because QUEUE is associated with VSI
+		 */
+		ch_vsi = ice_locate_vsi_using_queue(vsi, tc_fltr);
+	} else {
+		dev_err(dev, "Unable to add filter because of unsupported action %u (supported actions: fwd to tc, fwd to queue)\n",
+			tc_fltr->action.fltr_act);
+		return -EINVAL;
+	}
+
+	/* Must have valid "ch_vsi" (it could be main VSI or ADQ VSI */
+	if (!ch_vsi) {
+		dev_err(dev, "Unable to add filter because specified destination VSI doesn't exist\n");
+		return -EINVAL;
+	}
+
+	*dest_vsi = ch_vsi;
+	return 0;
+}
+
 /**
  * ice_add_tc_flower_adv_fltr - add appropriate filter rules
  * @vsi: Pointer to VSI
@@ -571,11 +686,10 @@ ice_add_tc_flower_adv_fltr(struct ice_vsi *vsi,
 		return -EOPNOTSUPP;
 	}
 
-	/* get the channel (aka ADQ VSI) */
-	if (tc_fltr->dest_vsi)
-		ch_vsi = tc_fltr->dest_vsi;
-	else
-		ch_vsi = vsi->tc_map_vsi[tc_fltr->action.tc_class];
+	/* validate forwarding action VSI and queue */
+	ret = ice_tc_forward_action(vsi, tc_fltr, &ch_vsi);
+	if (ret)
+		return ret;
 
 	lkups_cnt = ice_tc_count_lkups(flags, headers, tc_fltr);
 	list = kcalloc(lkups_cnt, sizeof(*list), GFP_ATOMIC);
@@ -589,25 +703,34 @@ ice_add_tc_flower_adv_fltr(struct ice_vsi *vsi,
 	}
 
 	rule_info.sw_act.fltr_act = tc_fltr->action.fltr_act;
-	if (tc_fltr->action.tc_class >= ICE_CHNL_START_TC) {
-		if (!ch_vsi) {
-			NL_SET_ERR_MSG_MOD(tc_fltr->extack, "Unable to add filter because specified destination doesn't exist");
-			ret = -EINVAL;
-			goto exit;
-		}
-
+	if (tc_fltr->action.fltr_act == ICE_FWD_TO_VSI) {
 		rule_info.sw_act.fltr_act = ICE_FWD_TO_VSI;
 		rule_info.sw_act.vsi_handle = ch_vsi->idx;
-		rule_info.priority = 7;
+		rule_info.priority = ICE_SWITCH_FLTR_PRIO_VSI;
 		rule_info.sw_act.src = hw->pf_id;
 		rule_info.rx = true;
 		dev_dbg(dev, "add switch rule for TC:%u vsi_idx:%u, lkups_cnt:%u\n",
-			tc_fltr->action.tc_class,
+			tc_fltr->action.fwd.tc.tc_class,
 			rule_info.sw_act.vsi_handle, lkups_cnt);
+	} else if (tc_fltr->action.fltr_act == ICE_FWD_TO_Q) {
+		rule_info.sw_act.fltr_act = ICE_FWD_TO_Q;
+		/* HW queue number in global space */
+		rule_info.sw_act.fwd_id.q_id = tc_fltr->action.fwd.q.hw_queue;
+		rule_info.sw_act.vsi_handle = ch_vsi->idx;
+		rule_info.priority = ICE_SWITCH_FLTR_PRIO_QUEUE;
+		rule_info.sw_act.src = hw->pf_id;
+		rule_info.rx = true;
+		dev_dbg(dev, "add switch rule action to forward to queue:%u (HW queue %u), lkups_cnt:%u\n",
+			tc_fltr->action.fwd.q.queue,
+			tc_fltr->action.fwd.q.hw_queue,
+			lkups_cnt);
 	} else {
 		rule_info.sw_act.flag |= ICE_FLTR_TX;
-		rule_info.sw_act.src = vsi->idx;
+		/* In case of Tx (LOOKUP_TX), src needs to be src VSI */
+		rule_info.sw_act.src = ch_vsi->idx;
+		/* 'Rx' is false, direction of rule(LOOKUPTRX) */
 		rule_info.rx = false;
+		rule_info.priority = ICE_SWITCH_FLTR_PRIO_VSI;
 	}
 
 	/* specify the cookie as filter_rule_id */
@@ -631,19 +754,14 @@ ice_add_tc_flower_adv_fltr(struct ice_vsi *vsi,
 	 */
 	tc_fltr->rid = rule_added.rid;
 	tc_fltr->rule_id = rule_added.rule_id;
-	if (tc_fltr->action.tc_class > 0 && ch_vsi) {
-		/* For PF ADQ, VSI type is set as ICE_VSI_CHNL, and
-		 * for PF ADQ filter, it is not yet set in tc_fltr,
-		 * hence store the dest_vsi ptr in tc_fltr
-		 */
-		if (ch_vsi->type == ICE_VSI_CHNL)
-			tc_fltr->dest_vsi = ch_vsi;
+	if (tc_fltr->action.fltr_act == ICE_FWD_TO_VSI ||
+	    tc_fltr->action.fltr_act == ICE_FWD_TO_Q) {
+		tc_fltr->dest_vsi_handle = rule_added.vsi_handle;
+		tc_fltr->dest_vsi = ch_vsi;
 		/* keep track of advanced switch filter for
-		 * destination VSI (channel VSI)
+		 * destination VSI
 		 */
 		ch_vsi->num_chnl_fltr++;
-		/* in this case, dest_id is VSI handle (sw handle) */
-		tc_fltr->dest_id = rule_added.vsi_handle;
 
 		/* keeps track of channel filters for PF VSI */
 		if (vsi->type == ICE_VSI_PF &&
@@ -651,10 +769,17 @@ ice_add_tc_flower_adv_fltr(struct ice_vsi *vsi,
 			      ICE_TC_FLWR_FIELD_ENC_DST_MAC)))
 			pf->num_dmac_chnl_fltrs++;
 	}
-	dev_dbg(dev, "added switch rule (lkups_cnt %u, flags 0x%x) for TC %u, rid %u, rule_id %u, vsi_idx %u\n",
-		lkups_cnt, flags,
-		tc_fltr->action.tc_class, rule_added.rid,
-		rule_added.rule_id, rule_added.vsi_handle);
+	if (tc_fltr->action.fltr_act == ICE_FWD_TO_VSI) {
+		dev_dbg(dev, "added switch rule (lkups_cnt %u, flags 0x%x) for TC %u, rid %u, rule_id %u, vsi_idx %u\n",
+			lkups_cnt, flags, tc_fltr->action.fwd.tc.tc_class,
+			rule_added.rid, rule_added.rule_id,
+			rule_added.vsi_handle);
+	} else if (tc_fltr->action.fltr_act == ICE_FWD_TO_Q) {
+		dev_dbg(dev, "added switch rule (lkups_cnt %u, flags 0x%x), action is forward to queue: %u (HW queue %u), rid %u, rule_id %u\n",
+			lkups_cnt, flags, tc_fltr->action.fwd.q.queue,
+			tc_fltr->action.fwd.q.hw_queue, rule_added.rid,
+			rule_added.rule_id);
+	}
 exit:
 	kfree(list);
 	return ret;
@@ -1122,40 +1247,72 @@ ice_add_switch_fltr(struct ice_vsi *vsi, struct ice_tc_flower_fltr *fltr)
 }
 
 /**
- * ice_handle_tclass_action - Support directing to a traffic class
+ * ice_handle_tclass_action - Support directing to a traffic class or queue
  * @vsi: Pointer to VSI
  * @cls_flower: Pointer to TC flower offload structure
  * @fltr: Pointer to TC flower filter structure
  *
- * Support directing traffic to a traffic class
+ * Support directing traffic to a traffic class or queue
  */
 static int
 ice_handle_tclass_action(struct ice_vsi *vsi,
 			 struct flow_cls_offload *cls_flower,
 			 struct ice_tc_flower_fltr *fltr)
 {
-	int tc = tc_classid_to_hwtc(vsi->netdev, cls_flower->classid);
-	struct ice_vsi *main_vsi;
-
-	if (tc < 0) {
-		NL_SET_ERR_MSG_MOD(fltr->extack, "Unable to add filter because specified destination is invalid");
-		return -EINVAL;
-	}
-	if (!tc) {
-		NL_SET_ERR_MSG_MOD(fltr->extack, "Unable to add filter because of invalid destination");
-		return -EINVAL;
-	}
-
-	if (!(vsi->all_enatc & BIT(tc))) {
-		NL_SET_ERR_MSG_MOD(fltr->extack, "Unable to add filter because of non-existence destination");
-		return -EINVAL;
-	}
+	unsigned int nrx = TC_H_MIN(cls_flower->classid);
+	u32 num_tc;
+	int tc;
+
+	num_tc = netdev_get_num_tc(vsi->netdev);
+
+	/* There are two regions which will have valid "classid" values:
+	 * 1. The first region will have a classid value of 1 through
+	 * num_tx_queues (i.e forward to queue).
+	 * 2. The second region represents the hardware traffic classes. These
+	 * are represented by classid values of TC_H_MIN_PRIORITY through
+	 * TC_H_MIN_PRIORITY + netdev_get_num_tc - 1. (i.e forward to TC)
+	 */
+	if (nrx < TC_H_MIN_PRIORITY) {
+		struct ice_hw *hw = &vsi->back->hw;
+		u32 global_qid, queue;
+		/* user specified queue, hence action is forward to queue */
+		if (nrx > vsi->num_rxq) {
+			NL_SET_ERR_MSG_MOD(fltr->extack,
+					   "Unable to add filter because specified queue is invalid");
+			return -ENXIO;
+		}
+		/* since nrx is 1 based */
+		queue = nrx - 1;
+
+		/* forward to queue */
+		fltr->action.fltr_act = ICE_FWD_TO_Q;
+		fltr->action.fwd.q.queue = queue;
+
+		/* determine corresponding HW queue */
+		global_qid = hw->func_caps.common_cap.rxq_first_id + queue;
+		fltr->action.fwd.q.hw_queue = global_qid;
+	} else if ((nrx - TC_H_MIN_PRIORITY) < num_tc) {
+		/* user specified hw_tc (it must be non-zero for ADQ TC, hence
+		 * action is forward to "hw_tc (aka ADQ channel number)"
+		 */
+		tc = nrx - TC_H_MIN_PRIORITY;
+		if (tc < ICE_CHNL_START_TC) {
+			NL_SET_ERR_MSG_MOD(fltr->extack,
+					   "Unable to add filter because of unsupported destination");
+			return -EOPNOTSUPP;
+		}
 
-	/* Redirect to a TC class or Queue Group */
-	main_vsi = ice_get_main_vsi(vsi->back);
-	if (!main_vsi || !main_vsi->netdev) {
+		if (!(vsi->all_enatc & BIT(tc))) {
+			NL_SET_ERR_MSG_MOD(fltr->extack,
+					   "Unable to add filter because of non-existence destination");
+			return -EINVAL;
+		}
+		/* forward to hw_tc (aka ADQ VSI) */
+		fltr->action.fltr_act = ICE_FWD_TO_VSI;
+		fltr->action.fwd.tc.tc_class = tc;
+	} else {
 		NL_SET_ERR_MSG_MOD(fltr->extack,
-				   "Unable to add filter because of invalid netdevice");
+				   "Unable to add filter because user specified neither queue nor hw_tc as forward action");
 		return -EINVAL;
 	}
 
@@ -1199,9 +1356,8 @@ ice_handle_tclass_action(struct ice_vsi *vsi,
 	 * lower netdev or any of MACVLAN. MACVLANs MAC address are added as
 	 * unicast MAC filter destined to main VSI.
 	 */
-	if (!ice_mac_fltr_exist(&main_vsi->back->hw,
-				fltr->outer_headers.l2_key.dst_mac,
-				main_vsi->idx)) {
+	if (!ice_mac_fltr_exist(&vsi->back->hw,
+				fltr->outer_headers.l2_key.dst_mac, vsi->idx)) {
 		NL_SET_ERR_MSG_MOD(fltr->extack,
 				   "Unable to add filter because legacy MAC filter for specified destination doesn't exist");
 		return -EINVAL;
@@ -1213,15 +1369,12 @@ ice_handle_tclass_action(struct ice_vsi *vsi,
 	if (fltr->flags & ICE_TC_FLWR_FIELD_VLAN) {
 		u16 vlan_id = be16_to_cpu(fltr->outer_headers.vlan_hdr.vlan_id);
 
-		if (!ice_vlan_fltr_exist(&main_vsi->back->hw, vlan_id,
-					 main_vsi->idx)) {
+		if (!ice_vlan_fltr_exist(&vsi->back->hw, vlan_id, vsi->idx)) {
 			NL_SET_ERR_MSG_MOD(fltr->extack,
 					   "Unable to add filter because legacy VLAN filter for specified destination doesn't exist");
 			return -EINVAL;
 		}
 	}
-	fltr->action.fltr_act = ICE_FWD_TO_VSI;
-	fltr->action.tc_class = tc;
 
 	return 0;
 }
@@ -1285,7 +1438,7 @@ static int ice_del_tc_fltr(struct ice_vsi *vsi, struct ice_tc_flower_fltr *fltr)
 
 	rule_rem.rid = fltr->rid;
 	rule_rem.rule_id = fltr->rule_id;
-	rule_rem.vsi_handle = fltr->dest_id;
+	rule_rem.vsi_handle = fltr->dest_vsi_handle;
 	err = ice_rem_adv_rule_by_id(&pf->hw, &rule_rem);
 	if (err) {
 		if (err == -ENOENT) {
diff --git a/drivers/net/ethernet/intel/ice/ice_tc_lib.h b/drivers/net/ethernet/intel/ice/ice_tc_lib.h
index e25e958f4396..1a234a2522e2 100644
--- a/drivers/net/ethernet/intel/ice/ice_tc_lib.h
+++ b/drivers/net/ethernet/intel/ice/ice_tc_lib.h
@@ -33,7 +33,20 @@ struct ice_indr_block_priv {
 };
 
 struct ice_tc_flower_action {
-	u32 tc_class;
+	/* forward action specific params */
+	union {
+		struct {
+			u32 tc_class; /* forward to hw_tc */
+			u32 rsvd;
+		} tc;
+		struct {
+			u32 queue; /* forward to queue */
+			/* to add filter in HW, it needs absolute queue number
+			 * in global space of queues (between 0...N)
+			 */
+			u32 hw_queue;
+		} q;
+	} fwd;
 	enum ice_sw_fwd_act_type fltr_act;
 };
 
@@ -106,11 +119,11 @@ struct ice_tc_flower_fltr {
 	 */
 	u16 rid;
 	u16 rule_id;
-	/* this could be queue/vsi_idx (sw handle)/queue_group, depending upon
-	 * destination type
+	/* VSI handle of the destination VSI (it could be main PF VSI, CHNL_VSI,
+	 * VF VSI)
 	 */
-	u16 dest_id;
-	/* if dest_id is vsi_idx, then need to store destination VSI ptr */
+	u16 dest_vsi_handle;
+	/* ptr to destination VSI */
 	struct ice_vsi *dest_vsi;
 	/* direction of fltr for eswitch use case */
 	enum ice_eswitch_fltr_direction direction;
@@ -137,12 +150,23 @@ struct ice_tc_flower_fltr {
  * @f: Pointer to tc-flower filter
  *
  * Criteria to determine of given filter is valid channel filter
- * or not is based on its "destination". If destination is hw_tc (aka tc_class)
- * and it is non-zero, then it is valid channel (aka ADQ) filter
+ * or not is based on its "destination".
+ * For forward to VSI action, if destination is valid hw_tc (aka tc_class)
+ * and in supported range of TCs for ADQ, then return true.
+ * For forward to queue, as long as dest_vsi is valid and it is of type
+ * VSI_CHNL (PF ADQ VSI is of type VSI_CHNL), return true.
+ * NOTE: For forward to queue, correct dest_vsi is still set in tc_fltr based
+ * on destination queue specified.
  */
 static inline bool ice_is_chnl_fltr(struct ice_tc_flower_fltr *f)
 {
-	return !!f->action.tc_class;
+	if (f->action.fltr_act == ICE_FWD_TO_VSI)
+		return f->action.fwd.tc.tc_class >= ICE_CHNL_START_TC &&
+			f->action.fwd.tc.tc_class < ICE_CHNL_MAX_TC;
+	else if (f->action.fltr_act == ICE_FWD_TO_Q)
+		return f->dest_vsi && f->dest_vsi->type == ICE_VSI_CHNL;
+
+	return false;
 }
 
 /**
-- 
2.31.1

[PATCH net-next 02/11] ice: use min_t() to make code cleaner in ice_gnss

[Tony Nguyen]

From: Wan Jiabing <wanjiabing@vivo.com>

Fix the following coccicheck warning:
./drivers/net/ethernet/intel/ice/ice_gnss.c:79:26-27: WARNING opportunity for min()

Signed-off-by: Wan Jiabing <wanjiabing@vivo.com>
Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_gnss.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
index 35579cf4283f..57586a2e6dec 100644
--- a/drivers/net/ethernet/intel/ice/ice_gnss.c
+++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
@@ -76,8 +76,7 @@ static void ice_gnss_read(struct kthread_work *work)
 	for (i = 0; i < data_len; i += bytes_read) {
 		u16 bytes_left = data_len - i;
 
-		bytes_read = bytes_left < ICE_MAX_I2C_DATA_SIZE ? bytes_left :
-					  ICE_MAX_I2C_DATA_SIZE;
+		bytes_read = min_t(typeof(bytes_left), bytes_left, ICE_MAX_I2C_DATA_SIZE);
 
 		err = ice_aq_read_i2c(hw, link_topo, ICE_GNSS_UBX_I2C_BUS_ADDR,
 				      cpu_to_le16(ICE_GNSS_UBX_EMPTY_DATA),
-- 
2.31.1

[PATCH net-next 03/11] ice: introduce common helper for retrieving VSI by vsi_num

[Tony Nguyen]

From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

Both ice_idc.c and ice_virtchnl.c carry their own implementation of a
helper function that is looking for a given VSI based on provided
vsi_num. Their functionality is the same, so let's introduce the common
function in ice.h that both of the mentioned sites will use.

This is a strictly cleanup thing, no functionality is changed.

Reviewed-by: Alexander Lobakin <alexandr.lobakin@intel.com>
Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Tested-by: Konrad Jankowski <konrad0.jankowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice.h          | 15 +++++++++++++
 drivers/net/ethernet/intel/ice/ice_idc.c      | 15 -------------
 drivers/net/ethernet/intel/ice/ice_virtchnl.c | 22 ++-----------------
 3 files changed, 17 insertions(+), 35 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index 517eb8bb0f9b..17fb1ddea7b0 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -772,6 +772,21 @@ static inline struct ice_vsi *ice_get_ctrl_vsi(struct ice_pf *pf)
 	return pf->vsi[pf->ctrl_vsi_idx];
 }
 
+/**
+ * ice_find_vsi - Find the VSI from VSI ID
+ * @pf: The PF pointer to search in
+ * @vsi_num: The VSI ID to search for
+ */
+static inline struct ice_vsi *ice_find_vsi(struct ice_pf *pf, u16 vsi_num)
+{
+	int i;
+
+	ice_for_each_vsi(pf, i)
+		if (pf->vsi[i] && pf->vsi[i]->vsi_num == vsi_num)
+			return  pf->vsi[i];
+	return NULL;
+}
+
 /**
  * ice_is_switchdev_running - check if switchdev is configured
  * @pf: pointer to PF structure
diff --git a/drivers/net/ethernet/intel/ice/ice_idc.c b/drivers/net/ethernet/intel/ice/ice_idc.c
index 25a436d342c2..7e941264ae21 100644
--- a/drivers/net/ethernet/intel/ice/ice_idc.c
+++ b/drivers/net/ethernet/intel/ice/ice_idc.c
@@ -47,21 +47,6 @@ void ice_send_event_to_aux(struct ice_pf *pf, struct iidc_event *event)
 	device_unlock(&pf->adev->dev);
 }
 
-/**
- * ice_find_vsi - Find the VSI from VSI ID
- * @pf: The PF pointer to search in
- * @vsi_num: The VSI ID to search for
- */
-static struct ice_vsi *ice_find_vsi(struct ice_pf *pf, u16 vsi_num)
-{
-	int i;
-
-	ice_for_each_vsi(pf, i)
-		if (pf->vsi[i] && pf->vsi[i]->vsi_num == vsi_num)
-			return  pf->vsi[i];
-	return NULL;
-}
-
 /**
  * ice_add_rdma_qset - Add Leaf Node for RDMA Qset
  * @pf: PF struct
diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
index 69ff4b929772..9bf52fa3c1b5 100644
--- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c
+++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
@@ -514,24 +514,6 @@ static void ice_vc_reset_vf_msg(struct ice_vf *vf)
 		ice_reset_vf(vf, 0);
 }
 
-/**
- * ice_find_vsi_from_id
- * @pf: the PF structure to search for the VSI
- * @id: ID of the VSI it is searching for
- *
- * searches for the VSI with the given ID
- */
-static struct ice_vsi *ice_find_vsi_from_id(struct ice_pf *pf, u16 id)
-{
-	int i;
-
-	ice_for_each_vsi(pf, i)
-		if (pf->vsi[i] && pf->vsi[i]->vsi_num == id)
-			return pf->vsi[i];
-
-	return NULL;
-}
-
 /**
  * ice_vc_isvalid_vsi_id
  * @vf: pointer to the VF info
@@ -544,7 +526,7 @@ bool ice_vc_isvalid_vsi_id(struct ice_vf *vf, u16 vsi_id)
 	struct ice_pf *pf = vf->pf;
 	struct ice_vsi *vsi;
 
-	vsi = ice_find_vsi_from_id(pf, vsi_id);
+	vsi = ice_find_vsi(pf, vsi_id);
 
 	return (vsi && (vsi->vf == vf));
 }
@@ -559,7 +541,7 @@ bool ice_vc_isvalid_vsi_id(struct ice_vf *vf, u16 vsi_id)
  */
 static bool ice_vc_isvalid_q_id(struct ice_vf *vf, u16 vsi_id, u8 qid)
 {
-	struct ice_vsi *vsi = ice_find_vsi_from_id(vf->pf, vsi_id);
+	struct ice_vsi *vsi = ice_find_vsi(vf->pf, vsi_id);
 	/* allocated Tx and Rx queues should be always equal for VF VSI */
 	return (vsi && (qid < vsi->alloc_txq));
 }
-- 
2.31.1

[PATCH net-next 04/11] ice: return ENOSPC when exceeding ICE_MAX_CHAIN_WORDS

[Tony Nguyen]

From: Wojciech Drewek <wojciech.drewek@intel.com>

When number of words exceeds ICE_MAX_CHAIN_WORDS, -ENOSPC
should be returned not -EINVAL. Do not overwrite this
error code in ice_add_tc_flower_adv_fltr.

Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>
Suggested-by: Marcin Szycik <marcin.szycik@linux.intel.com>
Acked-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Tested-by: Sandeep Penigalapati <sandeep.penigalapati@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_switch.c | 5 ++++-
 drivers/net/ethernet/intel/ice/ice_tc_lib.c | 1 -
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c
index 496250f9f8fc..9f0a4dfb4818 100644
--- a/drivers/net/ethernet/intel/ice/ice_switch.c
+++ b/drivers/net/ethernet/intel/ice/ice_switch.c
@@ -5992,9 +5992,12 @@ ice_add_adv_rule(struct ice_hw *hw, struct ice_adv_lkup_elem *lkups,
 				word_cnt++;
 	}
 
-	if (!word_cnt || word_cnt > ICE_MAX_CHAIN_WORDS)
+	if (!word_cnt)
 		return -EINVAL;
 
+	if (word_cnt > ICE_MAX_CHAIN_WORDS)
+		return -ENOSPC;
+
 	/* locate a dummy packet */
 	profile = ice_find_dummy_packet(lkups, lkups_cnt, rinfo->tun_type);
 
diff --git a/drivers/net/ethernet/intel/ice/ice_tc_lib.c b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
index e1e294a1654c..1308adcfde1b 100644
--- a/drivers/net/ethernet/intel/ice/ice_tc_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
@@ -745,7 +745,6 @@ ice_add_tc_flower_adv_fltr(struct ice_vsi *vsi,
 	} else if (ret) {
 		NL_SET_ERR_MSG_MOD(tc_fltr->extack,
 				   "Unable to add filter due to error");
-		ret = -EIO;
 		goto exit;
 	}
 
-- 
2.31.1

[PATCH net-next 05/11] ice: get switch id on switchdev devices

[Tony Nguyen]

From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>

Switch id should be the same for each netdevice on a driver.
The id must be unique between devices on the same system, but
does not need to be unique between devices on different systems.

The switch id is used to locate ports on a switch and to know if
aggregated ports belong to the same switch.

To meet this requirements, use pci_get_dsn as switch id value, as
this is unique value for each devices on the same system.

Implementing switch id is needed by automatic tools for kubernetes.

Set switch id by setting devlink port attribiutes and calling
devlink_port_attrs_set while creating pf (for uplink) and vf
(for representator) devlink port.

To get switch id (in switchdev mode):
cat /sys/class/net/$PF0/phys_switch_id

Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
Tested-by: Sandeep Penigalapati <sandeep.penigalapati@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_devlink.c | 22 ++++++++++++++++++++
 drivers/net/ethernet/intel/ice/ice_main.c    | 15 +++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
index a230edb38466..d12852d698af 100644
--- a/drivers/net/ethernet/intel/ice/ice_devlink.c
+++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
@@ -647,6 +647,23 @@ void ice_devlink_unregister(struct ice_pf *pf)
 	devlink_unregister(priv_to_devlink(pf));
 }
 
+/**
+ * ice_devlink_set_switch_id - Set unique switch id based on pci dsn
+ * @pf: the PF to create a devlink port for
+ * @ppid: struct with switch id information
+ */
+static void
+ice_devlink_set_switch_id(struct ice_pf *pf, struct netdev_phys_item_id *ppid)
+{
+	struct pci_dev *pdev = pf->pdev;
+	u64 id;
+
+	id = pci_get_dsn(pdev);
+
+	ppid->id_len = sizeof(id);
+	put_unaligned_be64(id, &ppid->id);
+}
+
 int ice_devlink_register_params(struct ice_pf *pf)
 {
 	struct devlink *devlink = priv_to_devlink(pf);
@@ -704,6 +721,9 @@ int ice_devlink_create_pf_port(struct ice_pf *pf)
 
 	attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL;
 	attrs.phys.port_number = pf->hw.bus.func;
+
+	ice_devlink_set_switch_id(pf, &attrs.switch_id);
+
 	devlink_port_attrs_set(devlink_port, &attrs);
 	devlink = priv_to_devlink(pf);
 
@@ -760,6 +780,8 @@ int ice_devlink_create_vf_port(struct ice_vf *vf)
 	attrs.pci_vf.pf = pf->hw.bus.func;
 	attrs.pci_vf.vf = vf->vf_id;
 
+	ice_devlink_set_switch_id(pf, &attrs.switch_id);
+
 	devlink_port_attrs_set(devlink_port, &attrs);
 	devlink = priv_to_devlink(pf);
 
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 0666a9105871..c3413ddfd011 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -296,6 +296,20 @@ static int ice_clear_promisc(struct ice_vsi *vsi, u8 promisc_m)
 	return status;
 }
 
+/**
+ * ice_get_devlink_port - Get devlink port from netdev
+ * @netdev: the netdevice structure
+ */
+static struct devlink_port *ice_get_devlink_port(struct net_device *netdev)
+{
+	struct ice_pf *pf = ice_netdev_to_pf(netdev);
+
+	if (!ice_is_switchdev_running(pf))
+		return NULL;
+
+	return &pf->devlink_port;
+}
+
 /**
  * ice_vsi_sync_fltr - Update the VSI filter list to the HW
  * @vsi: ptr to the VSI
@@ -8923,4 +8937,5 @@ static const struct net_device_ops ice_netdev_ops = {
 	.ndo_bpf = ice_xdp,
 	.ndo_xdp_xmit = ice_xdp_xmit,
 	.ndo_xsk_wakeup = ice_xsk_wakeup,
+	.ndo_get_devlink_port = ice_get_devlink_port,
 };
-- 
2.31.1

[PATCH net-next 06/11] ice: add newline to dev_dbg in ice_vf_fdir_dump_info

[Tony Nguyen]

From: Jacob Keller <jacob.e.keller@intel.com>

The debug print in ice_vf_fdir_dump_info does not end in newlines. This can
look confusing when reading the kernel log, as the next print will
immediately continue on the same line.

Fix this by adding the forgotten newline.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
index 8e38ee2faf58..dbc1965c0609 100644
--- a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
+++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
@@ -1349,7 +1349,7 @@ static void ice_vf_fdir_dump_info(struct ice_vf *vf)
 
 	fd_size = rd32(hw, VSIQF_FD_SIZE(vsi_num));
 	fd_cnt = rd32(hw, VSIQF_FD_CNT(vsi_num));
-	dev_dbg(dev, "VF %d: space allocated: guar:0x%x, be:0x%x, space consumed: guar:0x%x, be:0x%x",
+	dev_dbg(dev, "VF %d: space allocated: guar:0x%x, be:0x%x, space consumed: guar:0x%x, be:0x%x\n",
 		vf->vf_id,
 		(fd_size & VSIQF_FD_CNT_FD_GCNT_M) >> VSIQF_FD_CNT_FD_GCNT_S,
 		(fd_size & VSIQF_FD_CNT_FD_BCNT_M) >> VSIQF_FD_CNT_FD_BCNT_S,
-- 
2.31.1

[PATCH net-next 07/11] ice: always check VF VSI pointer values

[Tony Nguyen]

From: Jacob Keller <jacob.e.keller@intel.com>

The ice_get_vf_vsi function can return NULL in some cases, such as if
handling messages during a reset where the VSI is being removed and
recreated.

Several places throughout the driver do not bother to check whether this
VSI pointer is valid. Static analysis tools maybe report issues because
they detect paths where a potentially NULL pointer could be dereferenced.

Fix this by checking the return value of ice_get_vf_vsi everywhere.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
Tested-by: Konrad Jankowski <konrad0.jankowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_devlink.c  |  5 ++-
 drivers/net/ethernet/intel/ice/ice_repr.c     |  7 +++-
 drivers/net/ethernet/intel/ice/ice_sriov.c    | 32 +++++++++++++++++--
 drivers/net/ethernet/intel/ice/ice_vf_lib.c   | 28 +++++++++++++++-
 drivers/net/ethernet/intel/ice/ice_virtchnl.c |  5 +++
 .../ethernet/intel/ice/ice_virtchnl_fdir.c    |  7 +++-
 6 files changed, 77 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
index d12852d698af..3991d62473bf 100644
--- a/drivers/net/ethernet/intel/ice/ice_devlink.c
+++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
@@ -773,9 +773,12 @@ int ice_devlink_create_vf_port(struct ice_vf *vf)
 
 	pf = vf->pf;
 	dev = ice_pf_to_dev(pf);
-	vsi = ice_get_vf_vsi(vf);
 	devlink_port = &vf->devlink_port;
 
+	vsi = ice_get_vf_vsi(vf);
+	if (!vsi)
+		return -EINVAL;
+
 	attrs.flavour = DEVLINK_PORT_FLAVOUR_PCI_VF;
 	attrs.pci_vf.pf = pf->hw.bus.func;
 	attrs.pci_vf.vf = vf->vf_id;
diff --git a/drivers/net/ethernet/intel/ice/ice_repr.c b/drivers/net/ethernet/intel/ice/ice_repr.c
index 848f2adea563..a91b81c3088b 100644
--- a/drivers/net/ethernet/intel/ice/ice_repr.c
+++ b/drivers/net/ethernet/intel/ice/ice_repr.c
@@ -293,8 +293,13 @@ static int ice_repr_add(struct ice_vf *vf)
 	struct ice_q_vector *q_vector;
 	struct ice_netdev_priv *np;
 	struct ice_repr *repr;
+	struct ice_vsi *vsi;
 	int err;
 
+	vsi = ice_get_vf_vsi(vf);
+	if (!vsi)
+		return -EINVAL;
+
 	repr = kzalloc(sizeof(*repr), GFP_KERNEL);
 	if (!repr)
 		return -ENOMEM;
@@ -313,7 +318,7 @@ static int ice_repr_add(struct ice_vf *vf)
 		goto err_alloc;
 	}
 
-	repr->src_vsi = ice_get_vf_vsi(vf);
+	repr->src_vsi = vsi;
 	repr->vf = vf;
 	vf->repr = repr;
 	np = netdev_priv(repr->netdev);
diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
index 8915a9d39e36..29278e23691f 100644
--- a/drivers/net/ethernet/intel/ice/ice_sriov.c
+++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
@@ -46,7 +46,12 @@ static void ice_free_vf_entries(struct ice_pf *pf)
  */
 static void ice_vf_vsi_release(struct ice_vf *vf)
 {
-	ice_vsi_release(ice_get_vf_vsi(vf));
+	struct ice_vsi *vsi = ice_get_vf_vsi(vf);
+
+	if (WARN_ON(!vsi))
+		return;
+
+	ice_vsi_release(vsi);
 	ice_vf_invalidate_vsi(vf);
 }
 
@@ -104,6 +109,8 @@ static void ice_dis_vf_mappings(struct ice_vf *vf)
 
 	hw = &pf->hw;
 	vsi = ice_get_vf_vsi(vf);
+	if (WARN_ON(!vsi))
+		return;
 
 	dev = ice_pf_to_dev(pf);
 	wr32(hw, VPINT_ALLOC(vf->vf_id), 0);
@@ -341,6 +348,9 @@ static void ice_ena_vf_q_mappings(struct ice_vf *vf, u16 max_txq, u16 max_rxq)
 	struct ice_hw *hw = &vf->pf->hw;
 	u32 reg;
 
+	if (WARN_ON(!vsi))
+		return;
+
 	/* set regardless of mapping mode */
 	wr32(hw, VPLAN_TXQ_MAPENA(vf->vf_id), VPLAN_TXQ_MAPENA_TX_ENA_M);
 
@@ -386,6 +396,9 @@ static void ice_ena_vf_mappings(struct ice_vf *vf)
 {
 	struct ice_vsi *vsi = ice_get_vf_vsi(vf);
 
+	if (WARN_ON(!vsi))
+		return;
+
 	ice_ena_vf_msix_mappings(vf);
 	ice_ena_vf_q_mappings(vf, vsi->alloc_txq, vsi->alloc_rxq);
 }
@@ -1128,6 +1141,8 @@ static struct ice_vf *ice_get_vf_from_pfq(struct ice_pf *pf, u16 pfq)
 		u16 rxq_idx;
 
 		vsi = ice_get_vf_vsi(vf);
+		if (!vsi)
+			continue;
 
 		ice_for_each_rxq(vsi, rxq_idx)
 			if (vsi->rxq_map[rxq_idx] == pfq) {
@@ -1521,8 +1536,15 @@ static int ice_calc_all_vfs_min_tx_rate(struct ice_pf *pf)
 static bool
 ice_min_tx_rate_oversubscribed(struct ice_vf *vf, int min_tx_rate)
 {
-	int link_speed_mbps = ice_get_link_speed_mbps(ice_get_vf_vsi(vf));
-	int all_vfs_min_tx_rate = ice_calc_all_vfs_min_tx_rate(vf->pf);
+	struct ice_vsi *vsi = ice_get_vf_vsi(vf);
+	int all_vfs_min_tx_rate;
+	int link_speed_mbps;
+
+	if (WARN_ON(!vsi))
+		return false;
+
+	link_speed_mbps = ice_get_link_speed_mbps(vsi);
+	all_vfs_min_tx_rate = ice_calc_all_vfs_min_tx_rate(vf->pf);
 
 	/* this VF's previous rate is being overwritten */
 	all_vfs_min_tx_rate -= vf->min_tx_rate;
@@ -1566,6 +1588,10 @@ ice_set_vf_bw(struct net_device *netdev, int vf_id, int min_tx_rate,
 		goto out_put_vf;
 
 	vsi = ice_get_vf_vsi(vf);
+	if (!vsi) {
+		ret = -EINVAL;
+		goto out_put_vf;
+	}
 
 	/* when max_tx_rate is zero that means no max Tx rate limiting, so only
 	 * check if max_tx_rate is non-zero
diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
index 6578059d9479..aefd66a4db80 100644
--- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
@@ -220,8 +220,10 @@ static void ice_vf_clear_counters(struct ice_vf *vf)
 {
 	struct ice_vsi *vsi = ice_get_vf_vsi(vf);
 
+	if (vsi)
+		vsi->num_vlan = 0;
+
 	vf->num_mac = 0;
-	vsi->num_vlan = 0;
 	memset(&vf->mdd_tx_events, 0, sizeof(vf->mdd_tx_events));
 	memset(&vf->mdd_rx_events, 0, sizeof(vf->mdd_rx_events));
 }
@@ -251,6 +253,9 @@ static int ice_vf_rebuild_vsi(struct ice_vf *vf)
 	struct ice_vsi *vsi = ice_get_vf_vsi(vf);
 	struct ice_pf *pf = vf->pf;
 
+	if (WARN_ON(!vsi))
+		return -EINVAL;
+
 	if (ice_vsi_rebuild(vsi, true)) {
 		dev_err(ice_pf_to_dev(pf), "failed to rebuild VF %d VSI\n",
 			vf->vf_id);
@@ -514,6 +519,10 @@ int ice_reset_vf(struct ice_vf *vf, u32 flags)
 	ice_trigger_vf_reset(vf, flags & ICE_VF_RESET_VFLR, false);
 
 	vsi = ice_get_vf_vsi(vf);
+	if (WARN_ON(!vsi)) {
+		err = -EIO;
+		goto out_unlock;
+	}
 
 	ice_dis_vf_qs(vf);
 
@@ -572,6 +581,11 @@ int ice_reset_vf(struct ice_vf *vf, u32 flags)
 
 	vf->vf_ops->post_vsi_rebuild(vf);
 	vsi = ice_get_vf_vsi(vf);
+	if (WARN_ON(!vsi)) {
+		err = -EINVAL;
+		goto out_unlock;
+	}
+
 	ice_eswitch_update_repr(vsi);
 	ice_eswitch_replay_vf_mac_rule(vf);
 
@@ -610,6 +624,9 @@ void ice_dis_vf_qs(struct ice_vf *vf)
 {
 	struct ice_vsi *vsi = ice_get_vf_vsi(vf);
 
+	if (WARN_ON(!vsi))
+		return;
+
 	ice_vsi_stop_lan_tx_rings(vsi, ICE_NO_RESET, vf->vf_id);
 	ice_vsi_stop_all_rx_rings(vsi);
 	ice_set_vf_state_qs_dis(vf);
@@ -790,6 +807,9 @@ static int ice_vf_rebuild_host_mac_cfg(struct ice_vf *vf)
 	u8 broadcast[ETH_ALEN];
 	int status;
 
+	if (WARN_ON(!vsi))
+		return -EINVAL;
+
 	if (ice_is_eswitch_mode_switchdev(vf->pf))
 		return 0;
 
@@ -875,6 +895,9 @@ static int ice_vf_rebuild_host_tx_rate_cfg(struct ice_vf *vf)
 	struct ice_vsi *vsi = ice_get_vf_vsi(vf);
 	int err;
 
+	if (WARN_ON(!vsi))
+		return -EINVAL;
+
 	if (vf->min_tx_rate) {
 		err = ice_set_min_bw_limit(vsi, (u64)vf->min_tx_rate * 1000);
 		if (err) {
@@ -938,6 +961,9 @@ void ice_vf_rebuild_host_cfg(struct ice_vf *vf)
 	struct device *dev = ice_pf_to_dev(vf->pf);
 	struct ice_vsi *vsi = ice_get_vf_vsi(vf);
 
+	if (WARN_ON(!vsi))
+		return;
+
 	ice_vf_set_host_trust_cfg(vf);
 
 	if (ice_vf_rebuild_host_mac_cfg(vf))
diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
index 9bf52fa3c1b5..b1831f29f137 100644
--- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c
+++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
@@ -2342,6 +2342,11 @@ static int ice_vc_ena_vlan_stripping(struct ice_vf *vf)
 	}
 
 	vsi = ice_get_vf_vsi(vf);
+	if (!vsi) {
+		v_ret = VIRTCHNL_STATUS_ERR_PARAM;
+		goto error_param;
+	}
+
 	if (vsi->inner_vlan_ops.ena_stripping(vsi, ETH_P_8021Q))
 		v_ret = VIRTCHNL_STATUS_ERR_PARAM;
 
diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
index dbc1965c0609..c6a58343d81d 100644
--- a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
+++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
@@ -1344,7 +1344,12 @@ static void ice_vf_fdir_dump_info(struct ice_vf *vf)
 	pf = vf->pf;
 	hw = &pf->hw;
 	dev = ice_pf_to_dev(pf);
-	vf_vsi = pf->vsi[vf->lan_vsi_idx];
+	vf_vsi = ice_get_vf_vsi(vf);
+	if (!vf_vsi) {
+		dev_dbg(dev, "VF %d: invalid VSI pointer\n", vf->vf_id);
+		return;
+	}
+
 	vsi_num = ice_get_hw_vsi_num(hw, vf_vsi->idx);
 
 	fd_size = rd32(hw, VSIQF_FD_SIZE(vsi_num));
-- 
2.31.1

[PATCH net-next 08/11] ice: remove return value comment for ice_reset_all_vfs

[Tony Nguyen]

From: Jacob Keller <jacob.e.keller@intel.com>

Since commit fe99d1c06c16 ("ice: make ice_reset_all_vfs void"), the
ice_reset_all_vfs function has not returned anything. The function comment
still indicated it did. Fix this.

While here, also add a line to clarify the function resets all VFs at once
in response to hardware resets such as a PF reset.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_vf_lib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
index aefd66a4db80..24cf6a5b49fe 100644
--- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
@@ -359,12 +359,12 @@ ice_vf_clear_vsi_promisc(struct ice_vf *vf, struct ice_vsi *vsi, u8 promisc_m)
  * ice_reset_all_vfs - reset all allocated VFs in one go
  * @pf: pointer to the PF structure
  *
+ * Reset all VFs at once, in response to a PF or other device reset.
+ *
  * First, tell the hardware to reset each VF, then do all the waiting in one
  * chunk, and finally finish restoring each VF after the wait. This is useful
  * during PF routines which need to reset all VFs, as otherwise it must perform
  * these resets in a serialized fashion.
- *
- * Returns true if any VFs were reset, and false otherwise.
  */
 void ice_reset_all_vfs(struct ice_pf *pf)
 {
-- 
2.31.1

[PATCH net-next 09/11] ice: fix wording in comment for ice_reset_vf

[Tony Nguyen]

From: Jacob Keller <jacob.e.keller@intel.com>

The comment explaining ice_reset_vf has an extraneous "the" with the "if
the resets are disabled". Remove it.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_vf_lib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
index 24cf6a5b49fe..8f875a17755f 100644
--- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
@@ -477,8 +477,8 @@ static void ice_notify_vf_reset(struct ice_vf *vf)
  *   ICE_VF_RESET_NOTIFY - Send VF a notification prior to reset
  *   ICE_VF_RESET_LOCK - Acquire VF cfg_lock before resetting
  *
- * Returns 0 if the VF is currently in reset, if the resets are disabled, or
- * if the VF resets successfully. Returns an error code if the VF fails to
+ * Returns 0 if the VF is currently in reset, if resets are disabled, or if
+ * the VF resets successfully. Returns an error code if the VF fails to
  * rebuild.
  */
 int ice_reset_vf(struct ice_vf *vf, u32 flags)
-- 
2.31.1

[PATCH net-next 10/11] ice: add a function comment for ice_cfg_mac_antispoof

[Tony Nguyen]

From: Jacob Keller <jacob.e.keller@intel.com>

This function definition was missing a comment describing its
implementation. Add one.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_vf_lib.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
index 8f875a17755f..cd8e6b50968c 100644
--- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
@@ -657,6 +657,13 @@ struct ice_port_info *ice_vf_get_port_info(struct ice_vf *vf)
 	return vf->pf->hw.port_info;
 }
 
+/**
+ * ice_cfg_mac_antispoof - Configure MAC antispoof checking behavior
+ * @vsi: the VSI to configure
+ * @enable: whether to enable or disable the spoof checking
+ *
+ * Configure a VSI to enable (or disable) spoof checking behavior.
+ */
 static int ice_cfg_mac_antispoof(struct ice_vsi *vsi, bool enable)
 {
 	struct ice_vsi_ctx *ctx;
-- 
2.31.1

[PATCH net-next 11/11] ice: remove period on argument description in ice_for_each_vf

[Tony Nguyen]

From: Jacob Keller <jacob.e.keller@intel.com>

The ice_for_each_vf macros have comments describing the implementation. One
of the arguments has a period on the end, which is not our typical style.
Remove the unnecessary period.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_vf_lib.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.h b/drivers/net/ethernet/intel/ice/ice_vf_lib.h
index 831b667dc5b2..1b4380d6d949 100644
--- a/drivers/net/ethernet/intel/ice/ice_vf_lib.h
+++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.h
@@ -176,7 +176,7 @@ static inline u16 ice_vf_get_port_vlan_tpid(struct ice_vf *vf)
  * ice_for_each_vf - Iterate over each VF entry
  * @pf: pointer to the PF private structure
  * @bkt: bucket index used for iteration
- * @vf: pointer to the VF entry currently being processed in the loop.
+ * @vf: pointer to the VF entry currently being processed in the loop
  *
  * The bkt variable is an unsigned integer iterator used to traverse the VF
  * entries. It is *not* guaranteed to be the VF's vf_id. Do not assume it is.
@@ -192,7 +192,7 @@ static inline u16 ice_vf_get_port_vlan_tpid(struct ice_vf *vf)
  * ice_for_each_vf_rcu - Iterate over each VF entry protected by RCU
  * @pf: pointer to the PF private structure
  * @bkt: bucket index used for iteration
- * @vf: pointer to the VF entry currently being processed in the loop.
+ * @vf: pointer to the VF entry currently being processed in the loop
  *
  * The bkt variable is an unsigned integer iterator used to traverse the VF
  * entries. It is *not* guaranteed to be the VF's vf_id. Do not assume it is.
-- 
2.31.1

Re: [PATCH net-next 01/11] ice: Add support for classid based queue selection

[Jakub Kicinski]

On Thu, 28 Apr 2022 10:24:20 -0700 Tony Nguyen wrote:
> This patch uses TC flower filter's classid feature to support
> forwarding packets to a device queue. Such filters with action
> forward to queue will be the highest priority switch filter in

You say "forward" here..

> HW.
> Example:
> $ tc filter add dev ens4f0 protocol ip ingress flower\
>   dst_ip 192.168.1.12 ip_proto tcp dst_port 5001\
>   skip_sw classid ffff:0x5
> 
> The above command adds an ingress filter, the accepted packets
> will be directed to queue 4. The major number represents the ingress

..and "directed" here. TC is used for so many different things you 
really need to explain what your use case is.

> qdisc. The general rule is "classID's minor number - 1" upto max
> queues supported. The queue number is in hex format.

The "general rule" you speak of is a rule you'd like to establish,
or an existing rule?

RE: [PATCH net-next 01/11] ice: Add support for classid based queue selection

[Nambiar, Amritha]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Thursday, April 28, 2022 4:04 PM
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
> Cc: davem@davemloft.net; pabeni@redhat.com; edumazet@google.com;
> Nambiar, Amritha <amritha.nambiar@intel.com>; netdev@vger.kernel.org;
> Mogilappagari, Sudheer <sudheer.mogilappagari@intel.com>; Samudrala,
> Sridhar <sridhar.samudrala@intel.com>; Sreenivas, Bharathi
> <bharathi.sreenivas@intel.com>
> Subject: Re: [PATCH net-next 01/11] ice: Add support for classid based queue
> selection
> 
> On Thu, 28 Apr 2022 10:24:20 -0700 Tony Nguyen wrote:
> > This patch uses TC flower filter's classid feature to support
> > forwarding packets to a device queue. Such filters with action
> > forward to queue will be the highest priority switch filter in
> 
> You say "forward" here..
> 
> > HW.
> > Example:
> > $ tc filter add dev ens4f0 protocol ip ingress flower\
> >   dst_ip 192.168.1.12 ip_proto tcp dst_port 5001\
> >   skip_sw classid ffff:0x5
> >
> > The above command adds an ingress filter, the accepted packets
> > will be directed to queue 4. The major number represents the ingress
> 
> ..and "directed" here. TC is used for so many different things you
> really need to explain what your use case is.
> 

Sorry about using the terms "forward" and "direct" interchangeably in this
context. I should have been more consistent with the terminology. 

The use case is to accept incoming packets into a queue via TC ingress filter.
TC filters are offloaded to a hardware table called the "switch" table. This
table supports two types of actions in hardware termed as "forward to queue" and 
"forward to a VSI aka queue-group". Accepting packets into a queue using
ethtool filter is also supported, but this type of filter is added into a 
different hardware table called the "flow director" table. The flow director
table has certain restrictions that it can only have filters with the same packet
type. The switch table does not have this restriction.

> > qdisc. The general rule is "classID's minor number - 1" upto max
> > queues supported. The queue number is in hex format.
> 
> The "general rule" you speak of is a rule you'd like to establish,
> or an existing rule?

This is an existing rule already being used in the TX qdiscs. We are using
this in the ingress qdisc and offloading RX filters following the explanation
from Netdev 0x13 session presented by Jamal. Section 4.1 from
https://legacy.netdevconf.info/0x13/session.html?talk-tc-u-classifier
"There is one interesting tidbit on the above rule exposed
via the "classid 1:1" construct: the accepted packets will be
queued to DMA ring 0. If classid 1:2 was used then they
would be queued to DMA ring 1 etc. The general rule is the
"classid's minor number - 1" upto a max of DMA queues
supported by the NIC (64 in the case of the ixgbe). By definition, this is
how tc classids are intended to be used i.e they select queues (in this
case hardware ingress queues)."

Re: [PATCH net-next 01/11] ice: Add support for classid based queue selection

[Jakub Kicinski]

On Fri, 29 Apr 2022 23:43:06 +0000 Nambiar, Amritha wrote:
> > > HW.
> > > Example:
> > > $ tc filter add dev ens4f0 protocol ip ingress flower\
> > >   dst_ip 192.168.1.12 ip_proto tcp dst_port 5001\
> > >   skip_sw classid ffff:0x5
> > >
> > > The above command adds an ingress filter, the accepted packets
> > > will be directed to queue 4. The major number represents the ingress  
> > 
> > ..and "directed" here. TC is used for so many different things you
> > really need to explain what your use case is.
> >   
> 
> Sorry about using the terms "forward" and "direct" interchangeably in this
> context. I should have been more consistent with the terminology. 
> 
> The use case is to accept incoming packets into a queue via TC ingress filter.
> TC filters are offloaded to a hardware table called the "switch" table. This
> table supports two types of actions in hardware termed as "forward to queue" and 
> "forward to a VSI aka queue-group". Accepting packets into a queue using
> ethtool filter is also supported, but this type of filter is added into a 
> different hardware table called the "flow director" table. The flow director
> table has certain restrictions that it can only have filters with the same packet
> type. The switch table does not have this restriction.
> 
> > > qdisc. The general rule is "classID's minor number - 1" upto max
> > > queues supported. The queue number is in hex format.  
> > 
> > The "general rule" you speak of is a rule you'd like to establish,
> > or an existing rule?  
> 
> This is an existing rule already being used in the TX qdiscs. We are using
> this in the ingress qdisc and offloading RX filters following the explanation
> from Netdev 0x13 session presented by Jamal. Section 4.1 from
> https://legacy.netdevconf.info/0x13/session.html?talk-tc-u-classifier
> "There is one interesting tidbit on the above rule exposed
> via the "classid 1:1" construct: the accepted packets will be
> queued to DMA ring 0. If classid 1:2 was used then they
> would be queued to DMA ring 1 etc. The general rule is the
> "classid's minor number - 1" upto a max of DMA queues
> supported by the NIC (64 in the case of the ixgbe). By definition, this is
> how tc classids are intended to be used i.e they select queues (in this
> case hardware ingress queues)."

So we're faking mqprio behavior on ingress? I guess that's fine.

Wouldn't SKBEDIT_F_QUEUE_MAPPING be a more natural fit here?

RE: [PATCH net-next 01/11] ice: Add support for classid based queue selection

[Nambiar, Amritha]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Friday, April 29, 2022 5:17 PM
> To: Nambiar, Amritha <amritha.nambiar@intel.com>
> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>;
> davem@davemloft.net; pabeni@redhat.com; edumazet@google.com;
> netdev@vger.kernel.org; Mogilappagari, Sudheer
> <sudheer.mogilappagari@intel.com>; Samudrala, Sridhar
> <sridhar.samudrala@intel.com>; Sreenivas, Bharathi
> <bharathi.sreenivas@intel.com>; Jamal Hadi Salim <jhs@mojatatu.com>
> Subject: Re: [PATCH net-next 01/11] ice: Add support for classid based queue
> selection
> 
> On Fri, 29 Apr 2022 23:43:06 +0000 Nambiar, Amritha wrote:
> > > > HW.
> > > > Example:
> > > > $ tc filter add dev ens4f0 protocol ip ingress flower\
> > > >   dst_ip 192.168.1.12 ip_proto tcp dst_port 5001\
> > > >   skip_sw classid ffff:0x5
> > > >
> > > > The above command adds an ingress filter, the accepted packets
> > > > will be directed to queue 4. The major number represents the ingress
> > >
> > > ..and "directed" here. TC is used for so many different things you
> > > really need to explain what your use case is.
> > >
> >
> > Sorry about using the terms "forward" and "direct" interchangeably in this
> > context. I should have been more consistent with the terminology.
> >
> > The use case is to accept incoming packets into a queue via TC ingress filter.
> > TC filters are offloaded to a hardware table called the "switch" table. This
> > table supports two types of actions in hardware termed as "forward to
> queue" and
> > "forward to a VSI aka queue-group". Accepting packets into a queue using
> > ethtool filter is also supported, but this type of filter is added into a
> > different hardware table called the "flow director" table. The flow director
> > table has certain restrictions that it can only have filters with the same
> packet
> > type. The switch table does not have this restriction.
> >
> > > > qdisc. The general rule is "classID's minor number - 1" upto max
> > > > queues supported. The queue number is in hex format.
> > >
> > > The "general rule" you speak of is a rule you'd like to establish,
> > > or an existing rule?
> >
> > This is an existing rule already being used in the TX qdiscs. We are using
> > this in the ingress qdisc and offloading RX filters following the explanation
> > from Netdev 0x13 session presented by Jamal. Section 4.1 from
> > https://legacy.netdevconf.info/0x13/session.html?talk-tc-u-classifier
> > "There is one interesting tidbit on the above rule exposed
> > via the "classid 1:1" construct: the accepted packets will be
> > queued to DMA ring 0. If classid 1:2 was used then they
> > would be queued to DMA ring 1 etc. The general rule is the
> > "classid's minor number - 1" upto a max of DMA queues
> > supported by the NIC (64 in the case of the ixgbe). By definition, this is
> > how tc classids are intended to be used i.e they select queues (in this
> > case hardware ingress queues)."
> 
> So we're faking mqprio behavior on ingress? I guess that's fine.
> 
> Wouldn't SKBEDIT_F_QUEUE_MAPPING be a more natural fit here?

IIUC, currently the action skbedit queue_mapping is for transmit queue selection,
and the bound checking is w.r.to dev->real_num_tx_queues. Also, based on my
discussion with Alex (https://www.spinics.net/lists/netdev/msg761581.html), it
looks like this currently applies at the qdisc enqueue stage and not at the
classifier level. Alex had proposed a solution to make this more generic to work
with qdisc, ingress and egress hooks, but I haven't had a chance to get to it (being
on medical leave).

Re: [PATCH net-next 01/11] ice: Add support for classid based queue selection

[Jakub Kicinski]

On Sat, 30 Apr 2022 02:00:05 +0000 Nambiar, Amritha wrote:
> IIUC, currently the action skbedit queue_mapping is for transmit queue selection,
> and the bound checking is w.r.to dev->real_num_tx_queues. Also, based on my
> discussion with Alex (https://www.spinics.net/lists/netdev/msg761581.html), it
> looks like this currently applies at the qdisc enqueue stage and not at the
> classifier level.

They both apply at enqueue stage, AFAIU. Setting classid on ingress
does exactly nothing, no? :)

Neither is perfect, at least skbedit seems more straightforward. 
I suspect modern DC operator may have little familiarity with classful
qdiscs and what classid is. Plus, again, you're assuming mqprio's
interpretation like it's a TC-wide thing.

skbedit OTOH is used with a clsact qdisc.

Also it would be good if what we did had some applicability to SW. 
Maybe extend skbedit with a way of calling skb_record_rx_queue()?

Re: [PATCH net-next 01/11] ice: Add support for classid based queue selection

[Jamal Hadi Salim]

On 2022-04-29 22:42, Jakub Kicinski wrote:
> On Sat, 30 Apr 2022 02:00:05 +0000 Nambiar, Amritha wrote:
>> IIUC, currently the action skbedit queue_mapping is for transmit queue selection,
>> and the bound checking is w.r.to dev->real_num_tx_queues. Also, based on my
>> discussion with Alex (https://www.spinics.net/lists/netdev/msg761581.html), it
>> looks like this currently applies at the qdisc enqueue stage and not at the
>> classifier level.
> 
> They both apply at enqueue stage, AFAIU. Setting classid on ingress
> does exactly nothing, no? :)
> 
> Neither is perfect, at least skbedit seems more straightforward.
> I suspect modern DC operator may have little familiarity with classful
> qdiscs and what classid is. Plus, again, you're assuming mqprio's
> interpretation like it's a TC-wide thing.
> 
> skbedit OTOH is used with a clsact qdisc.
> 
> Also it would be good if what we did had some applicability to SW.
> Maybe extend skbedit with a way of calling skb_record_rx_queue()?

I am on the fence of "six of one, half a dozen of the other" ;->

TC classids have *always been used to identify queues* (or hierarchy of
but always leading to a single queue). Essentially a classid identity
is equivalent to a built-in action which says which queue to pick.
The data structure is very much engrained in the tc psyche.

When TX side HW queues(IIRC, mqprio) showed up there was ambiguity to
distinguish between a s/w queue vs a h/w queue hence queue_mapping
which allows us to override the *HW TX queue* selection - or at least
that was the intended goal.
Note: There are other ways to tell the h/w what queues to use on TX
(eg skb->priority) i.e there's no monopoly by queue_mapping.

Given lack of s/w queues on RX (hence lack of ambiguity) it seemed
natural that the classid could be used to describe the RX queue
identity for offload, it's just there.
I thought it was brilliant when Amritha first posted.

I think we should pick the simpler of "half-dozen or six".
The posted patch seems driver-only change i.e no core code
changes required (which other vendors could follow).
But i am not sure if that defines "simpler".

BTW:
Didnt follow the skb_record_rx_queue() thought - isnt that
always set by the driver based on which h/w queue the packet
arrived at? Whereas the semantics of this is to tell the h/w
what queue to use.

cheers,
jamal

Re: [PATCH net-next 01/11] ice: Add support for classid based queue selection

[Jakub Kicinski]

On Tue, 3 May 2022 06:32:01 -0400 Jamal Hadi Salim wrote:
> I am on the fence of "six of one, half a dozen of the other" ;->
> 
> TC classids have *always been used to identify queues* (or hierarchy of
> but always leading to a single queue). Essentially a classid identity
> is equivalent to a built-in action which says which queue to pick.
> The data structure is very much engrained in the tc psyche.
> 
> When TX side HW queues(IIRC, mqprio) showed up there was ambiguity to
> distinguish between a s/w queue vs a h/w queue hence queue_mapping
> which allows us to override the *HW TX queue* selection - or at least
> that was the intended goal.
> Note: There are other ways to tell the h/w what queues to use on TX
> (eg skb->priority) i.e there's no monopoly by queue_mapping.
> 
> Given lack of s/w queues on RX (hence lack of ambiguity) it seemed
> natural that the classid could be used to describe the RX queue
> identity for offload, it's just there.
> I thought it was brilliant when Amritha first posted.

Is it just me or is TC generally considered highly confusing?
IMO using a qdisc cls construct in clsact is only going to add 
to that.

Assigning classid can still be meaningful on ingress in case 
of a switch where there are actual qdiscs offloaded.

> I think we should pick the simpler of "half-dozen or six".
> The posted patch seems driver-only change i.e no core code
> changes required (which other vendors could follow).
> But i am not sure if that defines "simpler".

No core changes is not an argument we should take seriously upstream.

> BTW:
> Didnt follow the skb_record_rx_queue() thought - isnt that
> always set by the driver based on which h/w queue the packet
> arrived at? Whereas the semantics of this is to tell the h/w
> what queue to use.

Overriding the queue mapping in the SW could still be useful 
if TC wants to override the actual queue ID assigned by the NIC.

This way whether the action gets offloaded or not the resulting
skb will have the same metadata (in case of offload because it 
came on the right queue and the driver set the mapping, in case
of sw because we overwrote the mapping).

Re: [PATCH net-next 01/11] ice: Add support for classid based queue selection

[Jamal Hadi Salim]

On 2022-05-03 11:47, Jakub Kicinski wrote:
> On Tue, 3 May 2022 06:32:01 -0400 Jamal Hadi Salim wrote:
>> I am on the fence of "six of one, half a dozen of the other" ;->
>>
>> TC classids have *always been used to identify queues* (or hierarchy of
>> but always leading to a single queue). Essentially a classid identity
>> is equivalent to a built-in action which says which queue to pick.
>> The data structure is very much engrained in the tc psyche.
>>
>> When TX side HW queues(IIRC, mqprio) showed up there was ambiguity to
>> distinguish between a s/w queue vs a h/w queue hence queue_mapping
>> which allows us to override the *HW TX queue* selection - or at least
>> that was the intended goal.
>> Note: There are other ways to tell the h/w what queues to use on TX
>> (eg skb->priority) i.e there's no monopoly by queue_mapping.
>>
>> Given lack of s/w queues on RX (hence lack of ambiguity) it seemed
>> natural that the classid could be used to describe the RX queue
>> identity for offload, it's just there.
>> I thought it was brilliant when Amritha first posted.
> 
> Is it just me or is TC generally considered highly confusing?

Hopefully we can discuss what is confusing (otherwise this becomes the
internet troll discussion of "tc sucks").
The fact that folks find ways to use s/w in an unintended ways is
not unique to tc. The architecture is clean.

> IMO using a qdisc cls construct in clsact is only going to add
> to that.
> 
> Assigning classid can still be meaningful on ingress in case
> of a switch where there are actual qdiscs offloaded.
> 
>> I think we should pick the simpler of "half-dozen or six".
>> The posted patch seems driver-only change i.e no core code
>> changes required (which other vendors could follow).
>> But i am not sure if that defines "simpler".
> 
> No core changes is not an argument we should take seriously upstream.
>

I am afraid, that sounds like a blanket statement though.
It should be taken seriously if it helps maintainability
but like i said i wasnt sure having not seen the alternative.

>> BTW:
>> Didnt follow the skb_record_rx_queue() thought - isnt that
>> always set by the driver based on which h/w queue the packet
>> arrived at? Whereas the semantics of this is to tell the h/w
>> what queue to use.
> 
> Overriding the queue mapping in the SW could still be useful
> if TC wants to override the actual queue ID assigned by the NIC.
> 
> This way whether the action gets offloaded or not the resulting
> skb will have the same metadata (in case of offload because it
> came on the right queue and the driver set the mapping, in case
> of sw because we overwrote the mapping).

IIUC, you are suggesting that using the same semantics for ingress and
egress is more intuitive - and that is a fair arguement.
Hope you understand my view when I said it was half a dozen or six
given we have multiple approaches today on egress that signal the
hardware what queue to use (and classid's intent is/was to
select a queue).

cheers,
jamal