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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

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!