RE: [Intel-wired-lan] [PATCH net-next] ixgbe: Remove redundant self-assignments in ACI command execution

From: Kwapulinski, Piotr
Date: Tue Jan 14 2025 - 07:33:43 EST

>-----Original Message-----
>From: Dheeraj Reddy Jonnalagadda <dheeraj.linuxdev@xxxxxxxxx>
>Sent: Tuesday, January 14, 2025 1:23 AM
>To: Kwapulinski, Piotr <piotr.kwapulinski@xxxxxxxxx>
>Cc: andrew+netdev@xxxxxxx; Nguyen, Anthony L <anthony.l.nguyen@xxxxxxxxx>; davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; intel-wired-lan@xxxxxxxxxxxxxxxx; kuba@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; pabeni@xxxxxxxxxx; Kitszel, Przemyslaw <przemyslaw.kitszel@xxxxxxxxx>; Swiatkowski, Michal <michal.swiatkowski@xxxxxxxxx>
>Subject: Re: [Intel-wired-lan] [PATCH net-next] ixgbe: Remove redundant self-assignments in ACI command execution
>
>On Mon, Jan 13, 2025 at 03:23:31PM +0000, Kwapulinski, Piotr wrote:
>> >[Intel-wired-lan] [PATCH net-next] ixgbe: Remove redundant
>> >self-assignments in ACI command execution @ 2025-01-08 5:36 Dheeraj
>> >Reddy Jonnalagadda
>> > 2025-01-08 6:29 ` Michal Swiatkowski
>> > 0 siblings, 1 reply; 2+ messages in thread
>> >From: Dheeraj Reddy Jonnalagadda @ 2025-01-08 5:36 UTC (permalink /
>> >raw)
>> > To: anthony.l.nguyen, przemyslaw.kitszel
>> > Cc: andrew+netdev, davem, edumazet, kuba, pabeni, intel-wired-lan,
>> > netdev, linux-kernel, Dheeraj Reddy Jonnalagadda
>> >
>> >Remove redundant statements in ixgbe_aci_send_cmd_execute() where
>> >raw_desc[i] is assigned to itself. These self-assignments have no
>> >effect and can be safely removed.
>> >
>> >Fixes: 46761fd52a88 ("ixgbe: Add support for E610 FW Admin Command
>> >Interface")
>> >Closes:
>> >https://scan7.scan.coverity.com/#/project-view/52337/11354?selectedIs
>> >sue=1602757
>> >Signed-off-by: Dheeraj Reddy Jonnalagadda
>> >dheeraj.linuxdev@xxxxxxxxx<mailto:dheeraj.linuxdev@xxxxxxxxx>
>> >---
>> > drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c | 2 --
>> > 1 file changed, 2 deletions(-)
>> >
>> >diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>> >b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>> >index 683c668672d6..408c0874cdc2 100644
>> >--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>> >+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>> >@@ -145,7 +145,6 @@ static int ixgbe_aci_send_cmd_execute(struct ixgbe_hw *hw,
>> > 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];
>> > }
>> > }
>> >
>> >@@ -153,7 +152,6 @@ static int ixgbe_aci_send_cmd_execute(struct ixgbe_hw *hw,
>> > 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];
>> > }
>> > }
>> >
>>
>> Hello,
>> Possible solution may be as follows. I may also prepare the fix myself. Please let me know.
>> Thanks,
>> Piotr
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>> index e0f773c..af51e5a 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>> @@ -113,7 +113,8 @@ static int ixgbe_aci_send_cmd_execute(struct
>> ixgbe_hw *hw,
>>
>> /* Descriptor is written to specific registers */
>> for (i = 0; i < IXGBE_ACI_DESC_SIZE_IN_DWORDS; i++)
>> - IXGBE_WRITE_REG(hw, IXGBE_PF_HIDA(i), raw_desc[i]);
>> + IXGBE_WRITE_REG(hw, IXGBE_PF_HIDA(i),
>> + le32_to_cpu(raw_desc[i]));
>>
>> /* SW has to set PF_HICR.C bit and clear PF_HICR.SV and
>> * PF_HICR_EV
>> @@ -145,7 +146,7 @@ static int ixgbe_aci_send_cmd_execute(struct ixgbe_hw *hw,
>> 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];
>> + raw_desc[i] = cpu_to_le32(raw_desc[i]);
>> }
>> }
>>
>> @@ -153,7 +154,7 @@ static int ixgbe_aci_send_cmd_execute(struct ixgbe_hw *hw,
>> 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];
>> + raw_desc[i] = cpu_to_le32(raw_desc[i]);
>> }
>> }
>>
>
>Hello Piotr,
>
>Thank you for suggesting the fix. I will prepare the new patch and send it over.
>
>-Dheeraj

Hello,
As a result of internal review from Przemek, it may be improved as follows:

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
index e0f773c..0ec944c 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
@@ -113,7 +113,8 @@ static int ixgbe_aci_send_cmd_execute(struct ixgbe_hw *hw,

/* Descriptor is written to specific registers */
for (i = 0; i < IXGBE_ACI_DESC_SIZE_IN_DWORDS; i++)
- IXGBE_WRITE_REG(hw, IXGBE_PF_HIDA(i), raw_desc[i]);
+ IXGBE_WRITE_REG(hw, IXGBE_PF_HIDA(i),
+ cpu_to_le32(raw_desc[i]));

/* SW has to set PF_HICR.C bit and clear PF_HICR.SV and
* PF_HICR_EV
@@ -145,7 +146,7 @@ static int ixgbe_aci_send_cmd_execute(struct ixgbe_hw *hw,
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];
+ raw_desc[i] = le32_to_cpu(raw_desc[i]);
}
}

@@ -153,7 +154,7 @@ static int ixgbe_aci_send_cmd_execute(struct ixgbe_hw *hw,
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];
+ raw_desc[i] = le32_to_cpu(raw_desc[i]);
}
}

Thank you,
Piotr