Message-ID: <27edba00-5d5e-46e4-b51c-9a69ef11e9a8@intel.com>
Date: Tue, 2 Dec 2025 14:54:30 +0100
From: Jakub Slepecki <jakub.slepecki@...el.com>
To: "Loktionov, Aleksandr" <aleksandr.loktionov@...el.com>,
	"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>, "Kitszel, Przemyslaw"
	<przemyslaw.kitszel@...el.com>, "Nguyen, Anthony L"
	<anthony.l.nguyen@...el.com>, "michal.swiatkowski@...ux.intel.com"
	<michal.swiatkowski@...ux.intel.com>
Subject: Re: [PATCH iwl-next v2 4/8] ice: allow overriding lan_en, lb_en in
 switch

On 2025-12-01 8:37, Loktionov, Aleksandr wrote:
> For u8 fields if they are used as u8 value (not bit fields) using FIELD_ macros not good.
> What about compromise:
>   - Keep lan_en and lb_en as bool or u8 with clear comments.
>   - Do NOT use FIELD macros unless these fields are truly packed bitfields.

After patch, lan_en and lb_en are always dealt with via FIELD_ macros.
The confusing part could be in ice_fill_sw_info():

+	bool lan_en = false;
+	bool lb_en = false;

Where throughout the function we decide on VALUE for each (stored as bool
lan_en and bool lb_en), and only then we apply it:

+	if (!FIELD_GET(ICE_FLTR_INFO_LB_LAN_FORCE_M, fi->lb_en))
+		FIELD_MODIFY(ICE_FLTR_INFO_LB_LAN_VALUE_M, &fi->lb_en, lb_en);
+	if (!FIELD_GET(ICE_FLTR_INFO_LB_LAN_FORCE_M, fi->lan_en))
+		FIELD_MODIFY(ICE_FLTR_INFO_LB_LAN_VALUE_M, &fi->lan_en, lan_en);

I could tweak names of the variables or maybe hold them as u8:

static void ice_fill_sw_info(struct ice_hw *hw, struct ice_fltr_info *fi)
{
	u8 lan_en = fi->lan_en;
	u8 lb_en = fi->lb_en;
	...
		FIELD_MODIFY(ICE_FLTR_INFO_LB_LAN_VALUE_M, &lb_en, true);
	...
	if (!FIELD_GET(ICE_FLTR_INFO_LB_LAN_FORCE_M, lb_en))
		fi->lb_en = lb_en;
}

Or s/true/1/g per previous discussion.

This seems better at expressing the "tmp -> decide -> commit" than the
current version.  See draft patch at the bottom.

>   - If FORCE/ VALUE semantics are needed, either:
>     +Introduce a dedicated flags field with proper bitmask macros, OR

I addressed this option in my previous response.  Let's not exclude
it yet, but since there are still some alternatives I would prefer to
avoid it.

>     +Keep separate fields and handle FORCE logic explicitly in code without FIELD macros.
> 
> And handle FORCE logic explicitly:
> 
> if (!force_lb)
>      fi->lb_en = lb_en;
> if (!force_lan)
>      fi->lan_en = lan_en;
> 
> ?

I intend to force values in 8/8 in ice_fltr.c in
ice_fltr_add_macs_to_list().  Decision is made based on:

1. whether MAC is multicast or unicast,
2. whether VSI has PVID, and
3. whether VSI has VLAN 0.

There are also implicit conditions, because ice_fltr_add_macs_to_list()
is only called in particular cases after some expected changes in overall
driver state; compared to ice_fill_sw_info():

4. filter is being created,
5. filter is MAC or MAC,VLAN,
6. target is guaranteed to be a single VSI, and
7. VLAN filters are guaranteed to be already created.

I'm reluctant to move decision-making to ice_fill_sw_info(), because
of these implicit conditions (and possibly more; each would need to
be proven).  Overall, I see two meaningful options (with some variants):

a. In ice_fill_sw_info(), reconstruct all needed information (1-3 from
    *hw + *fi) and make the decision,
