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 <[email protected]>
>> Signed-off-by: Wojciech Drewek <[email protected]>
>> ---
>> 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;
>> }
