netdev - Re: Clause 45 and Clause 22 PHYs on one MDIO 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-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 21 Mar 2022 23:34:27 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Michael Walle <michael@...le.cc>
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: Clause 45 and Clause 22 PHYs on one MDIO bus

On Mon, Mar 21, 2022 at 10:41:56PM +0100, Michael Walle wrote:
> Am 2022-03-21 21:36, schrieb Andrew Lunn:
> > > Actually, it looks like mdiobus_c45_read() is really c45 only and only
> > > used for PHYs which just support c45 and not c45-over-c22 (?). I was
> > > mistaken by the heavy use of the function in phy_device.c. All the
> > > methods in phy-c45.c use phy_*_mmd() functions. Thus it might only be
> > > the mxl-gpy doing something fishy in its probe function.
> > 
> > Yes, there is something odd here. You should search back on the
> > mailing list.
> > 
> > If i remember correctly, it is something like it responds to both c22
> > and c45. If it is found via c22, phylib does not set phydev->is_c45,
> > and everything ends up going indirect. So the probe additionally tries
> > to find it via c45? Or something like that.
> 
> Yeah, found it: https://lore.kernel.org/netdev/YLaG9cdn6ewdffjV@lunn.ch/
> 
> But that means that if the controller is not c45 capable, it will always
> fail to probe, no?

The problem is around here:

https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy-c45.c#L455

phydev->c45_ids.mmds_present needs to be set. Which happens as part of
get_phy_c45_ids(). What probably needs to happen is the
mdiobus_c45_read() in that function need to change to phy_read_mmd()
so that it can use C45 over C22. The devil in the details is making
sure it does actually do C45 if C45 is available, otherwise we could
break devices on a C45 only bus, of which there is a few.

> I'll have to give it a try. First I was thinking that we wouldn't need
> it because a broken PHY driver could just set a quirk "broken_c45_access"
> or similar. But that would mean it has to be probed before any c45 PHY.
> Dunno if that will be true for the future. And it sounds rather fragile.
> So yes, a dt property might be a better option.

This is unfortunately not a PHY property, but a bus property. This bus
has a FUBAR PHY on it, which limits the protocols that can be used on
this bus. And there is no clear relationship between PHYs, you cannot
easily say which PHYs share the same bus. So even if the lan8814 was
to indicate it is FUBAR, you have no idea which other PHYs are
affected.

A DT property is more generic, and if done correct, could actually ban
C22 or it could ban C45.

    Andrew