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

[Jacob Keller <jacob.e.keller@intel.com>]

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

>>   	/* Read async Admin Command response */
>> -	if ((hicr & IXGBE_PF_HICR_EV) && !(hicr & IXGBE_PF_HICR_C)) {
>> -		for (i = 0; i < IXGBE_ACI_DESC_SIZE_IN_DWORDS; i++) {
>> +	if ((hicr & IXGBE_PF_HICR_EV) && !(hicr & IXGBE_PF_HICR_C))
>> +		for (i = 0; i < IXGBE_ACI_DESC_SIZE_IN_DWORDS; i++)
>>   			raw_desc[i] = IXGBE_READ_REG(hw, IXGBE_PF_HIDA_2(i));
>> -			raw_desc[i] = raw_desc[i];
> 
> and it exists twice. Which makes it even odder....
> 
>      Andrew