netdev - Re: [PATCH iwl-next v2 4/8] ice: allow overriding lan_en, lb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <32fd9c75-e133-4f53-b839-101a579fd79f@intel.com>
Date: Fri, 28 Nov 2025 12:55:58 +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-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!