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

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]

Message-Id: <20200928.123402.281706664300059159.davem@davemloft.net>
Date:   Mon, 28 Sep 2020 12:34:02 -0700 (PDT)
From:   David Miller <davem@...emloft.net>
To:     weifeng.voon@...el.com
Cc:     mcoquelin.stm32@...il.com, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, joabreu@...opsys.com,
        peppe.cavallaro@...com, andrew@...n.ch, alexandre.torgue@...com,
        boon.leong.ong@...el.com, chen.yong.seow@...el.com,
        mgross@...ux.intel.com, vee.khee.wong@...el.com
Subject: Re: [PATCH v1 net] net: stmmac: Modify configuration method of EEE
 timers

From: Voon Weifeng <weifeng.voon@...el.com>
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	1000000
> +#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.