diff mbox series

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

Message ID fd51738c-dcd6-4d61-b8c5-faa6ac0f1026@gmail.com (mailing list archive)
State New
Delegated to: Netdev Maintainers
Headers show
Series net: add and use phy_disable_eee | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1 this patch: 1
netdev/build_tools success Errors and warnings before: 0 (+23) this patch: 0 (+23)
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 3777 this patch: 3777
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 366 this patch: 366
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 29 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 69 this patch: 69
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-12-17--03-00 (tests: 795)

Commit Message

Heiner Kallweit Dec. 16, 2024, 9:31 p.m. UTC
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(+)

Comments

Andrew Lunn Dec. 17, 2024, 10:43 a.m. UTC | #1
> @@ -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
Heiner Kallweit Dec. 17, 2024, 8:50 p.m. UTC | #2
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
Andrew Lunn Dec. 17, 2024, 10:34 p.m. UTC | #3
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
Heiner Kallweit Dec. 18, 2024, 7:01 a.m. UTC | #4
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
diff mbox series

Patch

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);