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

Neftin, Sasha sasha.neftin at intel.com
Tue Sep 26 05:23:06 UTC 2023


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 at lunn.ch 
> <mailto:andrew at 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 at arista.com
>     <mailto:prasad at arista.com>>
>      >
>      > Acked-by: Sasha Neftin <sasha.neftin at intel.com
>     <mailto:sasha.neftin at 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
> 



More information about the Intel-wired-lan mailing list