netdev - Re: [PATCH v1 net-next] net: stmmac: Enable EEE HW LPI timer with auto SW/HW switching

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 12 Oct 2020 14:00:15 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Voon Weifeng <weifeng.voon@...el.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        Jose Abreu <joabreu@...opsys.com>,
        Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Andrew Lunn <andrew@...n.ch>,
        Alexandre Torgue <alexandre.torgue@...com>,
        Ong Boon Leong <boon.leong.ong@...el.com>,
        Seow Chen Yong <chen.yong.seow@...el.com>,
        Mark Gross <mgross@...ux.intel.com>,
        Wong Vee Khee <vee.khee.wong@...el.com>
Subject: Re: [PATCH v1 net-next] net: stmmac: Enable EEE HW LPI timer with
 auto SW/HW switching

On Fri,  9 Oct 2020 00:11:23 +0800 Voon Weifeng wrote:
> From: "Vineetha G. Jaya Kumaran" <vineetha.g.jaya.kumaran@...el.com>
> 
> This patch enables the HW LPI Timer which controls the automatic entry
> and exit of the LPI state.
> The EEE LPI timer value is configured through ethtool. The driver will
> auto select the LPI HW timer if the value in the HW timer supported range.
> Else, the driver will fallback to SW timer.
> 
> Signed-off-by: Vineetha G. Jaya Kumaran <vineetha.g.jaya.kumaran@...el.com>
> Signed-off-by: Voon Weifeng <weifeng.voon@...el.com>

minor nits, but the patch makes sense to me
please repost soon so it makes it into 5.10

> diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
> index df7de50497a0..f59c4a1c1674 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/common.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/common.h
> @@ -402,6 +402,9 @@ struct dma_features {
>  /* Default LPI timers */
>  #define STMMAC_DEFAULT_LIT_LS	0x3E8
>  #define STMMAC_DEFAULT_TWT_LS	0x1E
> +#define STMMAC_ET_MAX		0xFFFFF
> +#define LPI_ET_ENABLE		1
> +#define LPI_ET_DISABLE		0

Don't think you need defines for true / false values like that.
Just use literals.

>  
>  #define STMMAC_CHAIN_MODE	0x1
>  #define STMMAC_RING_MODE	0x2

> @@ -268,6 +269,7 @@ int stmmac_dvr_probe(struct device *device,
>  		     struct stmmac_resources *res);
>  void stmmac_disable_eee_mode(struct stmmac_priv *priv);
>  bool stmmac_eee_init(struct stmmac_priv *priv);
> +void stmmac_lpi_entry_timer_enable(struct stmmac_priv *priv, bool en);

This doesn't have to be declared in the header...

>  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);
>  
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 220626a8d499..908df3cb12cd 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -327,6 +327,11 @@ static void stmmac_enable_eee_mode(struct stmmac_priv *priv)
>   */
>  void stmmac_disable_eee_mode(struct stmmac_priv *priv)
>  {
> +	if (!priv->eee_sw_timer_en) {
> +		stmmac_lpi_entry_timer_enable(priv, LPI_ET_DISABLE);
> +		return;
> +	}
> +
>  	stmmac_reset_eee_mode(priv, priv->hw);
>  	del_timer_sync(&priv->eee_ctrl_timer);
>  	priv->tx_path_in_lpi_mode = false;
> @@ -347,6 +352,16 @@ static void stmmac_eee_ctrl_timer(struct timer_list *t)
>  	mod_timer(&priv->eee_ctrl_timer, STMMAC_LPI_T(priv->tx_lpi_timer));
>  }
>  
> +void stmmac_lpi_entry_timer_enable(struct stmmac_priv *priv, bool en)
> +{

Just move this function up in the file so it's defined before
stmmac_disable_eee_mode(). We prefer to limit forward declarations 
in the kernel.

Also the name of the function would probably be better as
..._timer_config() or such. I find it strange to call a function called
enable() to disable something.

> +	int 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;
> +	stmmac_set_eee_lpi_timer(priv, priv->hw, tx_lpi_timer);
> +}