[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 8 Aug 2023 15:39:01 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
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 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.
Powered by blists - more mailing lists