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

[Paolo Abeni <pabeni@redhat.com>]

On 3/5/26 9:50 AM, Nicolai Buchwitz wrote:
> On 4.3.2026 22:57, Andrew Lunn wrote:
>> On Tue, Mar 03, 2026 at 04:39:18PM +0100, Nicolai Buchwitz wrote:
>>> 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