| Message ID | 20240329092321.16843-3-wojciech.drewek@intel.com |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | ethtool: Max power support | expand |
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 <marcin.szycik@linux.intel.com> > Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com> > --- > 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; > }
On Fri, Mar 29, 2024 at 10:23:20AM +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 > 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 I'm confused. The cage has two power pins, if you look at the table here: https://www.embrionix.com/resource/how-to-design-with-video-SFP There is VccT and VccR. I would expect there is a power regulator supplying these pins. By default, you can draw 1W from that regulator. The board however might be designed to support more power, so those regulators could supply more power. And the board has also been designed to dump the heat if more power is consumed. So, ETHTOOL_A_MODULE_MIN_POWER_ALLOWED is about the minimum power that regulator can supply? Does that make any sense? ETHTOOL_A_MODULE_MAX_POWER_ALLOWED is about the maximum power the regulator can supply and the cooling system can dump heat? Then what does ETHTOOL_A_MODULE_MAX_POWER_SET mean? power in the cage? The cage is passive. It does not consume power. It is the module which does. Is this telling the module it can consume up to this amount of power? Andrew
On 29.03.2024 23:29, Jakub Kicinski wrote: > 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. Makes sense, although ETHTOOL_A_MODULE_POWER_LIMIT does not say if it's max or min limit. What about: ETHTOOL_A_MODULE_POWER_MAX_LIMIT ETHTOOL_A_MODULE_POWER_UPPER_LIMIT > >> 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 <marcin.szycik@linux.intel.com> >> Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com> >> --- >> 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 ? Makes sense > >> 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 ? Agree > >> @@ -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 I'll change the name > >> 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. Yep, makes sense > >> + 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. Sure > >> + return -EINVAL; > > ERANGE? Agree > >> + } 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)? Hmmm, I think we can allow to reset if the currently set limit is the default one. Right now only the driver could catch such scenario because we don't have a parameter that driver could use to inform the ethtool about the default value. I hope that answers your question since I'm not 100% sure if that's what you asked about :) > >> + 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? Yes, that was my intention. Using both of those attributes at the same time is not allowed. > > 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. Hmmm, makes sense > >> + 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; >> }
On Tue, 2 Apr 2024 13:25:07 +0200 Wojciech Drewek wrote: > On 29.03.2024 23:29, Jakub Kicinski wrote: > > 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. > > Makes sense, although ETHTOOL_A_MODULE_POWER_LIMIT does not say if > it's max or min limit. What about: > ETHTOOL_A_MODULE_POWER_MAX_LIMIT > ETHTOOL_A_MODULE_POWER_UPPER_LIMIT Is it possible to "limit" min power? 🧐️ This is not HTB where "unused power" can go to the sibling cage... > >> + } 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)? > > Hmmm, I think we can allow to reset if the currently set limit is the default one. > Right now only the driver could catch such scenario because we don't have a parameter > that driver could use to inform the ethtool about the default value. > I hope that answers your question since I'm not 100% sure if that's what you asked about :) Let me put it differently. How do we know that the driver doesn't support setting the power policy? AFAIU we assume driver supports it when it reports min_pwr_allowed || max_pwr_allowed from get. If that's not the case we should add a cap bit like cap_link_lanes_supported. So what I'm saying is that if driver doesn't support the feature, we should error out if user space gave us any tb[ETHTOOL_A_MODULE_MAX_POWER* attribute. > >> + 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? > > Yes, that was my intention. Using both of those attributes at the same time is not allowed. To be clear the code is: ret = ops->get_module_power_cfg(dev, &power, info->extack); if (ret < 0) return ret; power_new.max_pwr_set = power.max_pwr_set; ethnl_update_u32(&power_new.max_pwr_set, tb[ETHTOOL_A_MODULE_MAX_POWER_SET], &mod); // ... if (power_new.max_pwr_reset && power_new.max_pwr_set) { so if driver reports .max_pwr_set from get we may fall into this if I think you got it but anyway..
On 30.03.2024 23:14, Andrew Lunn wrote: > On Fri, Mar 29, 2024 at 10:23:20AM +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 >> 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 > > I'm confused. The cage has two power pins, if you look at the table > here: > > https://www.embrionix.com/resource/how-to-design-with-video-SFP > > There is VccT and VccR. I would expect there is a power regulator > supplying these pins. By default, you can draw 1W from that > regulator. The board however might be designed to support more power, > so those regulators could supply more power. And the board has also > been designed to dump the heat if more power is consumed. > > So, ETHTOOL_A_MODULE_MIN_POWER_ALLOWED is about the minimum power that > regulator can supply? Does that make any sense? > > ETHTOOL_A_MODULE_MAX_POWER_ALLOWED is about the maximum power the > regulator can supply and the cooling system can dump heat? > > Then what does ETHTOOL_A_MODULE_MAX_POWER_SET mean? power in the cage? > The cage is passive. It does not consume power. It is the module which > does. Is this telling the module it can consume up to this amount of > power? Right, all of those attributes describe restrictions for modules that can be plugged into the given cage. ETHTOOL_A_MODULE_MAX_POWER_SET is currently set maximum. The other two define the range of values that ETHTOOL_A_MODULE_MAX_POWER_SET can take. I hope that answers your question. > > Andrew >
On 02.04.2024 16:34, Jakub Kicinski wrote: > On Tue, 2 Apr 2024 13:25:07 +0200 Wojciech Drewek wrote: >> On 29.03.2024 23:29, Jakub Kicinski wrote: >>> 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. >> >> Makes sense, although ETHTOOL_A_MODULE_POWER_LIMIT does not say if >> it's max or min limit. What about: >> ETHTOOL_A_MODULE_POWER_MAX_LIMIT >> ETHTOOL_A_MODULE_POWER_UPPER_LIMIT > > Is it possible to "limit" min power? 🧐️ Right, I'll stick with ETHTOOL_A_MODULE_POWER_LIMIT > This is not HTB where "unused power" can go to the sibling cage... >>>> + } 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)? >> >> Hmmm, I think we can allow to reset if the currently set limit is the default one. >> Right now only the driver could catch such scenario because we don't have a parameter >> that driver could use to inform the ethtool about the default value. >> I hope that answers your question since I'm not 100% sure if that's what you asked about :) > > Let me put it differently. How do we know that the driver doesn't > support setting the power policy? AFAIU we assume driver supports > it when it reports min_pwr_allowed || max_pwr_allowed from get. > If that's not the case we should add a cap bit like > cap_link_lanes_supported. > > So what I'm saying is that if driver doesn't support the feature, > we should error out if user space gave us any > tb[ETHTOOL_A_MODULE_MAX_POWER* attribute Ok, I get now. Normally checking ops->set_module_power_cfg pointer would be enough but here we have two features in one callback. Right now I assumed that the driver will check which attributes were provided by the userspace and will print error (like I did in ice_set_module_power_cfg) if the driver does not support given attribute. You're saying that if min_pwr_allowed or max_pwr_allowed taken from get op are 0 than we should not allow to set max_pwr_reset and max_pwr_set? And similarly if policy was 0 than we should not allow to set it? I can implement whichever option you prefer. > >>>> + 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? >> >> Yes, that was my intention. Using both of those attributes at the same time is not allowed. > > To be clear the code is: > > ret = ops->get_module_power_cfg(dev, &power, info->extack); > if (ret < 0) > return ret; > > power_new.max_pwr_set = power.max_pwr_set; > > ethnl_update_u32(&power_new.max_pwr_set, > tb[ETHTOOL_A_MODULE_MAX_POWER_SET], &mod); > // ... > > if (power_new.max_pwr_reset && power_new.max_pwr_set) { > > so if driver reports .max_pwr_set from get we may fall into this if > I think you got it but anyway.. Oh, I see, I'll check the attributes at the beginning before reading them
On Wed, 3 Apr 2024 12:19:57 +0200 Wojciech Drewek wrote: > You're saying that if min_pwr_allowed or max_pwr_allowed taken from get op > are 0 than we should not allow to set max_pwr_reset and max_pwr_set? Yes, return -EOPNOTSUPP and point extack at whatever max_pwr attr user sent. If driver doesn't return any bounds from get() it must not support the configuration. > And similarly if policy was 0 than we should not allow to set it? You mean the limit? I'm not as sure about this one. We can either treat 0 as "unset" or as unsupported. Not sure what makes more sense for this case.
On 04.04.2024 02:18, Jakub Kicinski wrote: > On Wed, 3 Apr 2024 12:19:57 +0200 Wojciech Drewek wrote: >> You're saying that if min_pwr_allowed or max_pwr_allowed taken from get op >> are 0 than we should not allow to set max_pwr_reset and max_pwr_set? > > Yes, return -EOPNOTSUPP and point extack at whatever max_pwr attr user > sent. If driver doesn't return any bounds from get() it must not support > the configuration. Ok > >> And similarly if policy was 0 than we should not allow to set it? > > You mean the limit? I'm not as sure about this one. We can either > treat 0 as "unset" or as unsupported. Not sure what makes more sense > for this case. I was talking about ETHTOOL_A_MODULE_POWER_MODE_POLICY, attribute that is already present in ethtool.
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; }; /** @@ -804,11 +812,12 @@ struct ethtool_rxfh_param { * @get_eth_ctrl_stats: Query some of the IEEE 802.3 MAC Ctrl statistics. * @get_rmon_stats: Query some of the RMON (RFC 2819) statistics. * Set %ranges to a pointer to zero-terminated array of byte ranges. - * @get_module_power_cfg: Get the power mode policy for the plug-in module - * used by the network device and its operational power mode, if - * plugged-in. + * @get_module_power_cfg: Get the power configuration for the plug-in module + * used by the network device which includes: its power mode policy and + * operational power mode, if plugged-in; maximum power settings + * (min and max allowed power and max power currently set) * @set_module_power_cfg: Set the power mode policy for the plug-in module - * used by the network device. + * used by the network device and its maximum power. * @get_mm: Query the 802.3 MAC Merge layer state. * @set_mm: Set the 802.3 MAC Merge layer parameters. * @get_mm_stats: Query the 802.3 MAC Merge layer statistics. 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 */ /* add new constants above here */ __ETHTOOL_A_MODULE_CNT, diff --git a/net/ethtool/module.c b/net/ethtool/module.c index 193ca4642e04..9f63a276357e 100644 --- a/net/ethtool/module.c +++ b/net/ethtool/module.c @@ -69,6 +69,15 @@ static int module_reply_size(const struct ethnl_req_info *req_base, if (data->power.mode) len += nla_total_size(sizeof(u8)); /* _MODULE_POWER_MODE */ + if (data->power.min_pwr_allowed) + len += nla_total_size(sizeof(u32)); /* _MIN_POWER_ALLOWED */ + + if (data->power.max_pwr_allowed) + len += nla_total_size(sizeof(u32)); /* _MAX_POWER_ALLOWED */ + + if (data->power.max_pwr_set) + len += nla_total_size(sizeof(u32)); /* _MAX_POWER_SET */ + return len; } @@ -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; if (data->power.policy && nla_put_u8(skb, ETHTOOL_A_MODULE_POWER_MODE_POLICY, @@ -87,16 +97,30 @@ static int module_fill_reply(struct sk_buff *skb, nla_put_u8(skb, ETHTOOL_A_MODULE_POWER_MODE, data->power.mode)) return -EMSGSIZE; + temp = data->power.min_pwr_allowed; + if (temp && nla_put_u32(skb, ETHTOOL_A_MODULE_MIN_POWER_ALLOWED, temp)) + return -EMSGSIZE; + + temp = data->power.max_pwr_allowed; + if (temp && nla_put_u32(skb, ETHTOOL_A_MODULE_MAX_POWER_ALLOWED, temp)) + return -EMSGSIZE; + + temp = data->power.max_pwr_set; + if (temp && nla_put_u32(skb, ETHTOOL_A_MODULE_MAX_POWER_SET, temp)) + return -EMSGSIZE; + return 0; } /* MODULE_SET */ -const struct nla_policy ethnl_module_set_policy[ETHTOOL_A_MODULE_POWER_MODE_POLICY + 1] = { +const struct nla_policy ethnl_module_set_policy[ETHTOOL_A_MODULE_MAX + 1] = { [ETHTOOL_A_MODULE_HEADER] = NLA_POLICY_NESTED(ethnl_header_policy), [ETHTOOL_A_MODULE_POWER_MODE_POLICY] = NLA_POLICY_RANGE(NLA_U8, ETHTOOL_MODULE_POWER_MODE_POLICY_HIGH, ETHTOOL_MODULE_POWER_MODE_POLICY_AUTO), + [ETHTOOL_A_MODULE_MAX_POWER_SET] = { .type = NLA_U32 }, + [ETHTOOL_A_MODULE_MAX_POWER_RESET] = { .type = NLA_U8 }, }; static int @@ -106,7 +130,9 @@ ethnl_set_module_validate(struct ethnl_req_info *req_info, const struct ethtool_ops *ops = req_info->dev->ethtool_ops; struct nlattr **tb = info->attrs; - if (!tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY]) + if (!tb[ETHTOOL_A_MODULE_POWER_MODE_POLICY] && + !tb[ETHTOOL_A_MODULE_MAX_POWER_SET] && + !tb[ETHTOOL_A_MODULE_MAX_POWER_RESET]) return 0; if (!ops->get_module_power_cfg || !ops->set_module_power_cfg) { @@ -117,26 +143,64 @@ ethnl_set_module_validate(struct ethnl_req_info *req_info, return 1; } +static void +ethnl_update_policy(enum ethtool_module_power_mode_policy *dst, + const struct nlattr *attr, bool *mod) +{ + u8 val = *dst; + + ethnl_update_u8(&val, attr, mod); + + if (mod) + *dst = val; +} + 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) { + if (power_new.max_pwr_set > power.max_pwr_allowed) { + NL_SET_ERR_MSG(info->extack, "Provided value is higher than maximum allowed"); + return -EINVAL; + } 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); + + if (!mod) return 0; + if (power_new.max_pwr_reset && power_new.max_pwr_set) { + 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; } diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h index 9a333a8d04c1..6282f84811ce 100644 --- a/net/ethtool/netlink.h +++ b/net/ethtool/netlink.h @@ -432,7 +432,7 @@ extern const struct nla_policy ethnl_module_eeprom_get_policy[ETHTOOL_A_MODULE_E extern const struct nla_policy ethnl_stats_get_policy[ETHTOOL_A_STATS_SRC + 1]; extern const struct nla_policy ethnl_phc_vclocks_get_policy[ETHTOOL_A_PHC_VCLOCKS_HEADER + 1]; extern const struct nla_policy ethnl_module_get_policy[ETHTOOL_A_MODULE_HEADER + 1]; -extern const struct nla_policy ethnl_module_set_policy[ETHTOOL_A_MODULE_POWER_MODE_POLICY + 1]; +extern const struct nla_policy ethnl_module_set_policy[ETHTOOL_A_MODULE_MAX + 1]; extern const struct nla_policy ethnl_pse_get_policy[ETHTOOL_A_PSE_HEADER + 1]; extern const struct nla_policy ethnl_pse_set_policy[ETHTOOL_A_PSE_MAX + 1]; extern const struct nla_policy ethnl_rss_get_policy[ETHTOOL_A_RSS_CONTEXT + 1];