linux-kernel - Re: [PATCH net-next v2 1/1] net: dsa: microchip: Remove ineffective checks from ksz_set_mac

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aAuRAadDStfwfS1U@shell.armlinux.org.uk>
Date: Fri, 25 Apr 2025 14:41:21 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Oleksij Rempel <o.rempel@...gutronix.de>
Cc: "David S. Miller" <davem@...emloft.net>, Andrew Lunn <andrew@...n.ch>,
	Eric Dumazet <edumazet@...gle.com>,
	Florian Fainelli <f.fainelli@...il.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Vladimir Oltean <olteanv@...il.com>,
	Woojung Huh <woojung.huh@...rochip.com>, kernel@...gutronix.de,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH net-next v2 1/1] net: dsa: microchip: Remove ineffective
 checks from ksz_set_mac_eee()

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!