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

[Wojciech Drewek <wojciech.drewek@intel.com>]

On 29.03.2024 23:29, Jakub Kicinski wrote:
> 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.

Makes sense, although ETHTOOL_A_MODULE_POWER_LIMIT does not say if
it's max or min limit. What about:
ETHTOOL_A_MODULE_POWER_MAX_LIMIT
ETHTOOL_A_MODULE_POWER_UPPER_LIMIT

> 
>> 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 <marcin.szycik@linux.intel.com>
>> Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>
>> ---
>>  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 ?

Makes sense

> 
>> 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 ?

Agree

> 
>> @@ -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

I'll change the name

> 
>>  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.

Yep, makes sense

> 
>> +		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.

Sure

> 
>> +			return -EINVAL;
> 
> ERANGE?

Agree

> 
>> +		} 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)?

Hmmm, I think we can allow to reset if the currently set limit is the default one.
Right now only the driver could catch such scenario because we don't have a parameter
that driver could use to inform the ethtool about the default value.
I hope that answers your question since I'm not 100% sure if that's what you asked about :)

> 
>> +	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?

Yes, that was my intention. Using both of those attributes at the same time is not allowed.

> 
> 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.

Hmmm, makes sense

> 
>> +		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;
>>  }