netdev - Re: [Intel-wired-lan] [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: <c86bccd6-9e9e-4355-8e3b-81df181d3c44@molgen.mpg.de>
Date: Thu, 2 Oct 2025 09:06:22 +0200
From: Paul Menzel <pmenzel@...gen.mpg.de>
To: Jedrzej Jagielski <jedrzej.jagielski@...el.com>,
 Jacob E Keller <jacob.e.keller@...el.com>, Simon Horman <horms@...nel.org>,
 Alok Tiwari <alok.a.tiwari@...cle.com>,
 Piotr Kwapulinski <piotr.kwapulinski@...el.com>
Cc: Anthony L Nguyen <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: [Intel-wired-lan] [PATCH net-next] ixgbe: avoid redundant call to
 ixgbe_non_sfp_link_config()

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@...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>

With the comments above addressed:

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


Kind regards,

Paul