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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 3 Apr 2024 15:18:44 +0200
From: Wojciech Drewek <wojciech.drewek@...el.com>
To: Andrew Lunn <andrew@...n.ch>
CC: <netdev@...r.kernel.org>, <idosch@...dia.com>, <edumazet@...gle.com>,
	<marcin.szycik@...ux.intel.com>, <anthony.l.nguyen@...el.com>,
	<kuba@...nel.org>, <intel-wired-lan@...ts.osuosl.org>, <pabeni@...hat.com>,
	<przemyslaw.kitszel@...el.com>
Subject: Re: [Intel-wired-lan] [PATCH net-next 0/3] ethtool: Max power support



On 02.04.2024 16:46, Andrew Lunn wrote:
> On Tue, Apr 02, 2024 at 01:38:59PM +0200, Wojciech Drewek wrote:
>>
>>
>> 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 :) 
> 
> That is a DT thing. Helps make the units of an ABI obvious. However,
> milliwatts is pretty standard with the kernel of user APIs, e.g. all
> hwmon calls use milliwatts.
> 
>>>> 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.
> 
> So if i plug in a 1W QSFP device, it will let the magic smoke out
> because it is force fed 1.5W?
> 
> Looking at
> https://www.optcore.net/wp-content/uploads/2017/04/QSFP-MSA.pdf table
> 7 it indicates different power budget classifications. Power level 1
> is a Maximum power of 1.5W. So does your parameter represent this?  It
> is the minimum maximum power? And your other parameter is the maximum
> maximum power?

Exactly as you described, minimum-power-allowed is in fact minimum value
which maximum-power-set can be set to (so minimum maximum). the other
parameter is maximim maximum.

> 
> I agree with Jakub here, there needs to be documentation added
> explaining in detail what these parameters mean, and ideally,
> references to the specification.

I completely agree, I'll include documentation in the next version.
I see now that those parameters might look confusing, minimum-power-allowed
is not true minimum in fact. We can try to came up with better names
but it might get silly (minimum-maximum-power) XD.

> 
> Does
> 
> $ ethtool --set-module enp1s0f0np0 power-max-set 4000
> 
> actually talk to the SFP module and tell it the maximum power it can
> consume. So in this case, it is not the cage, but the module?

It does not work that way in ice example.

> 
> Or is it talking to some entity which is managing the overall power
> consumption of a number of cages, and asking it to allocate a maximum
> of 4W to this cage. It might return an error message saying there is
> no power budget left?

That's right, we talk to firmware to set those restrictions.
In the ice implementation, the driver is responsible for checking if the
overall board budget is not exceeded.

> 
> Or is it doing both?
> 
> Sorry to be picky, but at some point, somebody is going to want to
> implement this in the Linux SFP driver, and we want a consistent
> implementation cross different implementations.

No problem, I see that your points are totally valid.

> 
> 	Andrew