Re: [PATCH net v1 1/2] net: phy: Introduce phy_update_eee() to update eee_cfg values

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

On Thu, Nov 14, 2024 at 09:02:47AM +0000, Russell King (Oracle) wrote:
> On Wed, Nov 13, 2024 at 06:10:55PM +0800, Choong Yong Liang wrote:
> > On 12/11/2024 9:04 pm, Andrew Lunn wrote:
> > > On Tue, Nov 12, 2024 at 12:03:15PM +0100, Heiner Kallweit wrote:
> > > > In stmmac_ethtool_op_get_eee() you have the following:
> > > > 
> > > > edata->tx_lpi_timer = priv->tx_lpi_timer;
> > > > edata->tx_lpi_enabled = priv->tx_lpi_enabled;
> > > > return phylink_ethtool_get_eee(priv->phylink, edata);
> > > > 
> > > > You have to call phylink_ethtool_get_eee() first, otherwise the manually
> > > > set values will be overridden. However setting tx_lpi_enabled shouldn't
> > > > be needed if you respect phydev->enable_tx_lpi.
> > > 
> > > I agree with Heiner here, this sounds like a bug somewhere, not
> > > something which needs new code in phylib. Lets understand why it gives
> > > the wrong results.
> > > 
> > > 	Andrew
> > Hi Russell, Andrew, and Heiner, thanks a lot for your valuable feedback.
> > 
> > The current implementation of the 'ethtool --show-eee' command heavily
> > relies on the phy_ethtool_get_eee() in phy.c. The eeecfg values are set by
> > the 'ethtool --set-eee' command and the phy_support_eee() during the initial
> > state. The phy_ethtool_get_eee() calls eeecfg_to_eee(), which returns the
> > eeecfg containing tx_lpi_timer, tx_lpi_enabled, and eee_enable for the
> > 'ethtool --show-eee' command.
> 
> These three members you mention are user configuration members.
> 
> > The tx_lpi_timer and tx_lpi_enabled values stored in the MAC or PHY driver
> > are not retrieved by the 'ethtool --show-eee' command.
> 
> tx_lpi_timer is the only thing that the MAC driver should be concerned
> with - it needs to program the MAC according to the timer value
> specified. Whether LPI is enabled or not is determined by
> phydev->enable_tx_lpi. The MAC should be using nothing else.
> 
> > Currently, we are facing 3 issues:
> > 1. When we boot up our system and do not issue the 'ethtool --set-eee'
> > command, and then directly issue the 'ethtool --show-eee' command, it always
> > shows that EEE is disabled due to the eeecfg values not being set. However,
> > in the Maxliner GPY PHY, the driver EEE is enabled.
> 
> So the software state is out of sync with the hardware state. This is a
> bug in the GPY PHY driver.
> 
> If we look at the generic code, we can see that genphy_config_aneg()
> calls __genphy_config_aneg() which then goes on to call
> genphy_c45_an_config_eee_aneg(). genphy_c45_an_config_eee_aneg()
> writes the current EEE configuration to the PHY.
> 
> Now if we look at gpy_config_aneg(), it doesn't do this. Therefore,
> the GPY PHY is retaining its hardware state which is different from
> the software state. This is wrong.

Also note that phy_probe() reads the current configuration from the
PHY. The supported mask is set via phydev->drv->get_features,
which calls genphy_c45_pma_read_abilities() via the GPY driver and
genphy_c45_read_eee_abilities().

phy_probe() then moved on to genphy_c45_read_eee_adv(), which reads
the advertisement mask. If the advertising mask is non-zero, then
EEE is set as enabled.

From your description, it sounds like this isn't working right, and
needs to be debugged. For example, is the PHY changing its EEE
advertisement between phy_probe() and when it is up and running?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!