netdev - Re: [PATCH net] net: dsa: microchip: keep compatibility with device tree blobs with no phy-mode

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: <Yv5bsEpICDSuJUgs@shell.armlinux.org.uk>
Date:   Thu, 18 Aug 2022 16:33:04 +0100
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Vladimir Oltean <vladimir.oltean@....com>, netdev@...r.kernel.org,
        Woojung Huh <woojung.huh@...rochip.com>,
        UNGLinuxDriver@...rochip.com,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Michael Grzeschik <m.grzeschik@...gutronix.de>,
        Oleksij Rempel <linux@...pel-privat.de>,
        Thorsten Leemhuis <regressions@...mhuis.info>,
        Alvin Šipraga <alsi@...g-olufsen.dk>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Craig McQueen <craig@...ueen.id.au>
Subject: Re: [PATCH net] net: dsa: microchip: keep compatibility with device
 tree blobs with no phy-mode

On Thu, Aug 18, 2022 at 05:13:51PM +0200, Andrew Lunn wrote:
> > It is important to note that phy_device_create() initializes
> > dev->interface = PHY_INTERFACE_MODE_GMII, and so, when we use
> > phylink_create(PHY_INTERFACE_MODE_NA), no one will override this, and we
> > will end up with a PHY_INTERFACE_MODE_GMII interface inherited from the
> > PHY.
> 
> Is this actually a bug?
> 
> With pure phylib, you should call one of the connect functions, which
> underneath calls phy_attach_direct() which has a phy_interface_t. So
> the default in practice does not matter.
> 
> > All this means that in order to maintain compatibility with device tree
> > blobs where the phy-mode property is missing, we need to allow the
> > "gmii" phy-mode and treat it as "internal".
> 
> of_get_phy_mode() returns PHY_INTERFACE_MODE_NA if the property is
> missing, which also suggests this is a bug.
> 
> I wonder if we have any ports which actually rely on
> PHY_INTERFACE_MODE_GMII?

Loads now. Florian contributed the code to phylink that detects when
DSA initialises phylink with PHY_INTERFACE_MODE_NA, and then looks at
phy->interface _before_ calling phy_attach_direct() - and this is how
we end up with PHY_INTERFACE_MODE_GMII.

See:

commit 4904b6ea1f9dbf47107f50b1c502a22d0160712d
Author: Florian Fainelli <f.fainelli@...il.com>
Date:   Tue Dec 12 16:00:26 2017 -0800

    net: phy: phylink: Use PHY device interface if N/A

    We may not always be able to resolve a correct phy_interface_t value before
    actually connecting to the PHY device, when that happens, just have
    phylink_connect_phy() utilize what the PHY device/driver provided.

    Signed-off-by: Florian Fainelli <f.fainelli@...il.com>
    Signed-off-by: David S. Miller <davem@...emloft.net>

submitted for 4.16-rc1. DSA then used this in:

commit aab9c4067d2389d0adfc9c53806437df7b0fe3d5
Author: Florian Fainelli <f.fainelli@...il.com>
Date:   Thu May 10 13:17:36 2018 -0700

    net: dsa: Plug in PHYLINK support
...

for 4.18-rc1 to connect to its PHYs:

static int dsa_slave_phy_connect(struct net_device *slave_dev, int addr)
{
        struct dsa_port *dp = dsa_slave_to_port(slave_dev);
        struct dsa_switch *ds = dp->ds;

        slave_dev->phydev = mdiobus_get_phy(ds->slave_mii_bus, addr);
        if (!slave_dev->phydev) {
                netdev_err(slave_dev, "no phy at %d\n", addr);
                return -ENODEV;
        }

        return phylink_connect_phy(dp->pl, slave_dev->phydev);
}
...
        ret = phylink_of_phy_connect(dp->pl, port_dn, phy_flags);
        if (ret == -ENODEV) {
                /* We could not connect to a designated PHY or SFP, so use the
                 * switch internal MDIO bus instead
                 */
                ret = dsa_slave_phy_connect(slave_dev, dp->index);
                if (ret) {
                        netdev_err(slave_dev,
                                   "failed to connect to port %d: %d\n",
                                   dp->index, ret);
                        phylink_destroy(dp->pl);
                        return ret;
                }
        }

which will be used when there is no phy-handle property.

I extended the change in 4904b6ea1f9dbf47107f50b1c502a22d0160712d to also
apply to fwnode setups in:

commit a18e6521a7d95dae8c65b5b0ef6bbe624fbe808c
Author: Russell King (Oracle) <rmk+kernel@...linux.org.uk>
Date:   Fri Nov 19 16:28:06 2021 +0000

    net: phylink: handle NA interface mode in phylink_fwnode_phy_connect()

because we were ending up with DSA drivers using PHY_INTERFACE_MODE_NA
inside phylink, and phylink has always special-cased that and drivers
have taken it to mean "give me all interface modes that are supported"
which is not what DSA should be using. It also gives a uniform and
understandable behaviour from phylink for DSA, rather than having
phylink behave one way (where no phy-mode and no phy-handle are
specified) but in a completely different way when phy-handle without
phy-mode is specified. It seemed to be the sensible thing to do, and
Florian agreed at the time.

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