Message-ID: <20240617154712.76fa490a@kmaincent-XPS-13-7390>
Date: Mon, 17 Jun 2024 15:47:12 +0200
From: Kory Maincent <kory.maincent@...tlin.com>
To: Oleksij Rempel <o.rempel@...gutronix.de>
Cc: "David S. Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
 <pabeni@...hat.com>, Donald Hunter <donald.hunter@...il.com>, Thomas
 Petazzoni <thomas.petazzoni@...tlin.com>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, Dent Project
 <dentproject@...uxfoundation.org>, kernel@...gutronix.de,
 UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH net-next v3 1/7] net: ethtool: pse-pd: Expand C33 PSE
 status with class, power and extended state

Hello Oleksij,

Thanks for your complete reviews.

On Sat, 15 Jun 2024 13:22:36 +0200
Oleksij Rempel <o.rempel@...gutronix.de> wrote:

> Hi Köry,
> 
> Overall, it looks good. Some fields need clarification, so don't be
> surprised if I critique things I proposed myself. There are still
> aspects I don't fully understand :)

Me neither, lets continue dig this up together.

Figured out we could also base our substate according to 33.8 tables values.

> On Fri, Jun 14, 2024 at 04:33:17PM +0200, Kory Maincent wrote:
>  [...]  
> 
> > +/**
> > + * enum ethtool_c33_pse_ext_substate_class_num_events - class_num_events
> > states
> > + *      functions. IEEE 802.3-2022 33.2.4.4 Variables
> > + *
> > + * @ETHTOOL_C33_PSE_EXT_SUBSTATE_CLASS_NUM_EVENTS_CLASS_ERROR: Illegal
> > class
> > + *
> > + * class_num_events is variable indicating the number of classification
> > events
> > + * performed by the PSE. A variable that is set in an
> > implementation-dependent
> > + * manner.
> > + */
> > +enum ethtool_c33_pse_ext_substate_class_num_events {
> > +	ETHTOOL_C33_PSE_EXT_SUBSTATE_CLASS_NUM_EVENTS_CLASS_ERROR = 1,
> > +};  
> 
> I'm still not 100% sure by this name. class_num_events seems to be more
> PSE side configuration variable. The pd692x0 0x43 value says "Illegal
> class" without providing additional information. If I see it correctly,
> typical classification will end with POWER_NOT_AVAILABLE if we will
> detect not supported class. Something other should fail to detect an
> illegal class.
> 
> According to 33.2.4.7
> State diagrams we have CLASSIFICATION_EVAL function which evaluates
> results of classification.
> In case of class_num_events = 1, we have only tpdc_timer. In case of
> error, will we get some timer related error?
> 
> In case of class_num_events = 2, if i see it correctly, PSE is doing
> double classification and if results do not match, PSE will go to faul
> state. See CLASS_EV2->(mr_pd_class_detected != temp_var) case.
> 
> Is it what we have here?

Mmh not really indeed, maybe we can put it in error_condition substate?

> > + * @ETHTOOL_C33_PSE_EXT_SUBSTATE_ERROR_CONDITION_DETECTED_UNDERLOAD:
> > Underload
> > + *	state  
> 
> pd692x0 documentation says, underload condition is related to Iport < Imin.
> Sofar, I was not able to find Imin in the final IEEE 802.3 2022 spec.
> 
> There are some historical traces:
> https://www.ieee802.org/3/af/public/mar01/darshan_3_0301.pdf
> 
> Instead, underload condition seems to be part of Maintain Power Signature
> (MPS) monitoring. See 33.2.9 PSE power removal and 33.2.9.1.2 PSE DC MPS
> component requirements.
> 
> Probably, it should go to the ETHTOOL_C33_PSE_EXT_SUBSTATE_MPS

Ok.
 
> > + * @ETHTOOL_C33_PSE_EXT_SUBSTATE_ERROR_CONDITION_CONFIG_CHANGE:
> > Configuration
> > + *	change
> > + * @ETHTOOL_C33_PSE_EXT_SUBSTATE_ERROR_CONDITION_DETECTED_OVER_TEMP: Over
> > + *	temperature detected
> > + * @ETHTOOL_C33_PSE_EXT_SUBSTATE_ERROR_CONDITION_CONNECTION_OPEN: Port is
> > + *	not connected  
> 
> This seems to reflect DETECT_EVAL->(signature = open_circuit) case. So,
> it is probably not vendor specific error condition?
> 
> The difference between open and underload is probably:
> - open: Iport = 0, detection state
> - underload: Iport < Imin (or Ihold?), Iport can be 0. related to powered/MPS
>   state.

Should I put it under MPS substate then?