b. Before ice_fill_sw_info(), make the decision and store it, then use
    the result in ice_fill_sw_info().

FORCE is (b).  I chose it because it seemed to be overall cheapest
(LOC, memory use, number of operations) and (subjective) semantically
correct (ice_fltr.c is responsible for requesting filters for a VSI;
ice_fill_sw_info() is responsible for populating defaults).

We could also go all the way in the other direction:

struct ice_fltr_info {
	...
	bool lan_en;
	bool force_lan_en;
	bool lb_en;
	bool force_lb_en;
};

? (or s/bool/u8/)

Current working draft:
-- >8 --
Subject: [PATCH iwl-next v2 4/8] ice: allow overriding lan_en, lb_en in switch

Currently, lan_en and lb_en are determined based on switching mode,
destination MAC, and the lookup type, action type and flags of the rule
in question.  This gives little to no options for the user (such as
ice_fltr.c) to enforce rules to behave in a specific way.

Such functionality is needed to work with pairs of rules, for example,
when handling MAC forward to LAN together with MAC,VLAN forward to
loopback rules pair.  This case could not be easily deduced in a context
of a single filter without adding a specialized flag.

Instead of adding a specialized flag to mark special scenario rules,
we add a slightly more generic flag to the lan_en and lb_en themselves
for the ice_fltr.c to request specific destination flags later on, for
example, to override value:

     struct ice_fltr_info fi;
     fi.lb_en = ICE_FLTR_INFO_LB_LAN_FORCE_ENABLED;
     fi.lan_en = ICE_FLTR_INFO_LB_LAN_FORCE_DISABLED;

Signed-off-by: Jakub Slepecki <jakub.slepecki@...el.com>
---
  drivers/net/ethernet/intel/ice/ice_switch.c | 27 ++++++++++++++-------
  drivers/net/ethernet/intel/ice/ice_switch.h | 19 ++++++++++++---
  2 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c
index 04e5d653efce..3896edaa8652 100644
--- a/drivers/net/ethernet/intel/ice/ice_switch.c
+++ b/drivers/net/ethernet/intel/ice/ice_switch.c
@@ -2534,12 +2534,14 @@ int ice_get_initial_sw_cfg(struct ice_hw *hw)
   *
   * This helper function populates the lb_en and lan_en elements of the provided
   * ice_fltr_info struct using the switch's type and characteristics of the
