[PATCH net-next 00/17] net: stmmac: clean up and fix EEE implementation

[PATCH net-next 00/17] net: stmmac: clean up and fix EEE implementation

[Russell King (Oracle)]

Hi,

This is a rework of stmmac's EEE support in light of the addition of EEE
management to phylib. It's slightly more than 15 patches, but I think it
makes sense to be so.

Patch 1 adds configuration of the receive clock phy_eee_rx_clock_stop()
(which was part of another series, but is necessary for this patch set.)

Patch 2 converts stmmac to use phylib's tracking of tx_lpi_timer.

Patch 3 (new) corrects the data type used for things involving the LPI
timer. The user API uses u32, so stmmac should do too, rather than
blindly converting it to "int".

Patch 4 makes stmmac EEE state depend on phylib's enable_tx_lpi flag,
thus using phylib's resolution of EEE state.

Patch 5 removes redundant code from the ethtool EEE operations.

Patch 6 (new) removes some redundant code in stmmac_disable_eee_mode()
and renames it to stmmac_disable_sw_eee_mode() to better reflect its
purpose.

Patch 7 removes the driver private tx_lpi_enabled, which is managed by
phylib since patch 4.

Patch 8 removes the dependence of EEE error statistics on the EEE
enable state, instead depending on whether EEE is supported by the
hardware.

Patch 9 removes phy_init_eee(), instead using phy_eee_rx_clock_stop()
to configure whether the PHY may stop the receive clock.

Patch 10 removes priv->eee_tw_timer, which is only ever set to one
value at probe time, effectively it is a constant. Hence this is
unnecessary complexity.

Patch 11 moves priv->eee_enabled into stmmac_eee_init(), and placing
it under the protection of priv->lock, except when EEE is not
supported (where it becomes constant-false.)

Patch 12 moves priv->eee_active also into stmmac_eee_init(), so
the indication whether EEE should be enabled or not is passed in
to this function.

Since both priv->eee_enabled and priv->eee_active are assigned
true/false values, they should be typed "bool". Make it sew in
patch 13. No Singer machine required.

Patch 14 moves the initialisation of priv->eee_ctrl_timer to the
probe function - it makes no sense to re-initialise the timer each
time we want to start using it.

Patch 15 removes the unnecessary EEE handling in the driver tear-down
method. The core net code will have brought the interface down
already, meaning EEE has already been disabled.

Patch 16 reorganises the code to split the hardware LPI timer
control paths from the software LPI timer paths.

Patch 17 works on this further by eliminating
stmmac_lpi_entry_timer_config() and making direct calls to the new
functions. This reveals a potential bug where priv->eee_sw_timer_en
is set true when EEE is disabled. This is not addressed in this
series, but will be in a future separate patch - so that if fixing
that causes a regression, it can be handled separately.

 drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c  |   4 +-
 drivers/net/ethernet/stmicro/stmmac/hwif.h         |   2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac.h       |  10 +--
 .../net/ethernet/stmicro/stmmac/stmmac_ethtool.c   |  25 +-----
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  | 100 +++++++++++----------
 drivers/net/phy/phy.c                              |  27 ++++--
 include/linux/phy.h                                |   1 +
 7 files changed, 84 insertions(+), 85 deletions(-)

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

[PATCH net-next v2 01/17] net: phy: add configuration of rx clock stop mode

[Russell King (Oracle)]

Add a function to allow configuration of the PCS's clock stop enable
bit, used to configure whether the xMII receive clock can be stopped
during LPI mode.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phy.c | 27 ++++++++++++++++++++++-----
 include/linux/phy.h   |  1 +
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index e4b04cdaa995..a4b9fcc2503a 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1640,6 +1640,27 @@ void phy_mac_interrupt(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(phy_mac_interrupt);
 
+/**
+ * phy_eee_rx_clock_stop() - configure PHY receive clock in LPI
+ * @phydev: target phy_device struct
+ * @clk_stop_enable: flag to indicate whether the clock can be stopped
+ *
+ * Configure whether the PHY can disable its receive clock during LPI mode,
+ * See IEEE 802.3 sections 22.2.2.2, 35.2.2.10, and 45.2.3.1.4.
+ *
+ * Returns: 0 or negative error.
+ */
+int phy_eee_rx_clock_stop(struct phy_device *phydev, bool clk_stop_enable)
+{
+	/* Configure the PHY to stop receiving xMII
+	 * clock while it is signaling LPI.
+	 */
+	return phy_modify_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1,
+			      MDIO_PCS_CTRL1_CLKSTOP_EN,
+			      clk_stop_enable ? MDIO_PCS_CTRL1_CLKSTOP_EN : 0);
+}
+EXPORT_SYMBOL_GPL(phy_eee_rx_clock_stop);
+
 /**
  * phy_init_eee - init and check the EEE feature
  * @phydev: target phy_device struct
@@ -1664,11 +1685,7 @@ int phy_init_eee(struct phy_device *phydev, bool clk_stop_enable)
 		return -EPROTONOSUPPORT;
 
 	if (clk_stop_enable)
-		/* Configure the PHY to stop receiving xMII
-		 * clock while it is signaling LPI.
-		 */
-		ret = phy_set_bits_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1,
-				       MDIO_PCS_CTRL1_CLKSTOP_EN);
+		ret = phy_eee_rx_clock_stop(phydev, true);
 
 	return ret < 0 ? ret : 0;
 }
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 5bc71d59910c..4875465653ca 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -2096,6 +2096,7 @@ int phy_unregister_fixup(const char *bus_id, u32 phy_uid, u32 phy_uid_mask);
 int phy_unregister_fixup_for_id(const char *bus_id);
 int phy_unregister_fixup_for_uid(u32 phy_uid, u32 phy_uid_mask);
 
+int phy_eee_rx_clock_stop(struct phy_device *phydev, bool clk_stop_enable);
 int phy_init_eee(struct phy_device *phydev, bool clk_stop_enable);
 int phy_get_eee_err(struct phy_device *phydev);
 int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_keee *data);
-- 
2.30.2

[PATCH net-next v2 02/17] net: stmmac: move tx_lpi_timer tracking to phylib

[Russell King (Oracle)]

When stmmac_ethtool_op_get_eee() is called, stmmac sets the tx_lpi_timer
and tx_lpi_enabled members, and then calls into phylink and thus phylib.
phylib overwrites these members.

phylib will also cause a link down/link up transition when settings
that impact the MAC have been changed.

Convert stmmac to use the tx_lpi_timer setting in struct phy_device,
updating priv->tx_lpi_timer each time when the link comes up, rather
than trying to maintain this user setting itself. We initialise the
phylib tx_lpi_timer setting by doing a get_ee-modify-set_eee sequence
with the last known priv->tx_lpi_timer value. In order for this to work
correctly, we also need this member to be initialised earlier.

As stmmac_eee_init() is no longer called outside of stmmac_main.c, make
it static.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac.h  |  1 -
 .../ethernet/stmicro/stmmac/stmmac_ethtool.c  | 14 +------------
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 21 ++++++++++++++-----
 3 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index b8d631e559c0..df386fc948ec 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -406,7 +406,6 @@ int stmmac_dvr_probe(struct device *device,
 		     struct plat_stmmacenet_data *plat_dat,
 		     struct stmmac_resources *res);
 void stmmac_disable_eee_mode(struct stmmac_priv *priv);
