| Message ID | 20230922163804.7DDBA2440449@us122.sjc.aristanetworks.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [iwl-net] Revert "igc: set TP bit in 'supported' and 'advertising' fields of ethtool_link_ksettings" | expand |
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)
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
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@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>
> >
> > 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
>
> 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
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 >
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
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> ...
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> > > ...
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)
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(-)