Re: [PATCH net-next 1/6] ethtool: Add ability to control transceiver modules' power mode
From: Ido Schimmel <hidden>
Date: 2021-10-05 12:04:13
Subsystem:
networking [ethtool], networking [general], the rest · Maintainers:
Andrew Lunn, Jakub Kicinski, "David S. Miller", Eric Dumazet, Paolo Abeni, Linus Torvalds
On Tue, Oct 05, 2021 at 09:57:25AM +0300, Ido Schimmel wrote:
quoted hunk ↗ jump to hunk
On Mon, Oct 04, 2021 at 06:01:35PM -0700, Jakub Kicinski wrote:quoted
On Sun, 3 Oct 2021 10:32:14 +0300 Ido Schimmel wrote:quoted
From: Ido Schimmel <idosch@nvidia.com> Add a pair of new ethtool messages, 'ETHTOOL_MSG_MODULE_SET' and 'ETHTOOL_MSG_MODULE_GET', that can be used to control transceiver modules parameters and retrieve their status.Acked-by: Jakub Kicinski <kuba@kernel.org>Thanks!quoted
Couple of take it or leave it comments again, if you prefer to leave as is that's fine.I'll make whatever changes we conclude are necessary. See below.quoted
quoted
+enum ethtool_module_power_mode_policy { + ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH, + ETHTOOL_MODULE_POWER_MODE_POLICY_AUTO, +};I see you left this starting from 0, and we still need a valid bit, granted just internal to the core.I was under the impression that we were only talking about the power mode itself (which can be invalid if a module is unplugged), but not the policy [1]. I did change that like we discussed. [1] https://lore.kernel.org/netdev/YSYmFEDWJIu6eDvR@shredder/ (local)quoted
Would we not need a driver-facing valid bit later on when we extend the module API to control more params?To keep the driver-facing API simple I want to have different ethtool operations for different parameters (like rtnetlink and ndos). So, if a driver does not support a parameter, the operation will not be implemented and the attributes will not be dumped.quoted
Can't there be drivers which implement power but don't support the mode policy?I don't really see how. The policy is a host attribute (not module) determining how the host configures the power mode of the module. It always exists, but can be fixed. Do you still think we should make the change below?diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h index 1b126e8b5269..a2223b685451 100644 --- a/include/uapi/linux/ethtool.h +++ b/include/uapi/linux/ethtool.h@@ -721,7 +721,7 @@ enum ethtool_stringset { * administratively down. */ enum ethtool_module_power_mode_policy { - ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH, + ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH = 1, ETHTOOL_MODULE_POWER_MODE_POLICY_AUTO, };
I read your reply again about "still need a valid bit, granted just internal to the core". My confusion was that I thought only the valid bit in the driver-facing API bothered you, but you actually wanted me to remove all of them. How about the below (compile tested)?
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 1b126e8b5269..a2223b685451 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h@@ -721,7 +721,7 @@ enum ethtool_stringset { * administratively down. */ enum ethtool_module_power_mode_policy { - ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH, + ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH = 1, ETHTOOL_MODULE_POWER_MODE_POLICY_AUTO, };
diff --git a/net/ethtool/module.c b/net/ethtool/module.c
index 254ac84f9728..5d430b2bb20a 100644
--- a/net/ethtool/module.c
+++ b/net/ethtool/module.c@@ -13,7 +13,6 @@ struct module_req_info { struct module_reply_data { struct ethnl_reply_data base; struct ethtool_module_power_mode_params power; - u8 power_valid:1; }; #define MODULE_REPDATA(__reply_base) \
@@ -30,18 +29,11 @@ static int module_get_power_mode(struct net_device *dev, struct netlink_ext_ack *extack) { const struct ethtool_ops *ops = dev->ethtool_ops; - int ret; if (!ops->get_module_power_mode) return 0; - ret = ops->get_module_power_mode(dev, &data->power, extack); - if (ret < 0) - return ret; - - data->power_valid = true; - - return 0; + return ops->get_module_power_mode(dev, &data->power, extack); } static int module_prepare_data(const struct ethnl_req_info *req_base,
@@ -72,10 +64,10 @@ static int module_reply_size(const struct ethnl_req_info *req_base, struct module_reply_data *data = MODULE_REPDATA(reply_base); int len = 0; - if (data->power_valid) + if (data->power.policy) len += nla_total_size(sizeof(u8)); /* _MODULE_POWER_MODE_POLICY */ - if (data->power_valid && data->power.mode) + if (data->power.mode) len += nla_total_size(sizeof(u8)); /* _MODULE_POWER_MODE */ return len;
@@ -87,12 +79,12 @@ static int module_fill_reply(struct sk_buff *skb, { const struct module_reply_data *data = MODULE_REPDATA(reply_base); - if (data->power_valid && + if (data->power.policy && nla_put_u8(skb, ETHTOOL_A_MODULE_POWER_MODE_POLICY, data->power.policy)) return -EMSGSIZE; - if (data->power_valid && data->power.mode && + if (data->power.mode && nla_put_u8(skb, ETHTOOL_A_MODULE_POWER_MODE, data->power.mode)) return -EMSGSIZE;
@@ -116,7 +108,8 @@ const struct ethnl_request_ops ethnl_module_request_ops = { const struct nla_policy ethnl_module_set_policy[ETHTOOL_A_MODULE_POWER_MODE_POLICY + 1] = { [ETHTOOL_A_MODULE_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy), [ETHTOOL_A_MODULE_POWER_MODE_POLICY] = - NLA_POLICY_MAX(NLA_U8, ETHTOOL_MODULE_POWER_MODE_POLICY_AUTO), + NLA_POLICY_RANGE(NLA_U8, ETHTOOL_MODULE_POWER_MODE_HIGH, + ETHTOOL_MODULE_POWER_MODE_POLICY_AUTO), }; static int module_set_power_mode(struct net_device *dev, struct nlattr **tb,