Re: [PATCH v1 net] net: stmmac: Modify configuration method of EEE timers

2020-09-28 Thread David Miller 
From: Voon Weifeng 
Date: Mon, 28 Sep 2020 18:02:41 +0800

> @@ -90,11 +90,12 @@ static const u32 default_msg_level = (NETIF_MSG_DRV | 
> NETIF_MSG_PROBE |
> NETIF_MSG_LINK | NETIF_MSG_IFUP |
> 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);
> -MODULE_PARM_DESC(eee_timer, "LPI tx expiration time in msec");
> -#define STMMAC_LPI_T(x) (jiffies + msecs_to_jiffies(x))
> +#define STMMAC_DEFAULT_LPI_TIMER 100
> +#define STMMAC_LPI_T(x)  (jiffies + usecs_to_jiffies(x))
> +
> +static int tw_timer = STMMAC_DEFAULT_TWT_LS;
> +module_param(tw_timer, int, 0644);
> +MODULE_PARM_DESC(tw_timer, "LPI TW timer value in msec");

This is a great example of one of the many reasons why we disallow
module parameters in networking drivers.

This is a user facing value, and now you changed the name which breaks
things for anyone who was accessing this module parameter previously.

You have to find a way to specify this value using existing kernel
infrastructure such as ethtool or devlink, and not using a module
parameter.

So please get rid of this module parameter, and add a way to set this
value portably.

[PATCH v1 net] net: stmmac: Modify configuration method of EEE timers

2020-09-28 Thread Voon Weifeng 
From: "Vineetha G. Jaya Kumaran" 

Ethtool manual stated that the tx-timer is the "the amount of time the
device should stay in idle mode prior to asserting its Tx LPI". The
previous implementation for "ethtool --set-eee tx-timer" sets the LPI TW
timer duration which is not correct.

Hence, this patch fixes the the configuration of LPI TW timer via
module parameters instead of ethtool. And, "ethtool --set-eee tx-timer"
should configure EEE LPI timer.

Fixes: d765955d2ae0 ("stmmac: add the Energy Efficient Ethernet support")
Signed-off-by: Vineetha G. Jaya Kumaran 
Signed-off-by: Voon Weifeng 
---
 drivers/net/ethernet/stmicro/stmmac/common.h  |  1 +
 drivers/net/ethernet/stmicro/stmmac/stmmac.h  |  1 +
 .../ethernet/stmicro/stmmac/stmmac_ethtool.c  | 12 ++-
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 35 +++
 4 files changed, 33 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h 
b/drivers/net/ethernet/stmicro/stmmac/common.h
index 127f75862962..7338babfebad 100644
--- a/drivers/net/ethernet/stmicro/stmmac/common.h
+++ b/drivers/net/ethernet/stmicro/stmmac/common.h
@@ -395,6 +395,7 @@ struct dma_features {
 /* Default LPI timers */
 #define STMMAC_DEFAULT_LIT_LS  0x3E8
 #define STMMAC_DEFAULT_TWT_LS  0x1E
+#define STMMAC_TWT_MAX 0x
 
 #define STMMAC_CHAIN_MODE  0x1
 #define STMMAC_RING_MODE   0x2
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h 
b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 9c02fc754bf1..d5a83c45510e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -203,6 +203,7 @@ struct stmmac_priv {
int eee_enabled;
int eee_active;
int tx_lpi_timer;
+   int tx_lpi_enabled;
unsigned int mode;
unsigned int chain_mode;
int extend_desc;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c 
b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index 430a4b32ec1e..348bb8a9cf7f 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
@@ -665,6 +665,7 @@ static int stmmac_ethtool_op_get_eee(struct net_device *dev,
edata->eee_enabled = priv->eee_enabled;
edata->eee_active = priv->eee_active;
edata->tx_lpi_timer = priv->tx_lpi_timer;
+   edata->tx_lpi_enabled = priv->tx_lpi_enabled;
 
return phylink_ethtool_get_eee(priv->phylink, edata);
 }
@@ -678,6 +679,10 @@ 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);
 
@@ -685,7 +690,12 @@ static int stmmac_ethtool_op_set_eee(struct net_device 
*dev,
if (ret)
return ret;
 
-   priv->tx_lpi_timer = edata->tx_lpi_timer;
+   if (edata->eee_enabled && priv->tx_lpi_enabled &&
+   priv->tx_lpi_timer != edata->tx_lpi_timer) {
+   priv->tx_lpi_timer = edata->tx_lpi_timer;
+   stmmac_eee_init(priv);
+   }
+
return 0;
 }
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c 
b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 89b2b3472852..2d1b9b76d678 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -90,11 +90,12 @@ static const u32 default_msg_level = (NETIF_MSG_DRV | 
NETIF_MSG_PROBE |
  NETIF_MSG_LINK | NETIF_MSG_IFUP |
  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);
-MODULE_PARM_DESC(eee_timer, "LPI tx expiration time in msec");
-#define STMMAC_LPI_T(x) (jiffies + msecs_to_jiffies(x))
+#define STMMAC_DEFAULT_LPI_TIMER   100
+#define STMMAC_LPI_T(x)(jiffies + usecs_to_jiffies(x))
+
+static int tw_timer = STMMAC_DEFAULT_TWT_LS;
+module_param(tw_timer, int, 0644);
+MODULE_PARM_DESC(tw_timer, "LPI TW timer value in msec");
 
 /* By default the driver will use the ring mode to manage tx and rx 
descriptors,
  * but allow user to force to use the chain instead of the ring
@@ -130,8 +131,8 @@ static void stmmac_verify_args(void)
flow_ctrl = FLOW_OFF;
if (unlikely((pause < 0) || (pause > 0x)))
pause = PAUSE_TIME;
-   if (eee_timer < 0)
-   eee_timer = STMMAC_DEFAULT_LPI_TIMER;
+   if (tw_timer < 0 || tw_timer > STMMAC_TWT_MAX)
+   tw_timer = STMMAC_DEFAULT_TWT_LS;
 }
 
 /**
@@ -370,7 +371,7 @@ static void stmmac_eee_ctrl_timer(struct timer_list *t)
struct stmmac_priv *priv =