netdev - Re: [PATCH net-next 0/5] net: phy: C45-over-C22 access

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: <Y87WR/T395hKmgKm@shell.armlinux.org.uk>
Date:   Mon, 23 Jan 2023 18:47:35 +0000
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Michael Walle <michael@...le.cc>,
        Yisen Zhuang <yisen.zhuang@...wei.com>,
        Salil Mehta <salil.mehta@...wei.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Broadcom internal kernel review list 
        <bcm-kernel-feedback-list@...adcom.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Marek Behún <kabel@...nel.org>,
        Xu Liang <lxu@...linear.com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 0/5] net: phy: C45-over-C22 access

On Mon, Jan 23, 2023 at 07:03:18PM +0100, Andrew Lunn wrote:
> On Fri, Jan 20, 2023 at 11:40:06PM +0100, Michael Walle wrote:
> > After the c22 and c45 access split is finally merged. This can now be
> > posted again. The old version can be found here:
> > https://lore.kernel.org/netdev/20220325213518.2668832-1-michael@walle.cc/
> > Although all the discussion was here:
> > https://lore.kernel.org/netdev/20220323183419.2278676-1-michael@walle.cc/
> > 
> > The goal here is to get the GYP215 and LAN8814 running on the Microchip
> > LAN9668 SoC. The LAN9668 suppports one external bus and unfortunately, the
> > LAN8814 has a bug which makes it impossible to use C45 on that bus.
> > Fortunately, it was the intention of the GPY215 driver to be used on a C22
> > bus. But I think this could have never really worked, because the
> > phy_get_c45_ids() will always do c45 accesses and thus gpy_probe() will
> > fail.
> > 
> > Introduce C45-over-C22 support and use it if the MDIO bus doesn't support
> > C45. Also enable it when a PHY is promoted from C22 to C45.
> 
> I see this breaking up into two problems.
> 
> 1) Scanning the bus and finding device, be it by C22, C45, or C45 over C22.
> 
> 2) Allowing drivers to access C45 register spaces, without caring if
> it is C45 transfers or C45 over C22.
> 
> For scanning the bus we currently have:
> 
> 
>         if (bus->read) {
>                 err = mdiobus_scan_bus_c22(bus);
>                 if (err)
>                         goto error;
>         }
> 
>         prevent_c45_scan = mdiobus_prevent_c45_scan(bus);
> 
>         if (!prevent_c45_scan && bus->read_c45) {
>                 err = mdiobus_scan_bus_c45(bus);
>                 if (err)
>                         goto error;
>         }
> 
> I think we should be adding something like:
> 
> 	else {
> 		if (bus->read) {
> 	                err = mdiobus_scan_bus_c45_over_c22(bus);
> 	                if (err)
> 	                        goto error;
> 	        }
> 	}
> 
> That makes the top level pretty obvious what is going on.
> 
> But i think we need some more cleanup lower down. We now have a clean
> separation in MDIO bus drivers between C22 bus transactions and C45
> transactions bus. But further up it is less clear. PHY drivers should
> be using phy_read_mmd()/phy_write_mmd() etc, which means access the
> C45 address space, but says nothing about what bus transactions to
> use. So that is also quite clean.
> 
> The problem is in the middle.  get_phy_c45_devs_in_pkg() uses
> mdiobus_c45_read(). Does mdiobus_c45_read() mean perform a C45 bus
> transaction, or access the C45 address space? I would say it means
> perform a C45 bus transaction. It does not take a phydev, so we are
> below the concept of PHYs, and so C45 over C22 does not exist at this
> level.

C45-over-C22 is a PHY thing, it isn't generic. We shouldn't go poking
at the PHY C45-over-C22 registers unless we know for certain that the
C22 device we are accessing is a PHY, otherwise we could be writing
into e.g. a switch register or something else.

So, the mdiobus_* API should be the raw bus API. If we want C45 bus
cycles then mdiobus_c45_*() is the API that gives us that, vs C22 bus
cycles through the non-C45 API.

C45-over-C22 being a PHY thing is something that should be handled by
phylib, and currently is. The phylib accessors there will use C45 or
C45-over-C22 as appropriate.

The problem comes with PHYs that maybe don't expose C22 ID registers
but do have C45-over-C22. These aren't detectable without probing
using the C45-over-C22 PHY protocol, but doing that gratuitously will
end up writing values to e.g. switch registers and disrupting their
operation. So I regard that as a very dangerous thing to be doing.

Given that, it seems that such a case could not be automatically
probed, and thus must be described in firmware.

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