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

Message-ID: <aLQfGgYfTdcCFHtC@shell.armlinux.org.uk>
Date: Sun, 31 Aug 2025 11:08:26 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: markus.stockhausen@....de
Cc: andrew@...n.ch, 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()

On Sun, Aug 31, 2025 at 09:45:46AM +0200, markus.stockhausen@....de wrote:
> @Russell: Sorry for the inconvenience of my first mail. After a lengthy
> analysis session, I focused too much on the initial C45 enhancement 
> of the below function that you wrote. I sent it off too hastily.

It's important to send messages to the right people for a proper
discussion. Andrew may have some input on this - maybe as a result
of my ideas below.

> So, once again, here is some interesting finding regarding the 
> limitations of the Realtek MDIO driver. Remember that it can only run 
> in either C22 or C45. This time it is about autonegotiation restart.
> 
> 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;
> }
> 
> I assume that BIT(0) means MDIO_DEVS_C22_PRESENT. As I understand it,
> this basically uses C22 commands for C45 PHYs if C22 support is detected.
> Currently, I have no reasonable explanation for this additional check.

There was discussion and delving into the spec when this was added.
See https://lore.kernel.org/all/20170601102327.GF27796@n2100.armlinux.org.uk/

(side note: google is now useless for finding that, searching for "hook
up clause 45 autonegotiation restart" returns very few results, none of
them with the full history. I've had to search my mailboxes for it, find
the message ID, and then look it up on lore. We now have various web
high-bandwidth crawlers that do not respect robots.txt to thank for this
as sites have ended up blocking all crawlers using stuff like Anubis
proof of work.)

It's been a long time, and so I've forgotten the details now, so all
there is to go on are the above emails.

You may notice that the initial version I submitted just used
phydev->is_c45.

> 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;
}

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))

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