RE: [PATCH iwl-next v2 4/8] ice: allow overriding lan_en, lb_en in switch

Loktionov, Aleksandr
2025-12-01T07:37:05Z

> -----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;

?