[Intel-wired-lan] [PATCH] ice: set ethtool autoneg based on active cfg

[Intel-wired-lan] [PATCH] ice: set ethtool autoneg based on active cfg

[Ulrich Weber]

Current logic uses ICE_AQ_AN_COMPLETED information to
flag if autonegotiation is enabled or disabled.

Since new ethtool netlink interface checks if there is
a configuration change or not and ignores the call, if
there is no change, this makes is impossible to disable
autonegotiation on links without established autoneg.

This will change the logic to check the active phy
configuration if autoneg is enabled or not.

Signed-off-by: Ulrich Weber <ulrich.weber@gmail.com>
---
 src/ice_ethtool.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/ice_ethtool.c b/src/ice_ethtool.c
index e1eeb16babb3..8fe475944f2c 100644
--- a/src/ice_ethtool.c
+++ b/src/ice_ethtool.c
@@ -2857,10 +2857,6 @@ ice_get_link_ksettings(struct net_device *netdev,
 	else
 		ice_get_settings_link_down(ks, netdev);
 
-	/* set autoneg settings */
-	ks->base.autoneg = (hw_link_info->an_info & ICE_AQ_AN_COMPLETED) ?
-		AUTONEG_ENABLE : AUTONEG_DISABLE;
-
 	/* set media type settings */
 
 	switch (vsi->port_info->phy.media_type) {
@@ -2912,6 +2908,10 @@ ice_get_link_ksettings(struct net_device *netdev,
 	if (err)
 		goto done;
 
+	/* set autoneg settings based on active configuration */
+	ks->base.autoneg = ice_is_phy_caps_an_enabled(caps) ?
+		AUTONEG_ENABLE : AUTONEG_DISABLE;
+
 	/* Set the advertised flow control based on the PHY capability */
 	if ((caps->caps & ICE_AQC_PHY_EN_TX_LINK_PAUSE) &&
 	    (caps->caps & ICE_AQC_PHY_EN_RX_LINK_PAUSE)) {
@@ -2960,7 +2960,7 @@ ice_get_link_ksettings(struct net_device *netdev,
 		ethtool_link_ksettings_add_link_mode(ks, supported, FEC_RS);
 #endif /* ETHTOOL_GFECPARAM */
 
-	/* Set supported and advertised autoneg */
+	/* Set supported and advertised autoneg based on media */
 	if (ice_is_phy_caps_an_enabled(caps)) {
 		ethtool_link_ksettings_add_link_mode(ks, supported, Autoneg);
 		ethtool_link_ksettings_add_link_mode(ks, advertising, Autoneg);
-- 
2.40.1

Re: [Intel-wired-lan] [PATCH] ice: set ethtool autoneg based on active cfg

[Tony Nguyen]

On 3/14/2024 2:15 AM, Ulrich Weber wrote:
> Current logic uses ICE_AQ_AN_COMPLETED information to
> flag if autonegotiation is enabled or disabled.
> 
> Since new ethtool netlink interface checks if there is
> a configuration change or not and ignores the call, if
> there is no change, this makes is impossible to disable
> autonegotiation on links without established autoneg.
> 
> This will change the logic to check the active phy
> configuration if autoneg is enabled or not.
> 

Sounds like a bug fix, so you should target this to 'iwl-net' and also 
add a Fixes:

> Signed-off-by: Ulrich Weber <ulrich.weber@gmail.com>
> ---
>   src/ice_ethtool.c | 10 +++++-----

What tree are you using? This is not a kernel path.

>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/src/ice_ethtool.c b/src/ice_ethtool.c
> index e1eeb16babb3..8fe475944f2c 100644
> --- a/src/ice_ethtool.c
> +++ b/src/ice_ethtool.c
> @@ -2857,10 +2857,6 @@ ice_get_link_ksettings(struct net_device *netdev,
>   	else
>   		ice_get_settings_link_down(ks, netdev);
>   
> -	/* set autoneg settings */
> -	ks->base.autoneg = (hw_link_info->an_info & ICE_AQ_AN_COMPLETED) ?
> -		AUTONEG_ENABLE : AUTONEG_DISABLE;
> -
>   	/* set media type settings */
>   
>   	switch (vsi->port_info->phy.media_type) {
> @@ -2912,6 +2908,10 @@ ice_get_link_ksettings(struct net_device *netdev,
>   	if (err)
>   		goto done;
>   
> +	/* set autoneg settings based on active configuration */
> +	ks->base.autoneg = ice_is_phy_caps_an_enabled(caps) ?
> +		AUTONEG_ENABLE : AUTONEG_DISABLE;

Since this needs to move to after the PHY capabilities call anyways, 
it'd be nicer to put this with the rest of the autoneg code. You could 
probably utilize the existing ice_is_phy_caps_an_enabled check and not 
add a second call as well.

Thanks,
Tony

> +
>   	/* Set the advertised flow control based on the PHY capability */
>   	if ((caps->caps & ICE_AQC_PHY_EN_TX_LINK_PAUSE) &&
>   	    (caps->caps & ICE_AQC_PHY_EN_RX_LINK_PAUSE)) {
> @@ -2960,7 +2960,7 @@ ice_get_link_ksettings(struct net_device *netdev,
>   		ethtool_link_ksettings_add_link_mode(ks, supported, FEC_RS);
>   #endif /* ETHTOOL_GFECPARAM */
>   
> -	/* Set supported and advertised autoneg */
> +	/* Set supported and advertised autoneg based on media */
>   	if (ice_is_phy_caps_an_enabled(caps)) {
>   		ethtool_link_ksettings_add_link_mode(ks, supported, Autoneg);
>   		ethtool_link_ksettings_add_link_mode(ks, advertising, Autoneg);

Re: [Intel-wired-lan] [PATCH] ice: set ethtool autoneg based on active cfg

[Ulrich Weber]

[-- Attachment #1: Type: text/plain, Size: 3188 bytes --]

Hi Tony,

> On 15. Mar 2024, at 22:39, Tony Nguyen <anthony.l.nguyen@intel.com> wrote:
> 
> 
> 
> On 3/14/2024 2:15 AM, Ulrich Weber wrote:
>> Current logic uses ICE_AQ_AN_COMPLETED information to
>> flag if autonegotiation is enabled or disabled.
>> Since new ethtool netlink interface checks if there is
>> a configuration change or not and ignores the call, if
>> there is no change, this makes is impossible to disable
>> autonegotiation on links without established autoneg.
>> This will change the logic to check the active phy
>> configuration if autoneg is enabled or not.
> 
> Sounds like a bug fix, so you should target this to 'iwl-net' and also add a Fixes:
> 
>> Signed-off-by: Ulrich Weber <ulrich.weber@gmail.com <mailto:ulrich.weber@gmail.com>>
>> ---
>>  src/ice_ethtool.c | 10 +++++-----
> 
> What tree are you using? This is not a kernel path.
Wasn’t sure about where the patches should apply to,
So I used the latest sf release. Will adopt to net-next then.

> 
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>> diff --git a/src/ice_ethtool.c b/src/ice_ethtool.c
>> index e1eeb16babb3..8fe475944f2c 100644
>> --- a/src/ice_ethtool.c
>> +++ b/src/ice_ethtool.c
>> @@ -2857,10 +2857,6 @@ ice_get_link_ksettings(struct net_device *netdev,
>>  	else
>>  		ice_get_settings_link_down(ks, netdev);
>>  -	/* set autoneg settings */
>> -	ks->base.autoneg = (hw_link_info->an_info & ICE_AQ_AN_COMPLETED) ?
>> -		AUTONEG_ENABLE : AUTONEG_DISABLE;
>> -
>>  	/* set media type settings */
>>    	switch (vsi->port_info->phy.media_type) {
>> @@ -2912,6 +2908,10 @@ ice_get_link_ksettings(struct net_device *netdev,
>>  	if (err)
>>  		goto done;
>>  +	/* set autoneg settings based on active configuration */
>> +	ks->base.autoneg = ice_is_phy_caps_an_enabled(caps) ?
>> +		AUTONEG_ENABLE : AUTONEG_DISABLE;
> 
> Since this needs to move to after the PHY capabilities call anyways, it'd be nicer to put this with the rest of the autoneg code. You could probably utilize the existing ice_is_phy_caps_an_enabled check and not add a second call as well.
That’s what I tried first, but it didn’t work for me. That’s also the reason I changed the comment on the second ice_is_phy_caps_an_enabled() call.

I disabled autoeng by explicitly setting config.low_power_ctrl_an to 0:
ICE_AQC_REPORT_ACTIVE_CFG shows autoneg as disabled then,
while ICE_AQC_REPORT_TOPO_CAP_MEDIA shows it still as enabled.

Chers
 Ulrich

> 
> Thanks,
> Tony
> 
>> +
>>  	/* Set the advertised flow control based on the PHY capability */
>>  	if ((caps->caps & ICE_AQC_PHY_EN_TX_LINK_PAUSE) &&
>>  	    (caps->caps & ICE_AQC_PHY_EN_RX_LINK_PAUSE)) {
>> @@ -2960,7 +2960,7 @@ ice_get_link_ksettings(struct net_device *netdev,
>>  		ethtool_link_ksettings_add_link_mode(ks, supported, FEC_RS);
>>  #endif /* ETHTOOL_GFECPARAM */
>>  -	/* Set supported and advertised autoneg */
>> +	/* Set supported and advertised autoneg based on media */
>>  	if (ice_is_phy_caps_an_enabled(caps)) {
>>  		ethtool_link_ksettings_add_link_mode(ks, supported, Autoneg);
>>  		ethtool_link_ksettings_add_link_mode(ks, advertising, Autoneg);


[-- Attachment #2: Type: text/html, Size: 14337 bytes --]