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

Russell King (Oracle)
2025-04-25T13:41:36Z

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...