[PATCH net-next 0/9] net: phy: improve phylib EEE handling

[PATCH net-next 0/9] net: phy: improve phylib EEE handling

[Heiner Kallweit]

This series includes improvements for the EEE handling in phylib.

Heiner Kallweit (9):
  net: phy: rename eee_broken_modes to eee_disabled_modes
  net: phy: rename phy_set_eee_broken to phy_disable_eee_mode
  net: phy: c45: don't accept disabled EEE modes in
    genphy_c45_ethtool_set_eee
  net: phy: move definition of phy_is_started before
    phy_disable_eee_mode
  net: phy: improve phy_disable_eee_mode
  net: phy: remove disabled EEE modes from advertising in phy_probe
  net: phy: c45: Don't silently remove disabled EEE modes any longer
    when writing advertisement register
  net: phy: c45: use cached EEE advertisement in
    genphy_c45_ethtool_get_eee
  net: phy: c45: remove local advertisement parameter from
    genphy_c45_eee_is_active

 drivers/net/ethernet/realtek/r8169_main.c |  6 +--
 drivers/net/phy/phy-c45.c                 | 45 +++++++++--------------
 drivers/net/phy/phy-core.c                |  2 +-
 drivers/net/phy/phy.c                     |  4 +-
 drivers/net/phy/phy_device.c              | 23 ++++++------
 include/linux/phy.h                       | 24 ++++++------
 6 files changed, 47 insertions(+), 57 deletions(-)

-- 
2.47.1

[PATCH net-next 1/9] net: phy: rename eee_broken_modes to eee_disabled_modes

[Heiner Kallweit]

