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

[John Ernberg]

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?

Thanks! // John Ernberg

[1]: 
https://lore.kernel.org/netdev/20240306133734.4144808-1-john.ernberg@xxxxxxxx/