netdev - Re: [PATCH RFC net-next 4/5] net: phy: introduce is_c45_over

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:   Wed, 23 Mar 2022 21:07:18 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Michael Walle <michael@...le.cc>
Cc:     Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        "David S . Miller" <davem@...emloft.net>,
        Xu Liang <lxu@...linear.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC net-next 4/5] net: phy: introduce is_c45_over_c22 flag

On Wed, Mar 23, 2022 at 07:34:18PM +0100, Michael Walle wrote:
> The GPY215 driver supports indirect accesses to c45 over the c22
> registers. In its probe function phy_get_c45_ids() is called and the
> author descibed their use case as follows:
> 
>   The problem comes from condition "phydev->c45_ids.mmds_present &
>   MDIO_DEVS_AN".
> 
>   Our product supports both C22 and C45.
> 
>   In the real system, we found C22 was used by customers (with indirect
>   access to C45 registers when necessary).
> 
> So it is pretty clear that the intention was to have a method to use the
> c45 features over a c22-only MDIO bus. The purpose of calling
> phy_get_c45_ids() is to populate the .c45_ids for a PHY which wasn't
> probed as a c45 one. Thus, first rename the phy_get_c45_ids() function
> to reflect its actual meaning and second, add a new flag which indicates
> that this is actually a c45 PHY but behind a c22 bus. The latter is
> important for phylink because phylink will treat c45 in a special way by
> checking the .is_c45 property. But in our case this isn't set.

Thinking out loud...

1) We have a C22 only bus. Easy, C45 over C22 should be used.

2) We have a C45 only bus. Easy, C45 should be used, and it will of
   probed that way.

3) We have a C22 and C45 bus, but MDIOBUS_NO_CAP. It will probe C22,
   but ideally we want to swap to C45.

4) We have a C22 and C45 bus, MDIOBUS_C22_C45. It will probe C22, but
   ideally we want to swap to C45.

> @@ -99,7 +99,7 @@ static int gpy_probe(struct phy_device *phydev)
>  	int ret;
>  
>  	if (!phydev->is_c45) {
> -		ret = phy_get_c45_ids(phydev);
> +		ret = phy_get_c45_ids_by_c22(phydev);
>  		if (ret < 0)
>  			return ret;
>  	}

If we are inside the if, we know we probed C22. We have to achieve two
things:

1) Get the c45 ids,
2) Figure out if C45 works, or if C45 over C22 is needed.

I don't see how we are getting this second bit of information, if we
are explicitly using c45 over c22.

This _by_c22 is also making me think of the previous patch, where we
look at the bus capabilities. We are explicitly saying here was want
c45 over c22, and the PHY driver should know the PHY is capable of
it. So we don't need to look at the capabilities, just do it.

     Andrew