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