[PATCH net-next v2 1/1] net: dsa: microchip: Remove ineffective checks from ksz_set_mac_eee()

[PATCH net-next v2 1/1] net: dsa: microchip: Remove ineffective checks from ksz_set_mac_eee()

[Oleksij Rempel]

KSZ switches handle EEE internally via PHY advertisement and do not
support MAC-level configuration. The ksz_set_mac_eee() handler previously
rejected Tx LPI disable and timer changes, but provided no real control.

These checks now interfere with userspace attempts to disable EEE and no
longer reflect the actual hardware behavior. Replace the logic with a
no-op.

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

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index b45052497f8a..b4a8f2c6346f 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -3492,18 +3492,6 @@ static bool ksz_support_eee(struct dsa_switch *ds, int port)
 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;
 }

--
2.39.5

Re: [PATCH net-next v2 1/1] net: dsa: microchip: Remove ineffective checks from ksz_set_mac_eee()

[Russell King (Oracle)]

On Fri, Apr 25, 2025 at 01:08:45PM +0200, Oleksij Rempel wrote:
> KSZ switches handle EEE internally via PHY advertisement and do not
> support MAC-level configuration. The ksz_set_mac_eee() handler previously
> rejected Tx LPI disable and timer changes, but provided no real control.

Err what?

ksz does not set phylink_config->eee_enabled_default, so the default
state in phylink is eee_enabled = false, tx_lpi_enabled = false. It
doesn't set the default LPI timer, so tx_lpi_timer = 0.

As the driver does not implement the ability to change the LPI timer
enable nor the timer value, this seemed reasonable as the values are
not reported (being reported as zeros) and thus prevents modification
thereof.

Why do you want to allow people to change parameters that have no
effect?

> These checks now interfere with userspace attempts to disable EEE and no
> longer reflect the actual hardware behavior. Replace the logic with a
> no-op.

They don't.

ethtool --set-eee eee off

will work, because GEEE will return tx_lpi_enabled = 0 tx_lpi_timer = 0,
and when it attempts to set, it will be validated that these haven't
been changed.

This has been the behaviour of the driver for some time. Why do we want
now to allow userspace to change parameters that don't have any effect?

Did I waste my time doing a careful conversion of DSA drivers to phylink
managed EEE, trying to ensure that there was no change of behaviour. I'm
feeling like I shouldn't have bothered because that careful work is now
being undone by a patch stream that seems not to understand the code...

-- 
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 v2 1/1] net: dsa: microchip: Remove ineffective checks from ksz_set_mac_eee()

[Oleksij Rempel]

On Fri, Apr 25, 2025 at 02:41:21PM +0100, Russell King (Oracle) wrote:
> On Fri, Apr 25, 2025 at 01:08:45PM +0200, Oleksij Rempel wrote:
> > KSZ switches handle EEE internally via PHY advertisement and do not
> > support MAC-level configuration. The ksz_set_mac_eee() handler previously
> > rejected Tx LPI disable and timer changes, but provided no real control.
> 
> Err what?
> 
> ksz does not set phylink_config->eee_enabled_default, so the default
> state in phylink is eee_enabled = false, tx_lpi_enabled = false. It
> doesn't set the default LPI timer, so tx_lpi_timer = 0.
> 
> As the driver does not implement the ability to change the LPI timer
> enable nor the timer value, this seemed reasonable as the values are
> not reported (being reported as zeros) and thus prevents modification
> thereof.
> 
> Why do you want to allow people to change parameters that have no
> effect?

The original ksz_get_mac_eee() used to report tx_lpi_enabled = true,
which correctly reflected the internal EEE/LPI activity of the hardware.

