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

[John Ernberg]

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@xxxxxxxx> 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@xxxxxxxx>
> 
> [...]
> 
>> @@ -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