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

[John Ernberg]

Hi Florian,

On 3/21/24 17:13, Florian Fainelli wrote:
> On 3/21/24 09:02, John Ernberg wrote:
>> Hi Russell,
>>
>> On 3/20/24 20:44, Russell King (Oracle) wrote:
>>> On Wed, Mar 20, 2024 at 10:13:55AM -0700, Florian Fainelli wrote:
>>>>
>>>>
>>>> On 3/20/2024 9:54 AM, Russell King (Oracle) wrote:
>>>>> On Wed, Mar 20, 2024 at 03:25:54PM +0000, John Ernberg wrote:
>>>>>> Hi Russel,
>>>>>
>>>>> Growl. Hi Peter.
>>>>>
>>>>>> What we really want is the PHY to be suspended on suspend to RAM
>>>>>> regardless of us having had an initial link up or not.
>>>>>
>>>>> So what you're asking is for the PHY to be suspended when the system
>>>>> is entering suspend, which is a long time after the system booted and
>>>>> thus phy_probe() was called, and could be some time before the system
>>>>> resumes.
>>>>>
>>>>> I'm not sure what the relevance is of phy_probe() that was brought up
>>>>> previously then.
>>>>>
>>>>>> This worked prior to 4c0d2e96ba05 ("net: phy: consider that 
>>>>>> suspend2ram
>>>>>> may cut
>>>>>> off PHY power") which was added in Linux 5.11, and 557d5dc83f68 
>>>>>> ("net:
>>>>>> fec: use
>>>>>> mac-managed PHY PM") which was added in Linux 5.12.
>>>>>
>>>>> Looking at the former commit, that looks to me like it is only
>>>>> affecting the resume paths, not the suspend paths, so wouldn't have
>>>>> any impact itself on what happens when suspend happens.
>>>>>
>>>>> The latter commit states that it is a work around for an issue with a
>>>>> particular PHY. What happens if you revert just this commit, does your
>>>>> problem then go away?
>>
>> Our PHY does not begin working again without reverting both. 
>> phy_init_hw()
>> will remain an issue if it occurs after phy_start().
>>
>> The commit message in 557d5dc83f68 is not explaining nearly enough, I
>> spent a
>> few days on it before I proved that commit to be nearly correct (See 
>> whole
>> thread at [1]), it happened to just explode with that PHY. The issue is a
>> sequencing issue that was made more prominent by 4c0d2e96ba05, but it
>> existed
>> since around 2008. Because FEC is both MDIO controller and MAC, 
>> meaning the
>> resume of the link in a link up case runs phy_start() in the FEC resume
>> function, which will trigger a mdio bus resume when it completes, in turn
>> calling phy_init_hw() (before 4c0d2e96ba05 it was phy_resume() which
>> wasn't a
>> problem but still wrong sequence wise).
>>
>>>>>
>>>>> Also, please clarify. It seems that you are reporting a regression -
>>>>> it used to work for you prior to 557d5dc83f68, but 557d5dc83f68 stops
>>>>> it working for you?
>>>>>
>>>>>> Since FEC requires mac_managed_pm the generic PM suspend-resume paths
>>>>>> are not
>>>>>> taken. The resume sequencing with generic PM has been broken with the
>>>>>> FEC since
>>>>>> generic PM of the mdio bus was added, as the FEC will do phy_start()
>>>>>> (via FEC
>>>>>> resume) and then generic PM runs phy_init_hw() via mdio bus resume
>>>>>> (previously:
>>>>>> less damaging phy_resume()) due to how the FEC IP block works.
>>>>>
>>>>> That suggests that even with 557d5dc83f68 reverted, it's broken.
>>>>> Digging into the history, what you're referring to dates from January
>>>>> 2016, so are you reporting a regression that occured 8 _years_ ago,
>>>>> at which point I'd question why it's taken 8 years.
>>
>> A revert of those is absolutely wrong. Those commits are fixing bigger
>> issues.
>>
>>>>>
>>>>> Given the time that has passed, I don't think reverting commits is
>>>>> a sane approach. Quite what the right solution is though, I'm not
>>>>> sure.
>>>>>
>>>>>    From the description and the commits pointed to, I just don't see
>>>>> that there is anything that could've changed with respect to the first
>>>>> boot - if that has changed, then I think more research into what 
>>>>> caused
>>>>> it is needed.
>>>>>
>>>>> If it's the subsequent state after a suspend-resume cycle, then yes,
>>>>> I would agree that its possible that these changes broke this for you.
>>>>> Would clearing ndev->phydev->mac_managed_pm just before
>>>>> phy_disconnect() in fec_enet_close() fix it for you, so the suspend/
>>>>> resume paths for the PHY get used when the network interface is down?
>>>>>
>>>>> Maybe, however, that's something that should happen in any case inside
>>>>> phylib on phy_disconnect() as a matter of course, since the PHY will
>>>>> at that point be no longer under the control of the network driver for
>>>>> PM purposes. Could you give this idea a try please?
>>>>>
>>>>
>>>> On phy_disconnect() we will do a phy_detach() which calls 
>>>> phy_suspend().
>>>> Given that phy_disconnect() is called from fec_enet_close(), we 
>>>> still have a
>>>> MDIO bus registered and we are not trying to suspend the MDIO bus, 
>>>> so we
>>>> should have an effective phy_suspend() call here, what am I missing?
>>>
>>> I didn't look there, but if that is the case, then what is John's
>>> problem - I can't figure it out, something isn't adding up here.
>>>
>>
>> I could instead add extra phy_suspend() in the suspend path if the 
>> link is
>> down and the FEC is up and running. I rejected it originally thinking 
>> it was
>> a much dirtier fix, but maybe that is the more correct thing to do?
> 
> This does not seem like the proper solution, the only time where an 
> explicit phy_suspend() should be done in the Ethernet MAC's ->suspend() 
> routine is if the network device was brought up at the time 
> (netif_runninng() returns true) *and* you set mac_managed_pm = true 
> because you must precisely control the order in which the MAC and the 
> PHY get suspended with respect to each other (typically because the PHY 
> supplies a RX clock back to the MAC, and some of the MAC logic depends 
> upon it to operate properly, e.g.: perform a proper FIFO flush etc.).

I'm having some trouble understanding your message in context of my most 
recent reply to Russell, so please bear with me here as I will 
potentially ask a really dumb question:

Do I understand this correctly as what used to work in 5.10 was never 
meant to work and the behavior now is the correct one in the FEC case? 
Meaning that if the link has never been up the PHY must never be handled 
from a power management perspective?

The only PHY examples I have come across (though not many in total) the 
PHY has done some initial configuration of itself after POR or release 
of the reset line.

> 
>  From there, I see two distinct cases:
> 
> - the network device driver probed, but the network device was never 
> brought up in the first place in that case, I do not see a path whereby 
> the PHY would have been suspended, unless the boot firmware already took 
> care of that (which arguably it should if you are trying to be as power 
> efficient as possible), although arguably there could be a path within 
> the kernel where this is also done. It could get really complicated however

Generic PM via mdio_bus_phy_suspend() will suspend the PHY if it has a 
.suspend callback and mac_managed_pm isn't set.

mdio_bus_phy_may_suspend() will see that netdev is NULL, which means it 
returns the inverse of phy->suspended (which is false), meaning the 
function returns true. Thus phy_suspend() is called.

> 
> - the network device driver probed, and the network device was brought 
> up at least once (regardless of whether a link state was detected or 
> not), such that the PHY has gone through a phy_start()/phy_stop() cycle, 
> and upon phy_stop() a phy_suspend() has been called
> 
> It is safe to assume you fall in the first case only, or do you also see 
> a problem in the second case as well?

There is only a problem in the first case. The second case is working as 
expected.

> 
> If the first case, I am a bit torn as to how to best go about it. The 
> initial state of the PHY upon kernel boot can be a tad difficult to work 
> with:
> 
> - some people want to conserve power as much as possible (like you, like 
> me) and would not mind seeing a link break to achieve that state, nor do 
> they mind a partial or full reconfiguration of the PHY by its driver

For these devices where we see what we consider an issue we only really 
care about power consumption in suspend to RAM.

> 
> - some people want to avoid a link break and just inherit the existing 
> operational mode of the PHY such that they can have a working link as 
> quickly as possible

Thanks! // John Ernberg