-bool stmmac_eee_init(struct stmmac_priv *priv);
 int stmmac_reinit_queues(struct net_device *dev, u32 rx_cnt, u32 tx_cnt);
 int stmmac_reinit_ringparam(struct net_device *dev, u32 rx_size, u32 tx_size);
 int stmmac_bus_clks_config(struct stmmac_priv *priv, bool enabled);
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index 16b4d8c21c90..0429a99a8114 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
@@ -898,7 +898,6 @@ static int stmmac_ethtool_op_get_eee(struct net_device *dev,
 	if (!priv->dma_cap.eee)
 		return -EOPNOTSUPP;
 
-	edata->tx_lpi_timer = priv->tx_lpi_timer;
 	edata->tx_lpi_enabled = priv->tx_lpi_enabled;
 
 	return phylink_ethtool_get_eee(priv->phylink, edata);
@@ -908,7 +907,6 @@ static int stmmac_ethtool_op_set_eee(struct net_device *dev,
 				     struct ethtool_keee *edata)
 {
 	struct stmmac_priv *priv = netdev_priv(dev);
-	int ret;
 
 	if (!priv->dma_cap.eee)
 		return -EOPNOTSUPP;
@@ -920,17 +918,7 @@ static int stmmac_ethtool_op_set_eee(struct net_device *dev,
 	if (!edata->eee_enabled)
 		stmmac_disable_eee_mode(priv);
 
-	ret = phylink_ethtool_set_eee(priv->phylink, edata);
-	if (ret)
-		return ret;
-
-	if (edata->eee_enabled &&
-	    priv->tx_lpi_timer != edata->tx_lpi_timer) {
-		priv->tx_lpi_timer = edata->tx_lpi_timer;
-		stmmac_eee_init(priv);
-	}
-
-	return 0;
+	return phylink_ethtool_set_eee(priv->phylink, edata);
 }
 
 static u32 stmmac_usec2riwt(u32 usec, struct stmmac_priv *priv)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 99eaec8bac4a..9a9169ca7cd2 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -469,7 +469,7 @@ static void stmmac_eee_ctrl_timer(struct timer_list *t)
  *  can also manage EEE, this function enable the LPI state and start related
  *  timer.
  */
-bool stmmac_eee_init(struct stmmac_priv *priv)
+static bool stmmac_eee_init(struct stmmac_priv *priv)
 {
 	int eee_tw_timer = priv->eee_tw_timer;
 
@@ -1088,6 +1088,7 @@ static void stmmac_mac_link_up(struct phylink_config *config,
 		priv->eee_active =
 			phy_init_eee(phy, !(priv->plat->flags &
 				STMMAC_FLAG_RX_CLK_RUNS_IN_LPI)) >= 0;
+		priv->tx_lpi_timer = phy->eee_cfg.tx_lpi_timer;
 		priv->eee_enabled = stmmac_eee_init(priv);
 		priv->tx_lpi_enabled = priv->eee_enabled;
 		stmmac_set_eee_pls(priv, priv->hw, true);
@@ -1187,6 +1188,16 @@ static int stmmac_init_phy(struct net_device *dev)
 		ret = phylink_fwnode_phy_connect(priv->phylink, fwnode, 0);
 	}
 
+	if (ret == 0) {
+		struct ethtool_keee eee;
+
+		/* Configure phylib's copy of the LPI timer */
+		if (phylink_ethtool_get_eee(priv->phylink, &eee)) {
+			eee.tx_lpi_timer = priv->tx_lpi_timer;
+			phylink_ethtool_set_eee(priv->phylink, &eee);
+		}
+	}
+
 	if (!priv->plat->pmt) {
 		struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };
 
@@ -3439,10 +3450,6 @@ static int stmmac_hw_setup(struct net_device *dev, bool ptp_register)
 
 	priv->eee_tw_timer = STMMAC_DEFAULT_TWT_LS;
 
-	/* Convert the timer from msec to usec */
-	if (!priv->tx_lpi_timer)
-		priv->tx_lpi_timer = eee_timer * 1000;
-
 	if (priv->use_riwt) {
 		u32 queue;
 
@@ -3909,6 +3916,10 @@ static int __stmmac_open(struct net_device *dev,
 	u32 chan;
 	int ret;
 
+	/* Initialise the tx lpi timer, converting from msec to usec */
+	if (!priv->tx_lpi_timer)
+		priv->tx_lpi_timer = eee_timer * 1000;
+
 	ret = pm_runtime_resume_and_get(priv->device);
 	if (ret < 0)
 		return ret;
-- 
2.30.2

[PATCH net-next v2 03/17] net: stmmac: use correct type for tx_lpi_timer

[Russell King (Oracle)]

The ethtool interface uses u32 for tx_lpi_timer, and so does phylib.
Use u32 to store this internally within stmmac rather than "int"
which could misinterpret large values.

Since eee_timer is used to initialise priv->tx_lpi_timer, this also
should be unsigned to avoid a negative number being interpreted as a
very large positive number.

Also correct "value" in dwmac4_set_eee_lpi_entry_timer() to use u32
rather than int, which is derived from tx_lpi_timer, even though
masking with STMMAC_ET_MAX will truncate the sign bits. u32 is the
value argument type for writel().

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c | 4 ++--
 drivers/net/ethernet/stmicro/stmmac/hwif.h        | 2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac.h      | 2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 ++++----
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
index c36f90a782c5..9ed8620580a8 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
@@ -420,10 +420,10 @@ static void dwmac4_set_eee_pls(struct mac_device_info *hw, int link)
 	writel(value, ioaddr + GMAC4_LPI_CTRL_STATUS);
 }
 
-static void dwmac4_set_eee_lpi_entry_timer(struct mac_device_info *hw, int et)
+static void dwmac4_set_eee_lpi_entry_timer(struct mac_device_info *hw, u32 et)
 {
 	void __iomem *ioaddr = hw->pcsr;
-	int value = et & STMMAC_ET_MAX;
+	u32 value = et & STMMAC_ET_MAX;
 	int regval;
 
 	/* Program LPI entry timer value into register */
diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
index 2f7295b6c1c5..0f200b72c225 100644
--- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
+++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
@@ -363,7 +363,7 @@ struct stmmac_ops {
 	void (*set_eee_mode)(struct mac_device_info *hw,
 			     bool en_tx_lpi_clockgating);
 	void (*reset_eee_mode)(struct mac_device_info *hw);
-	void (*set_eee_lpi_entry_timer)(struct mac_device_info *hw, int et);
+	void (*set_eee_lpi_entry_timer)(struct mac_device_info *hw, u32 et);
 	void (*set_eee_timer)(struct mac_device_info *hw, int ls, int tw);
 	void (*set_eee_pls)(struct mac_device_info *hw, int link);
 	void (*debug)(struct stmmac_priv *priv, void __iomem *ioaddr,
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index df386fc948ec..984e708d019f 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -307,7 +307,7 @@ struct stmmac_priv {
 	int lpi_irq;
 	int eee_enabled;
 	int eee_active;
-	int tx_lpi_timer;
+	u32 tx_lpi_timer;
 	int tx_lpi_enabled;
 	int eee_tw_timer;
 	bool eee_sw_timer_en;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 9a9169ca7cd2..b0ef439b715b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -111,8 +111,8 @@ static const u32 default_msg_level = (NETIF_MSG_DRV | NETIF_MSG_PROBE |
 				      NETIF_MSG_IFDOWN | NETIF_MSG_TIMER);
 
 #define STMMAC_DEFAULT_LPI_TIMER	1000
-static int eee_timer = STMMAC_DEFAULT_LPI_TIMER;
-module_param(eee_timer, int, 0644);
+static unsigned int eee_timer = STMMAC_DEFAULT_LPI_TIMER;
+module_param(eee_timer, uint, 0644);
 MODULE_PARM_DESC(eee_timer, "LPI tx expiration time in msec");
 #define STMMAC_LPI_T(x) (jiffies + usecs_to_jiffies(x))
 
@@ -394,11 +394,11 @@ static inline u32 stmmac_rx_dirty(struct stmmac_priv *priv, u32 queue)
 
 static void stmmac_lpi_entry_timer_config(struct stmmac_priv *priv, bool en)
 {
-	int tx_lpi_timer;
+	u32 tx_lpi_timer;
 
 	/* Clear/set the SW EEE timer flag based on LPI ET enablement */
 	priv->eee_sw_timer_en = en ? 0 : 1;
-	tx_lpi_timer  = en ? priv->tx_lpi_timer : 0;
+	tx_lpi_timer = en ? priv->tx_lpi_timer : 0;
 	stmmac_set_eee_lpi_timer(priv, priv->hw, tx_lpi_timer);
 }
 
-- 
2.30.2

[PATCH net-next v2 04/17] net: stmmac: make EEE depend on phy->enable_tx_lpi

[Russell King (Oracle)]

Make stmmac EEE depend on phylib's evaluation of user settings and PHY
negotiation, as indicated by phy->enable_tx_lpi. This will ensure when
phylib has evaluated that the user has disabled LPI, phy_init_eee()
will not be called, and priv->eee_active will be false, causing LPI/EEE
to be disabled.

This is an interim measure - phy_init_eee() will be removed in a later
patch.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index b0ef439b715b..dbee2de08583 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1085,7 +1085,7 @@ static void stmmac_mac_link_up(struct phylink_config *config,
 
 	stmmac_mac_set(priv, priv->ioaddr, true);
 	if (phy && priv->dma_cap.eee) {
-		priv->eee_active =
+		priv->eee_active = phy->enable_tx_lpi &&
 			phy_init_eee(phy, !(priv->plat->flags &
 				STMMAC_FLAG_RX_CLK_RUNS_IN_LPI)) >= 0;
 		priv->tx_lpi_timer = phy->eee_cfg.tx_lpi_timer;
-- 
2.30.2

[PATCH net-next v2 05/17] net: stmmac: remove redundant code from ethtool EEE ops

[Russell King (Oracle)]

Setting edata->tx_lpi_enabled in stmmac_ethtool_op_get_eee() gets
overwritten by phylib, so there's no point setting this.

In stmmac_ethtool_op_set_eee(), now that stmmac is using the result of
phylib's evaluation of EEE, there is no need to handle anything in the
ethtool EEE ops other than calling through to the appropriate phylink
function, which will pass on to phylib the users request.

As stmmac_disable_eee_mode() is now no longer called from outside
stmmac_main.c, make it static.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac.h         | 1 -
 drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 9 ---------
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c    | 2 +-
 3 files changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 984e708d019f..2eee3c5c4d1e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -405,7 +405,6 @@ void stmmac_dvr_remove(struct device *dev);
 int stmmac_dvr_probe(struct device *device,
 		     struct plat_stmmacenet_data *plat_dat,
 		     struct stmmac_resources *res);
-void stmmac_disable_eee_mode(struct stmmac_priv *priv);
 int stmmac_reinit_queues(struct net_device *dev, u32 rx_cnt, u32 tx_cnt);
 int stmmac_reinit_ringparam(struct net_device *dev, u32 rx_size, u32 tx_size);
 int stmmac_bus_clks_config(struct stmmac_priv *priv, bool enabled);
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index 0429a99a8114..693f59c3c47a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
@@ -898,8 +898,6 @@ static int stmmac_ethtool_op_get_eee(struct net_device *dev,
 	if (!priv->dma_cap.eee)
 		return -EOPNOTSUPP;
 
-	edata->tx_lpi_enabled = priv->tx_lpi_enabled;
-
 	return phylink_ethtool_get_eee(priv->phylink, edata);
 }
 
@@ -911,13 +909,6 @@ static int stmmac_ethtool_op_set_eee(struct net_device *dev,
 	if (!priv->dma_cap.eee)
 		return -EOPNOTSUPP;
 
-	if (priv->tx_lpi_enabled != edata->tx_lpi_enabled)
-		netdev_warn(priv->dev,
-			    "Setting EEE tx-lpi is not supported\n");
-
-	if (!edata->eee_enabled)
-		stmmac_disable_eee_mode(priv);
-
 	return phylink_ethtool_set_eee(priv->phylink, edata);
 }
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index dbee2de08583..f895bdd75678 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -434,7 +434,7 @@ static int stmmac_enable_eee_mode(struct stmmac_priv *priv)
  * Description: this function is to exit and disable EEE in case of
  * LPI state is true. This is called by the xmit.
  */
-void stmmac_disable_eee_mode(struct stmmac_priv *priv)
+static void stmmac_disable_eee_mode(struct stmmac_priv *priv)
 {
 	if (!priv->eee_sw_timer_en) {
 		stmmac_lpi_entry_timer_config(priv, 0);
-- 
2.30.2

[PATCH net-next v2 06/17] net: stmmac: clean up stmmac_disable_eee_mode()

[Russell King (Oracle)]

stmmac_disable_eee_mode() is now only called from stmmac_xmit() when
both priv->tx_path_in_lpi_mode and priv->eee_sw_timer_en are true.
Therefore:

	if (!priv->eee_sw_timer_en)

in stmmac_disable_eee_mode() will never be true, so this is dead code.
Remove it, and rename the function to indicate that it now only deals
with software based EEE mode.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index f895bdd75678..47c57a558e5b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -429,18 +429,13 @@ static int stmmac_enable_eee_mode(struct stmmac_priv *priv)
 }
 
 /**
- * stmmac_disable_eee_mode - disable and exit from LPI mode
+ * stmmac_disable_sw_eee_mode - disable and exit from LPI mode
  * @priv: driver private structure
  * Description: this function is to exit and disable EEE in case of
  * LPI state is true. This is called by the xmit.
  */
-static void stmmac_disable_eee_mode(struct stmmac_priv *priv)
+static void stmmac_disable_sw_eee_mode(struct stmmac_priv *priv)
 {
-	if (!priv->eee_sw_timer_en) {
-		stmmac_lpi_entry_timer_config(priv, 0);
-		return;
-	}
-
 	stmmac_reset_eee_mode(priv, priv->hw);
 	del_timer_sync(&priv->eee_ctrl_timer);
 	priv->tx_path_in_lpi_mode = false;
@@ -4490,7 +4485,7 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 	first_tx = tx_q->cur_tx;
 
 	if (priv->tx_path_in_lpi_mode && priv->eee_sw_timer_en)
-		stmmac_disable_eee_mode(priv);
+		stmmac_disable_sw_eee_mode(priv);
 
 	/* Manage oversized TCP frames for GMAC4 device */
 	if (skb_is_gso(skb) && priv->tso) {
-- 
2.30.2

[PATCH net-next v2 07/17] net: stmmac: remove priv->tx_lpi_enabled

[Russell King (Oracle)]

Through using phylib's EEE state, priv->tx_lpi_enabled has become a
write-only variable. Remove it.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac.h      | 1 -
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 --
 2 files changed, 3 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 2eee3c5c4d1e..507b6ac14289 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -308,7 +308,6 @@ struct stmmac_priv {
 	int eee_enabled;
 	int eee_active;
 	u32 tx_lpi_timer;
-	int tx_lpi_enabled;
 	int eee_tw_timer;
 	bool eee_sw_timer_en;
 	unsigned int mode;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 47c57a558e5b..ba6de7b7d572 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -970,7 +970,6 @@ static void stmmac_mac_link_down(struct phylink_config *config,
 
 	stmmac_mac_set(priv, priv->ioaddr, false);
 	priv->eee_active = false;
-	priv->tx_lpi_enabled = false;
 	priv->eee_enabled = stmmac_eee_init(priv);
 	stmmac_set_eee_pls(priv, priv->hw, false);
 
@@ -1085,7 +1084,6 @@ static void stmmac_mac_link_up(struct phylink_config *config,
 				STMMAC_FLAG_RX_CLK_RUNS_IN_LPI)) >= 0;
 		priv->tx_lpi_timer = phy->eee_cfg.tx_lpi_timer;
 		priv->eee_enabled = stmmac_eee_init(priv);
-		priv->tx_lpi_enabled = priv->eee_enabled;
 		stmmac_set_eee_pls(priv, priv->hw, true);
 	}
 
-- 
2.30.2

[PATCH net-next v2 08/17] net: stmmac: report EEE error statistics if EEE is supported

[Russell King (Oracle)]

Report the number of EEE error statistics in the xstats even when EEE
is not enabled in hardware, but is supported. The PHY maintains this
counter even when EEE is not enabled.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index 693f59c3c47a..918a32f8fda8 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
@@ -654,7 +654,7 @@ static void stmmac_get_ethtool_stats(struct net_device *dev,
 					     (*(u32 *)p);
 			}
 		}
-		if (priv->eee_enabled) {
+		if (priv->dma_cap.eee) {
 			int val = phylink_get_eee_err(priv->phylink);
 			if (val)
 				priv->xstats.phy_eee_wakeup_error_n = val;
-- 
2.30.2

[PATCH net-next v2 09/17] net: stmmac: convert to use phy_eee_rx_clock_stop()

[Russell King (Oracle)]

Convert stmmac to use phy_eee_rx_clock_stop() to set the PHY receive
clock stop in LPI setting, rather than calling the legacy
phy_init_eee() function.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index ba6de7b7d572..6b66a25716b0 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1079,10 +1079,10 @@ static void stmmac_mac_link_up(struct phylink_config *config,
 
 	stmmac_mac_set(priv, priv->ioaddr, true);
 	if (phy && priv->dma_cap.eee) {
-		priv->eee_active = phy->enable_tx_lpi &&
-			phy_init_eee(phy, !(priv->plat->flags &
-				STMMAC_FLAG_RX_CLK_RUNS_IN_LPI)) >= 0;
+		phy_eee_rx_clock_stop(phy, !(priv->plat->flags &
+					     STMMAC_FLAG_RX_CLK_RUNS_IN_LPI));
 		priv->tx_lpi_timer = phy->eee_cfg.tx_lpi_timer;
+		priv->eee_active = phy->enable_tx_lpi;
 		priv->eee_enabled = stmmac_eee_init(priv);
 		stmmac_set_eee_pls(priv, priv->hw, true);
 	}
-- 
2.30.2

[PATCH net-next v2 10/17] net: stmmac: remove priv->eee_tw_timer

[Russell King (Oracle)]

priv->eee_tw_timer is only assigned during initialisation to a
constant value (STMMAC_DEFAULT_TWT_LS) and then never changed.

Remove priv->eee_tw_timer, and instead use STMMAC_DEFAULT_TWT_LS
for both uses in stmmac_eee_init().

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac.h      | 1 -
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 9 +++------
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 507b6ac14289..1556804cca38 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -308,7 +308,6 @@ struct stmmac_priv {
 	int eee_enabled;
 	int eee_active;
 	u32 tx_lpi_timer;
-	int eee_tw_timer;
 	bool eee_sw_timer_en;
 	unsigned int mode;
 	unsigned int chain_mode;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 6b66a25716b0..7cce2eb3d82e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -466,8 +466,6 @@ static void stmmac_eee_ctrl_timer(struct timer_list *t)
  */
 static bool stmmac_eee_init(struct stmmac_priv *priv)
 {
-	int eee_tw_timer = priv->eee_tw_timer;
-
 	/* Check if MAC core supports the EEE feature. */
 	if (!priv->dma_cap.eee)
 		return false;
@@ -480,7 +478,8 @@ static bool stmmac_eee_init(struct stmmac_priv *priv)
 			netdev_dbg(priv->dev, "disable EEE\n");
 			stmmac_lpi_entry_timer_config(priv, 0);
 			del_timer_sync(&priv->eee_ctrl_timer);
-			stmmac_set_eee_timer(priv, priv->hw, 0, eee_tw_timer);
+			stmmac_set_eee_timer(priv, priv->hw, 0,
+					     STMMAC_DEFAULT_TWT_LS);
 			if (priv->hw->xpcs)
 				xpcs_config_eee(priv->hw->xpcs,
 						priv->plat->mult_fact_100ns,
@@ -493,7 +492,7 @@ static bool stmmac_eee_init(struct stmmac_priv *priv)
 	if (priv->eee_active && !priv->eee_enabled) {
 		timer_setup(&priv->eee_ctrl_timer, stmmac_eee_ctrl_timer, 0);
 		stmmac_set_eee_timer(priv, priv->hw, STMMAC_DEFAULT_LIT_LS,
-				     eee_tw_timer);
+				     STMMAC_DEFAULT_TWT_LS);
 		if (priv->hw->xpcs)
 			xpcs_config_eee(priv->hw->xpcs,
 					priv->plat->mult_fact_100ns,
@@ -3441,8 +3440,6 @@ static int stmmac_hw_setup(struct net_device *dev, bool ptp_register)
 	else if (ptp_register)
 		stmmac_ptp_register(priv);
 
-	priv->eee_tw_timer = STMMAC_DEFAULT_TWT_LS;
-
 	if (priv->use_riwt) {
 		u32 queue;
 
-- 
2.30.2

[PATCH net-next v2 11/17] net: stmmac: move priv->eee_enabled into stmmac_eee_init()

[Russell King (Oracle)]

All call sites for stmmac_eee_init() assign the return code to
priv->eee_enabled. Rather than having this coded at each call site,
move the assignment inside stmmac_eee_init().

Since stmmac_init_eee() takes priv->lock before checking the state of
priv->eee_enabled, move the assignment within the locked region. Also,
stmmac_suspend() checks the state of this member under the lock. While
two concurrent calls to stmmac_init_eee() aren't possible, there is
a possibility that stmmac_suspend() may run concurrently with a change
of priv->eee_enabled unless we modify it under the lock.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c  | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 7cce2eb3d82e..cf294fe3f726 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -464,11 +464,13 @@ static void stmmac_eee_ctrl_timer(struct timer_list *t)
  *  can also manage EEE, this function enable the LPI state and start related
  *  timer.
  */
-static bool stmmac_eee_init(struct stmmac_priv *priv)
+static void stmmac_eee_init(struct stmmac_priv *priv)
 {
 	/* Check if MAC core supports the EEE feature. */
-	if (!priv->dma_cap.eee)
-		return false;
+	if (!priv->dma_cap.eee) {
+		priv->eee_enabled = false;
+		return;
+	}
 
 	mutex_lock(&priv->lock);
 
@@ -485,8 +487,9 @@ static bool stmmac_eee_init(struct stmmac_priv *priv)
 						priv->plat->mult_fact_100ns,
 						false);
 		}
+		priv->eee_enabled = false;
 		mutex_unlock(&priv->lock);
-		return false;
+		return;
 	}
 
 	if (priv->eee_active && !priv->eee_enabled) {
@@ -509,9 +512,10 @@ static bool stmmac_eee_init(struct stmmac_priv *priv)
 			  STMMAC_LPI_T(priv->tx_lpi_timer));
 	}
 
+	priv->eee_enabled = true;
+
 	mutex_unlock(&priv->lock);
 	netdev_dbg(priv->dev, "Energy-Efficient Ethernet initialized\n");
-	return true;
 }
 
 /* stmmac_get_tx_hwtstamp - get HW TX timestamps
@@ -969,7 +973,7 @@ static void stmmac_mac_link_down(struct phylink_config *config,
 
 	stmmac_mac_set(priv, priv->ioaddr, false);
 	priv->eee_active = false;
-	priv->eee_enabled = stmmac_eee_init(priv);
+	stmmac_eee_init(priv);
 	stmmac_set_eee_pls(priv, priv->hw, false);
 
 	if (stmmac_fpe_supported(priv))
@@ -1082,7 +1086,7 @@ static void stmmac_mac_link_up(struct phylink_config *config,
 					     STMMAC_FLAG_RX_CLK_RUNS_IN_LPI));
 		priv->tx_lpi_timer = phy->eee_cfg.tx_lpi_timer;
 		priv->eee_active = phy->enable_tx_lpi;
-		priv->eee_enabled = stmmac_eee_init(priv);
+		stmmac_eee_init(priv);
 		stmmac_set_eee_pls(priv, priv->hw, true);
 	}
 
-- 
2.30.2

[PATCH net-next v2 12/17] net: stmmac: move priv->eee_active into stmmac_eee_init()

[Russell King (Oracle)]

Since all call sites of stmmac_eee_init() assign priv->eee_active
immediately before, pass this state into stmmac_eee_init() and
assign priv->eee_active within this function.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index cf294fe3f726..1aedb49944ec 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -464,8 +464,10 @@ static void stmmac_eee_ctrl_timer(struct timer_list *t)
  *  can also manage EEE, this function enable the LPI state and start related
  *  timer.
  */
-static void stmmac_eee_init(struct stmmac_priv *priv)
+static void stmmac_eee_init(struct stmmac_priv *priv, bool active)
 {
+	priv->eee_active = active;
+
 	/* Check if MAC core supports the EEE feature. */
 	if (!priv->dma_cap.eee) {
 		priv->eee_enabled = false;
@@ -972,8 +974,7 @@ static void stmmac_mac_link_down(struct phylink_config *config,
 	struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev));
 
 	stmmac_mac_set(priv, priv->ioaddr, false);
-	priv->eee_active = false;
-	stmmac_eee_init(priv);
+	stmmac_eee_init(priv, false);
 	stmmac_set_eee_pls(priv, priv->hw, false);
 
 	if (stmmac_fpe_supported(priv))
@@ -1085,8 +1086,7 @@ static void stmmac_mac_link_up(struct phylink_config *config,
 		phy_eee_rx_clock_stop(phy, !(priv->plat->flags &
 					     STMMAC_FLAG_RX_CLK_RUNS_IN_LPI));
 		priv->tx_lpi_timer = phy->eee_cfg.tx_lpi_timer;
-		priv->eee_active = phy->enable_tx_lpi;
-		stmmac_eee_init(priv);
+		stmmac_eee_init(priv, phy->enable_tx_lpi);
 		stmmac_set_eee_pls(priv, priv->hw, true);
 	}
 
-- 
2.30.2

[PATCH net-next v2 13/17] net: stmmac: use boolean for eee_enabled and eee_active

[Russell King (Oracle)]

priv->eee_enabled and priv->eee_active are both assigned using boolean
values. Type them as bool rather than int.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 1556804cca38..25ad3f92e14d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -305,9 +305,9 @@ struct stmmac_priv {
 	int clk_csr;
 	struct timer_list eee_ctrl_timer;
 	int lpi_irq;
-	int eee_enabled;
-	int eee_active;
 	u32 tx_lpi_timer;
+	bool eee_enabled;
+	bool eee_active;
 	bool eee_sw_timer_en;
 	unsigned int mode;
 	unsigned int chain_mode;
-- 
2.30.2

[PATCH net-next v2 14/17] net: stmmac: move setup of eee_ctrl_timer to stmmac_dvr_probe()

[Russell King (Oracle)]

Move the initialisation of the EEE software timer to the probe function
as it is unnecessary to do this each time we enable software LPI.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 1aedb49944ec..aac45c16d7b5 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -495,7 +495,6 @@ static void stmmac_eee_init(struct stmmac_priv *priv, bool active)
 	}
 
 	if (priv->eee_active && !priv->eee_enabled) {
-		timer_setup(&priv->eee_ctrl_timer, stmmac_eee_ctrl_timer, 0);
 		stmmac_set_eee_timer(priv, priv->hw, STMMAC_DEFAULT_LIT_LS,
 				     STMMAC_DEFAULT_TWT_LS);
 		if (priv->hw->xpcs)
@@ -7399,6 +7398,8 @@ int stmmac_dvr_probe(struct device *device,
 
 	INIT_WORK(&priv->service_task, stmmac_service_task);
 
+	timer_setup(&priv->eee_ctrl_timer, stmmac_eee_ctrl_timer, 0);
+
 	/* Override with kernel parameters if supplied XXX CRS XXX
 	 * this needs to have multiple instances
 	 */
-- 
2.30.2

[PATCH net-next v2 15/17] net: stmmac: remove unnecessary EEE handling in stmmac_release()

[Russell King (Oracle)]

phylink_stop() will cause phylink to call the mac_link_down() operation
before phylink_stop() returns. As mac_link_down() will call
stmmac_eee_init(false), this will set both priv->eee_active and
priv->eee_enabled to be false, deleting the eee_ctrl_timer if
priv->eee_enabled was previously set.

As stmmac_release() calls phylink_stop() before checking whether
priv->eee_enabled is true, this is a condition that can never be
satisfied, and thus the code within this if() block will never be
executed. Remove it.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index aac45c16d7b5..7ba3c7a8f535 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -4027,11 +4027,6 @@ static int stmmac_release(struct net_device *dev)
 	/* Free the IRQ lines */
 	stmmac_free_irq(dev, REQ_IRQ_ERR_ALL, 0);
 
-	if (priv->eee_enabled) {
-		priv->tx_path_in_lpi_mode = false;
-		del_timer_sync(&priv->eee_ctrl_timer);
-	}
-
 	/* Stop TX/RX DMA and clear the descriptors */
 	stmmac_stop_all_dma(priv);
 
-- 
2.30.2

[PATCH net-next v2 16/17] net: stmmac: split hardware LPI timer control

[Russell King (Oracle)]

Provide stmmac_disable_hw_lpi_timer() and stmmac_enable_hw_lpi_timer()
to control the hardware transmit LPI timer.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c  | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 7ba3c7a8f535..8d4b9c42aac0 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -392,14 +392,24 @@ static inline u32 stmmac_rx_dirty(struct stmmac_priv *priv, u32 queue)
 	return dirty;
 }
 
-static void stmmac_lpi_entry_timer_config(struct stmmac_priv *priv, bool en)
+static void stmmac_disable_hw_lpi_timer(struct stmmac_priv *priv)
+{
+	stmmac_set_eee_lpi_timer(priv, priv->hw, 0);
+}
+
+static void stmmac_enable_hw_lpi_timer(struct stmmac_priv *priv)
 {
-	u32 tx_lpi_timer;
+	stmmac_set_eee_lpi_timer(priv, priv->hw, priv->tx_lpi_timer);
+}
 
+static void stmmac_lpi_entry_timer_config(struct stmmac_priv *priv, bool en)
+{
 	/* Clear/set the SW EEE timer flag based on LPI ET enablement */
 	priv->eee_sw_timer_en = en ? 0 : 1;
-	tx_lpi_timer = en ? priv->tx_lpi_timer : 0;
-	stmmac_set_eee_lpi_timer(priv, priv->hw, tx_lpi_timer);
+	if (en)
+		stmmac_enable_hw_lpi_timer(priv);
+	else
+		stmmac_disable_hw_lpi_timer(priv);
 }
 
 /**
-- 
2.30.2

[PATCH net-next v2 17/17] net: stmmac: remove stmmac_lpi_entry_timer_config()

[Russell King (Oracle)]

Remove stmmac_lpi_entry_timer_config(), setting priv->eee_sw_timer_en
at the original call sites, and calling the appropriate
stmmac_xxx_hw_lpi_timer() function. No functional change.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 21 +++++++------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 8d4b9c42aac0..3b600967cb65 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -402,16 +402,6 @@ static void stmmac_enable_hw_lpi_timer(struct stmmac_priv *priv)
 	stmmac_set_eee_lpi_timer(priv, priv->hw, priv->tx_lpi_timer);
 }
 
-static void stmmac_lpi_entry_timer_config(struct stmmac_priv *priv, bool en)
-{
-	/* Clear/set the SW EEE timer flag based on LPI ET enablement */
-	priv->eee_sw_timer_en = en ? 0 : 1;
-	if (en)
-		stmmac_enable_hw_lpi_timer(priv);
-	else
-		stmmac_disable_hw_lpi_timer(priv);
-}
-
 /**
  * stmmac_enable_eee_mode - check and enter in LPI mode
  * @priv: driver private structure
@@ -490,7 +480,8 @@ static void stmmac_eee_init(struct stmmac_priv *priv, bool active)
 	if (!priv->eee_active) {
 		if (priv->eee_enabled) {
 			netdev_dbg(priv->dev, "disable EEE\n");
-			stmmac_lpi_entry_timer_config(priv, 0);
+			priv->eee_sw_timer_en = true;
+			stmmac_disable_hw_lpi_timer(priv);
 			del_timer_sync(&priv->eee_ctrl_timer);
 			stmmac_set_eee_timer(priv, priv->hw, 0,
 					     STMMAC_DEFAULT_TWT_LS);
@@ -514,11 +505,15 @@ static void stmmac_eee_init(struct stmmac_priv *priv, bool active)
 	}
 
 	if (priv->plat->has_gmac4 && priv->tx_lpi_timer <= STMMAC_ET_MAX) {
+		/* Use hardware LPI mode */
 		del_timer_sync(&priv->eee_ctrl_timer);
 		priv->tx_path_in_lpi_mode = false;
-		stmmac_lpi_entry_timer_config(priv, 1);
+		priv->eee_sw_timer_en = false;
+		stmmac_enable_hw_lpi_timer(priv);
 	} else {
-		stmmac_lpi_entry_timer_config(priv, 0);
+		/* Use software LPI mode */
+		priv->eee_sw_timer_en = true;
+		stmmac_disable_hw_lpi_timer(priv);
 		mod_timer(&priv->eee_ctrl_timer,
 			  STMMAC_LPI_T(priv->tx_lpi_timer));
 	}
-- 
2.30.2

Re: [PATCH net-next v2 02/17] net: stmmac: move tx_lpi_timer tracking to phylib

[Andrew Lunn]

On Mon, Jan 06, 2025 at 12:24:53PM +0000, Russell King (Oracle) wrote:
> When stmmac_ethtool_op_get_eee() is called, stmmac sets the tx_lpi_timer
> and tx_lpi_enabled members, and then calls into phylink and thus phylib.
> phylib overwrites these members.
> 
> phylib will also cause a link down/link up transition when settings
> that impact the MAC have been changed.
> 
> Convert stmmac to use the tx_lpi_timer setting in struct phy_device,
> updating priv->tx_lpi_timer each time when the link comes up, rather
> than trying to maintain this user setting itself. We initialise the
> phylib tx_lpi_timer setting by doing a get_ee-modify-set_eee sequence
> with the last known priv->tx_lpi_timer value. In order for this to work
> correctly, we also need this member to be initialised earlier.
> 
> As stmmac_eee_init() is no longer called outside of stmmac_main.c, make
> it static.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v2 03/17] net: stmmac: use correct type for tx_lpi_timer

[Andrew Lunn]

On Mon, Jan 06, 2025 at 12:24:58PM +0000, Russell King (Oracle) wrote:
> The ethtool interface uses u32 for tx_lpi_timer, and so does phylib.
> Use u32 to store this internally within stmmac rather than "int"
> which could misinterpret large values.
> 
> Since eee_timer is used to initialise priv->tx_lpi_timer, this also
> should be unsigned to avoid a negative number being interpreted as a
> very large positive number.
> 
> Also correct "value" in dwmac4_set_eee_lpi_entry_timer() to use u32
> rather than int, which is derived from tx_lpi_timer, even though
> masking with STMMAC_ET_MAX will truncate the sign bits. u32 is the
> value argument type for writel().
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v2 04/17] net: stmmac: make EEE depend on phy->enable_tx_lpi

[Andrew Lunn]

On Mon, Jan 06, 2025 at 12:25:04PM +0000, Russell King (Oracle) wrote:
> Make stmmac EEE depend on phylib's evaluation of user settings and PHY
> negotiation, as indicated by phy->enable_tx_lpi. This will ensure when
> phylib has evaluated that the user has disabled LPI, phy_init_eee()
> will not be called, and priv->eee_active will be false, causing LPI/EEE
> to be disabled.
> 
> This is an interim measure - phy_init_eee() will be removed in a later
> patch.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v2 05/17] net: stmmac: remove redundant code from ethtool EEE ops

[Andrew Lunn]

On Mon, Jan 06, 2025 at 12:25:09PM +0000, Russell King (Oracle) wrote:
> Setting edata->tx_lpi_enabled in stmmac_ethtool_op_get_eee() gets
> overwritten by phylib, so there's no point setting this.
> 
> In stmmac_ethtool_op_set_eee(), now that stmmac is using the result of
> phylib's evaluation of EEE, there is no need to handle anything in the
> ethtool EEE ops other than calling through to the appropriate phylink
> function, which will pass on to phylib the users request.
> 
> As stmmac_disable_eee_mode() is now no longer called from outside
> stmmac_main.c, make it static.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v2 06/17] net: stmmac: clean up stmmac_disable_eee_mode()

[Andrew Lunn]

On Mon, Jan 06, 2025 at 12:25:14PM +0000, Russell King (Oracle) wrote:
> stmmac_disable_eee_mode() is now only called from stmmac_xmit() when
> both priv->tx_path_in_lpi_mode and priv->eee_sw_timer_en are true.
> Therefore:
> 
> 	if (!priv->eee_sw_timer_en)
> 
> in stmmac_disable_eee_mode() will never be true, so this is dead code.
> Remove it, and rename the function to indicate that it now only deals
> with software based EEE mode.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v2 07/17] net: stmmac: remove priv->tx_lpi_enabled

[Andrew Lunn]

On Mon, Jan 06, 2025 at 12:25:19PM +0000, Russell King (Oracle) wrote:
> Through using phylib's EEE state, priv->tx_lpi_enabled has become a
> write-only variable. Remove it.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v2 08/17] net: stmmac: report EEE error statistics if EEE is supported

[Andrew Lunn]

On Mon, Jan 06, 2025 at 12:25:24PM +0000, Russell King (Oracle) wrote:
> Report the number of EEE error statistics in the xstats even when EEE
> is not enabled in hardware, but is supported. The PHY maintains this
> counter even when EEE is not enabled.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v2 09/17] net: stmmac: convert to use phy_eee_rx_clock_stop()

[Andrew Lunn]

On Mon, Jan 06, 2025 at 12:25:29PM +0000, Russell King (Oracle) wrote:
> Convert stmmac to use phy_eee_rx_clock_stop() to set the PHY receive
> clock stop in LPI setting, rather than calling the legacy
> phy_init_eee() function.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v2 10/17] net: stmmac: remove priv->eee_tw_timer

[Andrew Lunn]

On Mon, Jan 06, 2025 at 12:25:34PM +0000, Russell King (Oracle) wrote:
> priv->eee_tw_timer is only assigned during initialisation to a
> constant value (STMMAC_DEFAULT_TWT_LS) and then never changed.
> 
> Remove priv->eee_tw_timer, and instead use STMMAC_DEFAULT_TWT_LS
> for both uses in stmmac_eee_init().
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v2 11/17] net: stmmac: move priv->eee_enabled into stmmac_eee_init()

[Andrew Lunn]

On Mon, Jan 06, 2025 at 12:25:39PM +0000, Russell King (Oracle) wrote:
> All call sites for stmmac_eee_init() assign the return code to
> priv->eee_enabled. Rather than having this coded at each call site,
> move the assignment inside stmmac_eee_init().
> 
> Since stmmac_init_eee() takes priv->lock before checking the state of
> priv->eee_enabled, move the assignment within the locked region. Also,
> stmmac_suspend() checks the state of this member under the lock. While
> two concurrent calls to stmmac_init_eee() aren't possible, there is
> a possibility that stmmac_suspend() may run concurrently with a change
> of priv->eee_enabled unless we modify it under the lock.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v2 12/17] net: stmmac: move priv->eee_active into stmmac_eee_init()

[Andrew Lunn]

On Mon, Jan 06, 2025 at 12:25:45PM +0000, Russell King (Oracle) wrote:
> Since all call sites of stmmac_eee_init() assign priv->eee_active
> immediately before, pass this state into stmmac_eee_init() and
> assign priv->eee_active within this function.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v2 13/17] net: stmmac: use boolean for eee_enabled and eee_active

[Andrew Lunn]

On Mon, Jan 06, 2025 at 12:25:50PM +0000, Russell King (Oracle) wrote:
> priv->eee_enabled and priv->eee_active are both assigned using boolean
> values. Type them as bool rather than int.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v2 14/17] net: stmmac: move setup of eee_ctrl_timer to stmmac_dvr_probe()

[Andrew Lunn]

On Mon, Jan 06, 2025 at 12:25:55PM +0000, Russell King (Oracle) wrote:
> Move the initialisation of the EEE software timer to the probe function
> as it is unnecessary to do this each time we enable software LPI.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v2 15/17] net: stmmac: remove unnecessary EEE handling in stmmac_release()

[Andrew Lunn]

On Mon, Jan 06, 2025 at 12:26:00PM +0000, Russell King (Oracle) wrote:
> phylink_stop() will cause phylink to call the mac_link_down() operation
> before phylink_stop() returns. As mac_link_down() will call
> stmmac_eee_init(false), this will set both priv->eee_active and
> priv->eee_enabled to be false, deleting the eee_ctrl_timer if
> priv->eee_enabled was previously set.
> 
> As stmmac_release() calls phylink_stop() before checking whether
> priv->eee_enabled is true, this is a condition that can never be
> satisfied, and thus the code within this if() block will never be
> executed. Remove it.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v2 16/17] net: stmmac: split hardware LPI timer control

[Andrew Lunn]

On Mon, Jan 06, 2025 at 12:26:05PM +0000, Russell King (Oracle) wrote:
> Provide stmmac_disable_hw_lpi_timer() and stmmac_enable_hw_lpi_timer()
> to control the hardware transmit LPI timer.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v2 17/17] net: stmmac: remove stmmac_lpi_entry_timer_config()

[Andrew Lunn]

On Mon, Jan 06, 2025 at 12:26:10PM +0000, Russell King (Oracle) wrote:
> Remove stmmac_lpi_entry_timer_config(), setting priv->eee_sw_timer_en
> at the original call sites, and calling the appropriate
> stmmac_xxx_hw_lpi_timer() function. No functional change.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v2 12/17] net: stmmac: move priv->eee_active into stmmac_eee_init()

