netdev - RE: [Intel-wired-lan] [PATCH iwl-next] ixgbe: e610: remove redundant assignment

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <PH7PR11MB59833D1AE8A3EC0AB6F87901F399A@PH7PR11MB5983.namprd11.prod.outlook.com>
Date: Thu, 5 Feb 2026 08:35:14 +0000
From: "Kwapulinski, Piotr" <piotr.kwapulinski@...el.com>
To: "Keller, Jacob E" <jacob.e.keller@...el.com>, Andrew Lunn <andrew@...n.ch>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>, "dan.carpenter@...aro.org"
	<dan.carpenter@...aro.org>, "horms@...nel.org" <horms@...nel.org>,
	"pmenzel@...gen.mpg.de" <pmenzel@...gen.mpg.de>
Subject: RE: [Intel-wired-lan] [PATCH iwl-next] ixgbe: e610: remove redundant
 assignment

>-----Original Message-----
>From: Keller, Jacob E <jacob.e.keller@...el.com> 
>Sent: Thursday, January 29, 2026 7:11 PM
>To: Andrew Lunn <andrew@...n.ch>; Kwapulinski, Piotr <piotr.kwapulinski@...el.com>
>Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org; dan.carpenter@...aro.org; horms@...nel.org; pmenzel@...gen.mpg.de
>Subject: Re: [Intel-wired-lan] [PATCH iwl-next] ixgbe: e610: remove redundant assignment
>
>
>
>On 1/29/2026 9:44 AM, Andrew Lunn wrote:
>>>   	/* Read sync Admin Command response */
>>> -	if ((hicr & IXGBE_PF_HICR_SV)) {
>>> -		for (i = 0; i < IXGBE_ACI_DESC_SIZE_IN_DWORDS; i++) {
>>> +	if ((hicr & IXGBE_PF_HICR_SV))
>>> +		for (i = 0; i < IXGBE_ACI_DESC_SIZE_IN_DWORDS; i++)
>>>   			raw_desc[i] = IXGBE_READ_REG(hw, IXGBE_PF_HIDA(i));
>>> -			raw_desc[i] = raw_desc[i];
>> 
>> Did you look through the history? When i see code like this it makes 
>> me want to have an understanding why it exists, since it looks so odd.
>> 
>> Is it a merge conflict resolution gone bad? Is it a typo and there is 
>> a cooked_desc[i] which could be set?
>> 
>
>Nope. Looking at git blame for the kernel, this appears to have been here since its submission. I then went and checked what was done in the out-of-tree releases out of curiosity, and it looks like there was originally a CPU_TO_LE32 macro which was doing byte swaps, and an equivalent when writing the data in to the registers.
>
>raw_desc[1] = read_reg(...);
>raw_desc[1] = cpu_to_le32(raw_desc[i]);
>
>I suspect the byte swapping got removed from the original upstream submission but no one noticed the extra assignment.
>
>Of course the register accesses always read the values in host order, and I can't imagine an OS not doing that...
>
>Hmm.. But now I have some questions...
>
>The raw_desc value comes from the desc parameter of the function. Thats libie_aq_desc now, and  its defined using __le* values..
>
>We're chunking up the descriptor buffer and writing it to a bunch of register blocks, and we don't convert from LE32 to CPU now... so on a BigEndian system this is going to not swap the values properly...
>
>Which makes me think the original change would be required on BE32 systems.. But.. even worse..
>
>I don't think we actually can just blindly copy the values as chunks of
>4 bytes and byte swap them.. That would re-arrange the fields, since the structure layout uses __le16:
>
>> struct libie_aq_desc {
>>         __le16  flags;
>>         __le16  opcode;
>
>These would get swapped out of order.
>
>>         __le16  datalen;
>>         __le16  retval;
>>         __le32  cookie_high;
>>         __le32  cookie_low;
>>         union {
>>                 u8      raw[16];
>>                 struct  libie_aqc_generic generic;
>>                 struct  libie_aqc_get_ver get_ver;
>>                 struct  libie_aqc_driver_ver driver_ver;
>>                 struct  libie_aqc_req_res res_owner;
>>                 struct  libie_aqc_list_caps get_cap;
>>                 struct  libie_aqc_fw_log fw_log;
>>         } params;
>> };
>> LIBIE_CHECK_STRUCT_LEN(32, libie_aq_desc);
>
>I actually don't know which is "correct", as I don't really understand what the register interface expects, and how it will get interpreted by the firmware...
>
>Maybe the byteswap of each 4-byte block is right? but I'm really uncertain now...
>
>Presumably it expects flags first and then opcode? we're writing it that way now on a LE system.. But on a BE system thats going to be byteswapped before going into the register.. so our 4byte chunk would end up potentially reversing the flags and opcode w.r.t what the firmware sees??
>
>Hmm.....
I'll work out the solution. Meanwhile, possibly it's good idea to take this patch since this code is unnecessary regardless of the final outcome.
Thanks,
Piotr

[...]