Date: Wed, 20 Mar 2024 19:44:05 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Florian Fainelli <f.fainelli@...il.com>
Cc: John Ernberg <john.ernberg@...ia.se>,
	Maxime Chevallier <maxime.chevallier@...tlin.com>,
	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>
Subject: Re: [PATCH net v3 2/2] net: fec: Suspend the PHY on probe

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?
> > 
> > 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.
> > 
> > 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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Powered by blists - more mailing lists