Re: [PATCH net-next v2 2/8] net: ethtool: pse-pd: Expand C33 PSE status with class, power and extended state

[Kory Maincent <kory.maincent@bootlin.com>]

Hello Oleksij,

On Mon, 10 Jun 2024 07:16:09 +0200
Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> Hi Köry,
> 
> Thank you for your work.
> 
> On Fri, Jun 07, 2024 at 09:30:19AM +0200, Kory Maincent wrote:
> > From: "Kory Maincent (Dent Project)" <kory.maincent@bootlin.com>  
> 
> ...
> 
> >  /**
> > diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> > index 8733a3117902..ef65ad4612d2 100644
> > --- a/include/uapi/linux/ethtool.h
> > +++ b/include/uapi/linux/ethtool.h
> > @@ -752,6 +752,47 @@ enum ethtool_module_power_mode {
> >  	ETHTOOL_MODULE_POWER_MODE_HIGH,
> >  };
> >  
> > +/* C33 PSE extended state */
> > +enum ethtool_c33_pse_ext_state {
> > +	ETHTOOL_C33_PSE_EXT_STATE_UNKNOWN = 1,  
> 
> I assume, In case the state is unknown, better to set it to 0 and not
> report it to the user space in the first place. Do we really need it?

The pd692x0 report this for the unknown state: "Port is not mapped to physical
port, port is in unknown state, or PD692x0 fails to communicate with PD69208
device allocated for this port."
Also it has a status for open port (not connected) state.
(ETHTOOL_C33_PSE_EXT_SUBSTATE_V_OPEN)
Do you prefer to use the same error for both state?
 
> > +	ETHTOOL_C33_PSE_EXT_STATE_DETECTION,
> > +	ETHTOOL_C33_PSE_EXT_STATE_CLASSIFICATION_FAILURE,
> > +	ETHTOOL_C33_PSE_EXT_STATE_HARDWARE_ISSUE,
> > +	ETHTOOL_C33_PSE_EXT_STATE_VOLTAGE_ISSUE,
> > +	ETHTOOL_C33_PSE_EXT_STATE_CURRENT_ISSUE,
> > +	ETHTOOL_C33_PSE_EXT_STATE_POWER_BUDGET_EXCEEDED,  
> 
> What is the difference between POWER_BUDGET_EXCEEDED and
> STATE_CURRENT_ISSUE->CRT_OVERLOAD? If there is some difference, it
> should be commented.

Current overload seems to be describing the "Overload current detection range
(Icut)" As described in the IEEE standard.
Not sure If budget exceeded should use the same error.

> Please provide comments describing how all of this states and substates
> should be used.

The enum errors I wrote is a bit subjective and are taken from the PD692x0
port status list. Go ahead to purpose any change, I have tried to make
categories that make sense but I might have made wrong choice.

> >  /**
> >   * enum ethtool_pse_types - Types of PSE controller.
> >   * @ETHTOOL_PSE_UNKNOWN: Type of PSE controller is unknown
> > diff --git a/include/uapi/linux/ethtool_netlink.h
> > b/include/uapi/linux/ethtool_netlink.h index b49b804b9495..ccbe8294dfd5
> > 100644 --- a/include/uapi/linux/ethtool_netlink.h
> > +++ b/include/uapi/linux/ethtool_netlink.h
> > @@ -915,6 +915,10 @@ enum {
> >  	ETHTOOL_A_C33_PSE_ADMIN_STATE,		/* u32 */
> >  	ETHTOOL_A_C33_PSE_ADMIN_CONTROL,	/* u32 */
> >  	ETHTOOL_A_C33_PSE_PW_D_STATUS,		/* u32 */
> > +	ETHTOOL_A_C33_PSE_PW_CLASS,		/* u32 */
> > +	ETHTOOL_A_C33_PSE_ACTUAL_PW,		/* u32 */
> > +	ETHTOOL_A_C33_PSE_EXT_STATE,		/* u8 */
> > +	ETHTOOL_A_C33_PSE_EXT_SUBSTATE,		/* u8 */  
> 
> Please, increase the size to u32 for state and substate.

Ack,

> >  	/* add new constants above here */
> >  	__ETHTOOL_A_PSE_CNT,
> > diff --git a/net/ethtool/pse-pd.c b/net/ethtool/pse-pd.c
> > index 2c981d443f27..3d74cfe7765b 100644
> > --- a/net/ethtool/pse-pd.c
> > +++ b/net/ethtool/pse-pd.c
> > @@ -86,7 +86,14 @@ static int pse_reply_size(const struct ethnl_req_info
> > *req_base, len += nla_total_size(sizeof(u32)); /* _C33_PSE_ADMIN_STATE */
> >  	if (st->c33_pw_status > 0)
> >  		len += nla_total_size(sizeof(u32)); /*
> > _C33_PSE_PW_D_STATUS */ -
> > +	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)
> > +		len += nla_total_size(sizeof(u8)); /* _C33_PSE_EXT_STATE */
> > +	if (st->c33_ext_state_info.__c33_pse_ext_substate)
> > +		len += nla_total_size(sizeof(u8)); /*
> > _C33_PSE_EXT_SUBSTATE */  
> 
> Substate can be properly decoded only if state is not zero.

Indeed, thanks for spotting that mistake.
 
> Please update Documentation/networking/ethtool-netlink.rst

Oh right, indeed. Forgot to add the netlink docs. 

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