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!
