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

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

[Nicolai Buchwitz]

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.

Reorder the function to call phy_ethtool_get_eee() first, then restore
the MAC-managed fields from the driver's private state.

Fixes: fe0d4fd9285e ("net: phy: Keep track of EEE configuration")
Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index a71cd729fde6..33fece36a95e 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1390,6 +1390,7 @@ static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_keee *e)
 {
 	struct bcmgenet_priv *priv = netdev_priv(dev);
 	struct ethtool_keee *p = &priv->eee;
+	int ret;
 
 	if (GENET_IS_V1(priv))
 		return -EOPNOTSUPP;
@@ -1397,10 +1398,19 @@ static int bcmgenet_get_eee(struct net_device *dev, struct ethtool_keee *e)
 	if (!dev->phydev)
 		return -ENODEV;
 
+	ret = phy_ethtool_get_eee(dev->phydev, e);
+	if (ret)
+		return ret;
+
+	/* Restore MAC-managed fields that phy_ethtool_get_eee() overwrites
+	 * with the PHY's eee_cfg (which is only set by phylink-managed MACs
+	 * via phy_support_eee()).
+	 */
+	e->eee_enabled = p->eee_enabled;
 	e->tx_lpi_enabled = p->tx_lpi_enabled;
 	e->tx_lpi_timer = bcmgenet_umac_readl(priv, UMAC_EEE_LPI_TIMER);
 
-	return phy_ethtool_get_eee(dev->phydev, e);
+	return 0;
 }
 
 static int bcmgenet_set_eee(struct net_device *dev, struct ethtool_keee *e)
-- 
2.51.0

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

[Andrew Lunn]

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.

       Andrew

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

[Nicolai Buchwitz]

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.

> 
>        Andrew

Nicolai

[1] 
https://lore.kernel.org/netdev/20260303160225.542613-1-nb@tipi-net.de/

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

[Paolo Abeni]

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

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

[Andrew Lunn]

On Thu, Mar 05, 2026 at 09:50:29AM +0100, 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

Great, so we are going in the right direction.

The whole point of the reworking of EEE and putting as much as
possible into the phylib was that many MAC drivers were broken, and
the behaviour between MAC drivers was inconsistent. I had a big
patchset which converted every MAC driver, removing nearly all the EEE
code. Unfortunately, not all the patches got merged, and we ended up
with some MAC drivers in a half way state.

Please look at removing all the state tracking in the MAC driver. When
the adjust_link callback is made, look at the state in phydev, and
configure the hardware based on that. As an example, look at

https://elixir.bootlin.com/linux/v6.19.3/source/drivers/net/ethernet/freescale/fec_main.c#L2129

	Andrew