[kernel test robot]

Hi Russell,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Russell-King-Oracle/net-stmmac-move-tx_lpi_timer-tracking-to-phylib/20250107-002808
base:   net-next/main
patch link:    https://lore.kernel.org/r/E1tUmAz-007VXn-0o%40rmk-PC.armlinux.org.uk
patch subject: [PATCH net-next v2 12/17] net: stmmac: move priv->eee_active into stmmac_eee_init()
config: i386-buildonly-randconfig-005-20250107 (https://download.01.org/0day-ci/archive/20250107/202501071547.L5CjLObQ-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250107/202501071547.L5CjLObQ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202501071547.L5CjLObQ-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:468: warning: Function parameter or struct member 'active' not described in 'stmmac_eee_init'


vim +468 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

d765955d2ae0b8 Giuseppe CAVALLARO       2012-06-27  458  
d765955d2ae0b8 Giuseppe CAVALLARO       2012-06-27  459  /**
732fdf0e5253e9 Giuseppe CAVALLARO       2014-11-18  460   * stmmac_eee_init - init EEE
32ceabcad3c8ab Giuseppe CAVALLARO       2013-04-08  461   * @priv: driver private structure
d765955d2ae0b8 Giuseppe CAVALLARO       2012-06-27  462   * Description:
732fdf0e5253e9 Giuseppe CAVALLARO       2014-11-18  463   *  if the GMAC supports the EEE (from the HW cap reg) and the phy device
732fdf0e5253e9 Giuseppe CAVALLARO       2014-11-18  464   *  can also manage EEE, this function enable the LPI state and start related
732fdf0e5253e9 Giuseppe CAVALLARO       2014-11-18  465   *  timer.
d765955d2ae0b8 Giuseppe CAVALLARO       2012-06-27  466   */
5ad24fd233fa83 Russell King (Oracle     2025-01-06  467) static void stmmac_eee_init(struct stmmac_priv *priv, bool active)
d765955d2ae0b8 Giuseppe CAVALLARO       2012-06-27 @468  {
5ad24fd233fa83 Russell King (Oracle     2025-01-06  469) 	priv->eee_active = active;
5ad24fd233fa83 Russell King (Oracle     2025-01-06  470) 
74371272f97fd1 Jose Abreu               2019-06-11  471  	/* Check if MAC core supports the EEE feature. */
418ee895284762 Russell King (Oracle     2025-01-06  472) 	if (!priv->dma_cap.eee) {
418ee895284762 Russell King (Oracle     2025-01-06  473) 		priv->eee_enabled = false;
418ee895284762 Russell King (Oracle     2025-01-06  474) 		return;
418ee895284762 Russell King (Oracle     2025-01-06  475) 	}
83bf79b6bb64e6 Giuseppe CAVALLARO       2014-03-10  476  
29555fa3de8656 Thierry Reding           2018-05-24  477  	mutex_lock(&priv->lock);
74371272f97fd1 Jose Abreu               2019-06-11  478  
74371272f97fd1 Jose Abreu               2019-06-11  479  	/* Check if it needs to be deactivated */
177d935a13703e Jon Hunter               2019-06-26  480  	if (!priv->eee_active) {
177d935a13703e Jon Hunter               2019-06-26  481  		if (priv->eee_enabled) {
38ddc59d65b6d9 LABBE Corentin           2016-11-16  482  			netdev_dbg(priv->dev, "disable EEE\n");
be1c7eae8c7dfc Vineetha G. Jaya Kumaran 2020-10-28  483  			stmmac_lpi_entry_timer_config(priv, 0);
83bf79b6bb64e6 Giuseppe CAVALLARO       2014-03-10  484  			del_timer_sync(&priv->eee_ctrl_timer);
26dbf37afd5d4b Russell King (Oracle     2025-01-06  485) 			stmmac_set_eee_timer(priv, priv->hw, 0,
26dbf37afd5d4b Russell King (Oracle     2025-01-06  486) 					     STMMAC_DEFAULT_TWT_LS);
d4aeaed80b0ebb Wong Vee Khee            2021-10-05  487  			if (priv->hw->xpcs)
d4aeaed80b0ebb Wong Vee Khee            2021-10-05  488  				xpcs_config_eee(priv->hw->xpcs,
d4aeaed80b0ebb Wong Vee Khee            2021-10-05  489  						priv->plat->mult_fact_100ns,
d4aeaed80b0ebb Wong Vee Khee            2021-10-05  490  						false);
177d935a13703e Jon Hunter               2019-06-26  491  		}
418ee895284762 Russell King (Oracle     2025-01-06  492) 		priv->eee_enabled = false;
0867bb9768deda Jon Hunter               2019-06-26  493  		mutex_unlock(&priv->lock);
418ee895284762 Russell King (Oracle     2025-01-06  494) 		return;
83bf79b6bb64e6 Giuseppe CAVALLARO       2014-03-10  495  	}
d765955d2ae0b8 Giuseppe CAVALLARO       2012-06-27  496  
74371272f97fd1 Jose Abreu               2019-06-11  497  	if (priv->eee_active && !priv->eee_enabled) {
74371272f97fd1 Jose Abreu               2019-06-11  498  		timer_setup(&priv->eee_ctrl_timer, stmmac_eee_ctrl_timer, 0);
74371272f97fd1 Jose Abreu               2019-06-11  499  		stmmac_set_eee_timer(priv, priv->hw, STMMAC_DEFAULT_LIT_LS,
26dbf37afd5d4b Russell King (Oracle     2025-01-06  500) 				     STMMAC_DEFAULT_TWT_LS);
656ed8b015f19b Wong Vee Khee            2021-09-30  501  		if (priv->hw->xpcs)
656ed8b015f19b Wong Vee Khee            2021-09-30  502  			xpcs_config_eee(priv->hw->xpcs,
656ed8b015f19b Wong Vee Khee            2021-09-30  503  					priv->plat->mult_fact_100ns,
656ed8b015f19b Wong Vee Khee            2021-09-30  504  					true);
71965352eedd0c Giuseppe CAVALLARO       2014-08-28  505  	}
d765955d2ae0b8 Giuseppe CAVALLARO       2012-06-27  506  
be1c7eae8c7dfc Vineetha G. Jaya Kumaran 2020-10-28  507  	if (priv->plat->has_gmac4 && priv->tx_lpi_timer <= STMMAC_ET_MAX) {
be1c7eae8c7dfc Vineetha G. Jaya Kumaran 2020-10-28  508  		del_timer_sync(&priv->eee_ctrl_timer);
be1c7eae8c7dfc Vineetha G. Jaya Kumaran 2020-10-28  509  		priv->tx_path_in_lpi_mode = false;
be1c7eae8c7dfc Vineetha G. Jaya Kumaran 2020-10-28  510  		stmmac_lpi_entry_timer_config(priv, 1);
be1c7eae8c7dfc Vineetha G. Jaya Kumaran 2020-10-28  511  	} else {
be1c7eae8c7dfc Vineetha G. Jaya Kumaran 2020-10-28  512  		stmmac_lpi_entry_timer_config(priv, 0);
be1c7eae8c7dfc Vineetha G. Jaya Kumaran 2020-10-28  513  		mod_timer(&priv->eee_ctrl_timer,
be1c7eae8c7dfc Vineetha G. Jaya Kumaran 2020-10-28  514  			  STMMAC_LPI_T(priv->tx_lpi_timer));
be1c7eae8c7dfc Vineetha G. Jaya Kumaran 2020-10-28  515  	}
388e201d41fa1e Vineetha G. Jaya Kumaran 2020-10-01  516  
418ee895284762 Russell King (Oracle     2025-01-06  517) 	priv->eee_enabled = true;
418ee895284762 Russell King (Oracle     2025-01-06  518) 
29555fa3de8656 Thierry Reding           2018-05-24  519  	mutex_unlock(&priv->lock);
38ddc59d65b6d9 LABBE Corentin           2016-11-16  520  	netdev_dbg(priv->dev, "Energy-Efficient Ethernet initialized\n");
d765955d2ae0b8 Giuseppe CAVALLARO       2012-06-27  521  }
d765955d2ae0b8 Giuseppe CAVALLARO       2012-06-27  522  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next v2 03/17] net: stmmac: use correct type for tx_lpi_timer

