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 <[email protected]>
---
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 <[email protected]>
With the comments above addressed:
Reviewed-by: Paul Menzel <[email protected]>
Kind regards,
Paul
