netdev - Re: [PATCH net-next 0/10] net: dsa: improve serdes integration

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20200306132926.GA18310@lunn.ch>
Date:   Fri, 6 Mar 2020 14:29:26 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        Vivien Didelot <vivien.didelot@...il.com>
Subject: Re: [PATCH net-next 0/10] net: dsa: improve serdes integration

> Unfortunately, that means that CPU and DSA ports without a fixed-link
> spec will stay down because phylink won't call mac_link_up() - so we're
> back to the poor integration of phylink for CPU and DSA ports problem.
> Even if phylink /were/ to call mac_link_up() for that situation,
> phylink has no information on the speed and duplex for such a port, so
> speed and duplex would be nonsense.
> 
> That conversion is very problematical.
> 
> I do have some patches that solve it by changing phylink, but it's
> quite a hack - the problem is detecting the uninitialised state in
> phylink_start(), which is really quite late.  You can find them in my
> "zii" branch:
> 
> net: dsa: mv88e6xxx: split out SPEED_MAX setting
> net: phylink/dsa: fix DSA and CPU links
> 
> So, I think we're back to... what do we do about the broken phylink
> integration for CPU and DSA ports.

Hi Russell

Here is what i have been playing with:

commit ea4c6b6ad0694a3cd857f504fb5e3a5887ffa062
Author: Andrew Lunn <andrew@...n.ch>
Date:   Wed Feb 12 14:49:18 2020 -0600

    net: dsa: Don't instantiate phylink for CPU/DSA ports unless needed
    
    By default, DSA drivers should configure CPU and DSA ports to their
    maximum speed. In many configurations this is sufficient to make the
    link work.
    
    In some cases it is necessary to configure the link to run slower,
    e.g. because of limitations of the SoC it is connected to. Or back to
    back PHYs are used and the PHY needs to be driven in order to
    establish link. In this case, phylink is used.
    
    Only instantiate phylink if it is required. If there is no PHY, or no
    fixed link properties, phylink can upset a link which works in the
    default configuration.
    
    Fixes: 0e27921816ad ("net: dsa: Use PHYLINK for the CPU/DSA ports")
    Signed-off-by: Andrew Lunn <andrew@...n.ch>

diff --git a/net/dsa/port.c b/net/dsa/port.c
index 9b54e5a76297..dc4da4dc44f5 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -629,9 +629,14 @@ static int dsa_port_phylink_register(struct dsa_port *dp)
 int dsa_port_link_register_of(struct dsa_port *dp)
 {
        struct dsa_switch *ds = dp->ds;
+       struct device_node *phy_np;
 
-       if (!ds->ops->adjust_link)
-               return dsa_port_phylink_register(dp);
+       if (!ds->ops->adjust_link) {
+               phy_np = of_parse_phandle(dp->dn, "phy-handle", 0);
+               if (of_phy_is_fixed_link(dp->dn) || phy_np)
+                       return dsa_port_phylink_register(dp);
+               return 0;
+       }
 
        dev_warn(ds->dev,
                 "Using legacy PHYLIB callbacks. Please migrate to PHYLINK!\n");
@@ -646,11 +651,12 @@ void dsa_port_link_unregister_of(struct dsa_port *dp)
 {
        struct dsa_switch *ds = dp->ds;
 
-       if (!ds->ops->adjust_link) {
+       if (!ds->ops->adjust_link && dp->pl) {
                rtnl_lock();
                phylink_disconnect_phy(dp->pl);
                rtnl_unlock();
                phylink_destroy(dp->pl);
+               dp->pl = NULL;
                return;
        }
 

If we go with this, we can assume we do know the speed, either from
fixed-link, or the PHY when it established the link.

There is maybe one use case not covered by my patch. A port might just
have a phy-mode property, e.g. 'rgmii-id', but no fixed link. I took a
quick look at the usual suspects in DT, and i didn't find an actual
example of this. And i also need to look at the code pre-phylink
integration to see if it was actually supported.

	    Andrew