netdev - RE: [PATCH net-next] ixgbe: avoid redundant call to ixgbe_non_sfp_link

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <PH0PR11MB59024649641B7ACB09ACD402F01AA@PH0PR11MB5902.namprd11.prod.outlook.com>
Date: Tue, 30 Sep 2025 08:33:15 +0000
From: "Jagielski, Jedrzej" <jedrzej.jagielski@...el.com>
To: "Keller, Jacob E" <jacob.e.keller@...el.com>, Simon Horman
	<horms@...nel.org>, Alok Tiwari <alok.a.tiwari@...cle.com>, "Kwapulinski,
 Piotr" <piotr.kwapulinski@...el.com>
CC: "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>, "davem@...emloft.net"
	<davem@...emloft.net>, "edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>
Subject: RE: [PATCH net-next] ixgbe: avoid redundant call to
 ixgbe_non_sfp_link_config()

From: Keller, Jacob E <jacob.e.keller@...el.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@...cle.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@...el.com>

>
>> ...
>>