netdev - [net-next] dsa: slave: support phy devices on external MII bus

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-next>] [day] [month] [year] [list]

Message-ID: <20171018162138.GA15371@lunn.ch>
Date:   Wed, 18 Oct 2017 18:21:38 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     mnhu@...vas.dk
Cc:     netdev <netdev@...r.kernel.org>,
        Florian Fainelli <f.fainelli@...il.com>
Subject: [net-next] dsa: slave: support phy devices on external MII bus

Hi Martin

Sorry for starting a new thread. I deleted the patchset from my mailbox.

Florian said:

> The logic goes like this:
> 
> - try to connect to the PHY via phy-handle
> - if we have a PHY we are connecting via phy-handle but we need to
>   divert MDIO reads/writes connect using its address on the diverted
>   bus
> - connect using a fixed PHY
> - finally try using the DSA slave MII bus which would connect to the switch internal PHYs

This is not quite correct. Looking at the code:

        phy_dn = of_parse_phandle(port_dn, "phy-handle", 0);
 ...

        if (phy_dn) {
                int phy_id = of_mdio_parse_addr(&slave_dev->dev, phy_dn);

                /* If this PHY address is part of phys_mii_mask, which means
                 * that we need to divert reads and writes to/from it, then we
                 * want to bind this device using the slave MII bus created by
                 * DSA to make that happen.
                 */
                if (!phy_is_fixed && phy_id >= 0 &&
                    (ds->phys_mii_mask & (1 << phy_id))) {
                        ret = dsa_slave_phy_connect(p, slave_dev, phy_id);
                        if (ret) {
                                netdev_err(slave_dev, "failed to connect to phy%d: %d\n", phy_id, ret);
                                of_node_put(phy_dn);
                                return ret;
                        }
                } else {
                        p->phy = of_phy_connect(slave_dev, phy_dn,

The first point really is:

- try to connect to the PHY via phy-handle, if the phy_id is not
valid, or if the phy_id does not map to a phy that the switch says
does not exist.

In your case, all these points are true, so it uses
dsa_slave_phy_connect().  But we actually want it to use
of_phy_connect(), which will use the correct bus.

For some Marvell chips, you cannot actually go on ds->phys_mii_mask.
These devices can have in built PHYs and SERDES interfaces which can
be assigned to ports. These SERDES interfaces could have external PHYs
connected to them, and be on the external MDIO bus. So
ds->phys_mii_mask indicates there is a PHY, but the phy-handle points
to a different phy.

So i think this code block needs to change. If we have a phy-handle,
use it.  i.e. what Florian _thinks_ it should be doing. If not, then
use dsa_slave_phy_connect().

    Andrew