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

Oleksij Rempel
2025-04-25T16:31:28Z

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 |