[Intel-wired-lan] [PATCH] [iwl-net] Revert "igc: set TP bit in 'supported' and 'advertising' fields of ethtool_link_ksettings"
Prasad Koya
prasad at arista.com
Mon Sep 25 19:40:58 UTC 2023
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;
+
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> 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>
> >
> > Acked-by: Sasha Neftin <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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20230925/1da0a7a3/attachment-0001.html>
More information about the Intel-wired-lan
mailing list