[PATCH net-next v1 0/4] Improve EEE control for KSZ switches and clarify ethtool output

[PATCH net-next v1 0/4] Improve EEE control for KSZ switches and clarify ethtool output

[Oleksij Rempel]

This patch series addresses two areas related to Energy Efficient
Ethernet (EEE) handling, focusing on better integration of Microchip KSZ
switches with the DSA framework and improving the clarity of ethtool
--show-eee output.

Part 1: Previously, I added a filter to the set_mac_eee() implementation
in the Microchip KSZ driver to reject unsupported configurations -
specifically, attempts to disable LPI while keeping EEE enabled, or to
change the Tx LPI timer. This was intended as a safeguard to clarify
what the driver does not support, since EEE is handled internally by the
KSZ hardware through PHY advertisement.

However, after many changes in the EEE framework, this filter has become
outdated and problematic. It now blocks users from disabling EEE
altogether. Since there is no MAC-level configuration required for KSZ
switches, and the driver already implements support_eee(), the cleanest
path forward is to remove the set_mac_eee() implementation entirely.

Part 2: Clarify ethtool output related to EEE
The ethtool --show-eee command could produce misleading output:
- It displayed "Advertised EEE link modes" even when EEE was disabled
- It omitted the link partner's EEE advertisement unless local EEE was
  enabled

The remaining patches focus on improving the usability and correctness
of the ethtool --show-eee output, particularly in cases where EEE is
disabled.

Tested with Microchip KSZ series hardware.

Oleksij Rempel (4):
  net: dsa: user: Skip set_mac_eee() if support_eee() is implemented
  net: dsa: microchip: Remove set_mac_eee() callback from KSZ driver
  net: phy: Don't report advertised EEE modes if EEE is disabled
  net: phy: Always read EEE LPA in genphy_c45_ethtool_get_eee()

 drivers/net/dsa/microchip/ksz_common.c | 19 -------------------
 drivers/net/phy/phy-c45.c              |  7 ++++---
 net/dsa/user.c                         |  6 ++----
 3 files changed, 6 insertions(+), 26 deletions(-)

--
2.39.5

[PATCH net-next v1 1/4] net: dsa: user: Skip set_mac_eee() if support_eee() is implemented

[Oleksij Rempel]

Some switches with integrated PHYs, like Microchip KSZ, manage EEE
internally based on PHY advertisement and link resolution. If
ds->ops->support_eee() is implemented, assume EEE is supported
and skip requiring set_mac_eee().

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 net/dsa/user.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/dsa/user.c b/net/dsa/user.c
index 804dc7dac4f2..87b78a4c1d6c 100644
--- a/net/dsa/user.c
+++ b/net/dsa/user.c
@@ -1246,14 +1246,12 @@ static int dsa_user_set_eee(struct net_device *dev, struct ethtool_keee *e)
 	/* If the port is using phylink managed EEE, then an unimplemented
 	 * set_mac_eee() is permissible.
 	 */
-	if (!phylink_mac_implements_lpi(ds->phylink_mac_ops)) {
+	if (ds->ops->set_mac_eee &&
+	    !phylink_mac_implements_lpi(ds->phylink_mac_ops)) {
 		/* Port's PHY and MAC both need to be EEE capable */
 		if (!dev->phydev)
 			return -ENODEV;
 
-		if (!ds->ops->set_mac_eee)
-			return -EOPNOTSUPP;
-
 		ret = ds->ops->set_mac_eee(ds, dp->index, e);
 		if (ret)
 			return ret;
-- 
2.39.5

Re: [PATCH net-next v1 1/4] net: dsa: user: Skip set_mac_eee() if support_eee() is implemented

[Russell King (Oracle)]

On Thu, Apr 24, 2025 at 03:02:19PM +0200, Oleksij Rempel wrote:
> Some switches with integrated PHYs, like Microchip KSZ, manage EEE
> internally based on PHY advertisement and link resolution. If
> ds->ops->support_eee() is implemented, assume EEE is supported
> and skip requiring set_mac_eee().
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

If you look at the conditions here, there's a path for legacy where
set_mac_eee() is mandatory (which is what you're changing to be
optional) and there's a path for phylink based EEE where set_mac_eee()
becomes optional.

I would rather we left legacy alone, except to remove it entirely.

-- 
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 v1 2/4] net: dsa: microchip: Remove set_mac_eee() callback from KSZ driver

[Oleksij Rempel]

KSZ switches manage EEE internally without any documented MAC-specific
configuration. The existing set_mac_eee() handler only rejected attempts
to disable LPI while keeping EEE on, or to change the Tx LPI timer, offering
no real control.

This now prevents users from disabling EEE when desired. Even if it worked
in the initial implementation, it has since bitrotted and no longer functions
as expected. Since support_eee() is implemented and EEE is handled via
PHY negotiation, drop the set_mac_eee() callback entirely.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz_common.c | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index b45052497f8a..e5924ad65658 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -3489,24 +3489,6 @@ static bool ksz_support_eee(struct dsa_switch *ds, int port)
 	return false;
 }
 
