[PATCH net-next] ixgbe: avoid redundant call to ixgbe_non_sfp_link_config()

[PATCH net-next] ixgbe: avoid redundant call to ixgbe_non_sfp_link_config()

[Alok Tiwari]

ixgbe_non_sfp_link_config() is called twice in ixgbe_open()
once to assign its return value to err and again in the
conditional check. This patch uses the stored err value
instead of calling the function a second time. This avoids
redundant work and ensures consistent error reporting.

Also fix a small typo in the ixgbe_remove() comment:
"The could be caused" -> "This could be caused".

Signed-off-by: Alok Tiwari <alok.a.tiwari@oracle.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 90d4e57b1c93..39ef604af3eb 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -7449,7 +7449,7 @@ int ixgbe_open(struct net_device *netdev)
 					 adapter->hw.link.link_info.link_cfg_err);
 
 		err = ixgbe_non_sfp_link_config(&adapter->hw);
-		if (ixgbe_non_sfp_link_config(&adapter->hw))
+		if (err)
 			e_dev_err("Link setup failed, err %d.\n", err);
 	}
 
@@ -12046,7 +12046,7 @@ static int ixgbe_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
  * @pdev: PCI device information struct
  *
  * ixgbe_remove is called by the PCI subsystem to alert the driver
- * that it should release a PCI device.  The could be caused by a
+ * that it should release a PCI device.  This could be caused by a
  * Hot-Plug event, or because the driver is going to be removed from
  * memory.
  **/
-- 
2.50.1

Re: [PATCH net-next] ixgbe: avoid redundant call to ixgbe_non_sfp_link_config()

[Simon Horman]

On Wed, Sep 24, 2025 at 12:33:54PM -0700, Alok Tiwari wrote:
> ixgbe_non_sfp_link_config() is called twice in ixgbe_open()
> once to assign its return value to err and again in the
> conditional check. This patch uses the stored err value
> instead of calling the function a second time. This avoids
> redundant work and ensures consistent error reporting.
> 
> Also fix a small typo in the ixgbe_remove() comment:
> "The could be caused" -> "This could be caused".
> 
> Signed-off-by: Alok Tiwari <alok.a.tiwari@oracle.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 90d4e57b1c93..39ef604af3eb 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -7449,7 +7449,7 @@ int ixgbe_open(struct net_device *netdev)
>  					 adapter->hw.link.link_info.link_cfg_err);
>  
>  		err = ixgbe_non_sfp_link_config(&adapter->hw);
> -		if (ixgbe_non_sfp_link_config(&adapter->hw))
> +		if (err)
>  			e_dev_err("Link setup failed, err %d.\n", err);
>  	}
>  

I am wondering if there is some intended side-effect of
calling ixgbe_non_sfp_link_config() twice.

...

Re: [PATCH net-next] ixgbe: avoid redundant call to ixgbe_non_sfp_link_config()

[Jacob Keller]

[-- Attachment #1.1: Type: text/plain, Size: 2024 bytes --]



On 9/25/2025 3:23 AM, Simon Horman wrote:
> On Wed, Sep 24, 2025 at 12:33:54PM -0700, Alok Tiwari wrote:
>> ixgbe_non_sfp_link_config() is called twice in ixgbe_open()
>> once to assign its return value to err and again in the
>> conditional check. This patch uses the stored err value
>> instead of calling the function a second time. This avoids
>> redundant work and ensures consistent error reporting.
>>
>> Also fix a small typo in the ixgbe_remove() comment:
>> "The could be caused" -> "This could be caused".
>>
>> Signed-off-by: Alok Tiwari <alok.a.tiwari@oracle.com>
>> ---
>>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index 90d4e57b1c93..39ef604af3eb 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -7449,7 +7449,7 @@ int ixgbe_open(struct net_device *netdev)
>>  					 adapter->hw.link.link_info.link_cfg_err);
>>  
>>  		err = ixgbe_non_sfp_link_config(&adapter->hw);
>> -		if (ixgbe_non_sfp_link_config(&adapter->hw))
>> +		if (err)
>>  			e_dev_err("Link setup failed, err %d.\n", err);
>>  	}
>>  
> 
> I am wondering if there is some intended side-effect of
> calling ixgbe_non_sfp_link_config() twice.
> 

Good question.

