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

[Ido Schimmel <idosch@idosch.org>]

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,
 };

> 
> > +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.

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