[PATCH] [iwl-net] Revert "igc: set TP bit in 'supported' and 'advertising' fields of ethtool_link_ksettings"

[PATCH] [iwl-net] Revert "igc: set TP bit in 'supported' and 'advertising' fields of ethtool_link_ksettings"

[Prasad Koya]

This reverts commit 9ac3fc2f42e5ffa1e927dcbffb71b15fa81459e2.

After the command "ethtool -s enps0 speed 100 duplex full autoneg on",
i.e., advertise only 100Mbps speed to the peer, "ethtool enps0" shows
advertised speeds as 100Mbps and 2500Mbps. Same behavior is seen
when changing the speed to 10Mbps or 1000Mbps.

This applies to I225/226 parts, which only supports copper mode.
Reverting to original till the ambiguity is resolved.

Fixes: 9ac3fc2f42e5 ("igc: set TP bit in 'supported' and 
'advertising' fields of ethtool_link_ksettings")
Signed-off-by: Prasad Koya <prasad@arista.com>
---
 drivers/net/ethernet/intel/igc/igc_ethtool.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
index 93bce729be76..0e2cb00622d1 100644
--- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
+++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
@@ -1708,8 +1708,6 @@ static int igc_ethtool_get_link_ksettings(struct net_device *netdev,
 	/* twisted pair */
 	cmd->base.port = PORT_TP;
 	cmd->base.phy_address = hw->phy.addr;
-	ethtool_link_ksettings_add_link_mode(cmd, supported, TP);
-	ethtool_link_ksettings_add_link_mode(cmd, advertising, TP);
 
 	/* advertising link modes */
 	if (hw->phy.autoneg_advertised & ADVERTISE_10_HALF)
-- 
2.41.0

Re: [PATCH] [iwl-net] Revert "igc: set TP bit in 'supported' and 'advertising' fields of ethtool_link_ksettings"

[Neftin, Sasha]

On 22/09/2023 19:38, Prasad Koya wrote:
> This reverts commit 9ac3fc2f42e5ffa1e927dcbffb71b15fa81459e2.
> 
> After the command "ethtool -s enps0 speed 100 duplex full autoneg on",
> i.e., advertise only 100Mbps speed to the peer, "ethtool enps0" shows
> advertised speeds as 100Mbps and 2500Mbps. Same behavior is seen
> when changing the speed to 10Mbps or 1000Mbps.
> 
> This applies to I225/226 parts, which only supports copper mode.
> Reverting to original till the ambiguity is resolved.
> 
> Fixes: 9ac3fc2f42e5 ("igc: set TP bit in 'supported' and
> 'advertising' fields of ethtool_link_ksettings")
> Signed-off-by: Prasad Koya <prasad@arista.com>

Acked-by: Sasha Neftin <sasha.neftin@intel.com>

> ---
>   drivers/net/ethernet/intel/igc/igc_ethtool.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> index 93bce729be76..0e2cb00622d1 100644
> --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
> +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> @@ -1708,8 +1708,6 @@ static int igc_ethtool_get_link_ksettings(struct net_device *netdev,
>   	/* twisted pair */
>   	cmd->base.port = PORT_TP;
>   	cmd->base.phy_address = hw->phy.addr;
> -	ethtool_link_ksettings_add_link_mode(cmd, supported, TP);
> -	ethtool_link_ksettings_add_link_mode(cmd, advertising, TP);
>   
>   	/* advertising link modes */
>   	if (hw->phy.autoneg_advertised & ADVERTISE_10_HALF)

Re: [PATCH] [iwl-net] Revert "igc: set TP bit in 'supported' and 'advertising' fields of ethtool_link_ksettings"

[Andrew Lunn]

On Sun, Sep 24, 2023 at 10:09:17AM +0300, Neftin, Sasha wrote:
> On 22/09/2023 19:38, Prasad Koya wrote:
> > This reverts commit 9ac3fc2f42e5ffa1e927dcbffb71b15fa81459e2.
> > 
> > After the command "ethtool -s enps0 speed 100 duplex full autoneg on",
> > i.e., advertise only 100Mbps speed to the peer, "ethtool enps0" shows
> > advertised speeds as 100Mbps and 2500Mbps. Same behavior is seen
> > when changing the speed to 10Mbps or 1000Mbps.
> > 
> > This applies to I225/226 parts, which only supports copper mode.
> > Reverting to original till the ambiguity is resolved.
> > 
> > Fixes: 9ac3fc2f42e5 ("igc: set TP bit in 'supported' and
> > 'advertising' fields of ethtool_link_ksettings")
> > Signed-off-by: Prasad Koya <prasad@arista.com>
> 
> Acked-by: Sasha Neftin <sasha.neftin@intel.com>
> 
> > ---
> >   drivers/net/ethernet/intel/igc/igc_ethtool.c | 2 --
> >   1 file changed, 2 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> > index 93bce729be76..0e2cb00622d1 100644
> > --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
> > +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> > @@ -1708,8 +1708,6 @@ static int igc_ethtool_get_link_ksettings(struct net_device *netdev,
> >   	/* twisted pair */
> >   	cmd->base.port = PORT_TP;
> >   	cmd->base.phy_address = hw->phy.addr;
> > -	ethtool_link_ksettings_add_link_mode(cmd, supported, TP);
> > -	ethtool_link_ksettings_add_link_mode(cmd, advertising, TP);

This looks very odd. Please can you confirm this revert really does
make ethtool report the correct advertisement when it has been limited
to 100Mbps. Because looking at this patch, i have no idea how this is
going wrong.

	Andrew

Re: [PATCH] [iwl-net] Revert "igc: set TP bit in 'supported' and 'advertising' fields of ethtool_link_ksettings"

[Andrew Lunn]

> However, to reset the link with new 'advertising' bits, code takes this path:
> 
> [  255.073847]  igc_setup_copper_link+0x73c/0x750
> [  255.073851]  igc_setup_link+0x4a/0x170
> [  255.073852]  igc_init_hw_base+0x98/0x100
> [  255.073855]  igc_reset+0x69/0xe0
> [  255.073857]  igc_down+0x22b/0x230
> [  255.073859]  igc_ethtool_set_link_ksettings+0x25f/0x270
> [  255.073863]  ethtool_set_link_ksettings+0xa9/0x140
> [  255.073866]  dev_ethtool+0x1236/0x2570
> 
> igc_setup_copper_link() calls igc_copper_link_autoneg(). 
> igc_copper_link_autoneg() changes phy->autoneg_advertised
> 
>     phy->autoneg_advertised &= phy->autoneg_mask;
> 
> and autoneg_mask is IGC_ALL_SPEED_DUPLEX_2500 which is 0xaf:
> 
> /* 1Gbps and 2.5Gbps half duplex is not supported, nor spec-compliant. */
> #define ADVERTISE_10_HALF       0x0001
> #define ADVERTISE_10_FULL       0x0002
> #define ADVERTISE_100_HALF      0x0004
> #define ADVERTISE_100_FULL      0x0008
> #define ADVERTISE_1000_HALF     0x0010 /* Not used, just FYI */
> #define ADVERTISE_1000_FULL     0x0020
> #define ADVERTISE_2500_HALF     0x0040 /* Not used, just FYI */
> #define ADVERTISE_2500_FULL     0x0080
> 
> #define IGC_ALL_SPEED_DUPLEX_2500 ( \
>     ADVERTISE_10_HALF | ADVERTISE_10_FULL | ADVERTISE_100_HALF | \
>     ADVERTISE_100_FULL | ADVERTISE_1000_FULL | ADVERTISE_2500_FULL)
> 
> so 0x20c8 & 0xaf becomes 0x88 ie., the TP bit (bit 7
> of ethtool_link_mode_bit_indices) in 0x20c8 got interpreted as
> ADVERTISE_2500_FULL. so after igc_reset(), hw->phy.autoneg_advertised is 0x88.
> Post that, 'ethtool <interface>' reports 2500Mbps can also be advertised. 

So you seem to understand the real problem here. It would be better to
keep the patch, which does in fact look sensible, and fix the real
problem, the mixup between TP and ADVERTISE_2500_FULL in the igb
code.

	 Andrew

Re: [PATCH] [iwl-net] Revert "igc: set TP bit in 'supported' and 'advertising' fields of ethtool_link_ksettings"

[Neftin, Sasha]

On 25/09/2023 22:40, Prasad Koya wrote:
> Hi,
> 
> Here is the ethtool output before and after changing the speed with the 
> commit 9ac3fc2f42e5ffa1e927dcbffb71b15fa81459e2:
> 
> -bash-4.2# ethtool ma1
> Settings for ma1:
>          Supported ports: [ TP ]
>          Supported link modes:   10baseT/Half 10baseT/Full
>                                  100baseT/Half 100baseT/Full
>                                  1000baseT/Full
>                                  2500baseT/Full
>          Supported pause frame use: Symmetric
>          Supports auto-negotiation: Yes
>          Supported FEC modes: Not reported
>          Advertised link modes:  10baseT/Half 10baseT/Full
>                                  100baseT/Half 100baseT/Full
>                                  1000baseT/Full
>                                  2500baseT/Full
>          Advertised pause frame use: Symmetric
>          Advertised auto-negotiation: Yes
>          Advertised FEC modes: Not reported
>          Speed: 1000Mb/s
>          Duplex: Full
>          Auto-negotiation: on
>          Port: Twisted Pair
>          PHYAD: 0
>          Transceiver: internal
>          MDI-X: off (auto)
>          Supports Wake-on: pumbg
>          Wake-on: d
>          Current message level: 0x00000007 (7)
>                                 drv probe link
>          Link detected: yes
> -bash-4.2#
> -bash-4.2# ethtool  -s ma1 speed 100 duplex full autoneg on
> -bash-4.2#
> -bash-4.2# ethtool ma1
> Settings for ma1:
>          Supported ports: [ TP ]
>          Supported link modes:   10baseT/Half 10baseT/Full
>                                  100baseT/Half 100baseT/Full
>                                  1000baseT/Full
>                                  2500baseT/Full
>          Supported pause frame use: Symmetric
>          Supports auto-negotiation: Yes
>          Supported FEC modes: Not reported
>          Advertised link modes:  100baseT/Full
>                                  2500baseT/Full
>          Advertised pause frame use: Symmetric
>          Advertised auto-negotiation: Yes
>          Advertised FEC modes: Not reported
>          Speed: 100Mb/s
>          Duplex: Full
>          Auto-negotiation: on
>          Port: Twisted Pair
>          PHYAD: 0
>          Transceiver: internal
>          MDI-X: off (auto)
>          Supports Wake-on: pumbg
>          Wake-on: d
>          Current message level: 0x00000007 (7)
>                                 drv probe link
>          Link detected: yes
> -bash-4.2#
> 
> With the patch reverted:
> 
> -bash-4.2# ethtool -s ma1 speed 100 duplex full autoneg on
> -bash-4.2#
> -bash-4.2# ethtool ma1
> Settings for ma1:
>          Supported ports: [ TP ]
>          Supported link modes:   10baseT/Half 10baseT/Full
>                                  100baseT/Half 100baseT/Full
>                                  1000baseT/Full
>                                  2500baseT/Full
>          Supported pause frame use: Symmetric
>          Supports auto-negotiation: Yes
>          Supported FEC modes: Not reported
>          Advertised link modes:  100baseT/Full
>          Advertised pause frame use: Symmetric
>          Advertised auto-negotiation: Yes
>          Advertised FEC modes: Not reported
>          Speed: 100Mb/s
>          Duplex: Full
>          Port: Twisted Pair
>          PHYAD: 0
>          Transceiver: internal
>          Auto-negotiation: on
>          MDI-X: off (auto)
>          Supports Wake-on: pumbg
>          Wake-on: d
>          Current message level: 0x00000007 (7)
>                                 drv probe link
>          Link detected: yes
> -bash-4.2#
> 
> with the patch enabled:
> ==================
> 
> Default 'advertising' field is: 0x8000000020ef
> ie., 10Mbps_half, 10Mbps_full, 100Mbps_half, 100Mbps_full, 
> 1000Mbps_full, Autoneg, TP, Pause and 2500Mbps_full bits set.
> 
> and 'hw->phy.autoneg_advertised' is 0xaf
> 
> During "ethtool -s ma1 speed 100 duplex full autoneg on"
> 
> ethtool sends 'advertising' as 0x20c8 ie., 100Mbps_full, Autoneg, TP, 
> Pause bits set which are correct.
> 
> However, to reset the link with new 'advertising' bits, code takes this 
> path:
> 
> [  255.073847]  igc_setup_copper_link+0x73c/0x750
> [  255.073851]  igc_setup_link+0x4a/0x170
> [  255.073852]  igc_init_hw_base+0x98/0x100
> [  255.073855]  igc_reset+0x69/0xe0
> [  255.073857]  igc_down+0x22b/0x230
> [  255.073859]  igc_ethtool_set_link_ksettings+0x25f/0x270
> [  255.073863]  ethtool_set_link_ksettings+0xa9/0x140
> [  255.073866]  dev_ethtool+0x1236/0x2570
> 
> igc_setup_copper_link() calls igc_copper_link_autoneg().  
> igc_copper_link_autoneg() changes phy->autoneg_advertised
> 
>      phy->autoneg_advertised &= phy->autoneg_mask;
> 
> and autoneg_mask is IGC_ALL_SPEED_DUPLEX_2500 which is 0xaf:
> 
> /* 1Gbps and 2.5Gbps half duplex is not supported, nor spec-compliant. */
> #define ADVERTISE_10_HALF       0x0001
> #define ADVERTISE_10_FULL       0x0002
> #define ADVERTISE_100_HALF      0x0004
> #define ADVERTISE_100_FULL      0x0008
> #define ADVERTISE_1000_HALF     0x0010 /* Not used, just FYI */
> #define ADVERTISE_1000_FULL     0x0020
> #define ADVERTISE_2500_HALF     0x0040 /* Not used, just FYI */
> #define ADVERTISE_2500_FULL     0x0080
> 
> #define IGC_ALL_SPEED_DUPLEX_2500 ( \
>      ADVERTISE_10_HALF | ADVERTISE_10_FULL | ADVERTISE_100_HALF | \
>      ADVERTISE_100_FULL | ADVERTISE_1000_FULL | ADVERTISE_2500_FULL)
> 
> so 0x20c8 & 0xaf becomes 0x88 ie., the TP bit (bit 7 
> of ethtool_link_mode_bit_indices) in 0x20c8 got interpreted as 
> ADVERTISE_2500_FULL. so after igc_reset(), hw->phy.autoneg_advertised is 
> 0x88. Post that, 'ethtool <interface>' reports 2500Mbps can also be 
> advertised.
> 
> @@ -445,9 +451,19 @@ static s32 igc_copper_link_autoneg(struct igc_hw *hw)
>          u16 phy_ctrl;
>          s32 ret_val;
> 
>          /* Perform some bounds checking on the autoneg advertisement
>           * parameter.
>           */
> +       if (!(phy->autoneg_advertised & ADVERTISED_2500baseX_Full))
> +               phy->autoneg_advertised &= ~ADVERTISE_2500_FULL;
> +       if ((phy->autoneg_advertised & ADVERTISED_2500baseX_Full))
> +               phy->autoneg_advertised |= ADVERTISE_2500_FULL;
> +

It will introduce more ambiguity. ADVERTISED_2500baseX_Full (is bit 15), 
2500 Base-X is a different type not supported by i225/6 parts. i225/6 
parts support 2500baseT_Full_BIT (bit 47 in new structure).
Look, ethtool used (same as igb) ethtool_convert_link_mode_to_legacy_u32 
method, but there is no option for 2500baseT_Full_BIT. (since i225 only 
copper mode, the TP advertisement was omitted intentionally in an 
original code, I thought).

>          phy->autoneg_advertised &= phy->autoneg_mask;
> 
> I see phy->autoneg_advertised modified similarly 
> in igc_phy_setup_autoneg() as well.
> 
> Above diff works for:
> 
> ethtool -s <intf> speed 10/100/1000 duplex full autoneg on
> or
> ethtool -s <intf> advertise 0x3f (0x03 or 0x0f etc)
> 
> but I haven't tested on a 2500 Mbps link. ADVERTISE_2500_FULL is there 
> only for igc.
> 
> Thanks
> Prasad
> 
> 
> On Sun, Sep 24, 2023 at 7:51 AM Andrew Lunn <andrew@lunn.ch 
> <mailto:andrew@lunn.ch>> wrote:
> 
>     On Sun, Sep 24, 2023 at 10:09:17AM +0300, Neftin, Sasha wrote:
>      > On 22/09/2023 19:38, Prasad Koya wrote:
>      > > This reverts commit 9ac3fc2f42e5ffa1e927dcbffb71b15fa81459e2.
>      > >
>      > > After the command "ethtool -s enps0 speed 100 duplex full
>     autoneg on",
>      > > i.e., advertise only 100Mbps speed to the peer, "ethtool enps0"
>     shows
>      > > advertised speeds as 100Mbps and 2500Mbps. Same behavior is seen
>      > > when changing the speed to 10Mbps or 1000Mbps.
>      > >
>      > > This applies to I225/226 parts, which only supports copper mode.
>      > > Reverting to original till the ambiguity is resolved.
>      > >
>      > > Fixes: 9ac3fc2f42e5 ("igc: set TP bit in 'supported' and
>      > > 'advertising' fields of ethtool_link_ksettings")
>      > > Signed-off-by: Prasad Koya <prasad@arista.com
>     <mailto:prasad@arista.com>>
>      >
>      > Acked-by: Sasha Neftin <sasha.neftin@intel.com
>     <mailto:sasha.neftin@intel.com>>
>      >
>      > > ---
>      > >   drivers/net/ethernet/intel/igc/igc_ethtool.c | 2 --
>      > >   1 file changed, 2 deletions(-)
>      > >
>      > > diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c
>     b/drivers/net/ethernet/intel/igc/igc_ethtool.c
>      > > index 93bce729be76..0e2cb00622d1 100644
>      > > --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
>      > > +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
>      > > @@ -1708,8 +1708,6 @@ static int
>     igc_ethtool_get_link_ksettings(struct net_device *netdev,
>      > >     /* twisted pair */
>      > >     cmd->base.port = PORT_TP;
>      > >     cmd->base.phy_address = hw->phy.addr;
>      > > -   ethtool_link_ksettings_add_link_mode(cmd, supported, TP);
>      > > -   ethtool_link_ksettings_add_link_mode(cmd, advertising, TP);
> 
>     This looks very odd. Please can you confirm this revert really does
>     make ethtool report the correct advertisement when it has been limited
>     to 100Mbps. Because looking at this patch, i have no idea how this is
>     going wrong.

Andrew, yes, I can confirm. (revert works).
We need a process fix, but it will be a different patch. Also, I prefer 
not to leave upstream code with a regression.

ethtool_convert_link_mode_to_legacy_u32 have no option to work with 
2500baseT_Full_BIT (47 > 32). Need to use a new structure for 
auto-negotiation advertisement, mask... (not u16). We need to think how 
to fix it right.

> 
>              Andrew
> 

Re: [Intel-wired-lan] [PATCH] [iwl-net] Revert "igc: set TP bit in 'supported' and 'advertising' fields of ethtool_link_ksettings"

[Neftin, Sasha]

On 26/09/2023 8:23, Neftin, Sasha wrote:
> On 25/09/2023 22:40, Prasad Koya wrote:
>> Hi,
>>
>> Here is the ethtool output before and after changing the speed with 
>> the commit 9ac3fc2f42e5ffa1e927dcbffb71b15fa81459e2:
>>
>> -bash-4.2# ethtool ma1
>> Settings for ma1:
>>          Supported ports: [ TP ]
>>          Supported link modes:   10baseT/Half 10baseT/Full
>>                                  100baseT/Half 100baseT/Full
>>                                  1000baseT/Full
>>                                  2500baseT/Full
>>          Supported pause frame use: Symmetric
>>          Supports auto-negotiation: Yes
>>          Supported FEC modes: Not reported
>>          Advertised link modes:  10baseT/Half 10baseT/Full
>>                                  100baseT/Half 100baseT/Full
>>                                  1000baseT/Full
>>                                  2500baseT/Full
>>          Advertised pause frame use: Symmetric
>>          Advertised auto-negotiation: Yes
>>          Advertised FEC modes: Not reported
>>          Speed: 1000Mb/s
>>          Duplex: Full
>>          Auto-negotiation: on
>>          Port: Twisted Pair
>>          PHYAD: 0
>>          Transceiver: internal
>>          MDI-X: off (auto)
>>          Supports Wake-on: pumbg
>>          Wake-on: d
>>          Current message level: 0x00000007 (7)
>>                                 drv probe link
>>          Link detected: yes
>> -bash-4.2#
>> -bash-4.2# ethtool  -s ma1 speed 100 duplex full autoneg on
>> -bash-4.2#
>> -bash-4.2# ethtool ma1
>> Settings for ma1:
>>          Supported ports: [ TP ]
>>          Supported link modes:   10baseT/Half 10baseT/Full
>>                                  100baseT/Half 100baseT/Full
>>                                  1000baseT/Full
>>                                  2500baseT/Full
>>          Supported pause frame use: Symmetric
>>          Supports auto-negotiation: Yes
>>          Supported FEC modes: Not reported
>>          Advertised link modes:  100baseT/Full
>>                                  2500baseT/Full
>>          Advertised pause frame use: Symmetric
>>          Advertised auto-negotiation: Yes
>>          Advertised FEC modes: Not reported
>>          Speed: 100Mb/s
>>          Duplex: Full
>>          Auto-negotiation: on
>>          Port: Twisted Pair
>>          PHYAD: 0
>>          Transceiver: internal
>>          MDI-X: off (auto)
>>          Supports Wake-on: pumbg
>>          Wake-on: d
>>          Current message level: 0x00000007 (7)
>>                                 drv probe link
>>          Link detected: yes
>> -bash-4.2#
>>
>> With the patch reverted:
>>
>> -bash-4.2# ethtool -s ma1 speed 100 duplex full autoneg on
>> -bash-4.2#
>> -bash-4.2# ethtool ma1
>> Settings for ma1:
>>          Supported ports: [ TP ]
>>          Supported link modes:   10baseT/Half 10baseT/Full
>>                                  100baseT/Half 100baseT/Full
>>                                  1000baseT/Full
>>                                  2500baseT/Full
>>          Supported pause frame use: Symmetric
>>          Supports auto-negotiation: Yes
>>          Supported FEC modes: Not reported
>>          Advertised link modes:  100baseT/Full
>>          Advertised pause frame use: Symmetric
>>          Advertised auto-negotiation: Yes
>>          Advertised FEC modes: Not reported
>>          Speed: 100Mb/s
>>          Duplex: Full
>>          Port: Twisted Pair
>>          PHYAD: 0
>>          Transceiver: internal
>>          Auto-negotiation: on
>>          MDI-X: off (auto)
>>          Supports Wake-on: pumbg
>>          Wake-on: d
>>          Current message level: 0x00000007 (7)
>>                                 drv probe link
>>          Link detected: yes
>> -bash-4.2#
>>
>> with the patch enabled:
>> ==================
>>
>> Default 'advertising' field is: 0x8000000020ef
>> ie., 10Mbps_half, 10Mbps_full, 100Mbps_half, 100Mbps_full, 
>> 1000Mbps_full, Autoneg, TP, Pause and 2500Mbps_full bits set.
>>
>> and 'hw->phy.autoneg_advertised' is 0xaf
>>
>> During "ethtool -s ma1 speed 100 duplex full autoneg on"
>>
>> ethtool sends 'advertising' as 0x20c8 ie., 100Mbps_full, Autoneg, TP, 
>> Pause bits set which are correct.
>>
>> However, to reset the link with new 'advertising' bits, code takes 
>> this path:
>>
>> [  255.073847]  igc_setup_copper_link+0x73c/0x750
>> [  255.073851]  igc_setup_link+0x4a/0x170
>> [  255.073852]  igc_init_hw_base+0x98/0x100
>> [  255.073855]  igc_reset+0x69/0xe0
>> [  255.073857]  igc_down+0x22b/0x230
>> [  255.073859]  igc_ethtool_set_link_ksettings+0x25f/0x270
>> [  255.073863]  ethtool_set_link_ksettings+0xa9/0x140
>> [  255.073866]  dev_ethtool+0x1236/0x2570
>>
>> igc_setup_copper_link() calls igc_copper_link_autoneg(). 
>> igc_copper_link_autoneg() changes phy->autoneg_advertised
>>
>>      phy->autoneg_advertised &= phy->autoneg_mask;
>>
>> and autoneg_mask is IGC_ALL_SPEED_DUPLEX_2500 which is 0xaf:
>>
>> /* 1Gbps and 2.5Gbps half duplex is not supported, nor spec-compliant. */
>> #define ADVERTISE_10_HALF       0x0001
>> #define ADVERTISE_10_FULL       0x0002
>> #define ADVERTISE_100_HALF      0x0004
>> #define ADVERTISE_100_FULL      0x0008
>> #define ADVERTISE_1000_HALF     0x0010 /* Not used, just FYI */
>> #define ADVERTISE_1000_FULL     0x0020
>> #define ADVERTISE_2500_HALF     0x0040 /* Not used, just FYI */
>> #define ADVERTISE_2500_FULL     0x0080
>>
>> #define IGC_ALL_SPEED_DUPLEX_2500 ( \
>>      ADVERTISE_10_HALF | ADVERTISE_10_FULL | ADVERTISE_100_HALF | \
>>      ADVERTISE_100_FULL | ADVERTISE_1000_FULL | ADVERTISE_2500_FULL)
>>
>> so 0x20c8 & 0xaf becomes 0x88 ie., the TP bit (bit 7 
>> of ethtool_link_mode_bit_indices) in 0x20c8 got interpreted as 
>> ADVERTISE_2500_FULL. so after igc_reset(), hw->phy.autoneg_advertised 
>> is 0x88. Post that, 'ethtool <interface>' reports 2500Mbps can also be 
>> advertised.
>>
>> @@ -445,9 +451,19 @@ static s32 igc_copper_link_autoneg(struct igc_hw 
>> *hw)
>>          u16 phy_ctrl;
>>          s32 ret_val;
>>
>>          /* Perform some bounds checking on the autoneg advertisement
>>           * parameter.
>>           */
>> +       if (!(phy->autoneg_advertised & ADVERTISED_2500baseX_Full))
>> +               phy->autoneg_advertised &= ~ADVERTISE_2500_FULL;
>> +       if ((phy->autoneg_advertised & ADVERTISED_2500baseX_Full))
>> +               phy->autoneg_advertised |= ADVERTISE_2500_FULL;
>> +
> 
> It will introduce more ambiguity. ADVERTISED_2500baseX_Full (is bit 15), 
> 2500 Base-X is a different type not supported by i225/6 parts. i225/6 
> parts support 2500baseT_Full_BIT (bit 47 in new structure).
> Look, ethtool used (same as igb) ethtool_convert_link_mode_to_legacy_u32 
> method, but there is no option for 2500baseT_Full_BIT. (since i225 only 
> copper mode, the TP advertisement was omitted intentionally in an 
> original code, I thought).
> 
>>          phy->autoneg_advertised &= phy->autoneg_mask;
>>
>> I see phy->autoneg_advertised modified similarly 
>> in igc_phy_setup_autoneg() as well.
>>
>> Above diff works for:
>>
>> ethtool -s <intf> speed 10/100/1000 duplex full autoneg on
>> or
>> ethtool -s <intf> advertise 0x3f (0x03 or 0x0f etc)
>>
>> but I haven't tested on a 2500 Mbps link. ADVERTISE_2500_FULL is there 
>> only for igc.
>>
>> Thanks
>> Prasad
>>
>>
>> On Sun, Sep 24, 2023 at 7:51 AM Andrew Lunn <andrew@lunn.ch 
>> <mailto:andrew@lunn.ch>> wrote:
>>
>>     On Sun, Sep 24, 2023 at 10:09:17AM +0300, Neftin, Sasha wrote:
>>      > On 22/09/2023 19:38, Prasad Koya wrote:
>>      > > This reverts commit 9ac3fc2f42e5ffa1e927dcbffb71b15fa81459e2.
>>      > >
>>      > > After the command "ethtool -s enps0 speed 100 duplex full
>>     autoneg on",
>>      > > i.e., advertise only 100Mbps speed to the peer, "ethtool enps0"
>>     shows
>>      > > advertised speeds as 100Mbps and 2500Mbps. Same behavior is seen
>>      > > when changing the speed to 10Mbps or 1000Mbps.
>>      > >
>>      > > This applies to I225/226 parts, which only supports copper mode.
>>      > > Reverting to original till the ambiguity is resolved.
>>      > >
>>      > > Fixes: 9ac3fc2f42e5 ("igc: set TP bit in 'supported' and
>>      > > 'advertising' fields of ethtool_link_ksettings")
>>      > > Signed-off-by: Prasad Koya <prasad@arista.com
>>     <mailto:prasad@arista.com>>
>>      >
>>      > Acked-by: Sasha Neftin <sasha.neftin@intel.com
>>     <mailto:sasha.neftin@intel.com>>
>>      >
>>      > > ---
>>      > >   drivers/net/ethernet/intel/igc/igc_ethtool.c | 2 --
>>      > >   1 file changed, 2 deletions(-)
>>      > >
>>      > > diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c
>>     b/drivers/net/ethernet/intel/igc/igc_ethtool.c
>>      > > index 93bce729be76..0e2cb00622d1 100644
>>      > > --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
>>      > > +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
>>      > > @@ -1708,8 +1708,6 @@ static int
>>     igc_ethtool_get_link_ksettings(struct net_device *netdev,
>>      > >     /* twisted pair */
>>      > >     cmd->base.port = PORT_TP;
>>      > >     cmd->base.phy_address = hw->phy.addr;
>>      > > -   ethtool_link_ksettings_add_link_mode(cmd, supported, TP);
>>      > > -   ethtool_link_ksettings_add_link_mode(cmd, advertising, TP);
>>
>>     This looks very odd. Please can you confirm this revert really does
>>     make ethtool report the correct advertisement when it has been 
>> limited
>>     to 100Mbps. Because looking at this patch, i have no idea how this is
>>     going wrong.
> 
> Andrew, yes, I can confirm. (revert works).
> We need a process fix, but it will be a different patch. Also, I prefer 
> not to leave upstream code with a regression.
> 
> ethtool_convert_link_mode_to_legacy_u32 have no option to work with 
> 2500baseT_Full_BIT (47 > 32). Need to use a new structure for 
> auto-negotiation advertisement, mask... (not u16). We need to think how 
> to fix it right.
> 
Colleagues, let us check the option not to use the 
'ethtool_convert_link_mode_to_legacy_u32' method. Looks like we can 
suggest another solution and not process the revert.

>>
>>              Andrew
>>
> 
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [PATCH] [iwl-net] Revert "igc: set TP bit in 'supported' and 'advertising' fields of ethtool_link_ksettings"

[Simon Horman]

On Fri, Sep 22, 2023 at 09:38:04AM -0700, Prasad Koya wrote:
> This reverts commit 9ac3fc2f42e5ffa1e927dcbffb71b15fa81459e2.
> 
> After the command "ethtool -s enps0 speed 100 duplex full autoneg on",
> i.e., advertise only 100Mbps speed to the peer, "ethtool enps0" shows
> advertised speeds as 100Mbps and 2500Mbps. Same behavior is seen
> when changing the speed to 10Mbps or 1000Mbps.
> 
> This applies to I225/226 parts, which only supports copper mode.
> Reverting to original till the ambiguity is resolved.
> 
> Fixes: 9ac3fc2f42e5 ("igc: set TP bit in 'supported' and 
> 'advertising' fields of ethtool_link_ksettings")

nit: I don't think it is correct to linewrap Fixes tags.

> Signed-off-by: Prasad Koya <prasad@arista.com>

...

Re: [PATCH] [iwl-net] Revert "igc: set TP bit in 'supported' and 'advertising' fields of ethtool_link_ksettings"

[Neftin, Sasha]

On 30/09/2023 19:17, Simon Horman wrote:
> On Fri, Sep 22, 2023 at 09:38:04AM -0700, Prasad Koya wrote:
>> This reverts commit 9ac3fc2f42e5ffa1e927dcbffb71b15fa81459e2.
>>
>> After the command "ethtool -s enps0 speed 100 duplex full autoneg on",
>> i.e., advertise only 100Mbps speed to the peer, "ethtool enps0" shows
>> advertised speeds as 100Mbps and 2500Mbps. Same behavior is seen
>> when changing the speed to 10Mbps or 1000Mbps.
>>
>> This applies to I225/226 parts, which only supports copper mode.
>> Reverting to original till the ambiguity is resolved.
>>
>> Fixes: 9ac3fc2f42e5 ("igc: set TP bit in 'supported' and
>> 'advertising' fields of ethtool_link_ksettings")
> 
> nit: I don't think it is correct to linewrap Fixes tags.

Simon, we decided not to recall this patch and will submit another one. 
(resolve 2500M and TP advertisement ambiguity).

> 
>> Signed-off-by: Prasad Koya <prasad@arista.com>
> 
> ...