| Message ID | fd51738c-dcd6-4d61-b8c5-faa6ac0f1026@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | net: add and use phy_disable_eee | expand |
> @@ -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? Andrew
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. > Andrew Heiner
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
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
On Tue, 17 Dec 2024 11:43:11 +0100 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? Hi Andrew, do you feel convinced? I think I messed up merging some EEE patches recently, an explicit Ack would boost my confidence..
On Mon, Dec 16, 2024 at 10:31:18PM +0100, Heiner Kallweit wrote: > If a MAC driver doesn't support EEE, then the PHY shouldn't advertise it. > Add phy_disable_eee() for this purpose. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Wed, Dec 18, 2024 at 06:59:09PM -0800, Jakub Kicinski wrote: > On Tue, 17 Dec 2024 11:43:11 +0100 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? > > Hi Andrew, do you feel convinced? I think I messed up merging some EEE > patches recently, an explicit Ack would boost my confidence.. For phylib, yes, we have to live with this unknown state. so these patches are O.K. For phylink, i would like Russells opinion. It would be better if we could avoid having the third state. Maybe we need a couple of cycles where if the MAC says nothing, but the PHY negotiates EEE, we issue a warning? We might then get an idea of how many systems are in this unknown category, and can encourage MAC driver Maintainers to add the missing EEE support. Russell? Andrew
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index b26bb33cd..fe18a12c4 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -2993,6 +2993,22 @@ void phy_support_eee(struct phy_device *phydev) } EXPORT_SYMBOL(phy_support_eee); +/** + * phy_disable_eee - Disable EEE for the PHY + * @phydev: Target phy_device struct + * + * This function is used by MAC drivers for MAC's which don't support EEE. + * It disables EEE on the PHY layer. + */ +void phy_disable_eee(struct phy_device *phydev) +{ + linkmode_zero(phydev->supported_eee); + linkmode_zero(phydev->advertising_eee); + phydev->eee_cfg.tx_lpi_enabled = false; + phydev->eee_cfg.eee_enabled = false; +} +EXPORT_SYMBOL_GPL(phy_disable_eee); + /** * phy_support_sym_pause - Enable support of symmetrical pause * @phydev: target phy_device struct diff --git a/include/linux/phy.h b/include/linux/phy.h index e597a32cc..5bc71d599 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -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); void phy_set_sym_pause(struct phy_device *phydev, bool rx, bool tx, bool autoneg); void phy_set_asym_pause(struct phy_device *phydev, bool rx, bool tx);
If a MAC driver doesn't support EEE, then the PHY shouldn't advertise it. Add phy_disable_eee() for this purpose. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/phy/phy_device.c | 16 ++++++++++++++++ include/linux/phy.h | 1 + 2 files changed, 17 insertions(+)