Re: [PATCH net-next 1/6] ethtool: Add ability to control transceiver modules' power mode
From: Jakub Kicinski <kuba@kernel.org>
Date: 2021-10-05 14:15:14
On Tue, 5 Oct 2021 09:57:20 +0300 Ido Schimmel wrote:
quoted
quoted
+static int module_set_power_mode(struct net_device *dev, struct nlattr **tb, + bool *p_mod, struct netlink_ext_ack *extack) +{ + struct ethtool_module_power_mode_params power = {}; + struct ethtool_module_power_mode_params power_new; + const struct ethtool_ops *ops = dev->ethtool_ops; + int ret; + + if (!tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY]) + return 0;Feels a little old school to allow set with no attrs, now that we do strict validation on attrs across netlink. What's the reason?The power mode policy is the first parameter that can be set via MODULE_SET, but in the future there can be more and it is valid for user space to only want to change a subset. In which case, we will skip over attributes that were not specified.
Ack, I guess catching the "no parameter specified" case may be more effort than it's worth. Nothing is going to break if we don't do it.
quoted hunk ↗ jump to hunk
quoted
quoted
+ if (!ops->get_module_power_mode || !ops->set_module_power_mode) { + NL_SET_ERR_MSG_ATTR(extack, + tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY], + "Setting power mode policy is not supported by this device"); + return -EOPNOTSUPP; + } + + power_new.policy = nla_get_u8(tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY]); + ret = ops->get_module_power_mode(dev, &power, extack); + if (ret < 0) + return ret; + *p_mod = power_new.policy != power.policy; + + return ops->set_module_power_mode(dev, &power_new, extack);Why still call set if *p_mod == false?Good question... Thinking about this again, this seems better:diff --git a/net/ethtool/module.c b/net/ethtool/module.c index 254ac84f9728..a6eefae906eb 100644 --- a/net/ethtool/module.c +++ b/net/ethtool/module.c@@ -141,7 +141,10 @@ static int module_set_power_mode(struct net_device *dev, struct nlattr **tb, ret = ops->get_module_power_mode(dev, &power, extack); if (ret < 0) return ret; - *p_mod = power_new.policy != power.policy; + + if (power_new.policy == power.policy) + return 0; + *p_mod = true; return ops->set_module_power_mode(dev, &power_new, extack); }That way we avoid setting 'mod' to 'false' if it was already 'true' because of other parameters that were changed in ethnl_set_module(). We don't have any other parameters right now, but this can change. Thanks for looking into this
👍