linux-kernel - 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]

Message-ID: <8304fb3578ee38525a158af768691e75@walle.cc>
Date:   Wed, 23 Mar 2022 23:38:43 +0100
From:   Michael Walle <michael@...le.cc>
To:     Andrew Lunn <andrew@...n.ch>
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

Am 2022-03-23 21:07, schrieb Andrew Lunn:
> 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.

I presume you are speaking of
https://elixir.bootlin.com/linux/v5.17/source/drivers/net/phy/mdio_bus.c#L700

Shouldn't that be the other way around then? How would you tell if
you can do 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.

That is related to how C45 capable PHYs are probed (your 4) above),
right? If the PHY would be probed correctly as C45 we wouldn't have
to worry about it. TBH I didn't consider that a valid case because
I thought there were other means to tell "treat this PHY as C45",
that is by the device tree compatible, for example.

Btw. all of this made me question if this is actually the correct
place, or if if shouldn't be handled in the core. With a flag
in the phy driver which might indicate its capable of doing
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.

Mh? I can't follow you here. Are you talking about the
probe_capabilites? These are for the bus probing, i.e. if you can
call mdiobus_c45_read().

-michael