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
