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

[Dheeraj Reddy Jonnalagadda <dheeraj.linuxdev@gmail.com>]

On Tue, Jan 14, 2025 at 12:32:47PM +0000, Kwapulinski, Piotr wrote:
> >-----Original Message-----
> >From: Dheeraj Reddy Jonnalagadda <dheeraj.linuxdev@gmail.com> 
> >Sent: Tuesday, January 14, 2025 1:23 AM
> >To: Kwapulinski, Piotr <piotr.kwapulinski@intel.com>
> >Cc: andrew+netdev@lunn.ch; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net; edumazet@google.com; intel-wired-lan@lists.osuosl.org; kuba@kernel.org; linux-kernel@vger.kernel.org; netdev@vger.kernel.org; pabeni@redhat.com; Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>; Swiatkowski, Michal <michal.swiatkowski@intel.com>
> >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@gmail.com<mailto:dheeraj.linuxdev@gmail.com>
> >> >---
> >> > 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

Hello Piotr,

Thank you. I will update the patch accordingly and send it over.

-Dheeraj