netdev - Re: [RFC v8 net-next 15/16] net: dsa: felix: add phylink_get

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20220911004435.7wa2ir3gl6w5qanx@skbuf>
Date:   Sun, 11 Sep 2022 00:44:36 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Colin Foster <colin.foster@...advantage.com>
CC:     "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Steen Hegelund <Steen.Hegelund@...rochip.com>,
        Russell King <linux@...linux.org.uk>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        "UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Claudiu Manoil <claudiu.manoil@....com>
Subject: Re: [RFC v8 net-next 15/16] net: dsa: felix: add phylink_get_caps
 capability

On Fri, Sep 09, 2022 at 11:33:59AM -0700, Colin Foster wrote:
> Regarding felix_phylink_get_caps() - does it make sense that
> mac_capabilities would be the same for all ports? This is where I've
> currently landed, and I want to make sure it doesn't have adverse
> effects on vsc9959 or seville:
> 
> static void felix_phylink_get_caps(struct dsa_switch *ds, int port,
>                                    struct phylink_config *config)
> {
>         struct ocelot *ocelot = ds->priv;
>         struct felix *felix;
>         u32 modes;
>         int i;
> 
>         felix = ocelot_to_felix(ocelot);
>         modes = felix->info->port_modes[port];
> 
>         /* This driver does not make use of the speed, duplex, pause or
>          * the advertisement in its mac_config, so it is safe to mark
>          * this driver as non-legacy.
>          */
>         config->legacy_pre_march2020 = false;
> 
>         for (i = 0; i < PHY_INTERFACE_MODE_MAX; i++)
>                 if (modes & felix_phy_match_table[i])
>                         __set_bit(i, config->supported_interfaces);

The current shape of the SERDES driver used with VSC9959 and VSC9953 is
such that dynamic changes to the SERDES protocol are not supported. So
at least for these 2 switches, please keep setting just the current
ocelot->ports[port]->phy_mode (i.e. what was set in the device tree).

> 
>         config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE | MAC_10 |
>                                    MAC_100 | MAC_1000FD;
> }
> 
> (this might be two patches - one for the match table and one for the
> mac_capabilities)
> 
> Seemingly because net/dsa/port.c checks for phylink_validate before it
> checks for mac_capabilties, it won't make a difference there, but this
> seems ... wrong? Or maybe it isn't wrong until I implement the QSGMII
> port that supports 2500FD (as in drivers/net/ethernet/mscc/ocelot_net.c
> ocelot_port_phylink_create())

I don't think there is any QSGMII port that supports 2500FD in VSC7514.
In general, the frequency at which QSGMII operates would not be able to
support that data rate.

That must be an artefact of me copying and pasting code from Felix to
Ocelot in commit e6e12df625f2 ("net: mscc: ocelot: convert to phylink"),
later transformed by Russell in commit 7258aa5094db ("net: ocelot_net:
use phylink_generic_validate()").

How about you do the other way around: populate config->mac_capabilities
such that it unconditionally also includes MAC_2500FD. You may have
noticed that phylink_generic_validate() calls phylink_get_linkmodes(),
which contains a logical AND between the capabilities reported by the
MAC, and the plausible capabilities given by state->interface. So it
would be quite within the expectations of this API for phylink to
exclude MAC_2500FD from mac_capabilities if PHY_INTERFACE_MODE_QSGMII is
used.

On the other hand, VSC9959 and VSC9953 do support actual 2.5G on
internal interfaces (and on USXGMII, in the case of VSC9959). They don't
use the generic phylink validation method right now, but it would be
good, in case they get mechanically converted or something, to keep
reporting MAC_2500FD whatever code you add right now.