[Simon Horman]

On Mon, Jan 06, 2025 at 12:24:58PM +0000, Russell King (Oracle) wrote:
> The ethtool interface uses u32 for tx_lpi_timer, and so does phylib.
> Use u32 to store this internally within stmmac rather than "int"
> which could misinterpret large values.
> 
> Since eee_timer is used to initialise priv->tx_lpi_timer, this also
> should be unsigned to avoid a negative number being interpreted as a
> very large positive number.
> 
> Also correct "value" in dwmac4_set_eee_lpi_entry_timer() to use u32
> rather than int, which is derived from tx_lpi_timer, even though
> masking with STMMAC_ET_MAX will truncate the sign bits. u32 is the
> value argument type for writel().
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

...

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 9a9169ca7cd2..b0ef439b715b 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -111,8 +111,8 @@ static const u32 default_msg_level = (NETIF_MSG_DRV | NETIF_MSG_PROBE |
>  				      NETIF_MSG_IFDOWN | NETIF_MSG_TIMER);
>  
>  #define STMMAC_DEFAULT_LPI_TIMER	1000
> -static int eee_timer = STMMAC_DEFAULT_LPI_TIMER;
> -module_param(eee_timer, int, 0644);
> +static unsigned int eee_timer = STMMAC_DEFAULT_LPI_TIMER;
> +module_param(eee_timer, uint, 0644);
>  MODULE_PARM_DESC(eee_timer, "LPI tx expiration time in msec");
>  #define STMMAC_LPI_T(x) (jiffies + usecs_to_jiffies(x))
>  