This bitmap is used also if the MAC doesn't support an EEE mode.
So the mode isn't necessarily broken in the PHY. Therefore rename
the bitmap.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy-c45.c    | 2 +-
 drivers/net/phy/phy-core.c   | 2 +-
 drivers/net/phy/phy_device.c | 2 +-
 include/linux/phy.h          | 6 +++---
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 0dac08e85..468d24611 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -686,7 +686,7 @@ static int genphy_c45_write_eee_adv(struct phy_device *phydev,
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(tmp);
 	int val, changed = 0;
 
-	linkmode_andnot(tmp, adv, phydev->eee_broken_modes);
+	linkmode_andnot(tmp, adv, phydev->eee_disabled_modes);
 
 	if (linkmode_intersects(phydev->supported_eee, PHY_EEE_CAP1_FEATURES)) {
 		val = linkmode_to_mii_eee_cap1_t(tmp);
diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 6bf3ec985..beeb0ef2f 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -388,7 +388,7 @@ void of_set_phy_supported(struct phy_device *phydev)
 void of_set_phy_eee_broken(struct phy_device *phydev)
 {
 	struct device_node *node = phydev->mdio.dev.of_node;
-	unsigned long *modes = phydev->eee_broken_modes;
+	unsigned long *modes = phydev->eee_disabled_modes;
 
 	if (!IS_ENABLED(CONFIG_OF_MDIO) || !node)
 		return;
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index bdc997f59..f6a5f986f 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3016,7 +3016,7 @@ void phy_disable_eee(struct phy_device *phydev)
 	phydev->eee_cfg.tx_lpi_enabled = false;
 	phydev->eee_cfg.eee_enabled = false;
 	/* don't let userspace re-enable EEE advertisement */
-	linkmode_fill(phydev->eee_broken_modes);
+	linkmode_fill(phydev->eee_disabled_modes);
 }
 EXPORT_SYMBOL_GPL(phy_disable_eee);
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 5bc71d599..c5dc2dbf0 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -629,7 +629,7 @@ struct macsec_ops;
  * @eee_cfg: User configuration of EEE
  * @lp_advertising: Current link partner advertised linkmodes
  * @host_interfaces: PHY interface modes supported by host
- * @eee_broken_modes: Energy efficient ethernet modes which should be prohibited
+ * @eee_disabled_modes: Energy efficient ethernet modes not to be advertised
  * @autoneg: Flag autoneg being used
  * @rate_matching: Current rate matching mode
  * @link: Current link state
@@ -745,7 +745,7 @@ struct phy_device {
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported_eee);
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising_eee);
 	/* Energy efficient ethernet modes which should be prohibited */
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(eee_broken_modes);
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(eee_disabled_modes);
 	bool enable_tx_lpi;
 	bool eee_active;
 	struct eee_config eee_cfg;
@@ -1324,7 +1324,7 @@ int phy_speed_down_core(struct phy_device *phydev);
  */
 static inline void phy_set_eee_broken(struct phy_device *phydev, u32 link_mode)
 {
-	linkmode_set_bit(link_mode, phydev->eee_broken_modes);
+	linkmode_set_bit(link_mode, phydev->eee_disabled_modes);
 }
 
 /**
-- 
2.47.1

[PATCH net-next 2/9] net: phy: rename phy_set_eee_broken to phy_disable_eee_mode

[Heiner Kallweit]

Consider that an EEE mode may not be broken but simply not supported
by the MAC, and rename function phy_set_eee_broken().

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 6 +++---
 include/linux/phy.h                       | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 5724f650f..bf368b32c 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -5222,9 +5222,9 @@ static int r8169_mdio_register(struct rtl8169_private *tp)
 
 	/* mimic behavior of r8125/r8126 vendor drivers */
 	if (tp->mac_version == RTL_GIGA_MAC_VER_61)
-		phy_set_eee_broken(tp->phydev,
-				   ETHTOOL_LINK_MODE_2500baseT_Full_BIT);
-	phy_set_eee_broken(tp->phydev, ETHTOOL_LINK_MODE_5000baseT_Full_BIT);
+		phy_disable_eee_mode(tp->phydev,
+				     ETHTOOL_LINK_MODE_2500baseT_Full_BIT);
+	phy_disable_eee_mode(tp->phydev, ETHTOOL_LINK_MODE_5000baseT_Full_BIT);
 
 	/* PHY will be woken up in rtl_open() */
 	phy_suspend(tp->phydev);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index c5dc2dbf0..7138bb074 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1318,11 +1318,11 @@ void of_set_phy_timing_role(struct phy_device *phydev);
 int phy_speed_down_core(struct phy_device *phydev);
 
 /**
- * phy_set_eee_broken - Mark an EEE mode as broken so that it isn't advertised.
+ * phy_disable_eee_mode - Don't advertise an EEE mode.
  * @phydev: The phy_device struct
- * @link_mode: The broken EEE mode
+ * @link_mode: The EEE mode to be disabled
  */
-static inline void phy_set_eee_broken(struct phy_device *phydev, u32 link_mode)
+static inline void phy_disable_eee_mode(struct phy_device *phydev, u32 link_mode)
 {
 	linkmode_set_bit(link_mode, phydev->eee_disabled_modes);
 }
-- 
2.47.1

[PATCH net-next 3/9] net: phy: c45: don't accept disabled EEE modes in genphy_c45_ethtool_set_eee

[Heiner Kallweit]

Link modes in phydev->eee_disabled_modes are filtered out by
genphy_c45_write_eee_adv() and won't be advertised. Therefore
don't accept such modes from userspace.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy-c45.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 468d24611..b566faba9 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -1559,6 +1559,11 @@ int genphy_c45_ethtool_set_eee(struct phy_device *phydev,
 				phydev_warn(phydev, "At least some EEE link modes are not supported.\n");
 				return -EINVAL;
 			}
