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] [thread-next>] [day] [month] [year] [list]

Message-ID: <a7b1bc0a-26f0-4256-b52f-3580711be98f@intel.com>
Date: Mon, 29 Sep 2025 16:04:11 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Simon Horman <horms@...nel.org>, Alok Tiwari <alok.a.tiwari@...cle.com>,
	Jedrzej Jagielski <jedrzej.jagielski@...el.com>, Piotr Kwapulinski
	<piotr.kwapulinski@...el.com>
CC: <anthony.l.nguyen@...el.com>, <davem@...emloft.net>,
	<edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>,
	<netdev@...r.kernel.org>, <intel-wired-lan@...ts.osuosl.org>
Subject: Re: [PATCH net-next] ixgbe: avoid redundant call to
 ixgbe_non_sfp_link_config()



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.

> ...
> 



Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (237 bytes)