netdev - Re: [PATCH net v3 2/2] net: fec: Suspend the PHY on probe

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: <9490ed31-dede-4a14-9c62-5ef83e30593a@actia.se>
Date: Tue, 19 Mar 2024 08:37:44 +0000
From: John Ernberg <john.ernberg@...ia.se>
To: Maxime Chevallier <maxime.chevallier@...tlin.com>
CC: Wei Fang <wei.fang@....com>, Shenwei Wang <shenwei.wang@....com>, "Clark
 Wang" <xiaoning.wang@....com>, NXP Linux Team <linux-imx@....com>, "David S.
 Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, "Jakub
 Kicinski" <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Heiner Kallweit
	<hkallweit1@...il.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Andrew Lunn
	<andrew@...n.ch>, Russell King <linux@...linux.org.uk>
Subject: Re: [PATCH net v3 2/2] net: fec: Suspend the PHY on probe

Hi Maxime,

Apologies for the delay in my response.

On 3/6/24 19:05, Maxime Chevallier wrote:
> Hello John,
> 
> I'm adding Andrew and Russell to the thread as PHY maintainers and
> reviewers.
> 
> On Wed, 6 Mar 2024 13:37:45 +0000
> John Ernberg <john.ernberg@...ia.se> wrote:
> 
>> Since the power management is now performed by the FEC instead of generic
>> pm the PHY will not suspend until the link has been up.
>>
>> Therefor suspend it on probe. It will be resumed by {of_,}phy_connect()
>> when the link is brought up.
>>
>> Since {of_,}phy_connect() and phy_disconnect() will resume and suspend the
>> PHY when the link is brought up and down respectively, and phy_stop() and
>> phy_start() will resume and suspend the PHY in the suspend-resume paths
>> there is no need for any additional calls anywhere.
>>
>> Signed-off-by: John Ernberg <john.ernberg@...ia.se>
> 
> [...]
> 
>> @@ -2539,8 +2539,10 @@ static int fec_enet_mii_init(struct platform_device *pdev)
>>   	/* find all the PHY devices on the bus and set mac_managed_pm to true */
>>   	for (addr = 0; addr < PHY_MAX_ADDR; addr++) {
>>   		phydev = mdiobus_get_phy(fep->mii_bus, addr);
>> -		if (phydev)
>> +		if (phydev) {
>>   			phydev->mac_managed_pm = true;
>> +			phy_suspend(phydev);
>> +		}
> 
> I don't think that's correct. here phy_suspend() is being called before
> the PHY got attached, so the PHY wasn't initialized at all at that
> point (which I guess is your issue as the PHY is still in the state it
> was configured into by the bootloader)
> 
> Following the code paths, it looks like this works for you because the
> PHY you're using has a .suspend callback populated, but for any PHY
> that uses the genphy driver, this will do nothing at all (the PHY isn't
> yet attached to the genphy ops, therefore genphy_suspend won't be
> called).

Thanks for highlighting this.

Yes, it's a problem for genphy, although due to when genphy is probed, 
it's always been a problem for genphy, even before this patch. Whereas 
PHYs with specific drivers worked before due to MDIO bus PM.

There is also a case where the phy driver module is not automatically 
loaded, in cases where request_module() fails, either due to the 
userspace helper feature being compiled out or other reasons, and the 
module is loaded manually later. I suspect for reasons like these the 
genphy probe happens so late. My solution here doesn't cover non-loaded 
modules either, but this could maybe be covered by moving phy_suspend() 
to phy_probe(). Unless there is an even more clever way to go about it 
which I can't see from inexperience.

If a PHY driver doesn't populate .suspend there's probably a good reason 
for it and it makes sense to not suspend such a PHY, so  I'm not 
concerned about an unpopulated .suspend.

Best regards // John Ernberg

> 
> Best regards,
> 
> Maxime