Hi Russell,

now that eee_timer is unsigned the following check in stmmac_verify_args()
can never be true. I guess it should be removed.

        if (eee_timer < 0)
                eee_timer = STMMAC_DEFAULT_LPI_TIMER;

Flagged by Smatch.

...

Re: [PATCH net-next v2 03/17] net: stmmac: use correct type for tx_lpi_timer

[Russell King (Oracle)]

On Tue, Jan 07, 2025 at 11:28:51AM +0000, Simon Horman wrote:
> On Mon, Jan 06, 2025 at 12:24:58PM +0000, Russell King (Oracle) wrote:
> > The ethtool interface uses u32 for tx_lpi_timer, and so does phylib.
> > Use u32 to store this internally within stmmac rather than "int"
> > which could misinterpret large values.
> > 
> > Since eee_timer is used to initialise priv->tx_lpi_timer, this also
> > should be unsigned to avoid a negative number being interpreted as a
> > very large positive number.
> > 
> > Also correct "value" in dwmac4_set_eee_lpi_entry_timer() to use u32
> > rather than int, which is derived from tx_lpi_timer, even though
> > masking with STMMAC_ET_MAX will truncate the sign bits. u32 is the
> > value argument type for writel().
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> 
> ...
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 9a9169ca7cd2..b0ef439b715b 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -111,8 +111,8 @@ static const u32 default_msg_level = (NETIF_MSG_DRV | NETIF_MSG_PROBE |
> >  				      NETIF_MSG_IFDOWN | NETIF_MSG_TIMER);
> >  
> >  #define STMMAC_DEFAULT_LPI_TIMER	1000
> > -static int eee_timer = STMMAC_DEFAULT_LPI_TIMER;
> > -module_param(eee_timer, int, 0644);
> > +static unsigned int eee_timer = STMMAC_DEFAULT_LPI_TIMER;
> > +module_param(eee_timer, uint, 0644);
> >  MODULE_PARM_DESC(eee_timer, "LPI tx expiration time in msec");
> >  #define STMMAC_LPI_T(x) (jiffies + usecs_to_jiffies(x))
> >  
> 
> Hi Russell,
> 
> now that eee_timer is unsigned the following check in stmmac_verify_args()
> can never be true. I guess it should be removed.
> 
>         if (eee_timer < 0)
>                 eee_timer = STMMAC_DEFAULT_LPI_TIMER;
> 

