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

[Heiner Kallweit <hkallweit1@gmail.com>]

On 17.12.2024 23:34, Andrew Lunn wrote:
> 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?
> 
Looking at patch 5 of Russell's series behavior doesn't change if
pl->mac_supports_eee is false. So I think he also goes with the three
states, at least initially, until all drivers using phylink have
implemented the new phylink ops. AFAICS this affects about 25 drivers.

> 	Andrew
Heiner