Re: [PATCH net-next 2/3] ethtool: Introduce max power support

[Wojciech Drewek <wojciech.drewek@intel.com>]

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