-static int ksz_set_mac_eee(struct dsa_switch *ds, int port,
-			   struct ethtool_keee *e)
-{
-	struct ksz_device *dev = ds->priv;
-
-	if (!e->tx_lpi_enabled) {
-		dev_err(dev->dev, "Disabling EEE Tx LPI is not supported\n");
-		return -EINVAL;
-	}
-
-	if (e->tx_lpi_timer) {
-		dev_err(dev->dev, "Setting EEE Tx LPI timer is not supported\n");
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
 static void ksz_set_xmii(struct ksz_device *dev, int port,
 			 phy_interface_t interface)
 {
@@ -4749,7 +4731,6 @@ static const struct dsa_switch_ops ksz_switch_ops = {
 	.cls_flower_del		= ksz_cls_flower_del,
 	.port_setup_tc		= ksz_setup_tc,
 	.support_eee		= ksz_support_eee,
-	.set_mac_eee		= ksz_set_mac_eee,
 	.port_get_default_prio	= ksz_port_get_default_prio,
 	.port_set_default_prio	= ksz_port_set_default_prio,
 	.port_get_dscp_prio	= ksz_port_get_dscp_prio,
-- 
2.39.5

[PATCH net-next v1 3/4] net: phy: Don't report advertised EEE modes if EEE is disabled

[Oleksij Rempel]

Currently, `ethtool --show-eee` reports "Advertised EEE link modes" even when
EEE is disabled, which can be misleading. For example:

  EEE settings for lan1:
          EEE status: disabled
          Tx LPI: disabled
          Supported EEE link modes:  100baseT/Full
                                     1000baseT/Full
          Advertised EEE link modes:  100baseT/Full
                                      1000baseT/Full
          Link partner advertised EEE link modes:  Not reported

This may lead to confusion for users who aren't familiar with kernel internals
but understand that EEE functionality depends on proper advertisement during
link negotiation. Seeing advertised EEE modes in this case might incorrectly
suggest that EEE is still being advertised.

After this change, if EEE is disabled, the output becomes:

  EEE settings for lan1:
          EEE status: disabled
          Tx LPI: disabled
          Supported EEE link modes:  100baseT/Full
                                     1000baseT/Full
          Advertised EEE link modes:  Not reported
          Link partner advertised EEE link modes:  Not reported

This better reflects the actual EEE configuration. The fix ensures
advertised EEE modes are only reported when eee_cfg.eee_enabled is true.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/phy-c45.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index bdd70d424491..8eb12433387d 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -1517,7 +1517,8 @@ int genphy_c45_ethtool_get_eee(struct phy_device *phydev,
 	data->eee_active = phydev->eee_active;
 	linkmode_andnot(data->supported, phydev->supported_eee,
 			phydev->eee_disabled_modes);
-	linkmode_copy(data->advertised, phydev->advertising_eee);
+	if (phydev->eee_cfg.eee_enabled)
+		linkmode_copy(data->advertised, phydev->advertising_eee);
 	return 0;
 }
 EXPORT_SYMBOL(genphy_c45_ethtool_get_eee);
-- 
2.39.5

Re: [PATCH net-next v1 3/4] net: phy: Don't report advertised EEE modes if EEE is disabled

[Russell King (Oracle)]

On Thu, Apr 24, 2025 at 03:02:21PM +0200, Oleksij Rempel wrote:
> Currently, `ethtool --show-eee` reports "Advertised EEE link modes" even when
> EEE is disabled, which can be misleading. For example:
> 
>   EEE settings for lan1:
>           EEE status: disabled
>           Tx LPI: disabled
>           Supported EEE link modes:  100baseT/Full
>                                      1000baseT/Full
>           Advertised EEE link modes:  100baseT/Full
>                                       1000baseT/Full
>           Link partner advertised EEE link modes:  Not reported
> 
> This may lead to confusion for users who aren't familiar with kernel internals
> but understand that EEE functionality depends on proper advertisement during
> link negotiation. Seeing advertised EEE modes in this case might incorrectly
> suggest that EEE is still being advertised.
> 
> After this change, if EEE is disabled, the output becomes:
> 
>   EEE settings for lan1:
>           EEE status: disabled
>           Tx LPI: disabled
>           Supported EEE link modes:  100baseT/Full
>                                      1000baseT/Full
>           Advertised EEE link modes:  Not reported
>           Link partner advertised EEE link modes:  Not reported
> 
> This better reflects the actual EEE configuration. The fix ensures
> advertised EEE modes are only reported when eee_cfg.eee_enabled is true.

No, this is a backwards step.

Tools like ethtool read-modify-write the settings. First they get, then
modify, then set.

This will have the effect that:

ethtool --set-eee eee off
ethtool --set-eee eee on

will clear the advertised link modes, which is not what one would
expect.

-- 
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 v1 3/4] net: phy: Don't report advertised EEE modes if EEE is disabled

[Andrew Lunn]

On Thu, Apr 24, 2025 at 03:02:21PM +0200, Oleksij Rempel wrote:
> Currently, `ethtool --show-eee` reports "Advertised EEE link modes" even when
> EEE is disabled, which can be misleading. For example:
> 
>   EEE settings for lan1:
>           EEE status: disabled
>           Tx LPI: disabled
>           Supported EEE link modes:  100baseT/Full
>                                      1000baseT/Full
>           Advertised EEE link modes:  100baseT/Full
>                                       1000baseT/Full
>           Link partner advertised EEE link modes:  Not reported

What is the behaviour for normal link mode advertisement? If i turn
autoneg off, do the advertised link modes disappear? Do they reappear
when i turn autoneg back on again?

I would expect EEE to follow what the normal link modes do. Assuming
the Read/modify/write does not break this.

	Andrew

Re: [PATCH net-next v1 3/4] net: phy: Don't report advertised EEE modes if EEE is disabled

[Russell King (Oracle)]

On Thu, Apr 24, 2025 at 04:30:40PM +0200, Andrew Lunn wrote:
> On Thu, Apr 24, 2025 at 03:02:21PM +0200, Oleksij Rempel wrote:
> > Currently, `ethtool --show-eee` reports "Advertised EEE link modes" even when
> > EEE is disabled, which can be misleading. For example:
> > 
> >   EEE settings for lan1:
> >           EEE status: disabled
> >           Tx LPI: disabled
> >           Supported EEE link modes:  100baseT/Full
> >                                      1000baseT/Full
> >           Advertised EEE link modes:  100baseT/Full
> >                                       1000baseT/Full
> >           Link partner advertised EEE link modes:  Not reported
> 
> What is the behaviour for normal link mode advertisement? If i turn
> autoneg off, do the advertised link modes disappear? Do they reappear
> when i turn autoneg back on again?
> 
> I would expect EEE to follow what the normal link modes do. Assuming
> the Read/modify/write does not break this.

It's difficult to compare, because ethtool is implemented differently
between modifying the link modes and the EEE stuff. ethtool -s autoneg
on uses this:

                        if (autoneg_wanted == AUTONEG_ENABLE &&
                            advertising_wanted == NULL &&
                            full_advertising_wanted == NULL) {
                                unsigned int i;

                                /* Auto negotiation enabled, but with
                                 * unspecified speed and duplex: enable all
                                 * supported speeds and duplexes.
                                 */

whereas do_seee() has no special handling.

So, if we want ethtool --set-eee eee off; ethtool --set-eee eee on *not*
to end up with the advertising mask being cleared, then we have to
preserve it, or force the advertising mask to something if the
advertising mask is empty and eee_enabled is true.

Or we preserve the advertising mask when eee_enabled is cleared, which
is what we do today.

I think, given the different implementations in ethtool, we can't just
say "we want it to be have the same" by just modifying the kernel.

-- 
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 v1 4/4] net: phy: Always read EEE LPA in genphy_c45_ethtool_get_eee()

[Oleksij Rempel]

Previously, genphy_c45_ethtool_get_eee() used genphy_c45_eee_is_active(),
which skips reading the EEE LPA register if local EEE is disabled. This
prevents ethtool from reporting the link partner's EEE capabilities in
that case.

Replace it with genphy_c45_read_eee_lpa(), which always reads the LPA
register regardless of local EEE state. This allows users to see the
link partner's EEE advertisement even when EEE is disabled locally.

Example before the patch:

  EEE settings for lan1:
          EEE status: disabled
          Tx LPI: disabled
          Supported EEE link modes:  100baseT/Full
                                     1000baseT/Full
          Advertised EEE link modes:  Not reported
          Link partner advertised EEE link modes:  Not reported

After the patch:

  EEE settings for lan1:
          EEE status: disabled
          Tx LPI: disabled
          Supported EEE link modes:  100baseT/Full
                                     1000baseT/Full
          Advertised EEE link modes:  Not reported
          Link partner advertised EEE link modes:  100baseT/Full
                                                   1000baseT/Full

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 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 8eb12433387d..9c582abc023a 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -1510,8 +1510,8 @@ int genphy_c45_ethtool_get_eee(struct phy_device *phydev,
 {
 	int ret;
 
-	ret = genphy_c45_eee_is_active(phydev, data->lp_advertised);
-	if (ret < 0)
+	ret = genphy_c45_read_eee_lpa(phydev, data->lp_advertised);
+	if (ret)
 		return ret;
 
 	data->eee_active = phydev->eee_active;
-- 
2.39.5

Re: [PATCH net-next v1 4/4] net: phy: Always read EEE LPA in genphy_c45_ethtool_get_eee()

[Russell King (Oracle)]

On Thu, Apr 24, 2025 at 03:02:22PM +0200, Oleksij Rempel wrote:
> Previously, genphy_c45_ethtool_get_eee() used genphy_c45_eee_is_active(),
> which skips reading the EEE LPA register if local EEE is disabled. This
> prevents ethtool from reporting the link partner's EEE capabilities in
> that case.
> 
> Replace it with genphy_c45_read_eee_lpa(), which always reads the LPA
> register regardless of local EEE state. This allows users to see the
> link partner's EEE advertisement even when EEE is disabled locally.
> 
> Example before the patch:
> 
>   EEE settings for lan1:
>           EEE status: disabled
>           Tx LPI: disabled
>           Supported EEE link modes:  100baseT/Full
>                                      1000baseT/Full
>           Advertised EEE link modes:  Not reported
>           Link partner advertised EEE link modes:  Not reported
> 
> After the patch:
> 
>   EEE settings for lan1:
>           EEE status: disabled
>           Tx LPI: disabled
>           Supported EEE link modes:  100baseT/Full
>                                      1000baseT/Full
>           Advertised EEE link modes:  Not reported
>           Link partner advertised EEE link modes:  100baseT/Full
>                                                    1000baseT/Full

Seems to me this takes the opposite view to patch 3... not sure there's
much consistency here.

However, I've no objection to reading the LPA EEE state and
reporting it.

-- 
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 v1 4/4] net: phy: Always read EEE LPA in genphy_c45_ethtool_get_eee()

[Andrew Lunn]

On Thu, Apr 24, 2025 at 02:16:01PM +0100, Russell King (Oracle) wrote:
> On Thu, Apr 24, 2025 at 03:02:22PM +0200, Oleksij Rempel wrote:
> > Previously, genphy_c45_ethtool_get_eee() used genphy_c45_eee_is_active(),
> > which skips reading the EEE LPA register if local EEE is disabled. This
> > prevents ethtool from reporting the link partner's EEE capabilities in
> > that case.
> > 
> > Replace it with genphy_c45_read_eee_lpa(), which always reads the LPA
> > register regardless of local EEE state. This allows users to see the
> > link partner's EEE advertisement even when EEE is disabled locally.
> > 
> > Example before the patch:
> > 
> >   EEE settings for lan1:
> >           EEE status: disabled
> >           Tx LPI: disabled
> >           Supported EEE link modes:  100baseT/Full
> >                                      1000baseT/Full
> >           Advertised EEE link modes:  Not reported
> >           Link partner advertised EEE link modes:  Not reported
> > 
> > After the patch:
> > 
> >   EEE settings for lan1:
> >           EEE status: disabled
> >           Tx LPI: disabled
> >           Supported EEE link modes:  100baseT/Full
> >                                      1000baseT/Full
> >           Advertised EEE link modes:  Not reported
> >           Link partner advertised EEE link modes:  100baseT/Full
> >                                                    1000baseT/Full
> 
> Seems to me this takes the opposite view to patch 3... not sure there's
> much consistency here.

+1

> However, I've no objection to reading the LPA EEE state and
> reporting it.

What happens with normal link mode LPA when autoneg is disabled? I
guess they are not reported because the PHY is not even listening for
the autoneg pulses. We could be inconsistent between normal LPA and
LPA EEE, but is that a good idea?

	Andrew

Re: [PATCH net-next v1 4/4] net: phy: Always read EEE LPA in genphy_c45_ethtool_get_eee()

[Russell King (Oracle)]

On Thu, Apr 24, 2025 at 04:34:27PM +0200, Andrew Lunn wrote:
> On Thu, Apr 24, 2025 at 02:16:01PM +0100, Russell King (Oracle) wrote:
> > However, I've no objection to reading the LPA EEE state and
> > reporting it.
> 
> What happens with normal link mode LPA when autoneg is disabled? I
> guess they are not reported because the PHY is not even listening for
> the autoneg pulses. We could be inconsistent between normal LPA and
> LPA EEE, but is that a good idea?

With autoneg state, that controls whether the various pages get
exchanged or not - which includes the EEE capabilties. This is the
big hammer for anything that is negotiated.

With EEE, as long as autoneg in the main config is true, the PHY will
exchange the EEE capability pages if it supports them. Our eee_enabled
is purely just a software switch, there's nothing that corresponds to it
in hardware, unlike autoneg which has a bit in BMCR.

We implement eee_enabled by clearing the advertisement in the hardware
but accepting (and remembering) the advertisement from userspace
unmodified.

The two things are entirely different in hardware.

Since:

 ethtool --set-eee eee off

Will use ETHTOOL_GEEE, modify eee_enabled to be false (via
do_generic_set), and then use ETHTOOL_SEEE to write it back, the
old advertisement will be passed back to the kernel in this case.

If we don't preserve the advertisement, then:

 ethtool --set-eee eee off

will clear the advertisement, and then:

 ethtool --set-eee eee on

will set eee_enabled true but we'll have an empty advertisement. Not
ideal.

If we think about forcing it for an empty advertisement to e.g. fully
populated, then:

 ethtool --set-eee eee on advertise 0

will surprisingly not end up with an empty advertisement.

So, I don't think it's realistic to come up with a way that --set-eee
behaves the same way as -s because of the way ethtool has been
implemented.

-- 
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 v1 4/4] net: phy: Always read EEE LPA in genphy_c45_ethtool_get_eee()

[Oleksij Rempel]

On Thu, Apr 24, 2025 at 03:47:03PM +0100, Russell King (Oracle) wrote:
> On Thu, Apr 24, 2025 at 04:34:27PM +0200, Andrew Lunn wrote:
> > On Thu, Apr 24, 2025 at 02:16:01PM +0100, Russell King (Oracle) wrote:
> > > However, I've no objection to reading the LPA EEE state and
> > > reporting it.
> > 
> > What happens with normal link mode LPA when autoneg is disabled? I
> > guess they are not reported because the PHY is not even listening for
> > the autoneg pulses. We could be inconsistent between normal LPA and
> > LPA EEE, but is that a good idea?
> 
> With autoneg state, that controls whether the various pages get
> exchanged or not - which includes the EEE capabilties. This is the
> big hammer for anything that is negotiated.
> 
> With EEE, as long as autoneg in the main config is true, the PHY will
> exchange the EEE capability pages if it supports them. Our eee_enabled
> is purely just a software switch, there's nothing that corresponds to it
> in hardware, unlike autoneg which has a bit in BMCR.
> 
> We implement eee_enabled by clearing the advertisement in the hardware
> but accepting (and remembering) the advertisement from userspace
> unmodified.
> 
> The two things are entirely different in hardware.
> 
> Since:
> 
>  ethtool --set-eee eee off
> 
> Will use ETHTOOL_GEEE, modify eee_enabled to be false (via
> do_generic_set), and then use ETHTOOL_SEEE to write it back, the
> old advertisement will be passed back to the kernel in this case.
> 
> If we don't preserve the advertisement, then:
> 
>  ethtool --set-eee eee off
> 
> will clear the advertisement, and then:
> 
>  ethtool --set-eee eee on
> 
> will set eee_enabled true but we'll have an empty advertisement. Not
> ideal.
> 
> If we think about forcing it for an empty advertisement to e.g. fully
> populated, then:
> 
>  ethtool --set-eee eee on advertise 0
> 
> will surprisingly not end up with an empty advertisement.
> 
> So, I don't think it's realistic to come up with a way that --set-eee
> behaves the same way as -s because of the way ethtool has been
> implemented.

Thank you for the detailed explanation. I completely forgot that
"advertising_eee" is part of a read-modify-write cycle in the ethtool flow.
That makes sense now. In this case, I agree - there's nothing much I can
do code-wise. In this case, the only thing I can do is document this
behavior on both the kernel and ethtool sides to avoid confusion for
others.

Best Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |