[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZmaMGWMOvILHy8Iu@pengutronix.de>
Date: Mon, 10 Jun 2024 07:16:09 +0200
From: Oleksij Rempel <o.rempel@...gutronix.de>
To: Kory Maincent <kory.maincent@...tlin.com>
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
Subject: Re: [PATCH net-next v2 2/8] net: ethtool: pse-pd: Expand C33 PSE
status with class, power and extended state
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@...tlin.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?
> + 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.
Please provide comments describing how all of this states and substates
should be used.
> + ETHTOOL_C33_PSE_EXT_STATE_CONFIG,
> + ETHTOOL_C33_PSE_EXT_STATE_TEMP_ISSUE,
> +};
...
> /**
> * 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.
> /* 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.
> return len;
> }
>
> @@ -117,6 +124,26 @@ static int pse_fill_reply(struct sk_buff *skb,
> st->c33_pw_status))
> return -EMSGSIZE;
>
> + if (st->c33_pw_class > 0 &&
> + nla_put_u32(skb, ETHTOOL_A_C33_PSE_PW_CLASS,
> + st->c33_pw_class))
> + return -EMSGSIZE;
> +
> + if (st->c33_actual_pw > 0 &&
> + nla_put_u32(skb, ETHTOOL_A_C33_PSE_ACTUAL_PW,
> + st->c33_actual_pw))
> + return -EMSGSIZE;
> +
> + if (st->c33_ext_state_info.c33_pse_ext_state > 0 &&
> + nla_put_u8(skb, ETHTOOL_A_C33_PSE_EXT_STATE,
> + st->c33_ext_state_info.c33_pse_ext_state))
> + return -EMSGSIZE;
> +
> + if (st->c33_ext_state_info.__c33_pse_ext_substate > 0 &&
> + nla_put_u8(skb, ETHTOOL_A_C33_PSE_EXT_SUBSTATE,
> + st->c33_ext_state_info.__c33_pse_ext_substate))
> + return -EMSGSIZE;
ditto.
Please update Documentation/networking/ethtool-netlink.rst
--
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 |
Powered by blists - more mailing lists