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

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

[Andrew Lunn]

>  	/* 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?

>  	/* 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

[PATCH iwl-next] ixgbe: e610: remove redundant assignment

[Piotr Kwapulinski]

Remove unnecessary code. No functional impact.

Fixes: 46761fd52a88 ("ixgbe: Add support for E610 FW Admin Command Interface")
Signed-off-by: Piotr Kwapulinski <piotr.kwapulinski@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
index c2f8189..6f380fc 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
@@ -142,20 +142,14 @@ static int ixgbe_aci_send_cmd_execute(struct ixgbe_hw *hw,
 			  IXGBE_PF_HICR);
 
 	/* 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];
-		}
-	}
 
 	/* 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];
-		}
-	}
 
 	/* Handle timeout and invalid state of HICR register */
 	if (hicr & IXGBE_PF_HICR_C)
-- 
2.47.1

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

[Jacob Keller]

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

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

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
> Of Piotr Kwapulinski
> Sent: Thursday, January 29, 2026 6:55 PM
> To: intel-wired-lan@lists.osuosl.org
> Cc: netdev@vger.kernel.org; dan.carpenter@linaro.org;
> horms@kernel.org; pmenzel@molgen.mpg.de; Kwapulinski, Piotr
> <piotr.kwapulinski@intel.com>
> Subject: [Intel-wired-lan] [PATCH iwl-next] ixgbe: e610: remove
> redundant assignment
> 
> Remove unnecessary code. No functional impact.
> 
> Fixes: 46761fd52a88 ("ixgbe: Add support for E610 FW Admin Command
> Interface")
> Signed-off-by: Piotr Kwapulinski <piotr.kwapulinski@intel.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
> b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
> index c2f8189..6f380fc 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
> @@ -142,20 +142,14 @@ static int ixgbe_aci_send_cmd_execute(struct
> ixgbe_hw *hw,
>  			  IXGBE_PF_HICR);
> 
>  	/* 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];
> -		}
> -	}
> 
>  	/* 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];
> -		}
> -	}
> 
>  	/* Handle timeout and invalid state of HICR register */
>  	if (hicr & IXGBE_PF_HICR_C)
> --
> 2.47.1

Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

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

[Kwapulinski, Piotr]

>-----Original Message-----
>From: Keller, Jacob E <jacob.e.keller@intel.com> 
>Sent: Thursday, January 29, 2026 7:11 PM
>To: Andrew Lunn <andrew@lunn.ch>; Kwapulinski, Piotr <piotr.kwapulinski@intel.com>
>Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; dan.carpenter@linaro.org; horms@kernel.org; pmenzel@molgen.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

[...]

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

[Jacob Keller]

On 2/5/2026 12:35 AM, Kwapulinski, Piotr wrote:
>> -----Original Message-----
>> From: Keller, Jacob E <jacob.e.keller@intel.com>
>> Sent: Thursday, January 29, 2026 7:11 PM
>> To: Andrew Lunn <andrew@lunn.ch>; Kwapulinski, Piotr <piotr.kwapulinski@intel.com>
>> Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; dan.carpenter@linaro.org; horms@kernel.org; pmenzel@molgen.mpg.de
>> Subject: Re: [Intel-wired-lan] [PATCH iwl-next] ixgbe: e610: remove redundant assignment
>>
>> 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
> 

Makes sense to me.

Thanks,
Jake

> [...]