- * switch rule being configured.
+ * switch rule being configured.  Elements are updated only if their FORCE bit
+ * is not set.
   */
  static void ice_fill_sw_info(struct ice_hw *hw, struct ice_fltr_info *fi)
  {
-	fi->lb_en = false;
-	fi->lan_en = false;
+	u8 lan_en = fi->lan_en;
+	u8 lb_en = fi->lb_en;
+
  	if ((fi->flag & ICE_FLTR_TX) &&
  	    (fi->fltr_act == ICE_FWD_TO_VSI ||
  	     fi->fltr_act == ICE_FWD_TO_VSI_LIST ||
@@ -2549,7 +2551,8 @@ static void ice_fill_sw_info(struct ice_hw *hw, struct ice_fltr_info *fi)
  		 * packets to the internal switch that will be dropped.
  		 */
  		if (fi->lkup_type != ICE_SW_LKUP_VLAN)
-			fi->lb_en = true;
+			FIELD_MODIFY(ICE_FLTR_INFO_LB_LAN_VALUE_M, &lb_en,
+				     true);
  
  		/* Set lan_en to TRUE if
  		 * 1. The switch is a VEB AND
@@ -2578,14 +2581,20 @@ static void ice_fill_sw_info(struct ice_hw *hw, struct ice_fltr_info *fi)
  			     !is_unicast_ether_addr(fi->l_data.mac.mac_addr)) ||
  			    (fi->lkup_type == ICE_SW_LKUP_MAC_VLAN &&
  			     !is_unicast_ether_addr(fi->l_data.mac.mac_addr)))
-				fi->lan_en = true;
+				FIELD_MODIFY(ICE_FLTR_INFO_LB_LAN_VALUE_M,
+					     &lan_en, true);
  		} else {
-			fi->lan_en = true;
+			FIELD_MODIFY(ICE_FLTR_INFO_LB_LAN_VALUE_M, &lan_en,
+				     true);
  		}
  	}
  
  	if (fi->flag & ICE_FLTR_TX_ONLY)
-		fi->lan_en = false;
+		FIELD_MODIFY(ICE_FLTR_INFO_LB_LAN_VALUE_M, &lan_en, false);
+	if (!FIELD_GET(ICE_FLTR_INFO_LB_LAN_FORCE_M, lb_en))
+		fi->lb_en = lb_en;
+	if (!FIELD_GET(ICE_FLTR_INFO_LB_LAN_FORCE_M, lan_en))
+		fi->lan_en = lan_en;
  }
  
  /**
@@ -2669,9 +2678,9 @@ ice_fill_sw_rule(struct ice_hw *hw, struct ice_fltr_info *f_info,
  		return;
  	}
  
-	if (f_info->lb_en)
+	if (FIELD_GET(ICE_FLTR_INFO_LB_LAN_VALUE_M, f_info->lb_en))
  		act |= ICE_SINGLE_ACT_LB_ENABLE;
-	if (f_info->lan_en)
+	if (FIELD_GET(ICE_FLTR_INFO_LB_LAN_VALUE_M, f_info->lan_en))
  		act |= ICE_SINGLE_ACT_LAN_ENABLE;
  
  	switch (f_info->lkup_type) {
diff --git a/drivers/net/ethernet/intel/ice/ice_switch.h b/drivers/net/ethernet/intel/ice/ice_switch.h
index 671d7a5f359f..e421c562626c 100644
--- a/drivers/net/ethernet/intel/ice/ice_switch.h
+++ b/drivers/net/ethernet/intel/ice/ice_switch.h
@@ -72,6 +72,14 @@ enum ice_src_id {
  	ICE_SRC_ID_LPORT,
  };
  
+#define ICE_FLTR_INFO_LB_LAN_VALUE_M BIT(0)
+#define ICE_FLTR_INFO_LB_LAN_FORCE_M BIT(1)
+#define ICE_FLTR_INFO_LB_LAN_FORCE_ENABLED			 \
+	(FIELD_PREP_CONST(ICE_FLTR_INFO_LB_LAN_VALUE_M, true) |  \
+	 FIELD_PREP_CONST(ICE_FLTR_INFO_LB_LAN_FORCE_M, true))
+#define ICE_FLTR_INFO_LB_LAN_FORCE_DISABLED			 \
+	(FIELD_PREP_CONST(ICE_FLTR_INFO_LB_LAN_FORCE_M, true))
+
  struct ice_fltr_info {
  	/* Look up information: how to look up packet */
  	enum ice_sw_lkup_type lkup_type;
@@ -131,9 +139,14 @@ struct ice_fltr_info {
  	 */
  	u8 qgrp_size;
  
-	/* Rule creations populate these indicators basing on the switch type */
-	u8 lb_en;	/* Indicate if packet can be looped back */
-	u8 lan_en;	/* Indicate if packet can be forwarded to the uplink */
+	/* Following members have two bits: VALUE and FORCE.  Rule creation will
+	 * populate VALUE bit of these members based on switch type, but only if
+	 * their FORCE bit is not set.
+	 *
+	 * See ICE_FLTR_INFO_LB_LAN_VALUE_M and ICE_FLTR_INFO_LB_LAN_FORCE_M.
+	 */
+	u8 lb_en;	/* VALUE bit: packet can be looped back */
+	u8 lan_en;	/* VALUE bit: packet can be forwarded to the uplink */
  };
  
  struct ice_update_recipe_lkup_idx_params {
-- 
2.43.0

Powered by blists - more mailing lists