| Message ID | 20250207105610.1875327-1-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2,1/1] net: sxgbe: rework EEE handling based on PHY negotiation | expand |
Hi, A more in-depth review... On Fri, Feb 07, 2025 at 11:56:10AM +0100, Oleksij Rempel wrote: > @@ -228,7 +212,7 @@ static void sxgbe_get_ethtool_stats(struct net_device *dev, > int i; > char *p; > > - if (priv->eee_enabled) { > + if (dev->phydev->eee_active) { > int val = phy_get_eee_err(dev->phydev); > > if (val) Why should the EEE error statistic be dependent on whether EEE has been negotiated? (I think a follow-up patch addressing this would be appropriate.) > diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c > index 12c8396b6942..8a385c92a6d1 100644 > --- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c > +++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c > @@ -87,7 +87,7 @@ static void sxgbe_enable_eee_mode(const struct sxgbe_priv_data *priv) > priv->hw->mac->set_eee_mode(priv->ioaddr); > } > > -void sxgbe_disable_eee_mode(struct sxgbe_priv_data * const priv) > +static void sxgbe_disable_eee_mode(struct sxgbe_priv_data * const priv) > { > /* Exit and disable EEE in case of we are in LPI state. */ > priv->hw->mac->reset_eee_mode(priv->ioaddr); Looking at this function, I think it's buggy and needs fixing prior to this patch. This function calls ->reset_eee_mode(), then synchronously deletes the timer. However, if the timer is running and fires after ->reset_eee_mode() has been called, and tx_path_in_lpi_mode is false, then sxgbe_eee_ctrl_timer() will execute, calling sxgbe_enable_eee_mode(). This will then call set_eee_mode(). In other words, this function is racy if not already called with tx_path_in_lpi_mode set true - and there are paths in this driver that does happen (just like in stmmac which I've been fixing - I suspect one or other driver copied the code since the structure and member names are identical.) I would suggest deleting the timer as the very first thing, much like what I did in: net: stmmac: delete software timer before disabling LPI In fact, given the similarity with stmmac, it's probably worth looking through my stmmac EEE cleanup patch series, and deciding which of those changes also apply to sxgbe. ... and given this, I ask again: should there be a generic software-timer EEE implementation so we're not having to patch multiple drivers for the same bug(s). > @@ -110,52 +110,25 @@ static void sxgbe_eee_ctrl_timer(struct timer_list *t) > mod_timer(&priv->eee_ctrl_timer, SXGBE_LPI_TIMER(eee_timer)); > } > > -/** > - * sxgbe_eee_init > - * @priv: private device pointer > - * Description: > - * If the EEE support has been enabled while configuring the driver, > - * if the GMAC actually supports the EEE (from the HW cap reg) and the > - * phy can also manage EEE, so enable the LPI state and start the timer > - * to verify if the tx path can enter in LPI state. > - */ > -bool sxgbe_eee_init(struct sxgbe_priv_data * const priv) > +static void sxgbe_eee_adjust(struct sxgbe_priv_data *priv) > { > - struct net_device *ndev = priv->dev; > - bool ret = false; > + struct phy_device *phydev = priv->dev->phydev; > > - /* MAC core supports the EEE feature. */ > - if (priv->hw_cap.eee) { > - /* Check if the PHY supports EEE */ > - if (phy_init_eee(ndev->phydev, true)) > - return false; > + if (!priv->hw_cap.eee) > + return; > > - timer_setup(&priv->eee_ctrl_timer, sxgbe_eee_ctrl_timer, 0); > - priv->eee_ctrl_timer.expires = SXGBE_LPI_TIMER(eee_timer); > + if (phydev->enable_tx_lpi) { > add_timer(&priv->eee_ctrl_timer); > - > priv->hw->mac->set_eee_timer(priv->ioaddr, > SXGBE_DEFAULT_LPI_TIMER, > - priv->tx_lpi_timer); > - > - pr_info("Energy-Efficient Ethernet initialized\n"); > - > - ret = true; > + phydev->eee_cfg.tx_lpi_timer); > + priv->eee_enabled = true; > + } else { > + sxgbe_disable_eee_mode(priv); > + priv->eee_enabled = false; Given what sxgbe_tx_all_clean() does, we need priv->eee_enabled set false and visible to sxgbe_tx_all_clean() before calling sxgbe_disable_eee_mode(), otherwise sxgbe_tx_all_clean() may race and add the eee_ctrl_timer back after sxgbe_disable_eee_mode() has removed it. > @@ -802,7 +785,7 @@ static void sxgbe_tx_all_clean(struct sxgbe_priv_data * const priv) > sxgbe_tx_queue_clean(tqueue); > } > > - if ((priv->eee_enabled) && (!priv->tx_path_in_lpi_mode)) { > + if (priv->eee_enabled && !priv->tx_path_in_lpi_mode) { > sxgbe_enable_eee_mode(priv); > mod_timer(&priv->eee_ctrl_timer, SXGBE_LPI_TIMER(eee_timer)); As noted above, this can race with sxgbe_disable_eee_mode(). Thanks.
> ... and given this, I ask again: should there be a generic > software-timer EEE implementation so we're not having to patch multiple > drivers for the same bug(s). Probably there should be. But given how little resources we have, i'm not sure it will ever get done. Is there anything special in the stmmac code? Workarounds for stmmac peculiarities? Could it be pulled out? Given you have looked at it, and fixed it up, it seems like the obvious candidate for code donation. Maybe we need to ask the next developer who submits a MAC driver with a software-timer EEE implementation to build the library? Andrew
diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_common.h b/drivers/net/ethernet/samsung/sxgbe/sxgbe_common.h index 1458939c3bf5..a8eed188d110 100644 --- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_common.h +++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_common.h @@ -526,6 +526,4 @@ int sxgbe_restore(struct net_device *ndev); const struct sxgbe_mtl_ops *sxgbe_get_mtl_ops(void); -void sxgbe_disable_eee_mode(struct sxgbe_priv_data * const priv); -bool sxgbe_eee_init(struct sxgbe_priv_data * const priv); #endif /* __SXGBE_COMMON_H__ */ diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_ethtool.c b/drivers/net/ethernet/samsung/sxgbe/sxgbe_ethtool.c index 4a439b34114d..a13360962e47 100644 --- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_ethtool.c +++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_ethtool.c @@ -140,8 +140,6 @@ static int sxgbe_get_eee(struct net_device *dev, if (!priv->hw_cap.eee) return -EOPNOTSUPP; - edata->tx_lpi_timer = priv->tx_lpi_timer; - return phy_ethtool_get_eee(dev->phydev, edata); } @@ -150,22 +148,8 @@ static int sxgbe_set_eee(struct net_device *dev, { struct sxgbe_priv_data *priv = netdev_priv(dev); - priv->eee_enabled = edata->eee_enabled; - - if (!priv->eee_enabled) { - sxgbe_disable_eee_mode(priv); - } else { - /* We are asking for enabling the EEE but it is safe - * to verify all by invoking the eee_init function. - * In case of failure it will return an error. - */ - priv->eee_enabled = sxgbe_eee_init(priv); - if (!priv->eee_enabled) - return -EOPNOTSUPP; - - /* Do not change tx_lpi_timer in case of failure */ - priv->tx_lpi_timer = edata->tx_lpi_timer; - } + if (!priv->hw_cap.eee) + return -EOPNOTSUPP; return phy_ethtool_set_eee(dev->phydev, edata); } @@ -228,7 +212,7 @@ static void sxgbe_get_ethtool_stats(struct net_device *dev, int i; char *p; - if (priv->eee_enabled) { + if (dev->phydev->eee_active) { int val = phy_get_eee_err(dev->phydev); if (val) diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c index 12c8396b6942..8a385c92a6d1 100644 --- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c +++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c @@ -87,7 +87,7 @@ static void sxgbe_enable_eee_mode(const struct sxgbe_priv_data *priv) priv->hw->mac->set_eee_mode(priv->ioaddr); } -void sxgbe_disable_eee_mode(struct sxgbe_priv_data * const priv) +static void sxgbe_disable_eee_mode(struct sxgbe_priv_data * const priv) { /* Exit and disable EEE in case of we are in LPI state. */ priv->hw->mac->reset_eee_mode(priv->ioaddr); @@ -110,52 +110,25 @@ static void sxgbe_eee_ctrl_timer(struct timer_list *t) mod_timer(&priv->eee_ctrl_timer, SXGBE_LPI_TIMER(eee_timer)); } -/** - * sxgbe_eee_init - * @priv: private device pointer - * Description: - * If the EEE support has been enabled while configuring the driver, - * if the GMAC actually supports the EEE (from the HW cap reg) and the - * phy can also manage EEE, so enable the LPI state and start the timer - * to verify if the tx path can enter in LPI state. - */ -bool sxgbe_eee_init(struct sxgbe_priv_data * const priv) +static void sxgbe_eee_adjust(struct sxgbe_priv_data *priv) { - struct net_device *ndev = priv->dev; - bool ret = false; + struct phy_device *phydev = priv->dev->phydev; - /* MAC core supports the EEE feature. */ - if (priv->hw_cap.eee) { - /* Check if the PHY supports EEE */ - if (phy_init_eee(ndev->phydev, true)) - return false; + if (!priv->hw_cap.eee) + return; - timer_setup(&priv->eee_ctrl_timer, sxgbe_eee_ctrl_timer, 0); - priv->eee_ctrl_timer.expires = SXGBE_LPI_TIMER(eee_timer); + if (phydev->enable_tx_lpi) { add_timer(&priv->eee_ctrl_timer); - priv->hw->mac->set_eee_timer(priv->ioaddr, SXGBE_DEFAULT_LPI_TIMER, - priv->tx_lpi_timer); - - pr_info("Energy-Efficient Ethernet initialized\n"); - - ret = true; + phydev->eee_cfg.tx_lpi_timer); + priv->eee_enabled = true; + } else { + sxgbe_disable_eee_mode(priv); + priv->eee_enabled = false; } - return ret; -} - -static void sxgbe_eee_adjust(const struct sxgbe_priv_data *priv) -{ - struct net_device *ndev = priv->dev; - - /* When the EEE has been already initialised we have to - * modify the PLS bit in the LPI ctrl & status reg according - * to the PHY link status. For this reason. - */ - if (priv->eee_enabled) - priv->hw->mac->set_eee_pls(priv->ioaddr, ndev->phydev->link); + priv->hw->mac->set_eee_pls(priv->ioaddr, phydev->link); } /** @@ -301,6 +274,16 @@ static int sxgbe_init_phy(struct net_device *ndev) return -ENODEV; } + if (priv->hw_cap.eee) { + phy_support_eee(phydev); + phy_eee_rx_clock_stop(priv->dev->phydev, true); + phydev->eee_cfg.tx_lpi_timer = SXGBE_DEFAULT_LPI_TIMER; + + /* configure timer which will be used for LPI handling */ + timer_setup(&priv->eee_ctrl_timer, sxgbe_eee_ctrl_timer, 0); + priv->eee_ctrl_timer.expires = SXGBE_LPI_TIMER(eee_timer); + } + netdev_dbg(ndev, "%s: attached to PHY (UID 0x%x) Link = %d\n", __func__, phydev->phy_id, phydev->link); @@ -802,7 +785,7 @@ static void sxgbe_tx_all_clean(struct sxgbe_priv_data * const priv) sxgbe_tx_queue_clean(tqueue); } - if ((priv->eee_enabled) && (!priv->tx_path_in_lpi_mode)) { + if (priv->eee_enabled && !priv->tx_path_in_lpi_mode) { sxgbe_enable_eee_mode(priv); mod_timer(&priv->eee_ctrl_timer, SXGBE_LPI_TIMER(eee_timer)); } @@ -1179,9 +1162,6 @@ static int sxgbe_open(struct net_device *dev) priv->hw->dma->rx_watchdog(priv->ioaddr, SXGBE_MAX_DMA_RIWT); } - priv->tx_lpi_timer = SXGBE_DEFAULT_LPI_TIMER; - priv->eee_enabled = sxgbe_eee_init(priv); - napi_enable(&priv->napi); netif_start_queue(dev); @@ -1207,9 +1187,6 @@ static int sxgbe_release(struct net_device *dev) { struct sxgbe_priv_data *priv = netdev_priv(dev); - if (priv->eee_enabled) - del_timer_sync(&priv->eee_ctrl_timer); - /* Stop and disconnect the PHY */ if (dev->phydev) { phy_stop(dev->phydev);