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

[John Ernberg]

Hi Florian,

On 3/25/24 10:27 PM, Florian Fainelli wrote:
> On 3/25/24 05:20, John Ernberg wrote:
>> 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?
> 
> I am not sure about that, because I do not believe this had been 
> explicitly accounted for, and that is what I am also trying to figure out.
> 
>> Meaning that if the link has never been up the PHY must never be handled
>> from a power management perspective?
> 
> Let's stray away from saying "link is UP" but instead say interface 
> administratively brought up (ip link set dev <foo> up) so we are on the 
> same page.
> 
>>
>> 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.
> 
> Correct.
> 
>>
>> 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.
> 
> OK, so right there is where you were were basically depending upon 
> mdio_bus_phy_may_suspend() being called to suspend your PHY device! I 
> don't think this had ever been envisioned to be working that way, if it 
> did that was a byproduct rather than a contract. Introducing 
> mac_managed_pm certainly did break that contract because now we no 
> longer have that "double" suspend and resume that used to happen.

I think I understand you know, thanks for clearing that up. Based on 
this, even if the missing suspend in cases where the interface was never 
administrative up is technically a regression it sounds like a much more 
robust fix to our problem would be to do what I considered a work-around 
before (using "ip link set <dev> up && ip link set <dev> down" at boot) 
would likely be the most correct way to go about it.

I will be dropping this patch from the set in the next iteration.

> 
>>
>>>
>>> - 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.
> 
> Thanks for the clarification.

Thanks! // John Ernberg