Re: [PATCH iwl-net v4 2/3] ice: honor autoneg disable requests when link is down

[Tony Nguyen <anthony.l.nguyen@intel.com>]

On 10/6/2026 7:24 AM, Aleksandr Loktionov wrote:
> From: Konrad Knitter <konrad.knitter@intel.com>
> 
> The ethtool setter uses the link-partner AN ability bit to decide whether a
> request to disable autonegotiation can be handled. That bit is valid only
> while the link is up, so a link-down request skips the configuration update
> and returns success.
> 
> Allow link-down requests through to PHY capability validation. On legacy
> MACs, retain the existing restriction for PHYs that advertise autoneg.
> Generic MACs configure Clause 37 through low_power_ctrl_an, and must keep
> AUTO_LINK_UPDT set so firmware applies the change. Handle that field for
> both enable and disable requests.
> 
> Fixes: 48cb27f2fd18 ("ice: Implement handlers for ethtool PHY/link operations")
> Signed-off-by: Konrad Knitter <konrad.knitter@intel.com>
> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> ---
> Changes since v3:
> - Allow link-down disable requests without relying on stale partner state.
> - Keep the generic-MAC enable bypass; validate E830 Autoneg support before
> 	restoring Clause 37 when its cached bit is clear.
> - Avoid marking unchanged Clause 37 requests as autoneg changes.
> - Correct the Fixes tag to the commit that introduced the ethtool handler.

This series did not apply cleanly, but I manually resolved it and ran 
Sashiko locally. This is what it reported.

I did apply the other two patches as they look ok and don't look to be 
related to this.

Thanks,
Tony
>   drivers/net/ethernet/intel/ice/ice_ethtool.c | 57 +++++++++++++++++++-
>   1 file changed, 55 insertions(+), 2 deletions(-)

...

> @@ -2535,12 +2557,31 @@ ice_setup_autoneg(struct ice_port_info *p, struct ethtool_link_ksettings *ks,
>   	if (autoneg_enabled == AUTONEG_ENABLE) {
>   		/* If autoneg was not already enabled */
>   		if (!(p->phy.link_info.an_info & ICE_AQ_AN_COMPLETED)) {
> +			if (ice_is_generic_mac(p->hw)) {
> +				if (!(config->low_power_ctrl_an &
> +				      ICE_AQC_PHY_AN_EN_CLAUSE37)) {
> +					config->low_power_ctrl_an |=
> +						ICE_AQC_PHY_AN_EN_CLAUSE37;
> +					config->caps |=
> +						ICE_AQ_PHY_ENA_AUTO_LINK_UPDT;
> +					*autoneg_changed = 1;
> +				}
> +				return 0;
> +			}

On generic MACs, this early return comes before the check below:
     if (!ethtool_link_ksettings_test_link_mode(ks, supported, Autoneg))

If the PHY does not have Autoneg in its supported mask, "ethtool -s 
<dev> autoneg on" used to fail with -EINVAL. With this change it sets 
CL37 and AUTO_LINK_UPDT, sends the config to firmware and returns success.

Should the supported check run before this branch? The E830 branch added
below still does the check, so the two MAC families now validate the 
same request differently.

>   			/* If autoneg is not supported, return error */
>   			if (!ethtool_link_ksettings_test_link_mode(ks,
>   								   supported,
>   								   Autoneg)) {
>   				netdev_info(netdev, "Autoneg not supported on this phy.\n");
>   				err = -EINVAL;
> +			} else if (p->hw->mac_type == ICE_MAC_E830) {
> +				if (!(config->low_power_ctrl_an &
> +				      ICE_AQC_PHY_AN_EN_CLAUSE37)) {
> +					config->low_power_ctrl_an |=
> +						ICE_AQC_PHY_AN_EN_CLAUSE37;
> +					config->caps |= ICE_AQ_PHY_ENA_AUTO_LINK_UPDT;
> +					*autoneg_changed = 1;
> +				}
>   			} else {
>   				/* Autoneg is allowed to change */
>   				config->caps |= ICE_AQ_PHY_ENA_AUTO_LINK_UPDT;
> @@ -2548,8 +2589,20 @@ ice_setup_autoneg(struct ice_port_info *p, struct ethtool_link_ksettings *ks,
>   			}
>   		}
>   	} else {
> -		/* If autoneg is currently enabled */
> -		if (p->phy.link_info.an_info & ICE_AQ_AN_COMPLETED) {
> +		/* If autoneg completed or link partner does not support AN */
> +		if (ice_autoneg_disable_allowed(p)) {
> +			if (ice_is_generic_mac(p->hw) ||
> +			    p->hw->mac_type == ICE_MAC_E830) {

The commit message describes two cases: generic MACs, and legacy MACs
that "retain the existing restriction". E830 is not a generic MAC, but 
it takes this branch. So E830 no longer returns "Autoneg cannot be 
disabled on this phy" when the PHY supports Autoneg, and the enable path 
now writes CL37 on E830.

Is the E830 change intended? If so, could the commit message describe it
and explain why, given this patch carries a Fixes: tag?
This branch sits behind ice_autoneg_disable_allowed(). If the link is 
up, ICE_AQ_AN_COMPLETED is clear and the partner advertises 
ICE_AQ_LP_AN_ABILITY, the helper returns false. The whole disable branch
is then skipped: CL37 stays set, autoneg_changed stays 0, and the 
function returns 0.

On these MACs, CL37 in low_power_ctrl_an is configuration and does not
depend on the partner's ability. Does "autoneg off" still succeed
silently and do nothing in that link state?

> +				if (config->low_power_ctrl_an &
> +				    ICE_AQC_PHY_AN_EN_CLAUSE37) {
> +					config->low_power_ctrl_an &=
> +						~ICE_AQC_PHY_AN_EN_CLAUSE37;
> +					config->caps |=
> +						ICE_AQ_PHY_ENA_AUTO_LINK_UPDT;
> +					*autoneg_changed = 1;
> +				}
> +				return 0;
> +			}


Only ICE_AQC_PHY_AN_EN_CLAUSE37 is cleared here. The Clause 28 and
Clause 73 enable bits and ICE_AQC_PHY_AN_MODE in caps are left alone.
ice_is_phy_caps_an_enabled() checks all of these to report the autoneg
state.

If CL37 is already clear, for example on a CL73 backplane or a CL28
BASE-T PHY, this returns 0 with autoneg_changed = 0. Would "ethtool -s
autoneg off" then report success while get_link_ksettings still shows
autoneg on? That is the same silent success the commit says it removes.

>   			/* If autoneg is supported 10GBASE_T is the only PHY
>   			 * that can disable it, so otherwise return error
>   			 */