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

[Vladimir Oltean <olteanv@gmail.com>]

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

Aha, ok, thanks for explaining.