Re: [Intel-wired-lan] [PATCH net-next 0/3] ethtool: Max power support

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

On 30.03.2024 22:57, Andrew Lunn wrote:
> On Fri, Mar 29, 2024 at 10:23:18AM +0100, Wojciech Drewek wrote:
>> Some ethernet modules use nonstandard power levels [1]. Extend ethtool
>> module implementation to support new attributes that will allow user
>> to change maximum power. Rename structures and functions to be more
>> generic. Introduce an example of the new API in ice driver.
>>
>> Ethtool examples:
>> $ ethtool --show-module enp1s0f0np0
>> Module parameters for enp1s0f0np0:
>> power-min-allowed: 1000 mW
>> power-max-allowed: 3000 mW
>> power-max-set: 1500 mW
>>
>> $ ethtool --set-module enp1s0f0np0 power-max-set 4000
> 
> We have had a device tree property for a long time:
> 
>   maximum-power-milliwatt:
>     minimum: 1000
>     default: 1000
>     description:
>       Maximum module power consumption Specifies the maximum power consumption
>       allowable by a module in the slot, in milli-Watts. Presently, modules can
>       be up to 1W, 1.5W or 2W.
> 
> Could you flip the name around to be consistent with DT?

Yea, I'm open to any name suggestion although I don't like the unit in the parameter name :) 

> 
>> minimum-power-allowed: 1000 mW
>> maximum-power-allowed: 3000 mW
>> maximum-power-set: 1500 mW
> 
> Also, what does minimum-power-allowed actually tell us? Do you imagine
> it will ever be below 1W because of bad board design? Do you have a
> bad board design which does not allow 1W?

Yes. in case of QSFP we don't support 1W, 1.5W is the minimum.
This parameter tells the user what is the lowest limit he can set.

> 
> Also, this is about the board, the SFP cage, not the actual SFP
> module?  Maybe the word cage needs to be in these names?

It's about cage. Thanks for bringing it to my attention because now I
see it might be misleading. I'm extending {set|show}-module command
but the changes are about max power in the cage. With that in mind
I agree that adding 'cage' to the names makes sense.

> 
> Do we want to be able to enumerate what the module itself supports?
> If so, we need to include module in the name, to identify the numbers
> are about the module, not the cage.

I hope that my previous paragraph answers this as well.

> 
>     Andrew