linux-kernel - Re: [RFC 08/11] net: phy: Allow mdio buses to auto-probe c45 devices

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: <20200525224143.GP1551@shell.armlinux.org.uk>
Date:   Mon, 25 May 2020 23:41:43 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Jeremy Linton <jeremy.linton@....com>
Cc:     Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org,
        davem@...emloft.net, f.fainelli@...il.com, hkallweit1@...il.com,
        madalin.bucur@....nxp.com, calvin.johnson@....nxp.com,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC 08/11] net: phy: Allow mdio buses to auto-probe c45 devices

On Mon, May 25, 2020 at 05:09:56PM -0500, Jeremy Linton wrote:
> Hi,
> 
> On 5/25/20 3:25 AM, Russell King - ARM Linux admin wrote:
> > On Sun, May 24, 2020 at 11:28:52PM -0500, Jeremy Linton wrote:
> > > Hi,
> > > 
> > > On 5/24/20 9:44 AM, Andrew Lunn wrote:
> > > > > +++ b/include/linux/phy.h
> > > > > @@ -275,6 +275,11 @@ struct mii_bus {
> > > > >    	int reset_delay_us;
> > > > >    	/* RESET GPIO descriptor pointer */
> > > > >    	struct gpio_desc *reset_gpiod;
> > > > > +	/* bus capabilities, used for probing */
> > > > > +	enum {
> > > > > +		MDIOBUS_C22_ONLY = 0,
> > > > > +		MDIOBUS_C45_FIRST,
> > > > > +	} probe_capabilities;
> > > > >    };
> > > > 
> > > > 
> > > > I'm not so keen on _FIRST. It suggest _LAST would also be valid.  But
> > > > that then suggests this is not a bus property, but a PHY property, and
> > > > some PHYs might need _FIRST and other phys need _LAST, and then you
> > > > have a bus which has both sorts of PHY on it, and you have a problem.
> > > > 
> > > > So i think it would be better to have
> > > > 
> > > > 	enum {
> > > > 		MDIOBUS_UNKNOWN = 0,
> > > > 		MDIOBUS_C22,
> > > > 		MDIOBUS_C45,
> > > > 		MDIOBUS_C45_C22,
> > > > 	} bus_capabilities;
> > > > 
> > > > Describe just what the bus master can support.
> > > 
> > > Yes, the naming is reasonable and I will update it in the next patch. I went
> > > around a bit myself with this naming early on, and the problem I saw was
> > > that a C45 capable master, can have C45 electrical phy's that only respond
> > > to c22 requests (AFAIK).
> > 
> > If you have a master that can only generate clause 45 cycles, and
> > someone is daft enough to connect a clause 22 only PHY to it, the
> > result is hardware that doesn't work - there's no getting around
> > that.  The MDIO interface can't generate the appropriate cycles to
> > access the clause 22 PHY.  So, this is not something we need care
> > about.
> > 
> > > So the MDIOBUS_C45 (I think I was calling it
> > > C45_ONLY) is an invalid selection. Not, that it wouldn't be helpful to have
> > > a C45_ONLY case, but that the assumption is that you wouldn't try and probe
> > > c22 registers, which I thought was a mistake.
> > 
> > MDIOBUS_C45 means "I can generate clause 45 cycles".
> > MDIOBUS_C22 means "I can generate clause 22 cycles".
> > MDIOBUS_C45_C22 means "I can generate both clause 45 and clause 22
> > cycles."
> 
> Hi, to be clear, we are talking about c45 controllers that can access the
> c22 register space via c45, or controllers which are electrically/level
> shifting to be compatible with c22 voltages/etc?

To me, Andrew was quite clear that these flags should describe what
the MDIO controller can support, and what I understand from Andrew's
email is:

If it can't support clause 45 accesses, then it should not advertise
MDIOBUS_C45 nor MDIOBUS_C45_C22.

If it can't support clause 22 accesses, then it should not advertise
MDIOBUS_C22.

And that's all there is to it.

If you want to talk about clause 45 access via the clause 22 register
set, that is a property of the PHY, not of the MDIO controller, and
the MDIO controller has no business attempting to describe that
property in any shape or form since it is a PHY property.

If you have access to clause 22 registers, then you likely have the
clause 22 ID registers, and the way phylib currently works, that will
also match a PHY driver.

Once we have a PHY and a driver bound, then even if it is a C45
driver, accesses using the phy_*_mmd() functions will _automatically_
switch to using C22 cycles to the indirect C45 access registers if
the PHY has not been detected as a C45 PHY (phydev->is_c45 is false.)

So, I'm coming to the conclusion that you're making way more work
here than is necessary, your changes to gratuitously change the way
stuff in phylib work which is not risk-free are completely unnecessary
and really not a risk worth taking when it's likely that we already
have the code mostly in place to be able to support your PHY.

I think there are some questionable uses of phydev->is_c45 that
your point about accessing C45 PHYs through C22 indirect registers
brings up which need to be resolved, but I really don't think that's
a completely separate issue.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up