Re: [PATCH net-next] net: dsa: mark parsed interface mode for legacy switch drivers

["Russell King (Oracle)" <linux@armlinux.org.uk>]

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@armlinux.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.

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