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@intel.com>
---
Dropping reviewed-by from Michał due to changes.
Changes in v2:
- Use FIELD_GET et al. when handling fi.lb_en and fi.lan_en.
- Rename /LB_LAN/s/_MASK/_M/ because one of uses would need to break
line.
---
drivers/net/ethernet/intel/ice/ice_switch.c | 21 +++++++++++++--------
drivers/net/ethernet/intel/ice/ice_switch.h | 8 ++++++++
2 files changed, 21 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c
index 04e5d653efce..b3f5cda1571e 100644
--- a/drivers/net/ethernet/intel/ice/ice_switch.c
+++ b/drivers/net/ethernet/intel/ice/ice_switch.c
@@ -2538,8 +2538,9 @@ int ice_get_initial_sw_cfg(struct ice_hw *hw)
*/
static void ice_fill_sw_info(struct ice_hw *hw, struct ice_fltr_info *fi)
{
- fi->lb_en = false;
- fi->lan_en = false;
+ bool lan_en = false;
+ bool lb_en = false;
+
if ((fi->flag & ICE_FLTR_TX) &&
(fi->fltr_act == ICE_FWD_TO_VSI ||
fi->fltr_act == ICE_FWD_TO_VSI_LIST ||
@@ -2549,7 +2550,7 @@ 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;
+ lb_en = true;
/* Set lan_en to TRUE if
* 1. The switch is a VEB AND
@@ -2578,14 +2579,18 @@ 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;
+ lan_en = true;
} else {
- fi->lan_en = true;
+ lan_en = true;
}
}
if (fi->flag & ICE_FLTR_TX_ONLY)
- fi->lan_en = false;
+ lan_en = false;
+ 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);
}
/**
@@ -2669,9 +2674,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..b694c131ad58 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;
--
2.43.0
> -----Original Message-----
> From: Slepecki, Jakub <jakub.slepecki@intel.com>
> Sent: Tuesday, November 25, 2025 9:35 AM
> To: intel-wired-lan@lists.osuosl.org
> Cc: linux-kernel@vger.kernel.org; netdev@vger.kernel.org; Kitszel,
> Przemyslaw <przemyslaw.kitszel@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; michal.swiatkowski@linux.intel.com; Slepecki,
> Jakub <jakub.slepecki@intel.com>; Loktionov, Aleksandr
> <aleksandr.loktionov@intel.com>
> 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@intel.com>
>
> ---
> Dropping reviewed-by from Michał due to changes.
>
> Changes in v2:
> - Use FIELD_GET et al. when handling fi.lb_en and fi.lan_en.
> - Rename /LB_LAN/s/_MASK/_M/ because one of uses would need to break
> line.
> ---
...
> if (fi->flag & ICE_FLTR_TX_ONLY)
> - fi->lan_en = false;
> + lan_en = false;
> + 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,
fi->lb_en and fi->lan_en are declared as bool in struct ice_fltr_info, but you are now treating them as bitfields using FIELD_GET and FIELD_MODIFY.
I realize it could be something like:
struct ice_fltr_info {
...
u8 lb_lan_flags; /* bitfield: value + force */
...
};
#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, 1) | \
FIELD_PREP_CONST(ICE_FLTR_INFO_LB_LAN_FORCE_M, 1))
#define ICE_FLTR_INFO_LB_LAN_FORCE_DISABLED \
(FIELD_PREP_CONST(ICE_FLTR_INFO_LB_LAN_FORCE_M, 1))
...
On 2025-11-25 9:59, Loktionov, Aleksandr wrote:
>> if (fi->flag & ICE_FLTR_TX_ONLY)
>> - fi->lan_en = false;
>> + lan_en = false;
>> + 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);
> fi->lb_en and fi->lan_en are declared as bool in struct ice_fltr_info,
> but you are now treating them as bitfields using FIELD_GET and
> FIELD_MODIFY.
I don't see what you mean here. Both members are u8 without a bit-field
declaration. Or do you mean they are used as bool or maybe the _en
suffix?
> I realize it could be something like:
> struct ice_fltr_info {
> ...
> u8 lb_lan_flags; /* bitfield: value + force */
> ...
> };
What I see from this sample is that you want me to: pack them, change
their name, and change their description. Is this correct?
I fully agree about the description. It's my mistake I left it as-is.
I'll update it according to the overall changes.
I don't think packing them is worth it. The memory gain is negligible
and the cost is primarily in readability and consistency: we've always
had two fields for these with clear responsibility for each, names
match with datasheet (both "lan en" and "lb en" will hit Table 7-12.),
and packing them would require twice as many constants.
Would the clarification in the description be enough to address your
concerns? Something like (please ignore bad line breaks):
struct ice_fltr_info {
...
/* Rule creation will populate VALUE bit of these members based on switch
* type if their FORCE bit is not set.
*/
u8 lb_en; /* VALUE bit: packet can be looped back */
u8 lan_en; /* VALUE bit: packet can be forwarded to uplink */
};
> #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, 1) | \
> FIELD_PREP_CONST(ICE_FLTR_INFO_LB_LAN_FORCE_M, 1))
> #define ICE_FLTR_INFO_LB_LAN_FORCE_DISABLED \
> (FIELD_PREP_CONST(ICE_FLTR_INFO_LB_LAN_FORCE_M, 1))
Does this mean you want me to use {1,0} instead of {true,false}?
In ice_fill_sw_info() I'd prefer to keep them as boolean because they are
semantically correct: we're calculating defaults and then we apply them if
specific values are not forced elsewhere. Maybe a comment or docs change
would be more in place? In ICE_FLTR_INFO_LB_LAN_FORCE_{ENABLED,DISABLED},
I used boolean to stay consistent with the ice_fill_sw_info().
But it's not a strong preference. If it's preferable, I'll change it
to {1,0} across the patch.
Thanks!
> -----Original Message-----
> From: Slepecki, Jakub <jakub.slepecki@intel.com>
> Sent: Friday, November 28, 2025 12:56 PM
> To: Loktionov, Aleksandr <aleksandr.loktionov@intel.com>; intel-wired-
> lan@lists.osuosl.org
> Cc: linux-kernel@vger.kernel.org; netdev@vger.kernel.org; Kitszel,
> Przemyslaw <przemyslaw.kitszel@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; michal.swiatkowski@linux.intel.com
> Subject: Re: [PATCH iwl-next v2 4/8] ice: allow overriding lan_en,
> lb_en in switch
>
> On 2025-11-25 9:59, Loktionov, Aleksandr wrote:
> >> if (fi->flag & ICE_FLTR_TX_ONLY)
> >> - fi->lan_en = false;
> >> + lan_en = false;
> >> + 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);
> > fi->lb_en and fi->lan_en are declared as bool in struct
> ice_fltr_info,
> > but you are now treating them as bitfields using FIELD_GET and
> > FIELD_MODIFY.
>
> I don't see what you mean here. Both members are u8 without a bit-
> field declaration. Or do you mean they are used as bool or maybe the
> _en suffix?
>
> > I realize it could be something like:
> > struct ice_fltr_info {
> > ...
> > u8 lb_lan_flags; /* bitfield: value + force */
> > ...
> > };
>
> What I see from this sample is that you want me to: pack them, change
> their name, and change their description. Is this correct?
>
> I fully agree about the description. It's my mistake I left it as-is.
> I'll update it according to the overall changes.
>
> I don't think packing them is worth it. The memory gain is negligible
> and the cost is primarily in readability and consistency: we've always
> had two fields for these with clear responsibility for each, names
> match with datasheet (both "lan en" and "lb en" will hit Table 7-12.),
> and packing them would require twice as many constants.
>
> Would the clarification in the description be enough to address your
> concerns? Something like (please ignore bad line breaks):
>
> struct ice_fltr_info {
> ...
> /* Rule creation will populate VALUE bit of these members based
> on switch
> * type if their FORCE bit is not set.
> */
> u8 lb_en; /* VALUE bit: packet can be looped back */
> u8 lan_en; /* VALUE bit: packet can be forwarded to uplink
> */
> };
>
> > #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, 1) | \
> > FIELD_PREP_CONST(ICE_FLTR_INFO_LB_LAN_FORCE_M, 1)) #define
> > ICE_FLTR_INFO_LB_LAN_FORCE_DISABLED \
> > (FIELD_PREP_CONST(ICE_FLTR_INFO_LB_LAN_FORCE_M, 1))
>
> Does this mean you want me to use {1,0} instead of {true,false}?
>
> In ice_fill_sw_info() I'd prefer to keep them as boolean because they
> are semantically correct: we're calculating defaults and then we apply
> them if specific values are not forced elsewhere. Maybe a comment or
> docs change would be more in place? In
> ICE_FLTR_INFO_LB_LAN_FORCE_{ENABLED,DISABLED},
> I used boolean to stay consistent with the ice_fill_sw_info().
>
> But it's not a strong preference. If it's preferable, I'll change it
> to {1,0} across the patch.
>
> Thanks!
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.
- If FORCE/ VALUE semantics are needed, either:
+Introduce a dedicated flags field with proper bitmask macros, OR
+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;
?
© 2016 - 2025 Red Hat, Inc.