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

[Jakub Kicinski <kuba@kernel.org>]

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