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: 0x8000000020efie., 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/0x2570igc_setup_copper_link() calls igc_copper_link_autoneg(). igc_copper_link_autoneg() changes phy->autoneg_advertisedphy->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 PrasadOn Sun, Sep 24, 2023 at 7:51 AM Andrew Lunn <and...@lunn.ch <mailto:and...@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 <pra...@arista.com <mailto:pra...@arista.com>> > > Acked-by: Sasha Neftin <sasha.nef...@intel.com <mailto:sasha.nef...@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
_______________________________________________ Intel-wired-lan mailing list Intel-wired-lan@osuosl.org https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
