| 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: <Y2T47CorBztXGgS4@shell.armlinux.org.uk>
Date: Fri, 4 Nov 2022 11:35:08 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Vladimir Oltean <vladimir.oltean@....com>,
Florian Fainelli <f.fainelli@...il.com>
Cc: netdev@...r.kernel.org, Claudiu Manoil <claudiu.manoil@....com>,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
UNGLinuxDriver@...rochip.com,
Heiner Kallweit <hkallweit1@...il.com>,
Sean Anderson <sean.anderson@...o.com>,
Colin Foster <colin.foster@...advantage.com>,
Andrew Lunn <andrew@...n.ch>,
Vivien Didelot <vivien.didelot@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next 4/4] net: dsa: remove phylink_validate() method
On Fri, Nov 04, 2022 at 11:24:44AM +0000, Russell King (Oracle) wrote:
> On Tue, Nov 01, 2022 at 01:48:06PM +0200, Vladimir Oltean wrote:
> > As of now, all DSA drivers use phylink_generic_validate() and there is
> > no known use case remaining for a driver-specific link mode validation
> > procedure. As such, remove this DSA operation and let phylink determine
> > what is supported based on config->mac_capabilities, which all drivers
> > provide.
> >
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> > ---
> > Not all DSA drivers provide config->mac_capabilities, for example
> > mv88e6060, lan9303 and vsc73xx don't. However, there have been users of
> > those drivers on recent kernels and no one reported that they fail to
> > establish a link, so I'm guessing that they work (somehow). But I must
> > admit I don't understand why phylink_generic_validate() works when
> > mac_capabilities=0. Anyway, these drivers did not provide a
> > phylink_validate() method before and do not provide one now, so nothing
> > changes for them.
>
> There is one remaining issue that needs to be properly addressed,
> which is the bcm_sf2 driver, which is basically buggy. The recent
> kernel build bot reports reminded me of this.
>
> I've tried talking to Florian about it, and didn't make much progress,
> so I'm carrying a patch in my tree which at least makes what is
> provided to phylink correct.
>
> See
> http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=net-queue&id=63d77c1f9db167fd74994860a4a899df5c957aab
> and all the FIXME comments in there.
>
> This driver really needs to be fixed before we kill DSA's
> phylink_validate method (although doing so doesn't change anything
> in mainline, but will remove my reminder that bcm_sf2 is still
> technically broken.)
Here's the corrected patch, along with a bit more commentry about the
problems that I had kicking around in another commit.
8<=====
From: Russell King <rmk+kernel@...linux.org.uk>
Subject: [PATCH] net: dsa: bcm_sf2: fix pause mode validation
The implementation appears not to appear to support pause modes on
anything but RGMII, RGMII_TXID, MII and REVMII interface modes. Let
phylink know that detail.
Moreover, RGMII_RXID and RGMII_ID appears to be unsupported.
(This may not be correct; particularly see the FIXMEs in this patch.)
Signed-off-by: Russell King <rmk+kernel@...linux.org.uk>
---
drivers/net/dsa/bcm_sf2.c | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index 18a3847bd82b..6676971128d1 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -727,6 +727,10 @@ static void bcm_sf2_sw_get_caps(struct dsa_switch *ds, int port,
__set_bit(PHY_INTERFACE_MODE_MII, interfaces);
__set_bit(PHY_INTERFACE_MODE_REVMII, interfaces);
__set_bit(PHY_INTERFACE_MODE_GMII, interfaces);
+
+ /* FIXME 1: Are RGMII_RXID and RGMII_ID actually supported?
+ * See FIXME 2 and FIXME 3 below.
+ */
phy_interface_set_rgmii(interfaces);
}
@@ -734,6 +738,28 @@ static void bcm_sf2_sw_get_caps(struct dsa_switch *ds, int port,
MAC_10 | MAC_100 | MAC_1000;
}
+static void bcm_sf2_sw_validate(struct dsa_switch *ds, int port,
+ unsigned long *supported,
+ struct phylink_link_state *state)
+{
+ u32 caps;
+
+ caps = dsa_to_port(ds, port)->pl_config.mac_capabilities;
+
+ /* Pause modes are only programmed for these modes - see FIXME 3.
+ * So, as pause modes are not configured for other modes, disable
+ * support for them. If FIXME 3 is updated to allow the other RGMII
+ * modes, these should be included here as well.
+ */
+ if (!(state->interface == PHY_INTERFACE_MODE_RGMII ||
+ state->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
+ state->interface == PHY_INTERFACE_MODE_MII ||
+ state->interface == PHY_INTERFACE_MODE_REVMII))
+ caps &= ~(MAC_ASYM_PAUSE | MAC_SYM_PAUSE);
+
+ phylink_validate_mask_caps(supported, state, caps);
+}
+
static void bcm_sf2_sw_mac_config(struct dsa_switch *ds, int port,
unsigned int mode,
const struct phylink_link_state *state)
@@ -747,6 +773,11 @@ static void bcm_sf2_sw_mac_config(struct dsa_switch *ds, int port,
return;
switch (state->interface) {
+ /* FIXME 2: Are RGMII_RXID and RGMII_ID actually supported? This
+ * switch statement means that the RGMII control register does not
+ * get programmed in these two modes, but surely it needs to at least
+ * set the port mode to EXT_GPHY?
+ */
case PHY_INTERFACE_MODE_RGMII:
id_mode_dis = 1;
fallthrough;
@@ -850,6 +881,10 @@ static void bcm_sf2_sw_mac_link_up(struct dsa_switch *ds, int port,
else
offset = CORE_STS_OVERRIDE_GMIIP2_PORT(port);
+ /* FIXME 3: Are RGMII_RXID and RGMII_ID actually supported?
+ * Why are pause modes only supported for a couple of RGMII
+ * modes? Should this be using phy_interface_mode_is_rgmii()?
+ */
if (interface == PHY_INTERFACE_MODE_RGMII ||
interface == PHY_INTERFACE_MODE_RGMII_TXID ||
interface == PHY_INTERFACE_MODE_MII ||
@@ -1207,6 +1242,7 @@ static const struct dsa_switch_ops bcm_sf2_ops = {
.get_ethtool_phy_stats = b53_get_ethtool_phy_stats,
.get_phy_flags = bcm_sf2_sw_get_phy_flags,
.phylink_get_caps = bcm_sf2_sw_get_caps,
+ .phylink_validate = bcm_sf2_sw_validate,
.phylink_mac_config = bcm_sf2_sw_mac_config,
.phylink_mac_link_down = bcm_sf2_sw_mac_link_down,
.phylink_mac_link_up = bcm_sf2_sw_mac_link_up,
--
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