Re: [PATCH net-next v3 5/7] net: ethtool: Add new power limit get and set features

[Oleksij Rempel]

On Sat, Jun 15, 2024 at 05:59:36PM +0200, Oleksij Rempel wrote:
> Hi Köry,
> 
> On Fri, Jun 14, 2024 at 04:33:21PM +0200, Kory Maincent wrote:
> > From: Kory Maincent (Dent Project) <kory.maincent@xxxxxxxxxxx>
> > 
> > This patch expands the status information provided by ethtool for PSE c33
> > with power limit. It also adds a call to pse_ethtool_set_pw_limit() to
> > configure the PSE control power limit.
> > 
> > Signed-off-by: Kory Maincent <kory.maincent@xxxxxxxxxxx>
> > ---
> > 
> > Change in v3:
> > - Add ethtool netlink documentation.
> > ---
> >  Documentation/networking/ethtool-netlink.rst |  8 ++++++
> >  include/uapi/linux/ethtool_netlink.h         |  1 +
> >  net/ethtool/pse-pd.c                         | 42 +++++++++++++++++++++++-----
> >  3 files changed, 44 insertions(+), 7 deletions(-)
> > 
> > diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
> > index 7dbf2ef3ac0e..a78b6aea84af 100644
> > --- a/Documentation/networking/ethtool-netlink.rst
> > +++ b/Documentation/networking/ethtool-netlink.rst
> > @@ -1737,6 +1737,7 @@ Kernel response contents:
> >                                                    PoE PSE.
> >    ``ETHTOOL_A_C33_PSE_EXT_SUBSTATE``         u32  power extended substatus of
> >                                                    the PoE PSE.
> > +  ``ETHTOOL_A_C33_PSE_PW_LIMIT``             u32  power limit of the PoE PSE.
> >    ======================================  ======  =============================
> >  
> >  When set, the optional ``ETHTOOL_A_PODL_PSE_ADMIN_STATE`` attribute identifies
> > @@ -1799,6 +1800,9 @@ Possible values are:
> >  		  ethtool_c33_pse_ext_substate_power_not_available
> >  		  ethtool_c33_pse_ext_substate_short_detected
> >  
> > +When set, the optional ``ETHTOOL_A_C33_PSE_PW_LIMIT`` attribute identifies
> > +the C33 PSE power limit in mW.
> > +
> >  PSE_SET
> >  =======
> >  
> > @@ -1810,6 +1814,7 @@ Request contents:
> >    ``ETHTOOL_A_PSE_HEADER``                nested  request header
> >    ``ETHTOOL_A_PODL_PSE_ADMIN_CONTROL``       u32  Control PoDL PSE Admin state
> >    ``ETHTOOL_A_C33_PSE_ADMIN_CONTROL``        u32  Control PSE Admin state
> > +  ``ETHTOOL_A_C33_PSE_PW_LIMIT``             u32  Control PoE PSE power limit
> >    ======================================  ======  =============================
> >  
> >  When set, the optional ``ETHTOOL_A_PODL_PSE_ADMIN_CONTROL`` attribute is used
> > @@ -1820,6 +1825,9 @@ to control PoDL PSE Admin functions. This option is implementing
> >  The same goes for ``ETHTOOL_A_C33_PSE_ADMIN_CONTROL`` implementing
> >  ``IEEE 802.3-2022`` 30.9.1.2.1 acPSEAdminControl.
> >  
> > +When set, the optional ``ETHTOOL_A_C33_PSE_PW_LIMIT`` attribute is used
> > +to control C33 PSE power limit in mW.
> 
> 
> The corresponding name int the IEEE 802.3-2022 seems to be pse_avail_pwr
> in 145.2.5.4 Variables and pse_available_power in 33.2.4.4 Variables.
> 
> This variable is using classes instead of mW. pd692x0 seems to use
> classes instead of mW too. May be it is better to use classes for UAPI
> too? 

Huh... i took some more time to investigate it. Looks like there is no
simple answer. Some devices seems to write power class on the box. Other
client devices write power consumption in watts. IEEE 802.3-2022
provides LLDP specification with PowerValue for watts and PowerClass for
classes. Different product user interfaces provide class and/or watts.
So, let's go with watts then. Please update the name to something like
pse_available_power_value or pse_available_power_value_limit and
document how it is related to State diagrams in the IEEE spec.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |