Message-ID: <ZNI7x9uMe6UP2Xhr@shell.armlinux.org.uk>
Date: Tue, 8 Aug 2023 13:57:43 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
	Florian Fainelli <f.fainelli@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net: dsa: mark parsed interface mode for legacy
 switch drivers

On Tue, Aug 08, 2023 at 03:39:01PM +0300, Vladimir Oltean wrote:
> On Tue, Aug 08, 2023 at 01:30:16PM +0100, Russell King (Oracle) wrote:
> > On Tue, Aug 08, 2023 at 03:06:52PM +0300, Vladimir Oltean wrote:
> > > Hi Russell,
> > > 
> > > On Tue, Aug 08, 2023 at 12:12:16PM +0100, Russell King (Oracle) wrote:
> > > > If we successfully parsed an interface mode with a legacy switch
> > > > driver, populate that mode into phylink's supported interfaces rather
> > > > than defaulting to the internal and gmii interfaces.
> > > > 
> > > > This hasn't caused an issue so far, because when the interface doesn't
> > > > match a supported one, phylink_validate() doesn't clear the supported
> > > > mask, but instead returns -EINVAL. phylink_parse_fixedlink() doesn't
> > > > check this return value, and merely relies on the supported ethtool
> > > > link modes mask being cleared. Therefore, the fixed link settings end
> > > > up being allowed despite validation failing.
> > > > 
> > > > Before this causes a problem, arrange for DSA to more accurately
> > > > populate phylink's supported interfaces mask so validation can
> > > > correctly succeed.
> > > > 
> > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@...linux.org.uk>
> > > > ---
> > > 
> > > How did you notice this? Is there any unconverted DSA switch which has a
> > > phy-mode which isn't PHY_INTERFACE_MODE_INTERNAL or PHY_INTERFACE_MODE_NA?
> > 
> > By looking at some of the legacy drivers, finding their DT compatibles
> > and then grepping the dts files.
> > 
> > For example, vitesse,vsc73* compatibles show up here:
> > 
> > arch/arm/boot/dts/gemini/gemini-sq201.dts
> > 
> > and generally, the ports are listed as:
> > 
> >                                 port@0 {
> >                                         reg = <0>;
> >                                         label = "lan1";
> >                                 };
> > 
> > except for the CPU port which has:
> > 
> >                                 vsc: port@6 {
> >                                         reg = <6>;
> >                                         label = "cpu";
> >                                         ethernet = <&gmac1>;
> >                                         phy-mode = "rgmii";
> >                                         fixed-link {
> >                                                 speed = <1000>;
> >                                                 full-duplex;
> >                                                 pause;
> >                                         };
> >                                 };
> > 
> > Since the vitesse DSA driver doesn't populate .phylink_get_caps, it
> > would have been failing as you discovered with dsa_loop before the
> > previous patch.
> > 
> > Fixing this by setting GMII and INTERNAL worked around the additional
> > check that was using that failure and will work fine for the LAN
> > ports as listed above.
> > 
> > However, that CPU port uses "rgmii" which doesn't match the GMII and
> > INTERNAL bits in the supported mask.
> > 
> > Since phylink_validate() does this:
> > 
> >         const unsigned long *interfaces = pl->config->supported_interfaces;
> > 
> > 	if (state->interface == PHY_INTERFACE_MODE_NA)
> > 
> > ... it isn't, so we move on...
> > 
> >         if (!test_bit(state->interface, interfaces))
> >                 return -EINVAL;
> > 
> > This will trigger and phylink_validate() in phylink_parse_fixedlink()
> > will return -EINVAL without touching the passed supported mask.
> > 
> > phylink_parse_fixedlink() does:
> > 
> >         bitmap_fill(pl->supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
> >         linkmode_copy(pl->link_config.advertising, pl->supported);
> >         phylink_validate(pl, MLO_AN_FIXED, pl->supported, &pl->link_config);
> > 
> > and then we have:
> > 
> >         s = phy_lookup_setting(pl->link_config.speed, pl->link_config.duplex,
> >                                pl->supported, true);
> > 
> > ...
> >         if (s) {
> > 		... success ...
> >         } else {
> >                 phylink_warn(pl, "fixed link %s duplex %dMbps not recognised\n",
> >                              pl->link_config.duplex == DUPLEX_FULL ? "full" : "half",
> >                              pl->link_config.speed);
> >         }
> > 
> > So, since phylink_validate() with an apparently unsupported interface
> > exits early with -EINVAL, pl->supported ends up with all bits set,
> > and phy_lookup_setting() allows any speed.
> > 
> > If someone decides to fix that phylink_validate() error checking, then
> > this will then lead to a warning/failure.
> > 
> > I want to avoid that happening - fixing that latent bug before it
> > becomes a problem.
> 
> Aha, ok, thanks for explaining.

Thanks for the r-b.

At risk of delaying this patch through further discussion... so I'll
say now that we're going off into discussions about future changes.

I believe all DSA drivers that provide .phylink_get_caps fill in the
.mac_capabilities member, which leaves just a few drivers that do not,
which are:

$ git grep -l dsa_switch_ops.*= drivers/net/dsa/ | xargs grep -L '\.phylink_get_caps'
drivers/net/dsa/dsa_loop.c
drivers/net/dsa/mv88e6060.c
drivers/net/dsa/realtek/rtl8366rb.c
drivers/net/dsa/vitesse-vsc73xx-core.c

I've floated the idea to Linus W and Arinc about setting
.mac_capabilities in the non-phylink_get_caps path as well, suggesting:

	MAC_1000 | MAC_100 | MAC_10 | MAC_SYM_PAUSE | MAC_ASYM_PAUSE

support more than 1G speeds. I think the only exception to that may
be dsa_loop, but as I think that makes use of the old fixed-link
software emulated PHYs, I believe that would be limited to max. 1G
as well.

If we did set .mac_capabilities, then dsa_port_phylink_validate() would
always call phylink_generic_validate() for all DSA drivers, and at that
point, we don't need dsa_port_phylink_validate() anymore as it provides
nothing that isn't already done inside phylink.

Once dsa_port_phylink_validate() is gone, then I believe there are no
drivers populating the .validate method in phylink_mac_ops, which
then means there is the possibility to remove that method.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Powered by blists - more mailing lists