Thanks for finding that. The parameter description doesn't seem to
detail whether this is intentional behaviour or not, or whether it is
"because someone used int and we shouldn't have negative values here".

I can't see why someone would pass a negative number for eee_timer
given that it already defaults to STMMAC_DEFAULT_LPI_TIMER.

So, I'm tempted to remove this.

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

Re: [PATCH net-next v2 03/17] net: stmmac: use correct type for tx_lpi_timer

[Simon Horman]

On Tue, Jan 07, 2025 at 11:57:12AM +0000, Russell King (Oracle) wrote:
> On Tue, Jan 07, 2025 at 11:28:51AM +0000, Simon Horman wrote:
> > On Mon, Jan 06, 2025 at 12:24:58PM +0000, Russell King (Oracle) wrote:
> > > The ethtool interface uses u32 for tx_lpi_timer, and so does phylib.
> > > Use u32 to store this internally within stmmac rather than "int"
> > > which could misinterpret large values.
> > > 
> > > Since eee_timer is used to initialise priv->tx_lpi_timer, this also
> > > should be unsigned to avoid a negative number being interpreted as a
> > > very large positive number.
> > > 
> > > Also correct "value" in dwmac4_set_eee_lpi_entry_timer() to use u32
> > > rather than int, which is derived from tx_lpi_timer, even though
> > > masking with STMMAC_ET_MAX will truncate the sign bits. u32 is the
> > > value argument type for writel().
> > > 
> > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > 
> > ...
> > 
> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > index 9a9169ca7cd2..b0ef439b715b 100644
> > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > @@ -111,8 +111,8 @@ static const u32 default_msg_level = (NETIF_MSG_DRV | NETIF_MSG_PROBE |
> > >  				      NETIF_MSG_IFDOWN | NETIF_MSG_TIMER);
> > >  
> > >  #define STMMAC_DEFAULT_LPI_TIMER	1000
> > > -static int eee_timer = STMMAC_DEFAULT_LPI_TIMER;
> > > -module_param(eee_timer, int, 0644);
> > > +static unsigned int eee_timer = STMMAC_DEFAULT_LPI_TIMER;
> > > +module_param(eee_timer, uint, 0644);
> > >  MODULE_PARM_DESC(eee_timer, "LPI tx expiration time in msec");
> > >  #define STMMAC_LPI_T(x) (jiffies + usecs_to_jiffies(x))
> > >  
> > 
> > Hi Russell,
> > 
> > now that eee_timer is unsigned the following check in stmmac_verify_args()
> > can never be true. I guess it should be removed.
> > 
> >         if (eee_timer < 0)
> >                 eee_timer = STMMAC_DEFAULT_LPI_TIMER;
> > 
> 
> Thanks for finding that. The parameter description doesn't seem to
> detail whether this is intentional behaviour or not, or whether it is
> "because someone used int and we shouldn't have negative values here".
> 
> I can't see why someone would pass a negative number for eee_timer
> given that it already defaults to STMMAC_DEFAULT_LPI_TIMER.
> 
> So, I'm tempted to remove this.

