netdev - Re: [PATCH] [iwl-net] Revert "igc: set TP bit in 'supported' and 'advertising' fields of ethtool_link

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Date: Mon, 25 Sep 2023 21:49:47 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Prasad Koya <prasad@...sta.com>
Cc: "Neftin, Sasha" <sasha.neftin@...el.com>,
	intel-wired-lan@...ts.osuosl.org, kuba@...nel.org,
	davem@...emloft.net, pabeni@...hat.com, jesse.brandeburg@...el.com,
	anthony.l.nguyen@...el.com, netdev@...r.kernel.org,
	"lifshits, Vitaly" <vitaly.lifshits@...el.com>, edumazet@...gle.com,
	"Ruinskiy, Dima" <dima.ruinskiy@...el.com>
Subject: Re: [PATCH] [iwl-net] Revert "igc: set TP bit in 'supported' and
 'advertising' fields of ethtool_link_ksettings"

> 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