| 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: <YauArR7bd6Xh4ISt@shell.armlinux.org.uk>
Date: Sat, 4 Dec 2021 14:52:29 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Florian Fainelli <f.fainelli@...il.com>,
Andrew Lunn <andrew@...n.ch>,
Heiner Kallweit <hkallweit1@...il.com>,
Tom Lendacky <thomas.lendacky@....com>
Cc: Vivien Didelot <vivien.didelot@...il.com>,
Vladimir Oltean <olteanv@...il.com>,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
Claudiu Manoil <claudiu.manoil@....com>,
George McCollister <george.mccollister@...il.com>,
Hauke Mehrtens <hauke@...ke-m.de>,
Kurt Kanzenbach <kurt@...utronix.de>,
Vladimir Oltean <vladimir.oltean@....com>,
Woojung Huh <woojung.huh@...rochip.com>,
"David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH RFC net-next 05/12] net: dsa: bcm_sf2: convert to
phylink_generic_validate()
On Sat, Dec 04, 2021 at 08:59:12AM +0000, Russell King (Oracle) wrote:
> On Fri, Dec 03, 2021 at 08:18:22PM -0800, Florian Fainelli wrote:
> > Now, with respect to the fixed link ports reporting 1000baseKX/Full this is
> > introduced by switching to your patch, it works before and it "breaks"
> > after.
> >
> > The first part that is a bit weird is that we seem to be calling
> > phylink_generic_validate() twice in a row from the same call site.
> >
> > For fixed link ports, instead of masking with what the fixed link actually
> > supports, we seem to be using a supported mask which is all 1s which seems a
> > bit excessive for a fixed link.
> >
> > This is an excerpt with the internal PHY:
> >
> > [ 4.210890] brcm-sf2 f0b00000.ethernet_switch gphy (uninitialized):
> > Calling phylink_generic_validate
> > [ 4.220063] before phylink_get_linkmodes: 0000000,00000000,00010fc0
> > [ 4.226357] phylink_get_linkmodes: caps: 0xffffffff mac_capabilities:
> > 0xff
> > [ 4.233258] after phylink_get_linkmodes: c000018,00000200,00036fff
> > [ 4.239463] before anding supported with mask: 0000000,00000000,000062ff
> > [ 4.246189] after anding supported with mask: 0000000,00000000,000062ff
> > [ 4.252829] before anding advertising with mask:
> > c000018,00000200,00036fff
> > [ 4.259729] after anding advertising with mask: c000018,00000200,00036fff
> > [ 4.266546] brcm-sf2 f0b00000.ethernet_switch gphy (uninitialized): PHY
> > [f0b403c0.mdio--1:05] driver [Broadcom BCM7445] (irq=POLL)
> >
> > and this is what a fixed link port looks like:
> >
> > [ 4.430765] brcm-sf2 f0b00000.ethernet_switch rgmii_2 (uninitialized):
> > Calling phylink_generic_validate
> > [ 4.440205] before phylink_get_linkmodes: 0000000,00000000,00010fc0
> > [ 4.446500] phylink_get_linkmodes: caps: 0xff mac_capabilities: 0xff
> > [ 4.452880] after phylink_get_linkmodes: c000018,00000200,00036fff
> > [ 4.459085] before anding supported with mask: fffffff,ffffffff,ffffffff
> > [ 4.465811] after anding supported with mask: c000018,00000200,00036fff
> > [ 4.472450] before anding advertising with mask:
> > c000018,00000200,00036fff
> > [ 4.479349] after anding advertising with mask: c000018,00000200,00036fff
> >
> > or maybe the problem is with phylink_get_ksettings... ran out of time
> > tonight to look further into it.
>
> It will be:
>
> s = phy_lookup_setting(pl->link_config.speed, pl->link_config.duplex,
> pl->supported, true);
> linkmode_zero(pl->supported);
> phylink_set(pl->supported, MII);
> phylink_set(pl->supported, Pause);
> phylink_set(pl->supported, Asym_Pause);
> phylink_set(pl->supported, Autoneg);
> if (s) {
> __set_bit(s->bit, pl->supported);
> __set_bit(s->bit, pl->link_config.lp_advertising);
>
> Since 1000baseKX_Full is set in the supported mask, phy_lookup_setting()
> returns the first entry it finds in the supported table:
>
> /* 1G */
> PHY_SETTING( 1000, FULL, 1000baseKX_Full ),
> PHY_SETTING( 1000, FULL, 1000baseT_Full ),
> PHY_SETTING( 1000, HALF, 1000baseT_Half ),
> PHY_SETTING( 1000, FULL, 1000baseT1_Full ),
> PHY_SETTING( 1000, FULL, 1000baseX_Full ),
>
> Consequently, 1000baseKX_Full is preferred over 1000baseT_Full.
>
> Fixed links don't specify their underlying technology, only the speed
> and duplex, so going from speed and duplex to an ethtool link mode is
> not easy. I suppose we could drop 1000baseKX_Full from the supported
> bitmap in phylink_parse_fixedlink() before the first phylink_validate()
> call. Alternatively, the table could be re-ordered. It was supposed to
> be grouped by speed and sorted in descending match priority as specified
> by the comment above the table. Does it really make sense that
> 1000baseKX_Full is supposed to be preferred over all the other 1G
> speeds? I suppose that's a question for Tom Lendacky
> <thomas.lendacky@....com>, who introduced this in 3e7077067e80
> ("phy: Expand phy speed/duplex settings array") back in 2014.
Here's a patch for one of my suggestions above. Tom, I'd appreciate
if you could look at this please. Thanks.
8<===
From: "Russell King (Oracle)" <rmk+kernel@...linux.org.uk>
Subject: [PATCH net-next] net: phy: prefer 1000baseT over 1000baseKX
The PHY settings table is supposed to be sorted by descending match
priority - in other words, earlier entries are preferred over later
entries.
The order of 1000baseKX/Full and 1000baseT/Full is such that we
prefer 1000baseKX/Full over 1000baseT/Full, but 1000baseKX/Full is
a lot rarer than 1000baseT/Full, and thus is much less likely to
be preferred.
This causes phylink problems - it means a fixed link specifying a
speed of 1G and full duplex gets an ethtool linkmode of 1000baseKX/Full
rather than 1000baseT/Full as would be expected - and since we offer
userspace a software emulation of a conventional copper PHY, we want
to offer copper modes in preference to anything else. However, we do
still want to allow the rarer modes as well.
Hence, let's reorder these two modes to prefer copper.
Signed-off-by: Russell King (Oracle) <rmk+kernel@...linux.org.uk>
---
drivers/net/phy/phy-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 2870c33b8975..271fc01f7f7f 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -162,11 +162,11 @@ static const struct phy_setting settings[] = {
PHY_SETTING( 2500, FULL, 2500baseT_Full ),
PHY_SETTING( 2500, FULL, 2500baseX_Full ),
/* 1G */
- PHY_SETTING( 1000, FULL, 1000baseKX_Full ),
PHY_SETTING( 1000, FULL, 1000baseT_Full ),
PHY_SETTING( 1000, HALF, 1000baseT_Half ),
PHY_SETTING( 1000, FULL, 1000baseT1_Full ),
PHY_SETTING( 1000, FULL, 1000baseX_Full ),
+ PHY_SETTING( 1000, FULL, 1000baseKX_Full ),
/* 100M */
PHY_SETTING( 100, FULL, 100baseT_Full ),
PHY_SETTING( 100, FULL, 100baseT1_Full ),
--
2.30.2
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Powered by blists - more mailing lists