I'm not sure either. It did cross my mind that the check could be changed,
to disallow illegal values (if the range of legal values is not all
possible unsigned integer values). But it was just an idea without any
inspection of the code or thought about it's practicality. And my first
instinct was the same as yours: remove the check.

Re: [PATCH net-next v2 03/17] net: stmmac: use correct type for tx_lpi_timer

[Russell King (Oracle)]

On Tue, Jan 07, 2025 at 02:41:03PM +0000, Simon Horman wrote:
> On Tue, Jan 07, 2025 at 11:57:12AM +0000, Russell King (Oracle) wrote:
> > On Tue, Jan 07, 2025 at 11:28:51AM +0000, Simon Horman wrote:
> > > On Mon, Jan 06, 2025 at 12:24:58PM +0000, Russell King (Oracle) wrote:
> > > > The ethtool interface uses u32 for tx_lpi_timer, and so does phylib.
> > > > Use u32 to store this internally within stmmac rather than "int"
> > > > which could misinterpret large values.
> > > > 
> > > > Since eee_timer is used to initialise priv->tx_lpi_timer, this also
> > > > should be unsigned to avoid a negative number being interpreted as a
> > > > very large positive number.
> > > > 
> > > > Also correct "value" in dwmac4_set_eee_lpi_entry_timer() to use u32
> > > > rather than int, which is derived from tx_lpi_timer, even though
> > > > masking with STMMAC_ET_MAX will truncate the sign bits. u32 is the
> > > > value argument type for writel().
> > > > 
> > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > > 
> > > ...
> > > 
> > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > index 9a9169ca7cd2..b0ef439b715b 100644
> > > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > @@ -111,8 +111,8 @@ static const u32 default_msg_level = (NETIF_MSG_DRV | NETIF_MSG_PROBE |
> > > >  				      NETIF_MSG_IFDOWN | NETIF_MSG_TIMER);
> > > >  
> > > >  #define STMMAC_DEFAULT_LPI_TIMER	1000
> > > > -static int eee_timer = STMMAC_DEFAULT_LPI_TIMER;
> > > > -module_param(eee_timer, int, 0644);
> > > > +static unsigned int eee_timer = STMMAC_DEFAULT_LPI_TIMER;
> > > > +module_param(eee_timer, uint, 0644);
> > > >  MODULE_PARM_DESC(eee_timer, "LPI tx expiration time in msec");
> > > >  #define STMMAC_LPI_T(x) (jiffies + usecs_to_jiffies(x))
> > > >  
> > > 
> > > Hi Russell,
> > > 
> > > now that eee_timer is unsigned the following check in stmmac_verify_args()
> > > can never be true. I guess it should be removed.
> > > 
> > >         if (eee_timer < 0)
> > >                 eee_timer = STMMAC_DEFAULT_LPI_TIMER;
> > > 
> > 
> > Thanks for finding that. The parameter description doesn't seem to
> > detail whether this is intentional behaviour or not, or whether it is
> > "because someone used int and we shouldn't have negative values here".
> > 
> > I can't see why someone would pass a negative number for eee_timer
> > given that it already defaults to STMMAC_DEFAULT_LPI_TIMER.
> > 
> > So, I'm tempted to remove this.
> 
> I'm not sure either. It did cross my mind that the check could be changed,
> to disallow illegal values (if the range of legal values is not all
> possible unsigned integer values). But it was just an idea without any
> inspection of the code or thought about it's practicality. And my first
> instinct was the same as yours: remove the check.

