Re: [PATCH net] net: bcmgenet: fix bcmgenet_get_eee() clobbered by phy_ethtool_get_eee()

On 3/5/26 9:50 AM, Nicolai Buchwitz wrote:

On 4.3.2026 22:57, Andrew Lunn wrote:

quoted

On Tue, Mar 03, 2026 at 04:39:18PM +0100, Nicolai Buchwitz wrote:

quoted

bcmgenet_get_eee() sets the MAC-managed tx_lpi_enabled and 
tx_lpi_timer
fields, then calls phy_ethtool_get_eee() which internally calls
eeecfg_to_eee() — overwriting eee_enabled, tx_lpi_enabled and
tx_lpi_timer with the PHY's eee_cfg values.  For non-phylink MACs like
GENET, these PHY-level fields are never initialized (they are only set
by phylink via phy_support_eee()), so the ethtool report always shows
eee_enabled=false and tx_lpi_enabled=false regardless of the actual 
MAC
state.

I think the MAC driver is missing a call to phy_support_eee() to let
phylib know the MAC supports EEE. Have you tried that.

Thanks for the suggestion. I've incorporated phy_support_eee() and 
tested it
with a Raspberry CM4.

After applying the patch, the PHY correctly advertises EEE, negotiates 
it with
the link partner, and ethtool reports:

   EEE status: enabled - active

However, reading UMAC_EEE_CTRL directly:

   UMAC_EEE_CTRL      = 0x00000040  (DIS_EEE_10M set, EEE_EN = 0)
   UMAC_EEE_LPI_TIMER = 0x00000022  (34 us, hardware reset default)

EEE_EN (bit 3) is never set - the MAC does not actually enter LPI.
priv->eee.eee_enabled stays false at init, so bcmgenet_mac_config()
never calls bcmgenet_eee_enable_set(). The result is ethtool advertising
"enabled - active" while zero power savings happen, which is worse than
the original bug.

Likely root cause: phy_support_eee() sets eee_cfg.eee_enabled=true, 
which
phy_ethtool_get_eee() -> eeecfg_to_eee() then reflects back as
eee_enabled=true - but that is PHY eee_cfg state, not MAC state. The
fix should override eee_enabled and tx_lpi_enabled from priv->eee after
calling phy_ethtool_get_eee(), since those fields are MAC-managed:

   ret = phy_ethtool_get_eee(dev->phydev, e);
   if (ret)
     return ret;

   e->eee_enabled    = priv->eee.eee_enabled;
   e->tx_lpi_enabled = priv->eee.tx_lpi_enabled;
   e->tx_lpi_timer   = bcmgenet_umac_readl(priv, UMAC_EEE_LPI_TIMER);

This correctly separates what the PHY negotiated (eee_active, link 
modes)
from what the MAC is configured to do.

The deeper problem - that the MAC never enables LPI even when EEE is
successfully negotiated - was submitted separately to net-next [1]. It
initializes priv->eee.eee_enabled=true in bcmgenet_open() for GENET v2+,
matching what mvneta, mvpp2 and others do.

Given how the two patches interact, I think they should go through net
as a 2-patch fix series - if there is consensus on that approach. 
Sending
the fix alone would ship the misleading "enabled - active, no actual 
LPI"
state.

Makes sense to me, but it's probably better if you wait for Andrew's ack
before proceeding.

/P