After commit [0945a7b44220 ("net: dsa: ksz: remove setting of tx_lpi
parameters")], ksz_get_mac_eee() was removed, and now tx_lpi_enabled defaults
to false via the phylink fallback.

This leads to two problems:

- Userspace is unable to disable EEE cleanly (ethtool --set-eee lan1 eee off
  fails with -EINVAL).

- At the same time, userspace sees tx_lpi_enabled = false, which does not match
  hardware behavior (EEE/LPI is active).

At the moment, keeping the old validation checks blocks userspace from
disabling EEE at all.

But removing all validation risks accepting nonsensical parameter combinations.

Right now, I am not fully sure what the best way forward is.
 
-- 
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 |

Re: [PATCH net-next v2 1/1] net: dsa: microchip: Remove ineffective checks from ksz_set_mac_eee()

[Russell King (Oracle)]

On Fri, Apr 25, 2025 at 04:26:37PM +0200, Oleksij Rempel wrote:
> On Fri, Apr 25, 2025 at 02:41:21PM +0100, Russell King (Oracle) wrote:
> > On Fri, Apr 25, 2025 at 01:08:45PM +0200, Oleksij Rempel wrote:
> > > KSZ switches handle EEE internally via PHY advertisement and do not
> > > support MAC-level configuration. The ksz_set_mac_eee() handler previously
> > > rejected Tx LPI disable and timer changes, but provided no real control.
> > 
> > Err what?
> > 
> > ksz does not set phylink_config->eee_enabled_default, so the default
> > state in phylink is eee_enabled = false, tx_lpi_enabled = false. It
> > doesn't set the default LPI timer, so tx_lpi_timer = 0.
> > 
> > As the driver does not implement the ability to change the LPI timer
> > enable nor the timer value, this seemed reasonable as the values are
> > not reported (being reported as zeros) and thus prevents modification
> > thereof.
> > 
> > Why do you want to allow people to change parameters that have no
> > effect?
> 
> The original ksz_get_mac_eee() used to report tx_lpi_enabled = true,
> which correctly reflected the internal EEE/LPI activity of the hardware.

Are you sure it did _actually_ did return that?

Yes, ksz_get_mac_eee() set e->tx_lpi_enabled = true, but if you read the
commit 0945a7b44220 message, you will see that DSA calls
phylink_ethtool_get_eee() after this function, which then calls into
phy_ethtool_get_eee(), and phy_ethtool_get_eee() overwrites *all*
members of struct ethtool_keee.

Thus, userspace doesn't see tx_lpi_enabled set.

Please wind back to before commit 0945a7b44220 to confirm this - I
think you'll find that this bug was introduced in commit
fe0d4fd9285e "net: phy: Keep track of EEE configuration".

> After commit [0945a7b44220 ("net: dsa: ksz: remove setting of tx_lpi
> parameters")], ksz_get_mac_eee() was removed, and now tx_lpi_enabled defaults
> to false via the phylink fallback.

As stated above, I think this driver has had a problem for over a year
now, caused ultimately by the incomplete submission of Andrew's patch
set. I think you'll find that if you try the comparing the ksz behaviour
of commit fe0d4fd9285e^ with commit fe0d4fd9285e, you'll find that's
where this behaviour changed.

-- 
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 v2 1/1] net: dsa: microchip: Remove ineffective checks from ksz_set_mac_eee()

[Oleksij Rempel]

On Fri, Apr 25, 2025 at 03:43:50PM +0100, Russell King (Oracle) wrote:
> On Fri, Apr 25, 2025 at 04:26:37PM +0200, Oleksij Rempel wrote:
> > On Fri, Apr 25, 2025 at 02:41:21PM +0100, Russell King (Oracle) wrote:
> > > On Fri, Apr 25, 2025 at 01:08:45PM +0200, Oleksij Rempel wrote:
> > > > KSZ switches handle EEE internally via PHY advertisement and do not
> > > > support MAC-level configuration. The ksz_set_mac_eee() handler previously
> > > > rejected Tx LPI disable and timer changes, but provided no real control.
> > > 
> > > Err what?
> > > 
> > > ksz does not set phylink_config->eee_enabled_default, so the default
> > > state in phylink is eee_enabled = false, tx_lpi_enabled = false. It
> > > doesn't set the default LPI timer, so tx_lpi_timer = 0.
> > > 
> > > As the driver does not implement the ability to change the LPI timer
i > > enable nor the timer value, this seemed reasonable as the values are
> > > not reported (being reported as zeros) and thus prevents modification
> > > thereof.
> > > 
> > > Why do you want to allow people to change parameters that have no
> > > effect?
> > 
> > The original ksz_get_mac_eee() used to report tx_lpi_enabled = true,
> > which correctly reflected the internal EEE/LPI activity of the hardware.
> 
> Are you sure it did _actually_ did return that?
> 
> Yes, ksz_get_mac_eee() set e->tx_lpi_enabled = true, but if you read the
> commit 0945a7b44220 message, you will see that DSA calls
> phylink_ethtool_get_eee() after this function, which then calls into
> phy_ethtool_get_eee(), and phy_ethtool_get_eee() overwrites *all*
> members of struct ethtool_keee.
> 
> Thus, userspace doesn't see tx_lpi_enabled set.
> 
> Please wind back to before commit 0945a7b44220 to confirm this - I
> think you'll find that this bug was introduced in commit
> fe0d4fd9285e "net: phy: Keep track of EEE configuration".
> 
> > After commit [0945a7b44220 ("net: dsa: ksz: remove setting of tx_lpi
> > parameters")], ksz_get_mac_eee() was removed, and now tx_lpi_enabled defaults
> > to false via the phylink fallback.
> 
> As stated above, I think this driver has had a problem for over a year
> now, caused ultimately by the incomplete submission of Andrew's patch
> set. I think you'll find that if you try the comparing the ksz behaviour
> of commit fe0d4fd9285e^ with commit fe0d4fd9285e, you'll find that's
> where this behaviour changed.

thank you again for your detailed explanations.

After carefully analyzing the situation, I fully agree with your
assessment.

The key point is that the change in behavior was introduced already by
commit [fe0d4fd9285e ("net: phy: Keep track of EEE configuration")],
where phy_ethtool_get_eee() started overwriting the complete
ethtool_keee structure based on phydev->eee_cfg.

Since the KSZ DSA driver and the DSA framework do not request the
PHYlink framework to enable EEE by default, tx_lpi_enabled correctly
remains false.  However, because of how phy_probe() initializes
eee_enabled based on PHY advertisement, userspace will observe that EEE
is enabled, but Tx LPI is disabled, leading to an inconsistent state.

Thus, the current driver behavior is consistent with the framework
expectations.
My initial concern was based on the assumption that we still reported
EEE active by MAC default, which is no longer the case.
Therefore, there is no need to adjust ksz_set_mac_eee(), and I will
withdraw the patch.

Additionally, it seems that setting eee_enabled automatically based on
advertisement in phy_probe() is no longer appropriate.
If you agree, I would propose a patch to remove this initialization.

Thank you again for your patience and for helping to clarify this
situation.

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 |

Re: [PATCH net-next v2 1/1] net: dsa: microchip: Remove ineffective checks from ksz_set_mac_eee()

[Russell King (Oracle)]

On Fri, Apr 25, 2025 at 06:31:18PM +0200, Oleksij Rempel wrote:
> Additionally, it seems that setting eee_enabled automatically based on
> advertisement in phy_probe() is no longer appropriate.
> If you agree, I would propose a patch to remove this initialization.

Remember how eee_enabled is implemented. It controls whether we program
the EEE advertisement with the contents of the software advertisement
state, or clear it.

So, if the hardware has a non-zero advertisement, then it is completely
correct that we read the advertisement and set eee_enabled to be true.
The alternative is, we set eee_enabled false and clear the
advertisement.

We can't leave the advertisement and set eee_enabled to be false. That
is inconsistent with the way we handle any attempt to set the
eee_enabled state.

I think the correct approach in this case is to set
config->eee_enabled_default to be true in ksz_phylink_get_caps(),
thereby correcting the bug introduced in March 2024. That has the
effect of setting phy->eee_cfg.tx_lpi_enabled, which means we should
report tx_lpi_enabled as true when reading the EEE state.

For the stable kernels between March 2024 and the integration of
phylink EEE support, a similar approach will be necessary to restore
the pre-March 2024 behaviour, but that will be much harder to correct
as the DSA driver doesn't have an appropriate hook to set that field
at the appropriate time.

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