Subject: Re: [PATCH net-next 2/3] ethtool: Introduce max power support

From: Jakub Kicinski <[email-masked]>
Date: Fri, 29 Mar 2024 15:29:54 -0700

On Fri, 29 Mar 2024 10:23:20 +0100 Wojciech Drewek wrote:
> Some modules use nonstandard power levels. Adjust ethtool
> module implementation to support new attributes that will allow user
> to change maximum power.
> 
> Add three new get attributes:
> ETHTOOL_A_MODULE_MAX_POWER_SET (used for set as well) - currently set
>   maximum power in the cage

1) I'd keep the ETHTOOL_A_MODULE_POWER_ prefix, consistently.

2) The _SET makes it sound like an action. Can we go with
   ETHTOOL_A_MODULE_POWER_MAX ? Or ETHTOOL_A_MODULE_POWER_LIMIT?
   Yes, ETHTOOL_A_MODULE_POWER_LIMIT
        ETHTOOL_A_MODULE_POWER_MAX
        ETHTOOL_A_MODULE_POWER_MIN
   would sound pretty good to me.

> ETHTOOL_A_MODULE_MIN_POWER_ALLOWED - minimum power allowed in the
>   cage reported by device
> ETHTOOL_A_MODULE_MAX_POWER_ALLOWED - maximum power allowed in the
>   cage reported by device
> 
> Add two new set attributes:
> ETHTOOL_A_MODULE_MAX_POWER_SET (used for get as well) - change
>   maximum power in the cage to the given value (milliwatts)
> ETHTOOL_A_MODULE_MAX_POWER_RESET - reset maximum power setting to the
>   default value
> 
> Reviewed-by: Marcin Szycik <[email-masked]>
> Signed-off-by: Wojciech Drewek <[email-masked]>
> ---
>  include/linux/ethtool.h              | 17 +++++--
>  include/uapi/linux/ethtool_netlink.h |  4 ++
>  net/ethtool/module.c                 | 74 ++++++++++++++++++++++++++--
>  net/ethtool/netlink.h                |  2 +-
>  4 files changed, 87 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index f3af6b31c9f1..74ed8997443a 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -510,10 +510,18 @@ struct ethtool_module_eeprom {
>   * @policy: The power mode policy enforced by the host for the plug-in module.
>   * @mode: The operational power mode of the plug-in module. Should be filled by
>   *	device drivers on get operations.
> + * @min_pwr_allowed: minimum power allowed in the cage reported by device
> + * @max_pwr_allowed: maximum power allowed in the cage reported by device
> + * @max_pwr_set: maximum power currently set in the cage
> + * @max_pwr_reset: restore default minimum power
>   */
>  struct ethtool_module_power_params {
>  	enum ethtool_module_power_mode_policy policy;
>  	enum ethtool_module_power_mode mode;
> +	u32 min_pwr_allowed;
> +	u32 max_pwr_allowed;
> +	u32 max_pwr_set;
> +	u8 max_pwr_reset;

bool ?

> diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
> index 3f89074aa06c..f7cd446b2a83 100644
> --- a/include/uapi/linux/ethtool_netlink.h
> +++ b/include/uapi/linux/ethtool_netlink.h
> @@ -882,6 +882,10 @@ enum {
>  	ETHTOOL_A_MODULE_HEADER,		/* nest - _A_HEADER_* */
>  	ETHTOOL_A_MODULE_POWER_MODE_POLICY,	/* u8 */
>  	ETHTOOL_A_MODULE_POWER_MODE,		/* u8 */
> +	ETHTOOL_A_MODULE_MAX_POWER_SET,		/* u32 */
> +	ETHTOOL_A_MODULE_MIN_POWER_ALLOWED,	/* u32 */
> +	ETHTOOL_A_MODULE_MAX_POWER_ALLOWED,	/* u32 */
> +	ETHTOOL_A_MODULE_MAX_POWER_RESET,	/* u8 */

flag ?

> @@ -77,6 +86,7 @@ static int module_fill_reply(struct sk_buff *skb,
>  			     const struct ethnl_reply_data *reply_base)
>  {
>  	const struct module_reply_data *data = MODULE_REPDATA(reply_base);
> +	u32 temp;

tmp ? temp sounds too much like temperature in context of power

>  static int
>  ethnl_set_module(struct ethnl_req_info *req_info, struct genl_info *info)
>  {
>  	struct ethtool_module_power_params power = {};
>  	struct ethtool_module_power_params power_new;
> -	const struct ethtool_ops *ops;
>  	struct net_device *dev = req_info->dev;
>  	struct nlattr **tb = info->attrs;
> +	const struct ethtool_ops *ops;
>  	int ret;
> +	bool mod;
>  
>  	ops = dev->ethtool_ops;
>  
> -	power_new.policy = nla_get_u8(tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY]);
>  	ret = ops->get_module_power_cfg(dev, &power, info->extack);
>  	if (ret < 0)
>  		return ret;
>  
> -	if (power_new.policy == power.policy)
> +	power_new.max_pwr_set = power.max_pwr_set;
> +	power_new.policy = power.policy;
> +
> +	ethnl_update_u32(&power_new.max_pwr_set,
> +			 tb[ETHTOOL_A_MODULE_MAX_POWER_SET], &mod);
> +	if (mod) {

I think we can use if (tb[ETHTOOL_A_MODULE_MAX_POWER_SET]) here
Less error prone for future additions.

> +		if (power_new.max_pwr_set > power.max_pwr_allowed) {
> +			NL_SET_ERR_MSG(info->extack, "Provided value is higher than maximum allowed");

NL_SET_ERR_MSG_ATTR() to point at the bad attribute.

> +			return -EINVAL;

ERANGE?

> +		} else if (power_new.max_pwr_set < power.min_pwr_allowed) {
> +			NL_SET_ERR_MSG(info->extack, "Provided value is lower than minimum allowed");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	ethnl_update_policy(&power_new.policy,
> +			    tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY], &mod);
> +	ethnl_update_u8(&power_new.max_pwr_reset,
> +			tb[ETHTOOL_A_MODULE_MAX_POWER_RESET], &mod);

I reckon reset should not be allowed if none of the max_pwr values 
are set (i.e. most likely driver doesn't support the config)?

> +	if (!mod)
>  		return 0;
>  
> +	if (power_new.max_pwr_reset && power_new.max_pwr_set) {

Mmm. How is that gonna work? The driver is going to set max_pwr_set
to what's currently configured. So the user is expected to send
ETHTOOL_A_MODULE_MAX_POWER_SET = 0
ETHTOOL_A_MODULE_MAX_POWER_RESET = 1
to reset?

Just:

	if (tb[ETHTOOL_A_MODULE_MAX_POWER_RESET] &&
	    tb[ETHTOOL_A_MODULE_MAX_POWER_SET])

And you can validate this before doing any real work.

> +		NL_SET_ERR_MSG(info->extack, "Maximum power set and reset cannot be used at the same time");
> +		return 0;
> +	}
> +
>  	ret = ops->set_module_power_cfg(dev, &power_new, info->extack);
>  	return ret < 0 ? ret : 1;
>  }
-- 
pw-bot: cr

---

From: Jakub Kicinski <[email-masked]>
Date: Fri, 29 Mar 2024 15:29:54 -0700

bool ?

flag ?

ERANGE?

Just:

-- 
pw-bot: cr

DKIM-Filter: OpenDKIM Filter v2.11.0 a.mx.secunet.com D1BDB208AC
Date: Fri, 29 Mar 2024 15:29:54 -0700
From: Jakub Kicinski <[email-masked]>
To: Wojciech Drewek <[email-masked]>
Cc: [email-masked], [email-masked],
 [email-masked], [email-masked], [email-masked],
 [email-masked], [email-masked], [email-masked],
 [email-masked]
Subject: Re: [PATCH net-next 2/3] ethtool: Introduce max power support
 +g1aHuH1cQ/QeAgQCXYEegrgI8XDQCauwLHUWvvDWABg1RqxNb/lfT
 giMPN8yjWrizroJ1XsP/U264BcYyzh/IX3578v58UORDYqsnxouY3b
 4v1+DftHhAiKY+QdfgF4wIN1h/Bu8JDDb25HJtLWgBMRPijB+J1m1t
 wCT1YjhNkAxbfaIbLn6Q9QBPzPqziLSQhvc98kVDn0MHaGoTeoHmYG
 81aP3zZjz6qRqxDewCEaCm/qw7Sgrt/nanuwrW1OR9eq9pN0CSGe3L
 qBCrI1dR0FO5UZ2uM1gZeE7ogfFWkGt6Owf6Ms/v1bHPxqSRs0+G5M
 nnYYK3otKZckzORTDoQ8mqJtTi6v1EpbUxx9STFIEYOv8E7gQvR6se
 HYFTWE6V8jE0PnCY4W2+DZd6/MiCBDcLrsoL5UG3dKJ8XyrPgy4Vel
 1HUb/aGRBOGKZphe3i9C7iEYm6+dmEm3VTUmUESPbF6xM0q3l0Btn3
 9rvVaPLU5NdvPPedZUKTyDdQy5cXVhc2gF99nFFrqPV8XSVEjEtsHp
 6fn61W8bbZd7HGjIB6xpZhji/UifucyMz60fHrvx+ekIqjs8PX3x09
 T+L6TSpbByPZSU9DJ8X7ut/VSTZhKPtI97tRH916p1ObG0/9Nq8quH
 CihcEz82pCJpfFNDBNl0a8IveSCpIa3Tnq5xcOtR8RxVh/nmpfuo7+
 wuHzpcxxA7yYja47HfzyObrJaVs4TH6lbECpIfOq1YB7XQ8vjXhkw9
 2G72/SdJXvPAhGXz7pruM6HatbjeUeJttXaNpMWN0IQXXMMMNH8v79
 ZBDVwPOq12vI790hphl9eu26jmPIoetdPqfbrLYdlruu/syjvwGFkq
 BpceUQeB/4kfscbwzgx4Q7Ln98wk+5KEhd9VUbr4okyzGhAOd/+zqQ
 G3Np+PPUgdwkU2jf5NC/DvxgHweh+27GVE4N8NqY6eZ8IiBGPYeOKa
 /dAWE4pIWFGYzI7kMadejaSj40nGuw0rDJzYlgNR8hXk1+gAOJpk5d
 UK3Tlj4Yanj0jR4YYvVUImBEs2oKD0YkYcvg39AyYKbNbhvGjQF2Dt
 Dr4mhfbjIJqMUPX/1hveXCbb9NDa0QaZHJiFw6JRbgD8fZbCpXTImi
 EMVUKSdETuSLopQXhZwoZ0WuIIpAsyAWYT4vijBfEAvwf0ksgyheLY
 sFoIQ/oMmKIswTWTEnKrAKk/CfyJYLKfEl6BFpIAiXkJEJgBHYS6Kc
 ESViz6e1hcAiUjwmYpDzcSgnJwpg2GJqJaTJsWTgJXtyZE+R5ZOKEr
 B/OmYHG5AYJj+PTObEAoiamlye4q1MzRSJEtxVYO1p0vh1fOMFUeY9
 FshjsGUgBs+TteX3pK+wA9/Lz3mxGHMymcrE4FIWIuNCcqEBUTPodb
 GI6QTGZPMUjnxKfIEDdhr6BGYqJAdoKMGKwPKJzjQguDMSm0qtpDFR
 l2HMSQWWLIsiLeUoyiXMAcrGVOoeEX/DM0RzD2lSJaJME3uJaDLl1D
 IPSuKDLBkDBGO9Ih1/RdWLYmkxVckLkRdL8dVS7DUFpoK4HMpPZcZj
 SgOw/0OyNi9WOBU5M2GGIzhKbMp2UdBJeydLtcPVNwpQhABlcrmlxS
 IXHdOUxb0MOi1LbinFQ5aNZM7y7ckWdILlmIzqNBtSVjIY3yxtrcCB
 ILK7vP3PyFQqirtZKisZzrAfwDM8/0lkPgt6QebYwruJr6FhS2jM2A
 yAr8VolCdzKQz31Hxy3KfISsnzYSZQEqajmZCFJI8AV158mk1BoaXo
 9cNoyYduWU5TGX4Wm7+XTi7wOxmSP4UbN8xP1fgdwKtsCjwvMqks72
 I8pmABJZTzCuEbA10aczJD6fpHnoH936Hah0kYl3HwIS8xLy9l6XWJ
 zwtNdocyCmpc5wanIvxBIfChQ9i+SEtFVpTRyIN6l+gVjcFzDQcIxR
 AwFIJmTOUk/E+aRBYQtZQLq3gBixfqdAlVo7VZKuexIl7KY9WgLnDm
 NC/nKtX7SoaQOVxdCTWC2BLLHx1bSenxAbMD5T3afnjUIoDTru+msU
 DyFLh75C6A1mxES5nMQ70V5k1liiKlhTDZeGYpah4xfsTOJ8m5JCF/
 YMunchUGy2ibKDNEM4zw6TCDhbMlFx4ouN+S+JK3M8f4RZopiG/SmB
 iZ0SlJ/ikVaSMModztFABGUuKDGGWBe4mRkSVxn7AXGqEvCJZ/Hxs4
 rGTDNyR/J4eW5PlIjTj/TzPyAQB5OYdARCUZt6pMNuSEZN6pqodyK4
 2k5fVJrWttdPRksMyzGZ32WcrnXAjgGvZpFTAKlj7KpwqUe8szjCkl
 +a3MLRzrZZTgcMeiEDmPOHxwHpUJH0bNDB9eWQLnyXTSRZHkirDbJH
 VwiKyMcnLKaR/PyNXSKMRF6tNGtUk49kliGvCpmtGHF0dw7HlmDP35
 +czC10mS4/InmOKdjnAA0zvPt4D/AsaXde/OJAAAAQrOAjw/eG1sIH
 ZlcnNpb249IjEuMCIgZW5jb2Rpbmc9InV0Zi0xNiI/Pg0KPEVtYWls
 U2V0Pg0KICA8VmVyc2lvbj4xNS4wLjAuMDwvVmVyc2lvbj4NCiAgPE
 VtYWlscz4NCiAgICA8RW1haWwgU3RhcnRJbmRleD0iMTE4MyI+DQog
 ICAgICA8RW1haWxTdHJpbmc+bWFyY2luLnN6eWNpa0BsaW51eC5pbn
 RlbC5jb208L0VtYWlsU3RyaW5nPg0KICAgIDwvRW1haWw+DQogICAg
 PEVtYWlsIFN0YXJ0SW5kZXg9IjEyNDkiPg0KICAgICAgPEVtYWlsU3
 RyaW5nPndvamNpZWNoLmRyZXdla0BpbnRlbC5jb208L0VtYWlsU3Ry
 aW5nPg0KICAgIDwvRW1haWw+DQogIDwvRW1haWxzPg0KPC9FbWFpbF
 NldD4BDIUHPD94bWwgdmVyc2lvbj0iMS4wIiBlbmNvZGluZz0idXRm
 LTE2Ij8+DQo8Q29udGFjdFNldD4NCiAgPFZlcnNpb24+MTUuMC4wLj
 A8L1ZlcnNpb24+DQogIDxDb250YWN0cz4NCiAgICA8Q29udGFjdCBT
 dGFydEluZGV4PSIxMTY4Ij4NCiAgICAgIDxQZXJzb24gU3RhcnRJbm
 RleD0iMTE2OCI+DQogICAgICAgIDxQZXJzb25TdHJpbmc+TWFyY2lu
 PC9QZXJzb25TdHJpbmc+DQogICAgICA8L1BlcnNvbj4NCiAgICAgID
 xFbWFpbHM+DQogICAgICAgIDxFbWFpbCBTdGFydEluZGV4PSIxMTgz
 Ij4NCiAgICAgICAgICA8RW1haWxTdHJpbmc+bWFyY2luLnN6eWNpa0
 BsaW51eC5pbnRlbC5jb208L0VtYWlsU3RyaW5nPg0KICAgICAgICA8
 L0VtYWlsPg0KICAgICAgPC9FbWFpbHM+DQogICAgICA8Q29udGFjdF
 N0cmluZz5NYXJjaW4gU3p5Y2lrICZsdDttYXJjaW4uc3p5Y2lrQGxp
 bnV4LmludGVsLmNvbTwvQ29udGFjdFN0cmluZz4NCiAgICA8L0Nvbn
 RhY3Q+DQogICAgPENvbnRhY3QgU3RhcnRJbmRleD0iMTIzMiI+DQog
 ICAgICA8UGVyc29uIFN0YXJ0SW5kZXg9IjEyMzIiPg0KICAgICAgIC
 A8UGVyc29uU3RyaW5nPldvamNpZWNoIERyZXdlazwvUGVyc29uU3Ry
 aW5nPg0KICAgICAgPC9QZXJzb24+DQogICAgICA8RW1haWxzPg0KIC
 AgICAgICA8RW1haWwgU3RhcnRJbmRleD0iMTI0OSI+DQogICAgICAg
 ICAgPEVtYWlsU3RyaW5nPndvamNpZWNoLmRyZXdla0BpbnRlbC5jb2
 08L0VtYWlsU3RyaW5nPg0KICAgICAgICA8L0VtYWlsPg0KICAgICAg
 PC9FbWFpbHM+DQogICAgICA8Q29udGFjdFN0cmluZz5Xb2pjaWVjaC
 BEcmV3ZWsgJmx0O3dvamNpZWNoLmRyZXdla0BpbnRlbC5jb208L0Nv
 bnRhY3RTdHJpbmc+DQogICAgPC9Db250YWN0Pg0KICA8L0NvbnRhY3
 RzPg0KPC9Db250YWN0U2V0PgEOzwFSZXRyaWV2ZXJPcGVyYXRvciwx
 MCwyO1JldHJpZXZlck9wZXJhdG9yLDExLDM7UG9zdERvY1BhcnNlck
 9wZXJhdG9yLDEwLDE7UG9zdERvY1BhcnNlck9wZXJhdG9yLDExLDA7
 UG9zdFdvcmRCcmVha2VyRGlhZ25vc3RpY09wZXJhdG9yLDEwLDc7UG
 9zdFdvcmRCcmVha2VyRGlhZ25vc3RpY09wZXJhdG9yLDExLDA7VHJh
 bnNwb3J0V3JpdGVyUHJvZHVjZXIsMjAsMjg=
X-MS-Exchange-Forest-IndexAgent: 1 4643
X-MS-Exchange-Forest-EmailMessageHash: 10B105DC
X-MS-Exchange-Forest-Language: en
X-MS-Exchange-Organization-Processed-By-Journaling: Journal Agent

bool ?

flag ?

ERANGE?

Just:

-- 
pw-bot: cr