| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d7bb8b96-f347-45d5-81af-4b7b422908c8@intel.com>
Date: Tue, 26 Sep 2023 14:00:06 +0300
From: "Neftin, Sasha" <sasha.neftin@...el.com>
To: Prasad Koya <prasad@...sta.com>, Andrew Lunn <andrew@...n.ch>
CC: "Ruinskiy, Dima" <dima.ruinskiy@...el.com>, "lifshits, Vitaly"
<vitaly.lifshits@...el.com>, <intel-wired-lan@...ts.osuosl.org>,
<jesse.brandeburg@...el.com>, <edumazet@...gle.com>,
<anthony.l.nguyen@...el.com>, <netdev@...r.kernel.org>, <kuba@...nel.org>,
<pabeni@...hat.com>, <davem@...emloft.net>
Subject: Re: [Intel-wired-lan] [PATCH] [iwl-net] Revert "igc: set TP bit in
'supported' and 'advertising' fields of ethtool_link_ksettings"
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@...n.ch
>> <mailto:andrew@...n.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@...sta.com
>> <mailto:prasad@...sta.com>>
>> >
>> > Acked-by: Sasha Neftin <sasha.neftin@...el.com
>> <mailto:sasha.neftin@...el.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@...osl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Powered by blists - more mailing lists