It looks like this was introduced by 4600cdf9f5ac ("ixgbe: Enable link
management in E610 device") which added the calls to ixgbe_open. Of
interest, we do also call this function in ixgbe_up_complete which is
called by ixgbe_open, but only if ixgbe_is_sfp() is false. Not sure why
E610 needs special casing here.

I don't see a reason we need two calls, it looks redundant, and even if
it has some necessary side effect.. that should at least deserve a
comment explaining why.

Hopefully someone from the ixgbe team can pipe in and explain or ACK
this change.

> ...
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

RE: [PATCH net-next] ixgbe: avoid redundant call to ixgbe_non_sfp_link_config()

[Jagielski, Jedrzej]

From: Keller, Jacob E <jacob.e.keller@intel.com> 
Sent: Tuesday, September 30, 2025 1:04 AM
>On 9/25/2025 3:23 AM, Simon Horman wrote:
>> On Wed, Sep 24, 2025 at 12:33:54PM -0700, Alok Tiwari wrote:
>>> ixgbe_non_sfp_link_config() is called twice in ixgbe_open()
>>> once to assign its return value to err and again in the
>>> conditional check. This patch uses the stored err value
>>> instead of calling the function a second time. This avoids
>>> redundant work and ensures consistent error reporting.
>>>
>>> Also fix a small typo in the ixgbe_remove() comment:
>>> "The could be caused" -> "This could be caused".
>>>
>>> Signed-off-by: Alok Tiwari <alok.a.tiwari@oracle.com>
>>> ---
>>>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> index 90d4e57b1c93..39ef604af3eb 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>> @@ -7449,7 +7449,7 @@ int ixgbe_open(struct net_device *netdev)
>>>  					 adapter->hw.link.link_info.link_cfg_err);
>>>  
>>>  		err = ixgbe_non_sfp_link_config(&adapter->hw);
>>> -		if (ixgbe_non_sfp_link_config(&adapter->hw))
>>> +		if (err)
>>>  			e_dev_err("Link setup failed, err %d.\n", err);
>>>  	}
>>>  
>> 
>> I am wondering if there is some intended side-effect of
>> calling ixgbe_non_sfp_link_config() twice.
>> 
>
>Good question.
>
>It looks like this was introduced by 4600cdf9f5ac ("ixgbe: Enable link
>management in E610 device") which added the calls to ixgbe_open. Of
>interest, we do also call this function in ixgbe_up_complete which is
>called by ixgbe_open, but only if ixgbe_is_sfp() is false. Not sure why
>E610 needs special casing here.
>
>I don't see a reason we need two calls, it looks redundant, and even if
>it has some necessary side effect.. that should at least deserve a
>comment explaining why.
>
>Hopefully someone from the ixgbe team can pipe in and explain or ACK
>this change.


Thanks for your vigilance! :) but i am afraid there is no reason for
having it doubled here

Unfortunately it looks like it has been introduced by mistake
and is indeed redundant.

Reviewed-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>

>
>> ...
>> 

Re: [Intel-wired-lan] [PATCH net-next] ixgbe: avoid redundant call to ixgbe_non_sfp_link_config()

[Paul Menzel]

Dear Alok, dear Simon, dear Jake, dear Jedrzej,


Thank you for your patch and review.

Am 30.09.25 um 10:33 schrieb Jagielski, Jedrzej:
> From: Keller, Jacob E
> Sent: Tuesday, September 30, 2025 1:04 AM
>> On 9/25/2025 3:23 AM, Simon Horman wrote:
>>> On Wed, Sep 24, 2025 at 12:33:54PM -0700, Alok Tiwari wrote:
>>>> ixgbe_non_sfp_link_config() is called twice in ixgbe_open()
>>>> once to assign its return value to err and again in the
>>>> conditional check. This patch uses the stored err value
>>>> instead of calling the function a second time. This avoids
>>>> redundant work and ensures consistent error reporting.

Using 75/75 characters per line would save a line.

Also, following up on the discussion, resending the patch with a 
comment, that calling this twice was not done intentionally would be great.

>>>> Also fix a small typo in the ixgbe_remove() comment:
>>>> "The could be caused" -> "This could be caused".

Personally I prefer separate patches for such things, making reverting 
easier.

>>>> Signed-off-by: Alok Tiwari <alok.a.tiwari@oracle.com>
>>>> ---
>>>>   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 4 ++--
>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> index 90d4e57b1c93..39ef604af3eb 100644
>>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> @@ -7449,7 +7449,7 @@ int ixgbe_open(struct net_device *netdev)
>>>>   					 adapter->hw.link.link_info.link_cfg_err);
>>>>   
>>>>   		err = ixgbe_non_sfp_link_config(&adapter->hw);
>>>> -		if (ixgbe_non_sfp_link_config(&adapter->hw))
>>>> +		if (err)
>>>>   			e_dev_err("Link setup failed, err %d.\n", err);
>>>>   	}
>>>>   
>>>
>>> I am wondering if there is some intended side-effect of
>>> calling ixgbe_non_sfp_link_config() twice.
>>>
>>
>> Good question.
>>
>> It looks like this was introduced by 4600cdf9f5ac ("ixgbe: Enable link
>> management in E610 device") which added the calls to ixgbe_open. Of
>> interest, we do also call this function in ixgbe_up_complete which is
>> called by ixgbe_open, but only if ixgbe_is_sfp() is false. Not sure why
>> E610 needs special casing here.
>>
>> I don't see a reason we need two calls, it looks redundant, and even if
>> it has some necessary side effect.. that should at least deserve a
>> comment explaining why.
>>
>> Hopefully someone from the ixgbe team can pipe in and explain or ACK
>> this change.
> 
> Thanks for your vigilance! :) but i am afraid there is no reason for
> having it doubled here
> 
> Unfortunately it looks like it has been introduced by mistake
> and is indeed redundant.
> 
> Reviewed-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>

With the comments above addressed:

Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul

RE: [Intel-wired-lan] [PATCH net-next] ixgbe: avoid redundant call to ixgbe_non_sfp_link_config()

[Rinitha, SX]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Alok Tiwari
> Sent: 25 September 2025 01:04
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net; edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; horms@kernel.org; netdev@vger.kernel.org; intel-wired-lan@lists.osuosl.org
> Cc: alok.a.tiwari@oracle.com
> Subject: [Intel-wired-lan] [PATCH net-next] ixgbe: avoid redundant call to ixgbe_non_sfp_link_config()
>
> ixgbe_non_sfp_link_config() is called twice in ixgbe_open() once to assign its return value to err and again in the conditional check. This patch uses the stored err value instead of calling the function a second time. This avoids redundant work and ensures consistent error reporting.
>
> Also fix a small typo in the ixgbe_remove() comment:
> "The could be caused" -> "This could be caused".
>
> Signed-off-by: Alok Tiwari <alok.a.tiwari@oracle.com>
> ---
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>

Tested-by: Rinitha S <sx.rinitha@intel.com> (A Contingent worker at Intel)