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: <aAu41tjRIir8oMK7@pengutronix.de>
Date: Fri, 25 Apr 2025 18:31:18 +0200
From: Oleksij Rempel <o.rempel@...gutronix.de>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
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 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 |