From: Paul Menzel <[email protected]> Sent: Wednesday, July 1, 2026 2:27 PM >Dear Jedrzej, > > >Thank you for your patch. > >Am 01.07.26 um 13:35 schrieb Jedrzej Jagielski: >> For the E610 family, similarly to the E8xx adapters, the default behavior >> is for the PHY link to remain up even when the corresponding OS interface >> is down. >> >> Add function setting down the PHY config IXGBE_ACI_PHY_ENA_LINK bit >> what leads to disabling PHY link. > >It’d extend it a little: > >… by factoring the code out into ixgbe_handle_link_down(), and call it >in ixgbe_close().
Hi Paul thanks for suggestions, sure, the commit msg will be extended. > >> Align functionality with the implementation of the ice driver. > >Please add a paragraph detailing regression potential. Are there users >that might depend on the current default, as uncommon it might be? There's no regression potential imho changing behavior is volountary and is done via ethtool i will rephrase it if that's unclear from the commit msg > >> Let user to configure link-down-on-close enablement through ethtool. > >Please provide examples, and how to test your change. Doing this you can >also paste the new log messages. > >> Reviewed-by: Aleksandr Loktionov <[email protected]> >> Signed-off-by: Jedrzej Jagielski <[email protected]> >> --- >> drivers/net/ethernet/intel/ixgbe/ixgbe.h | 1 + >> drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c | 35 ++++++++++++++++++- >> drivers/net/ethernet/intel/ixgbe/ixgbe_e610.h | 1 + >> .../net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 15 ++++++++ >> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 27 +++++++++++--- >> 5 files changed, 73 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h >> b/drivers/net/ethernet/intel/ixgbe/ixgbe.h >> index 30f62174acf2..7bbb82dd962c 100644 >> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h >> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h >> @@ -685,6 +685,7 @@ struct ixgbe_adapter { >> #define IXGBE_FLAG2_MOD_POWER_UNSUPPORTED BIT(22) >> #define IXGBE_FLAG2_API_MISMATCH BIT(23) >> #define IXGBE_FLAG2_FW_ROLLBACK BIT(24) >> +#define IXGBE_FLAG2_LINK_DOWN_ON_CLOSE BIT(25) >> >> /* Tx fast path data */ >> int num_tx_queues; >> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c >> b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c >> index da445fb673fc..46d8a3ea86b8 100644 >> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c >> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c >> @@ -1923,6 +1923,33 @@ void ixgbe_fc_autoneg_e610(struct ixgbe_hw *hw) >> hw->fc.current_mode = hw->fc.requested_mode; >> } >> >> +/** >> + * ixgbe_disable_phy_link - force phy link to get down >> + * @hw: pointer to hardware structure >> + * >> + * Send 0x0601 with the IXGBE_ACI_PHY_ENA_LINK bit set down. >> + * >> + * Return: the exit code of the operation. > >At least for me it’s not that helpful. Shouldn’t the return values be >listed? What is success? What is failure? Yeah i know that's not ideal but i also don't think mentioning all possible retvals of ixgbe_aci_get_phy_caps and ixgbe_aci_set_phy_cfg is the best way > >> + */ >> +int ixgbe_disable_phy_link(struct ixgbe_hw *hw) >> +{ >> + struct ixgbe_aci_cmd_get_phy_caps_data pcaps = {}; >> + struct ixgbe_aci_cmd_set_phy_cfg_data pcfg = {}; >> + int err; >> + >> + err = ixgbe_aci_get_phy_caps(hw, false, IXGBE_ACI_REPORT_ACTIVE_CFG, >> + &pcaps); >> + if (err) >> + return err; >> + >> + ixgbe_copy_phy_caps_to_cfg(&pcaps, &pcfg); >> + >> + pcfg.caps &= ~IXGBE_ACI_PHY_ENA_LINK; >> + pcfg.caps |= IXGBE_ACI_PHY_ENA_AUTO_LINK_UPDT; >> + >> + return ixgbe_aci_set_phy_cfg(hw, &pcfg); >> +} >> + >> /** >> * ixgbe_disable_rx_e610 - Disable RX unit >> * @hw: pointer to hardware structure >> @@ -2207,6 +2234,7 @@ int ixgbe_setup_phy_link_e610(struct ixgbe_hw *hw) >> u8 rmode = IXGBE_ACI_REPORT_TOPO_CAP_MEDIA; >> u64 sup_phy_type_low, sup_phy_type_high; >> u64 phy_type_low = 0, phy_type_high = 0; >> + bool force_on_required; >> int err; >> >> err = ixgbe_aci_get_link_info(hw, false, NULL); >> @@ -2272,6 +2300,11 @@ int ixgbe_setup_phy_link_e610(struct ixgbe_hw *hw) >> phy_type_high |= IXGBE_PHY_TYPE_HIGH_10G_USXGMII; >> } >> >> + /* If IXGBE_ACI_PHY_ENA_LINK has been explicitly disabled that means >> + * we need to force interface enablement after reaching that point > >It’d be great, if you rephrased “that point”. ok, will try > >> + */ >> + force_on_required = !(pcfg.caps & IXGBE_ACI_PHY_ENA_LINK); >> + >> /* Mask the set values to avoid requesting unsupported link types. */ >> phy_type_low &= sup_phy_type_low; >> pcfg.phy_type_low = cpu_to_le64(phy_type_low); >> @@ -2280,7 +2313,7 @@ int ixgbe_setup_phy_link_e610(struct ixgbe_hw *hw) >> >> if (pcfg.phy_type_high != pcaps.phy_type_high || >> pcfg.phy_type_low != pcaps.phy_type_low || >> - pcfg.caps != pcaps.caps) { >> + pcfg.caps != pcaps.caps || force_on_required) { >> pcfg.caps |= IXGBE_ACI_PHY_ENA_LINK; >> pcfg.caps |= IXGBE_ACI_PHY_ENA_AUTO_LINK_UPDT; >> >> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.h >> b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.h >> index 2cb76a3d30ae..59044d67ebeb 100644 >> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.h >> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.h >> @@ -50,6 +50,7 @@ int ixgbe_cfg_phy_fc(struct ixgbe_hw *hw, >> enum ixgbe_fc_mode req_mode); >> int ixgbe_setup_fc_e610(struct ixgbe_hw *hw); >> void ixgbe_fc_autoneg_e610(struct ixgbe_hw *hw); >> +int ixgbe_disable_phy_link(struct ixgbe_hw *hw); >> void ixgbe_disable_rx_e610(struct ixgbe_hw *hw); >> int ixgbe_init_phy_ops_e610(struct ixgbe_hw *hw); >> int ixgbe_identify_phy_e610(struct ixgbe_hw *hw); >> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c >> b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c >> index 4dfae53b4ea1..0fcb9d738984 100644 >> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c >> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c >> @@ -139,6 +139,8 @@ static const char >> ixgbe_priv_flags_strings[][ETH_GSTRING_LEN] = { >> "vf-ipsec", >> #define IXGBE_PRIV_FLAGS_AUTO_DISABLE_VF BIT(2) >> "mdd-disable-vf", >> +#define IXGBE_PRIV_LINK_DOWN_ON_CLOSE BIT(3) >> + "link-down-on-close", >> }; >> >> #define IXGBE_PRIV_FLAGS_STR_LEN ARRAY_SIZE(ixgbe_priv_flags_strings) >> @@ -3842,6 +3844,9 @@ static u32 ixgbe_get_priv_flags(struct net_device >> *netdev) >> if (adapter->flags2 & IXGBE_FLAG2_AUTO_DISABLE_VF) >> priv_flags |= IXGBE_PRIV_FLAGS_AUTO_DISABLE_VF; >> >> + if (adapter->flags2 & IXGBE_FLAG2_LINK_DOWN_ON_CLOSE) >> + priv_flags |= IXGBE_PRIV_LINK_DOWN_ON_CLOSE; >> + >> return priv_flags; >> } >> >> @@ -3879,6 +3884,16 @@ static int ixgbe_set_priv_flags(struct net_device >> *netdev, u32 priv_flags) >> } >> } >> >> + flags2 &= ~IXGBE_FLAG2_LINK_DOWN_ON_CLOSE; >> + if (priv_flags & IXGBE_PRIV_LINK_DOWN_ON_CLOSE) { >> + if (adapter->hw.mac.type == ixgbe_mac_e610) { >> + flags2 |= IXGBE_FLAG2_LINK_DOWN_ON_CLOSE; >> + } else { >> + e_info(probe, "Cannot set private flags: Unsupported >> hardware\n"); > >Please print hw.mac.type, and mention, that it’s only supported on E610. yeah why not > >> + return -EOPNOTSUPP; >> + } >> + } >> + >> if (flags2 != adapter->flags2) { >> adapter->flags2 = flags2; >> >> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c >> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c >> index 62c2d83e1577..58ee4a186039 100644 >> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c >> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c >> @@ -7544,6 +7544,17 @@ static void ixgbe_close_suspend(struct ixgbe_adapter >> *adapter) >> ixgbe_free_all_rx_resources(adapter); >> } >> >> +static void ixgbe_handle_link_down(struct ixgbe_adapter *adapter) >> +{ >> + struct net_device *netdev = adapter->netdev; >> + >> + if (test_bit(__IXGBE_PTP_RUNNING, &adapter->state)) >> + ixgbe_ptp_start_cyclecounter(adapter); >> + >> + e_info(drv, "NIC Link is Down\n"); >> + netif_carrier_off(netdev); >> +} >> + >> /** >> * ixgbe_close - Disables a network interface >> * @netdev: network interface device structure >> @@ -7566,6 +7577,16 @@ int ixgbe_close(struct net_device *netdev) >> >> ixgbe_fdir_filter_exit(adapter); >> >> + if (adapter->flags2 & IXGBE_FLAG2_LINK_DOWN_ON_CLOSE) { >> + int err; >> + >> + err = ixgbe_disable_phy_link(&adapter->hw); >> + if (err) >> + e_warn(drv, "Cannot set PHY link down\n"); > >Log the error? you mean to change the log lvl? Thanks for your review! > >> + >> + ixgbe_handle_link_down(adapter); >> + } >> + >> ixgbe_release_hw_control(adapter); >> >> return 0; >> @@ -8244,11 +8265,7 @@ static void ixgbe_watchdog_link_is_down(struct >> ixgbe_adapter *adapter) >> if (ixgbe_is_sfp(hw) && hw->mac.type == ixgbe_mac_82598EB) >> adapter->flags2 |= IXGBE_FLAG2_SEARCH_FOR_SFP; >> >> - if (test_bit(__IXGBE_PTP_RUNNING, &adapter->state)) >> - ixgbe_ptp_start_cyclecounter(adapter); >> - >> - e_info(drv, "NIC Link is Down\n"); >> - netif_carrier_off(netdev); >> + ixgbe_handle_link_down(adapter); >> } >> >> static bool ixgbe_ring_tx_pending(struct ixgbe_adapter *adapter) > > >Kind regards, > >Paul