My reasoning is as follows:

In the existing code with the module paramter is a signed int, then it
take a value up to INT_MAX. Provided sizeof(int) == sizeof(u32), then
this can be reported through the ethtool API. However, ethtool can set
the timer to U32_MAX which can exceed INT_MAX in this case. The driver
doesn't stop this, and uses a software based timer for any delay greater
than the capabilities of the hardware timer (if any.)

So, through ethtool one can set the LPI delay to anything between 0 and
U32_MAX, whereas through the module parameter it's between 0 and
INT_MAX. values between INT_MIN and -1 inclusive result in the default
being used.

It is, of course, absurd to have a negative delay, or even a delay
anywhere near INT_MAX or U32_MAX.

I'll separate out the change to eee_timer so if necessary, that can be
reverted without reverting the entire patch. Oh goodo, an extra patch
for a patchset which already exceeds netdev's 15 patches...

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

Re: [PATCH net-next v2 03/17] net: stmmac: use correct type for tx_lpi_timer

[Russell King (Oracle)]

On Mon, Jan 06, 2025 at 05:45:37PM +0100, Andrew Lunn wrote:
> On Mon, Jan 06, 2025 at 12:24:58PM +0000, Russell King (Oracle) wrote:
> > The ethtool interface uses u32 for tx_lpi_timer, and so does phylib.
> > Use u32 to store this internally within stmmac rather than "int"
> > which could misinterpret large values.
> > 
> > Since eee_timer is used to initialise priv->tx_lpi_timer, this also
> > should be unsigned to avoid a negative number being interpreted as a
> > very large positive number.
> > 
> > Also correct "value" in dwmac4_set_eee_lpi_entry_timer() to use u32
> > rather than int, which is derived from tx_lpi_timer, even though
> > masking with STMMAC_ET_MAX will truncate the sign bits. u32 is the
> > value argument type for writel().
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>

FYI, because of splitting this patch, I've dropped your r-b when
posting v3.

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

Re: [PATCH net-next v2 03/17] net: stmmac: use correct type for tx_lpi_timer

[Simon Horman]

On Tue, Jan 07, 2025 at 03:26:06PM +0000, Russell King (Oracle) wrote:
> On Tue, Jan 07, 2025 at 02:41:03PM +0000, Simon Horman wrote:
> > On Tue, Jan 07, 2025 at 11:57:12AM +0000, Russell King (Oracle) wrote:
> > > On Tue, Jan 07, 2025 at 11:28:51AM +0000, Simon Horman wrote:
> > > > On Mon, Jan 06, 2025 at 12:24:58PM +0000, Russell King (Oracle) wrote:
> > > > > The ethtool interface uses u32 for tx_lpi_timer, and so does phylib.
> > > > > Use u32 to store this internally within stmmac rather than "int"
> > > > > which could misinterpret large values.
> > > > > 
> > > > > Since eee_timer is used to initialise priv->tx_lpi_timer, this also
> > > > > should be unsigned to avoid a negative number being interpreted as a
> > > > > very large positive number.
> > > > > 
> > > > > Also correct "value" in dwmac4_set_eee_lpi_entry_timer() to use u32
> > > > > rather than int, which is derived from tx_lpi_timer, even though
> > > > > masking with STMMAC_ET_MAX will truncate the sign bits. u32 is the
> > > > > value argument type for writel().
> > > > > 
> > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > > > 
> > > > ...
> > > > 
> > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > > index 9a9169ca7cd2..b0ef439b715b 100644
> > > > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > > @@ -111,8 +111,8 @@ static const u32 default_msg_level = (NETIF_MSG_DRV | NETIF_MSG_PROBE |
> > > > >  				      NETIF_MSG_IFDOWN | NETIF_MSG_TIMER);
> > > > >  
> > > > >  #define STMMAC_DEFAULT_LPI_TIMER	1000
> > > > > -static int eee_timer = STMMAC_DEFAULT_LPI_TIMER;
> > > > > -module_param(eee_timer, int, 0644);
> > > > > +static unsigned int eee_timer = STMMAC_DEFAULT_LPI_TIMER;
> > > > > +module_param(eee_timer, uint, 0644);
> > > > >  MODULE_PARM_DESC(eee_timer, "LPI tx expiration time in msec");
> > > > >  #define STMMAC_LPI_T(x) (jiffies + usecs_to_jiffies(x))
> > > > >  
> > > > 
> > > > Hi Russell,
> > > > 
> > > > now that eee_timer is unsigned the following check in stmmac_verify_args()
> > > > can never be true. I guess it should be removed.
> > > > 
> > > >         if (eee_timer < 0)
> > > >                 eee_timer = STMMAC_DEFAULT_LPI_TIMER;
> > > > 
> > > 
> > > Thanks for finding that. The parameter description doesn't seem to
> > > detail whether this is intentional behaviour or not, or whether it is
> > > "because someone used int and we shouldn't have negative values here".
> > > 
> > > I can't see why someone would pass a negative number for eee_timer
> > > given that it already defaults to STMMAC_DEFAULT_LPI_TIMER.
> > > 
> > > So, I'm tempted to remove this.
> > 
> > I'm not sure either. It did cross my mind that the check could be changed,
> > to disallow illegal values (if the range of legal values is not all
> > possible unsigned integer values). But it was just an idea without any
> > inspection of the code or thought about it's practicality. And my first
> > instinct was the same as yours: remove the check.
> 
> My reasoning is as follows:
> 
> In the existing code with the module paramter is a signed int, then it
> take a value up to INT_MAX. Provided sizeof(int) == sizeof(u32), then
> this can be reported through the ethtool API. However, ethtool can set
> the timer to U32_MAX which can exceed INT_MAX in this case. The driver
> doesn't stop this, and uses a software based timer for any delay greater
> than the capabilities of the hardware timer (if any.)
> 
> So, through ethtool one can set the LPI delay to anything between 0 and
> U32_MAX, whereas through the module parameter it's between 0 and
> INT_MAX. values between INT_MIN and -1 inclusive result in the default
> being used.
> 
> It is, of course, absurd to have a negative delay, or even a delay
> anywhere near INT_MAX or U32_MAX.
> 
> I'll separate out the change to eee_timer so if necessary, that can be
> reverted without reverting the entire patch. Oh goodo, an extra patch
> for a patchset which already exceeds netdev's 15 patches...

Thanks Russell, FWIIW this sounds entirely reasonable to me.