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: <aAufpsLhs8GLMm_b@shell.armlinux.org.uk>
Date: Fri, 25 Apr 2025 15:43:50 +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 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!