linux-kernel - Re: [PATCH net-next v3 02/11] net: phy: introduce phy_has_c45

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: <ZMrYcbY65yOZyaxo@shell.armlinux.org.uk>
Date:   Wed, 2 Aug 2023 23:28:01 +0100
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Michael Walle <mwalle@...nel.org>,
        Heiner Kallweit <hkallweit1@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Yisen Zhuang <yisen.zhuang@...wei.com>,
        Salil Mehta <salil.mehta@...wei.com>,
        Florian Fainelli <florian.fainelli@...adcom.com>,
        Broadcom internal kernel review list 
        <bcm-kernel-feedback-list@...adcom.com>,
        Marek Behún <kabel@...nel.org>,
        Xu Liang <lxu@...linear.com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Simon Horman <simon.horman@...igine.com>
Subject: Re: [PATCH net-next v3 02/11] net: phy: introduce
 phy_has_c45_registers()

On Thu, Aug 03, 2023 at 12:21:03AM +0200, Andrew Lunn wrote:
> On Wed, Aug 02, 2023 at 06:10:27PM +0100, Russell King (Oracle) wrote:
> > On Wed, Aug 02, 2023 at 06:15:19PM +0200, Andrew Lunn wrote:
> > > > I'm confused now. Andrew suggested to split it into four different
> > > > functions:
> > > > 
> > > > phy_has_c22_registers()
> > > > phy_has_c45_registers()
> > > > phy_has_c22_transfers()
> > > > phy_has_c45_transfers()
> > > > 
> > > > Without a functional change. That is, either return phydev->is_c45
> > > > or the inverse.
> > > 
> > > Without a functional change at this step of introducing the four
> > > functions. Then later really implement them to do what the name
> > > implies. Doing it in steps helps with bisect when it breaks something.
> > > 
> > > It could also be that not all four are needed, or not all four are
> > > possible. But the four express the full combinations of transfers and
> > > registers.
> > 
> > I'm left wondering how you think that phy_has_c45_registers() can
> > be implemented in a useful way, because I'm at a total loss...
> 
> get_phy_c22_id() looks at the ID registers, and see if its mostly
> Fs. If so, it says -ENODEV. So phy_has_c22_registers() could be
> get_phy_c22_id() != -ENODEV. And you can extend that with the logic
> you quoted from the standard. get_phy_c45_ids() also returns -ENODEV
> if there are no devices in the package. So phy_has_c45_registers()
> could be get_phy_c45_id() != -ENODEV.

No, this is not correct, because get_phy_c45_id() uses mdiobus c45
reads - if the bus doesn't support c45 transfers, then get_phy_c45_id()
fails.

This doesn't tell us whether the PHY has clause 45 registers at all,
it just tells us that we probed the PHY successfully using clause 45
accesses.

The fact of the matter is that there is no way in the current phylib
code (and probably no way to probe it either) to determine whether a
PHY has clause 45 registers or not. To see why, take for example the
AR8035. This clearly has clause 45 registers, because it has MMDs 3
and 7 implemented. However, these are only accessible through the
indirect access method via clause 22 cycles.

So, does it make sense for AR8035 to return false from
phy_has_c45_registers() ? No it doesn't, because it clearly does have
clause 45 registers!

The same can be said about Marvell 88e151x PHYs. And probably many
other clause 22 PHYs.

Hence why I disagree with your suggestion.

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