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.
> ...
>
OpenPGP_signature.asc
Description: OpenPGP digital signature
