Re: [PATCH net-next 1/3] net: phy: add phy_disable_eee

[Andrew Lunn <andrew@lunn.ch>]

On Tue, Dec 17, 2024 at 09:50:12PM +0100, Heiner Kallweit wrote:
> On 17.12.2024 11:43, Andrew Lunn wrote:
> >> @@ -2071,6 +2071,7 @@ void phy_advertise_eee_all(struct phy_device *phydev);
> >>  void phy_support_sym_pause(struct phy_device *phydev);
> >>  void phy_support_asym_pause(struct phy_device *phydev);
> >>  void phy_support_eee(struct phy_device *phydev);
> >> +void phy_disable_eee(struct phy_device *phydev);
> > 
> > So we have three states:
> > 
> > MAC tells PHYLIB it does support EEE
> > MAC tells PHYLIB it does not support EEE
> > MAC says nothing.
> > 
> > Do we really want this?
> > 
> > For phylink, i think we have a nice new clean design and can say, if
> > the MAC does not indicate it supports EEE, we turn it off in the
> > PHY. For phylib, we have more of a mess, and there could be MACs
> > actually doing EEE by default using default setting but with no user
> > space control. Do we want to keep this, or should we say any MAC which
> > does not call phy_support_eee() before phy_start() would have EEE
> > disabled in the PHY?
> > 
> The case "MAC says nothing" isn't desirable. However, if we did what
> you mention, we'd silently change the behavior of several drivers,
> resulting in disabled EEE and higher power consumption.
> I briefly grepped the kernel source for phy_start() and found about
> 70 drivers. Some of them have the phylib EEE call, and in some cases
> like cpsw the MAC doesn't support EEE. But what remains is IMO too
> many drivers where we'd change the behavior.

So for phylib, we keep with the three states. But phylink? Can we
disable EEE when the MAC says nothing?

	Andrew