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