> > +enum ethtool_c33_pse_ext_substate_option_detect_ted {
> > +	ETHTOOL_C33_PSE_EXT_SUBSTATE_OPTION_DETECT_TED_DET_IN_PROCESS = 1,
> > +	ETHTOOL_C33_PSE_EXT_SUBSTATE_OPTION_DETECT_TED_IMPROPER_CAP_DET,  
> 
> The pd692x0 0x25 may be reported in two cases:
> Fail due to out-of-range capacitor value or
> Fail due to detected short value
> 
> On one side, this seems to be related to MONITOR_INRUSH function.
> "33.2.7.5 Output current in POWER_UP mode
> 
> The PSE shall limit the maximum current sourced at the PI during
> POWER_UP. The maximum inrush current sourced by the PSE shall not exceed the
> PSE inrush template in Figure 33–13."
> 
> On other side, pd692x0 documentation is using 0x1C or 0x25 or 0xA7
> values together with "INVALID SIG" description. In this case, this
> values are related to signature detection stage, not power up or
> tinrush_timer stage. In this case, i assume:
> 0x25 and 0xa7 refers to Table 33–6 or Table 145–8 Invalid PD detection
> signature electrical characteristics.
> 
> Not sure about 0x1c - Non-802.3AF/AT powered device. Is it something
> between Table 33–5 and Table 33–6? 
> 
> CCing UNGLinuxDriver@...rochip.com
> 
> May be you will need to contact Microchip directly. Usually it helps :)

Lets keep it like that for now?

> > +enum ethtool_c33_pse_ext_substate_pd_dll_power_type {
> > +
> > ETHTOOL_C33_PSE_EXT_SUBSTATE_PD_DLL_POWER_TYPE_NON_802_3AF_AT_DEVICE = 1,
> > +};  
> 
> Here i was potentially wrong. LLDP stage is after power up, and this
> values was probably set on early stage of signature detection. How can
> we detect a device which is not conform to the 802.3AF/AT standard? Is
> it something pre-802.3AF/AT, micorosemi specific vendor specific signature?

Don't really know.
 
> > +/**
> > + * enum ethtool_c33_pse_ext_substate_power_not_available -
> > power_not_available
> > + *	states functions. IEEE 802.3-2022 33.2.4.4 Variables
> > + *
> > + * @ETHTOOL_C33_PSE_EXT_SUBSTATE_POWER_NOT_AVAILABLE_BUDGET_EXCEEDED: Power
> > + *	budget exceeded
> > + * @ETHTOOL_C33_PSE_EXT_SUBSTATE_POWER_NOT_AVAILABLE_PM_STATIC: Power
> > + *	Management-Static  
> 
> > + * @ETHTOOL_C33_PSE_EXT_SUBSTATE_POWER_NOT_AVAILABLE_PM_STATIC_OVL: Power
> > + *	Management-Static-ovl  
> 
> Here we need some comment updates. Here is my understanding, taken out
> of thin air:
> 0x20 - We have per controller limit, but no limit per port is configured,
>        in this case, if PD classification request more power then
>        allowed by per controller budget, we will get this error.
>        AllPortsPower + NewPortPower > ControllerMaxPower
> 0x3c - We have per port limit configured and it is over the controller
>        budget.
>        AllPortsMaxPower + NewPortMaxPower > ControllerMaxPower
> 0x3D - PD Class requesting more power that the Port configured port limit.
>        PDClassPower > PortMaxPower
> 
> How about:
>  *
> @ETHTOOL_C33_PSE_EXT_SUBSTATE_POWER_NOT_AVAILABLE_CONTROLLER_BUDGET_EXCEEDED:
> Power
>  *   budget exceeded for the controller
>  *
> @ETHTOOL_C33_PSE_EXT_SUBSTATE_POWER_NOT_AVAILABLE_PORT_POWER_LIMIT_EXCEEDS_CONTROLLER_BUDGET:
> Configured
>  *   port power limit exceeded controller power budget
>  *
> @ETHTOOL_C33_PSE_EXT_SUBSTATE_POWER_NOT_AVAILABLE_PD_REQUEST_EXCEEDS_PORT_LIMIT:
> Power
>  *   request from PD exceeds port limit

Yes seems right.
 
> > + * @ETHTOOL_C33_PSE_EXT_SUBSTATE_POWER_NOT_AVAILABLE_HW_PW_LIMIT: Power
> > + * denied due to Hardware power limit  
> 
> Not sure i understand this one correctly. Is it something like - all previous
> errors can be solved by proper configuration, but on this one we can't do
> anything. The HW is the limit. Correct? :)

Suppose so. ;)
 
> > +	if (st->c33_pw_class > 0)
> > +		len += nla_total_size(sizeof(u32)); /* _C33_PSE_PW_CLASS */
> > +	if (st->c33_actual_pw > 0)
> > +		len += nla_total_size(sizeof(u32)); /* _C33_PSE_ACTUAL_PW
> > */
> > +	if (st->c33_ext_state_info.c33_pse_ext_state > 0)
> > +		len += nla_total_size(sizeof(u32)); /* _C33_PSE_EXT_STATE
> > */
> > +	if (st->c33_ext_state_info.__c33_pse_ext_substate > 0)
> > +		len += nla_total_size(sizeof(u32)); /*
> > _C33_PSE_EXT_SUBSTATE */  
> 
> Hm, we still may include __c33_pse_ext_substate even if c33_pse_ext_state ==
> 0.

Right indeed. Will fix it.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Powered by blists - more mailing lists