netdev - Re: C22/C45 decision in phy_restart

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

Message-ID: <a78381ed-cee6-4a1f-b352-886d6e92f7cf@lunn.ch>
Date: Sun, 31 Aug 2025 17:35:54 +0200
From: Andrew Lunn <andrew@...n.ch>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: markus.stockhausen@....de, hkallweit1@...il.com, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	netdev@...r.kernel.org, jan@....eu,
	'Chris Packham' <chris.packham@...iedtelesis.co.nz>
Subject: Re: C22/C45 decision in phy_restart_aneg()

> > In our case, this function fails for C45 PHYs because the bus and PHY are 
> > locked down to this single mode. It's stupid, of course, but that's how 
> > it is. I see two options for fixing the issue:
> > 
> > 1) Mask the C22 presence in the bus for all PHYs when running in C45.
> > 2) Drop the C22 condition check in the above function.
> 
> I guess we also need to make these kinds of tests conditional on
> whether the bus supports C45 or not.
> 
> static bool mdiobus_supports_c22(struct mii_bus *bus)
> {
> 	return bus->read && bus->write;
> }
> 
> static bool mdiobus_supports_c45(struct mii_bus *bus)
> {
> 	return bus->read_c45 && bus->write_c45;
> }

Something i've said in the past is that we have a mess regarding what
does phydev->is_c45 mean? And really, this is an extension of that
mess.

There are two separate C45 concepts which get mixed up in is_c45.

One concept is, does the PHY have the C45 register address space?  The
second concept is, how do you access the C45 address space? Via C45
bus transactions, or C45 over C22 bus transactions.

The extension here is, does the PHY have the C22 address space, and
what sort of bus transactions do you use to access the C22 address
space. It is however simplified, there is no C22 over C45 as far as i
know. So it is actually, can you do C22 bus transactions or not?

I've tried in the past to get developers to sort out this mess, but it
never got very far. Maybe we can use this as an opportunity to address
a little bit of the problem?

> This should be fine, because we already require MII bus drivers to only
> populate these methods only when they're supported (see commit
> fbfe97597c77 ("net: phy: Decide on C45 capabilities based on presence
> of method").
> 
> 	if (phydev->is_c45 && !(phydev->c45_ids.devices_in_package & BIT(0))
> 
> can then become:
> 
> 	if (phydev->is_c45 &&
> 	    !(mdiobus_supports_c22(phydev->mdio.bus) &&
> 	      phydev->c45_ids.devices_in_package & MDIO_DEVS_C22PRESENT))

So what we are asking here is, can we do C22 bus transfers, and does
the PHY have the C22 address space? And looking in the code, there are
at least three places this is needed:

int phy_restart_aneg(struct phy_device *phydev)
{
        int ret;

        if (phydev->is_c45 && !(phydev->c45_ids.devices_in_package & BIT(0)))
                ret = genphy_c45_restart_aneg(phydev);
        else
                ret = genphy_restart_aneg(phydev);

        return ret;
}

int phy_aneg_done(struct phy_device *phydev)
{
        if (phydev->drv && phydev->drv->aneg_done)
                return phydev->drv->aneg_done(phydev);
        else if (phydev->is_c45)
                return genphy_c45_aneg_done(phydev);
        else
                return genphy_aneg_done(phydev);
}

This i _think_ is currently broken, i would expect the same condition
to apply.

int phy_config_aneg(struct phy_device *phydev)
{
        if (phydev->drv->config_aneg)
                return phydev->drv->config_aneg(phydev);

        /* Clause 45 PHYs that don't implement Clause 22 registers are not
         * allowed to call genphy_config_aneg()
         */
        if (phydev->is_c45 && !(phydev->c45_ids.devices_in_package & BIT(0)))
                return genphy_c45_config_aneg(phydev);

        return genphy_config_aneg(phydev);
}

So maybe we should add new members to phydev?

@bus_has_c22: The bus can perform C22 transactions
@phy_has_c22_regs: The PHY has the C22 register address space.

During probe, we set bus_has_c22 based on Russells
mdiobus_supports_c22() above. phy_has_c22_regs should be set if we
discover the PHY by C22 ID registers, or
phydev->c45_ids.devices_in_package indicates the PHY has C22
registers. We also need to handle the PHY is instantiated via ID
values in DT. After the PHY has probed, we can read C22 registers 2
and 3, and see if the ID look valid, i.e. mostly not F, same as we do
in get_phy_c22_id().

The condition then becomes:

        if (phydev->is_c45 && !(phydev->bus_has_c22 && phydev->phy_has_c22_regs))

Then maybe some time in the future, the is_c45 will get replaced with
something similar?

	Andrew