From: Keller, Jacob E <[email protected]> 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 <[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]> > >> ... >>
