Re: [PATCH net-next 1/6] ethtool: Add ability to control transceiver modules' power mode

[Ido Schimmel <idosch@idosch.org>]

On Tue, Oct 05, 2021 at 09:57:25AM +0300, Ido Schimmel wrote:
> On Mon, Oct 04, 2021 at 06:01:35PM -0700, Jakub Kicinski wrote:
> > On Sun,  3 Oct 2021 10:32:14 +0300 Ido Schimmel wrote:
> > > 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!
> 
> > 
> > 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.
> 
> > 
> > > +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/
> 
> > 
> > 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.
> 
> > 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,