+			linkmode_and(tmp, adv, phydev->eee_disabled_modes);
+			if (!linkmode_empty(tmp)) {
+				phydev_warn(phydev, "At least some EEE link modes are disabled.\n");
+				return -EINVAL;
+			}
 			linkmode_copy(phydev->advertising_eee, adv);
 		} else if (linkmode_empty(phydev->advertising_eee)) {
 			phy_advertise_eee_all(phydev);
-- 
2.47.1

Re: [PATCH net-next 3/9] net: phy: c45: don't accept disabled EEE modes in genphy_c45_ethtool_set_eee

[Russell King (Oracle)]

On Sat, Jan 11, 2025 at 10:06:02AM +0100, Heiner Kallweit wrote:
> Link modes in phydev->eee_disabled_modes are filtered out by
> genphy_c45_write_eee_adv() and won't be advertised. Therefore
> don't accept such modes from userspace.

Why do we need this? Surely if the MAC doesn't support modes, then they
should be filtered out of phydev->supported_eee so that userspace knows
that the mode is not supported by the network interface as a whole, just
like we do for phydev->supported.

That would give us the checking here.

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

Re: [PATCH net-next 3/9] net: phy: c45: don't accept disabled EEE modes in genphy_c45_ethtool_set_eee

[Heiner Kallweit]

On 11.01.2025 10:21, Russell King (Oracle) wrote:
> On Sat, Jan 11, 2025 at 10:06:02AM +0100, Heiner Kallweit wrote:
>> Link modes in phydev->eee_disabled_modes are filtered out by
>> genphy_c45_write_eee_adv() and won't be advertised. Therefore
>> don't accept such modes from userspace.
> 
> Why do we need this? Surely if the MAC doesn't support modes, then they
> should be filtered out of phydev->supported_eee so that userspace knows
> that the mode is not supported by the network interface as a whole, just
> like we do for phydev->supported.
> 
> That would give us the checking here.
> 
Removing EEE modes to be disabled from supported_eee is problematic
because of how genphy_c45_write_eee_adv() works.

Let's say we have a 2.5Gbps PHY and want to disable EEE at 2.5Gbps. If we
remove 2.5Gbps from supported_eee, then the following check is false:
if (linkmode_intersects(phydev->supported_eee, PHY_EEE_CAP2_FEATURES))
What would result in the 2.5Gbps mode not getting disabled.

Re: [PATCH net-next 3/9] net: phy: c45: don't accept disabled EEE modes in genphy_c45_ethtool_set_eee

[Russell King (Oracle)]

On Sat, Jan 11, 2025 at 10:44:25AM +0100, Heiner Kallweit wrote:
> On 11.01.2025 10:21, Russell King (Oracle) wrote:
> > On Sat, Jan 11, 2025 at 10:06:02AM +0100, Heiner Kallweit wrote:
> >> Link modes in phydev->eee_disabled_modes are filtered out by
> >> genphy_c45_write_eee_adv() and won't be advertised. Therefore
> >> don't accept such modes from userspace.
> > 
> > Why do we need this? Surely if the MAC doesn't support modes, then they
> > should be filtered out of phydev->supported_eee so that userspace knows
> > that the mode is not supported by the network interface as a whole, just
> > like we do for phydev->supported.
> > 
> > That would give us the checking here.
> > 
> Removing EEE modes to be disabled from supported_eee is problematic
> because of how genphy_c45_write_eee_adv() works.
> 
> Let's say we have a 2.5Gbps PHY and want to disable EEE at 2.5Gbps. If we
> remove 2.5Gbps from supported_eee, then the following check is false:
> if (linkmode_intersects(phydev->supported_eee, PHY_EEE_CAP2_FEATURES))
> What would result in the 2.5Gbps mode not getting disabled.

Ok. Do we at least remove the broken modes from the supported mask
reported to userspace?

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

Re: [PATCH net-next 3/9] net: phy: c45: don't accept disabled EEE modes in genphy_c45_ethtool_set_eee

[Heiner Kallweit]

On 11.01.2025 11:01, Russell King (Oracle) wrote:
> On Sat, Jan 11, 2025 at 10:44:25AM +0100, Heiner Kallweit wrote:
>> On 11.01.2025 10:21, Russell King (Oracle) wrote:
>>> On Sat, Jan 11, 2025 at 10:06:02AM +0100, Heiner Kallweit wrote:
>>>> Link modes in phydev->eee_disabled_modes are filtered out by
>>>> genphy_c45_write_eee_adv() and won't be advertised. Therefore
>>>> don't accept such modes from userspace.
>>>
>>> Why do we need this? Surely if the MAC doesn't support modes, then they
>>> should be filtered out of phydev->supported_eee so that userspace knows
>>> that the mode is not supported by the network interface as a whole, just
>>> like we do for phydev->supported.
>>>
>>> That would give us the checking here.
>>>
>> Removing EEE modes to be disabled from supported_eee is problematic
>> because of how genphy_c45_write_eee_adv() works.
>>
>> Let's say we have a 2.5Gbps PHY and want to disable EEE at 2.5Gbps. If we
>> remove 2.5Gbps from supported_eee, then the following check is false:
>> if (linkmode_intersects(phydev->supported_eee, PHY_EEE_CAP2_FEATURES))
>> What would result in the 2.5Gbps mode not getting disabled.
> 
> Ok. Do we at least remove the broken modes from the supported mask
> reported to userspace?
> 
I think that's something we could do in addition, to provide a hint to the
user about unavailable modes. It wouldn't remove the need for the check here.
ethtool doesn't check the advertisement against the supported modes.
And even if it would, we must not rely on input from user space being sane.

Re: [PATCH net-next 3/9] net: phy: c45: don't accept disabled EEE modes in genphy_c45_ethtool_set_eee

[Russell King (Oracle)]

On Sat, Jan 11, 2025 at 02:19:04PM +0100, Heiner Kallweit wrote:
> On 11.01.2025 11:01, Russell King (Oracle) wrote:
> > On Sat, Jan 11, 2025 at 10:44:25AM +0100, Heiner Kallweit wrote:
> >> On 11.01.2025 10:21, Russell King (Oracle) wrote:
> >>> On Sat, Jan 11, 2025 at 10:06:02AM +0100, Heiner Kallweit wrote:
> >>>> Link modes in phydev->eee_disabled_modes are filtered out by
> >>>> genphy_c45_write_eee_adv() and won't be advertised. Therefore
> >>>> don't accept such modes from userspace.
> >>>
> >>> Why do we need this? Surely if the MAC doesn't support modes, then they
> >>> should be filtered out of phydev->supported_eee so that userspace knows
> >>> that the mode is not supported by the network interface as a whole, just
> >>> like we do for phydev->supported.
> >>>
> >>> That would give us the checking here.
> >>>
> >> Removing EEE modes to be disabled from supported_eee is problematic
> >> because of how genphy_c45_write_eee_adv() works.
> >>
> >> Let's say we have a 2.5Gbps PHY and want to disable EEE at 2.5Gbps. If we
> >> remove 2.5Gbps from supported_eee, then the following check is false:
> >> if (linkmode_intersects(phydev->supported_eee, PHY_EEE_CAP2_FEATURES))
> >> What would result in the 2.5Gbps mode not getting disabled.
> > 
> > Ok. Do we at least remove the broken modes from the supported mask
> > reported to userspace?
> > 
> I think that's something we could do in addition, to provide a hint to the
> user about unavailable modes. It wouldn't remove the need for the check here.
> ethtool doesn't check the advertisement against the supported modes.
> And even if it would, we must not rely on input from user space being sane.

I disagree with some of this. Userspace should expect:

- read current settings
- copy supported modes to advertised modes
- write current settings

to work. If it fails, then how does ethtool, or even the user, work out
which link modes are actually supported or not.

If we're introducing a failure on the "disabled" modes, then that is
a user-breaking change, and we need to avoid that. The current code
silently ignored the broken modes, your new code would error out on
the above action - and that's a bug.

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

Re: [PATCH net-next 3/9] net: phy: c45: don't accept disabled EEE modes in genphy_c45_ethtool_set_eee

[Heiner Kallweit]

On 11.01.2025 16:13, Russell King (Oracle) wrote:
> On Sat, Jan 11, 2025 at 02:19:04PM +0100, Heiner Kallweit wrote:
>> On 11.01.2025 11:01, Russell King (Oracle) wrote:
>>> On Sat, Jan 11, 2025 at 10:44:25AM +0100, Heiner Kallweit wrote:
>>>> On 11.01.2025 10:21, Russell King (Oracle) wrote:
>>>>> On Sat, Jan 11, 2025 at 10:06:02AM +0100, Heiner Kallweit wrote:
>>>>>> Link modes in phydev->eee_disabled_modes are filtered out by
>>>>>> genphy_c45_write_eee_adv() and won't be advertised. Therefore
>>>>>> don't accept such modes from userspace.
>>>>>
>>>>> Why do we need this? Surely if the MAC doesn't support modes, then they
>>>>> should be filtered out of phydev->supported_eee so that userspace knows
>>>>> that the mode is not supported by the network interface as a whole, just
>>>>> like we do for phydev->supported.
>>>>>
>>>>> That would give us the checking here.
>>>>>
>>>> Removing EEE modes to be disabled from supported_eee is problematic
>>>> because of how genphy_c45_write_eee_adv() works.
>>>>
>>>> Let's say we have a 2.5Gbps PHY and want to disable EEE at 2.5Gbps. If we
>>>> remove 2.5Gbps from supported_eee, then the following check is false:
>>>> if (linkmode_intersects(phydev->supported_eee, PHY_EEE_CAP2_FEATURES))
>>>> What would result in the 2.5Gbps mode not getting disabled.
>>>
>>> Ok. Do we at least remove the broken modes from the supported mask
>>> reported to userspace?
>>>
>> I think that's something we could do in addition, to provide a hint to the
>> user about unavailable modes. It wouldn't remove the need for the check here.
>> ethtool doesn't check the advertisement against the supported modes.
>> And even if it would, we must not rely on input from user space being sane.
> 
> I disagree with some of this. Userspace should expect:
> 
> - read current settings
> - copy supported modes to advertised modes
> - write current settings
> 
> to work. If it fails, then how does ethtool, or even the user, work out
> which link modes are actually supported or not.
> 
> If we're introducing a failure on the "disabled" modes, then that is
> a user-breaking change, and we need to avoid that. The current code
> silently ignored the broken modes, your new code would error out on
> the above action - and that's a bug.
> 
OK, then I think what we can/should do:
- filter out disabled EEE modes when populating data->supported in
  genphy_c45_ethtool_get_eee
- silently filter out disabled EEE modes from user space provided
  EEE advertisement in genphy_c45_ethtool_set_eee

Re: [PATCH net-next 3/9] net: phy: c45: don't accept disabled EEE modes in genphy_c45_ethtool_set_eee

[Andrew Lunn]

> > I disagree with some of this. Userspace should expect:
> > 
> > - read current settings
> > - copy supported modes to advertised modes
> > - write current settings
> > 
> > to work. If it fails, then how does ethtool, or even the user, work out
> > which link modes are actually supported or not.
> > 
> > If we're introducing a failure on the "disabled" modes, then that is
> > a user-breaking change, and we need to avoid that. The current code
> > silently ignored the broken modes, your new code would error out on
> > the above action - and that's a bug.
> > 
> OK, then I think what we can/should do:
> - filter out disabled EEE modes when populating data->supported in
>   genphy_c45_ethtool_get_eee
> - silently filter out disabled EEE modes from user space provided
>   EEE advertisement in genphy_c45_ethtool_set_eee

Ideally, the kAPI should work just the same as normal advertised
modes. The read API returns what can actually be used, and write API
expects a subset of that.

But maybe we have too much history and cannot enforce the subset
without regressions, we just silently ignore the extra modes?

It might be too much plumbing, but it would be nice to include an
extack saying some modes have been ignored? I _think_ extack can be
used without an error.

	Andrew

Re: [PATCH net-next 3/9] net: phy: c45: don't accept disabled EEE modes in genphy_c45_ethtool_set_eee

[Heiner Kallweit]

On 11.01.2025 18:31, Andrew Lunn wrote:
>>> I disagree with some of this. Userspace should expect:
>>>
>>> - read current settings
>>> - copy supported modes to advertised modes
>>> - write current settings
>>>
>>> to work. If it fails, then how does ethtool, or even the user, work out
>>> which link modes are actually supported or not.
>>>
>>> If we're introducing a failure on the "disabled" modes, then that is
>>> a user-breaking change, and we need to avoid that. The current code
>>> silently ignored the broken modes, your new code would error out on
>>> the above action - and that's a bug.
>>>
>> OK, then I think what we can/should do:
>> - filter out disabled EEE modes when populating data->supported in
>>   genphy_c45_ethtool_get_eee
>> - silently filter out disabled EEE modes from user space provided
>>   EEE advertisement in genphy_c45_ethtool_set_eee
> 
> Ideally, the kAPI should work just the same as normal advertised
> modes. The read API returns what can actually be used, and write API
> expects a subset of that.
> 
> But maybe we have too much history and cannot enforce the subset
> without regressions, we just silently ignore the extra modes?
> 
> It might be too much plumbing, but it would be nice to include an
> extack saying some modes have been ignored? I _think_ extack can be
> used without an error.
> 
Nice idea, this could be done as a follow-up. I'll have a look into this.

> 	Andrew
> 
Heiner
> 
> 
> 

[PATCH net-next 4/9] net: phy: move definition of phy_is_started before phy_disable_eee_mode

[Heiner Kallweit]

In preparation of a follow-up patch, move phy_is_started() to before
phy_disable_eee_mode().

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 include/linux/phy.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 7138bb074..ad71d3a3b 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1318,22 +1318,22 @@ void of_set_phy_timing_role(struct phy_device *phydev);
 int phy_speed_down_core(struct phy_device *phydev);
 
 /**
- * phy_disable_eee_mode - Don't advertise an EEE mode.
+ * phy_is_started - Convenience function to check whether PHY is started
  * @phydev: The phy_device struct
- * @link_mode: The EEE mode to be disabled
  */
-static inline void phy_disable_eee_mode(struct phy_device *phydev, u32 link_mode)
+static inline bool phy_is_started(struct phy_device *phydev)
 {
-	linkmode_set_bit(link_mode, phydev->eee_disabled_modes);
+	return phydev->state >= PHY_UP;
 }
 
 /**
- * phy_is_started - Convenience function to check whether PHY is started
+ * phy_disable_eee_mode - Don't advertise an EEE mode.
  * @phydev: The phy_device struct
+ * @link_mode: The EEE mode to be disabled
  */
-static inline bool phy_is_started(struct phy_device *phydev)
+static inline void phy_disable_eee_mode(struct phy_device *phydev, u32 link_mode)
 {
-	return phydev->state >= PHY_UP;
+	linkmode_set_bit(link_mode, phydev->eee_disabled_modes);
 }
 
 void phy_resolve_aneg_pause(struct phy_device *phydev);
-- 
2.47.1

[PATCH net-next 5/9] net: phy: improve phy_disable_eee_mode

[Heiner Kallweit]

If a mode is to be disabled, remove it from advertising_eee.
Disabling EEE modes shall be done before calling phy_start(),
warn if that's not the case.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 include/linux/phy.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index ad71d3a3b..fce29aaa9 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1333,7 +1333,10 @@ static inline bool phy_is_started(struct phy_device *phydev)
  */
 static inline void phy_disable_eee_mode(struct phy_device *phydev, u32 link_mode)
 {
+	WARN_ON(phy_is_started(phydev));
+
 	linkmode_set_bit(link_mode, phydev->eee_disabled_modes);
+	linkmode_clear_bit(link_mode, phydev->advertising_eee);
 }
 
 void phy_resolve_aneg_pause(struct phy_device *phydev);
-- 
2.47.1

Re: [PATCH net-next 5/9] net: phy: improve phy_disable_eee_mode

[Andrew Lunn]

On Sat, Jan 11, 2025 at 10:07:43AM +0100, Heiner Kallweit wrote:
> If a mode is to be disabled, remove it from advertising_eee.
> Disabling EEE modes shall be done before calling phy_start(),
> warn if that's not the case.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  include/linux/phy.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index ad71d3a3b..fce29aaa9 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -1333,7 +1333,10 @@ static inline bool phy_is_started(struct phy_device *phydev)
>   */
>  static inline void phy_disable_eee_mode(struct phy_device *phydev, u32 link_mode)
>  {
> +	WARN_ON(phy_is_started(phydev));
> +
>  	linkmode_set_bit(link_mode, phydev->eee_disabled_modes);
> +	linkmode_clear_bit(link_mode, phydev->advertising_eee);
>  }

Thinking out loud...

Maybe we should make this more like phy_remove_link_mode()? That
function also removes the link mode from phydev->supported at the same
time.

	Andrew

Re: [PATCH net-next 5/9] net: phy: improve phy_disable_eee_mode

[Russell King (Oracle)]

On Sat, Jan 11, 2025 at 06:38:07PM +0100, Andrew Lunn wrote:
> On Sat, Jan 11, 2025 at 10:07:43AM +0100, Heiner Kallweit wrote:
> > If a mode is to be disabled, remove it from advertising_eee.
> > Disabling EEE modes shall be done before calling phy_start(),
> > warn if that's not the case.
> > 
> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> > ---
> >  include/linux/phy.h | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/include/linux/phy.h b/include/linux/phy.h
> > index ad71d3a3b..fce29aaa9 100644
> > --- a/include/linux/phy.h
> > +++ b/include/linux/phy.h
> > @@ -1333,7 +1333,10 @@ static inline bool phy_is_started(struct phy_device *phydev)
> >   */
> >  static inline void phy_disable_eee_mode(struct phy_device *phydev, u32 link_mode)
> >  {
> > +	WARN_ON(phy_is_started(phydev));
> > +
> >  	linkmode_set_bit(link_mode, phydev->eee_disabled_modes);
> > +	linkmode_clear_bit(link_mode, phydev->advertising_eee);
> >  }
> 
> Thinking out loud...
> 
> Maybe we should make this more like phy_remove_link_mode()? That
> function also removes the link mode from phydev->supported at the same
> time.

That's what I would expect, but phydev->supported_eee is also used to
determine which registers are programmed. So, removing linkmodes from
it risks not programming a register to disable an EEE mode.

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

[PATCH net-next 6/9] net: phy: remove disabled EEE modes from advertising in phy_probe

[Heiner Kallweit]

A PHY driver may populate eee_disabled_modes in its probe or get_features
callback, therefore filter the EEE advertisement read from the PHY.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy_device.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index f6a5f986f..ff4b4d42b 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3613,22 +3613,21 @@ static int phy_probe(struct device *dev)
 	if (err)
 		goto out;
 
-	/* There is no "enabled" flag. If PHY is advertising, assume it is
-	 * kind of enabled.
-	 */
-	phydev->eee_cfg.eee_enabled = !linkmode_empty(phydev->advertising_eee);
+	/* Get the EEE modes we want to prohibit. */
+	of_set_phy_eee_broken(phydev);
 
 	/* Some PHYs may advertise, by default, not support EEE modes. So,
-	 * we need to clean them.
+	 * we need to clean them. In addition remove all disabled EEE modes.
 	 */
-	if (phydev->eee_cfg.eee_enabled)
-		linkmode_and(phydev->advertising_eee, phydev->supported_eee,
-			     phydev->advertising_eee);
+	linkmode_and(phydev->advertising_eee, phydev->supported_eee,
+		     phydev->advertising_eee);
+	linkmode_andnot(phydev->advertising_eee, phydev->advertising_eee,
+			phydev->eee_disabled_modes);
 
-	/* Get the EEE modes we want to prohibit. We will ask
-	 * the PHY stop advertising these mode later on
+	/* There is no "enabled" flag. If PHY is advertising, assume it is
+	 * kind of enabled.
 	 */
-	of_set_phy_eee_broken(phydev);
+	phydev->eee_cfg.eee_enabled = !linkmode_empty(phydev->advertising_eee);
 
 	/* Get master/slave strap overrides */
 	of_set_phy_timing_role(phydev);
-- 
2.47.1

[PATCH net-next 7/9] net: phy: c45: Don't silently remove disabled EEE modes any longer when writing advertisement register

[Heiner Kallweit]

advertising_eee is adjusted now whenever an EEE mode gets disabled.
Therefore we can remove the silent removal of disabled EEE modes here.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy-c45.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index b566faba9..ef58d3b23 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -683,13 +683,10 @@ EXPORT_SYMBOL_GPL(genphy_c45_read_mdix);
 static int genphy_c45_write_eee_adv(struct phy_device *phydev,
 				    unsigned long *adv)
 {
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(tmp);
 	int val, changed = 0;
 
-	linkmode_andnot(tmp, adv, phydev->eee_disabled_modes);
-
 	if (linkmode_intersects(phydev->supported_eee, PHY_EEE_CAP1_FEATURES)) {
-		val = linkmode_to_mii_eee_cap1_t(tmp);
+		val = linkmode_to_mii_eee_cap1_t(adv);
 
 		/* IEEE 802.3-2018 45.2.7.13 EEE advertisement 1
 		 * (Register 7.60)
@@ -707,7 +704,7 @@ static int genphy_c45_write_eee_adv(struct phy_device *phydev,
 	}
 
 	if (linkmode_intersects(phydev->supported_eee, PHY_EEE_CAP2_FEATURES)) {
-		val = linkmode_to_mii_eee_cap2_t(tmp);
+		val = linkmode_to_mii_eee_cap2_t(adv);
 
 		/* IEEE 802.3-2022 45.2.7.16 EEE advertisement 2
 		 * (Register 7.62)
-- 
2.47.1

[PATCH net-next 8/9] net: phy: c45: use cached EEE advertisement in genphy_c45_ethtool_get_eee

[Heiner Kallweit]

Now that disabled EEE modes are considered when populating
advertising_eee, we can use this bitmap here instead of reading
the PHY register.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy-c45.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index ef58d3b23..6d05aef5f 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -1516,13 +1516,13 @@ int genphy_c45_ethtool_get_eee(struct phy_device *phydev,
 {
 	int ret;
 
-	ret = genphy_c45_eee_is_active(phydev, data->advertised,
-				       data->lp_advertised);
+	ret = genphy_c45_eee_is_active(phydev, NULL, data->lp_advertised);
 	if (ret < 0)
 		return ret;
 
 	data->eee_active = phydev->eee_active;
 	linkmode_copy(data->supported, phydev->supported_eee);
+	linkmode_copy(data->advertised, phydev->advertising_eee);
 
 	return 0;
 }
-- 
2.47.1

[PATCH net-next 9/9] net: phy: c45: remove local advertisement parameter from genphy_c45_eee_is_active

[Heiner Kallweit]

After the last user has gone, we can remove the local advertisement
parameter from genphy_c45_eee_is_active.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy-c45.c | 31 +++++++++----------------------
 drivers/net/phy/phy.c     |  4 ++--
 include/linux/phy.h       |  3 +--
 3 files changed, 12 insertions(+), 26 deletions(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 6d05aef5f..7dd113df0 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -1464,42 +1464,29 @@ EXPORT_SYMBOL_GPL(genphy_c45_plca_get_status);
 /**
  * genphy_c45_eee_is_active - get EEE status
  * @phydev: target phy_device struct
- * @adv: variable to store advertised linkmodes
  * @lp: variable to store LP advertised linkmodes
  *
- * Description: this function will read local and link partner PHY
- * advertisements. Compare them return current EEE state.
+ * Description: this function will read link partner PHY advertisement
+ * and compare it to local advertisement to return current EEE state.
  */
-int genphy_c45_eee_is_active(struct phy_device *phydev, unsigned long *adv,
-			     unsigned long *lp)
+int genphy_c45_eee_is_active(struct phy_device *phydev, unsigned long *lp)
 {
-	__ETHTOOL_DECLARE_LINK_MODE_MASK(tmp_adv) = {};
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(tmp_lp) = {};
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(common);
-	bool eee_active;
 	int ret;
 
-	ret = genphy_c45_read_eee_adv(phydev, tmp_adv);
-	if (ret)
-		return ret;
-
 	ret = genphy_c45_read_eee_lpa(phydev, tmp_lp);
 	if (ret)
 		return ret;
 
-	linkmode_and(common, tmp_adv, tmp_lp);
-	if (!linkmode_empty(tmp_adv) && !linkmode_empty(common))
-		eee_active = phy_check_valid(phydev->speed, phydev->duplex,
-					     common);
-	else
-		eee_active = false;
-
-	if (adv)
-		linkmode_copy(adv, tmp_adv);
 	if (lp)
 		linkmode_copy(lp, tmp_lp);
 
-	return eee_active;
+	linkmode_and(common, phydev->advertising_eee, tmp_lp);
+	if (linkmode_empty(common))
+		return 0;
+
+	return phy_check_valid(phydev->speed, phydev->duplex, common);
 }
 EXPORT_SYMBOL(genphy_c45_eee_is_active);
 
@@ -1516,7 +1503,7 @@ int genphy_c45_ethtool_get_eee(struct phy_device *phydev,
 {
 	int ret;
 
-	ret = genphy_c45_eee_is_active(phydev, NULL, data->lp_advertised);
+	ret = genphy_c45_eee_is_active(phydev, data->lp_advertised);
 	if (ret < 0)
 		return ret;
 
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index e4b04cdaa..5812a3f12 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -988,7 +988,7 @@ static int phy_check_link_status(struct phy_device *phydev)
 	if (phydev->link && phydev->state != PHY_RUNNING) {
 		phy_check_downshift(phydev);
 		phydev->state = PHY_RUNNING;
-		err = genphy_c45_eee_is_active(phydev, NULL, NULL);
+		err = genphy_c45_eee_is_active(phydev, NULL);
 		phydev->eee_active = err > 0;
 		phydev->enable_tx_lpi = phydev->eee_cfg.tx_lpi_enabled &&
 					phydev->eee_active;
@@ -1657,7 +1657,7 @@ int phy_init_eee(struct phy_device *phydev, bool clk_stop_enable)
 	if (!phydev->drv)
 		return -EIO;
 
-	ret = genphy_c45_eee_is_active(phydev, NULL, NULL);
+	ret = genphy_c45_eee_is_active(phydev, NULL);
 	if (ret < 0)
 		return ret;
 	if (!ret)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index fce29aaa9..5a6dcbd8e 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -2016,8 +2016,7 @@ int genphy_c45_plca_set_cfg(struct phy_device *phydev,
 			    const struct phy_plca_cfg *plca_cfg);
 int genphy_c45_plca_get_status(struct phy_device *phydev,
 			       struct phy_plca_status *plca_st);
-int genphy_c45_eee_is_active(struct phy_device *phydev, unsigned long *adv,
-			     unsigned long *lp);
+int genphy_c45_eee_is_active(struct phy_device *phydev, unsigned long *lp);
 int genphy_c45_ethtool_get_eee(struct phy_device *phydev,
 			       struct ethtool_keee *data);
 int genphy_c45_ethtool_set_eee(struct phy_device *phydev,
-- 
2.47.1