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: <Y87L5r8uzINALLw4@lunn.ch>
Date:   Mon, 23 Jan 2023 19:03:18 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Michael Walle <michael@...le.cc>
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>,
        Russell King <linux@...linux.org.uk>,
        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 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.

So i think we need to review all calls to
mdiobus_c45_read/mdiobus_c45_write() etc and see if they mean C45 bus
transaction or C45 address space. Those meaning address space should
be changed to phy_read_mmd()/phy_write_mmd().

get_phy_device(), get_phy_c45_devs_in_pkg(), get_phy_c45_ids(),
phy_c45_probe_present() however do not deal with phydev, so cannot use
phy_read_mmd()/phy_write_mmd(). They probably need the bool is_c45
replaced with an enum indicating what sort of bus transaction should
be performed. Depending on that value, they can call
mdiobus_c45_read() or mmd_phy_indirect() and __mdiobus_read().

I don't have time at the moment, but i would like to dig more into
phydev->is_c45. has_c45 makes sense to indicate it has c45 address
space. But we need to see if it is every used to indicate to use c45
transactions. But it is clear we need a new member to indicate if C45
or C45 over C22 should be performed, and this should be set by how the
PHY was found in the first place.

    Andrew