[PATCH ethtool 0/3] Add extended link state

[PATCH ethtool 0/3] Add extended link state

[Amit Cohen]

Currently, device drivers can only indicate to user space if the network
link is up or down, without additional information.

This patch set expand link-state to allow these drivers to expose more
information to user space about the link state. The information can save
users' time when trying to understand why a link is not operationally up,
for example.

The above is achieved by extending the existing ethtool LINKSTATE_GET
command with attributes that carry the extended state.

For example, no link due to missing cable:

$ ethtool ethX
...
Link detected: no (No cable)

Beside the general extended state, drivers can pass additional
information about the link state using the sub-state field. For example:

$ ethtool ethX
...
Link detected: no (Autoneg, No partner detected)

Amit Cohen (3):
  netlink: expand ETHTOOL_LINKSTATE with extended state attributes
  common: add infrastructure to convert kernel values to userspace
    strings
  netlink: settings: expand linkstate_reply_cb() to support link
    extended state

 common.c                     | 171 +++++++++++++++++++++++++++++++++++
 common.h                     |   2 +
 netlink/desc-ethtool.c       |   2 +
 netlink/settings.c           |  59 +++++++++++-
 uapi/linux/ethtool_netlink.h |   2 +
 5 files changed, 233 insertions(+), 3 deletions(-)

-- 
2.20.1

[PATCH ethtool 1/3] netlink: expand ETHTOOL_LINKSTATE with extended state attributes

[Amit Cohen]

Add ETHTOOL_A_LINKSTATE_EXT_STATE to expose general extended state.

Add ETHTOOL_A_LINKSTATE_EXT_SUBSTATE to expose more information in
addition to the extended state.

Signed-off-by: Amit Cohen <amitc@mellanox.com>
---
 netlink/desc-ethtool.c       | 2 ++
 uapi/linux/ethtool_netlink.h | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/netlink/desc-ethtool.c b/netlink/desc-ethtool.c
index 98b898e..bce22e2 100644
--- a/netlink/desc-ethtool.c
+++ b/netlink/desc-ethtool.c
@@ -95,6 +95,8 @@ static const struct pretty_nla_desc __linkstate_desc[] = {
 	NLATTR_DESC_BOOL(ETHTOOL_A_LINKSTATE_LINK),
 	NLATTR_DESC_U32(ETHTOOL_A_LINKSTATE_SQI),
 	NLATTR_DESC_U32(ETHTOOL_A_LINKSTATE_SQI_MAX),
+	NLATTR_DESC_U8(ETHTOOL_A_LINKSTATE_EXT_STATE),
+	NLATTR_DESC_U8(ETHTOOL_A_LINKSTATE_EXT_SUBSTATE),
 };
 
 static const struct pretty_nla_desc __debug_desc[] = {
diff --git a/uapi/linux/ethtool_netlink.h b/uapi/linux/ethtool_netlink.h
index b18e7bc..0922ca6 100644
--- a/uapi/linux/ethtool_netlink.h
+++ b/uapi/linux/ethtool_netlink.h
@@ -236,6 +236,8 @@ enum {
 	ETHTOOL_A_LINKSTATE_LINK,		/* u8 */
 	ETHTOOL_A_LINKSTATE_SQI,		/* u32 */
 	ETHTOOL_A_LINKSTATE_SQI_MAX,		/* u32 */
+	ETHTOOL_A_LINKSTATE_EXT_STATE,		/* u8 */
+	ETHTOOL_A_LINKSTATE_EXT_SUBSTATE,	/* u8 */
 
 	/* add new constants above here */
 	__ETHTOOL_A_LINKSTATE_CNT,
-- 
2.20.1

[PATCH ethtool 2/3] common: add infrastructure to convert kernel values to userspace strings

[Amit Cohen]

Add enums and functions to covert extended state values sent from kernel to
appropriate strings to expose in userspace.

Signed-off-by: Amit Cohen <amitc@mellanox.com>
---
 common.c | 171 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 common.h |   2 +
 2 files changed, 173 insertions(+)

diff --git a/common.c b/common.c
index 2630e73..ac848f7 100644
--- a/common.c
+++ b/common.c
@@ -127,6 +127,177 @@ static char *unparse_wolopts(int wolopts)
 	return buf;
 }
 
+enum link_ext_state {
+	ETHTOOL_LINK_EXT_STATE_AUTONEG,
+	ETHTOOL_LINK_EXT_STATE_LINK_TRAINING_FAILURE,
+	ETHTOOL_LINK_EXT_STATE_LINK_LOGICAL_MISMATCH,
+	ETHTOOL_LINK_EXT_STATE_BAD_SIGNAL_INTEGRITY,
+	ETHTOOL_LINK_EXT_STATE_NO_CABLE,
+	ETHTOOL_LINK_EXT_STATE_CABLE_ISSUE,
+	ETHTOOL_LINK_EXT_STATE_EEPROM_ISSUE,
+	ETHTOOL_LINK_EXT_STATE_CALIBRATION_FAILURE,
+	ETHTOOL_LINK_EXT_STATE_POWER_BUDGET_EXCEEDED,
+	ETHTOOL_LINK_EXT_STATE_OVERHEAT,
+};
+
+enum link_ext_substate_autoneg {
+	ETHTOOL_LINK_EXT_SUBSTATE_AN_NO_PARTNER_DETECTED = 1,
+	ETHTOOL_LINK_EXT_SUBSTATE_AN_ACK_NOT_RECEIVED,
+	ETHTOOL_LINK_EXT_SUBSTATE_AN_NEXT_PAGE_EXCHANGE_FAILED,
+	ETHTOOL_LINK_EXT_SUBSTATE_AN_NO_PARTNER_DETECTED_FORCE_MODE,
+	ETHTOOL_LINK_EXT_SUBSTATE_AN_FEC_MISMATCH_DURING_OVERRIDE,
+	ETHTOOL_LINK_EXT_SUBSTATE_AN_NO_HCD,
+};
+
+enum link_ext_substate_link_training {
+	ETHTOOL_LINK_EXT_SUBSTATE_LT_KR_FRAME_LOCK_NOT_ACQUIRED = 1,
+	ETHTOOL_LINK_EXT_SUBSTATE_LT_KR_LINK_INHIBIT_TIMEOUT,
+	ETHTOOL_LINK_EXT_SUBSTATE_LT_KR_LINK_PARTNER_DID_NOT_SET_RECEIVER_READY,
+	ETHTOOL_LINK_EXT_SUBSTATE_LT_REMOTE_FAULT,
+};
+
+enum link_ext_substate_link_logical_mismatch {
+	ETHTOOL_LINK_EXT_SUBSTATE_LLM_PCS_DID_NOT_ACQUIRE_BLOCK_LOCK = 1,
+	ETHTOOL_LINK_EXT_SUBSTATE_LLM_PCS_DID_NOT_ACQUIRE_AM_LOCK,
+	ETHTOOL_LINK_EXT_SUBSTATE_LLM_PCS_DID_NOT_GET_ALIGN_STATUS,
+	ETHTOOL_LINK_EXT_SUBSTATE_LLM_FC_FEC_IS_NOT_LOCKED,
+	ETHTOOL_LINK_EXT_SUBSTATE_LLM_RS_FEC_IS_NOT_LOCKED,
+};
+
+enum link_ext_substate_bad_signal_integrity {
+	ETHTOOL_LINK_EXT_SUBSTATE_BSI_LARGE_NUMBER_OF_PHYSICAL_ERRORS = 1,
+	ETHTOOL_LINK_EXT_SUBSTATE_BSI_UNSUPPORTED_RATE,
+};
+
+enum ethtool_link_ext_substate_cable_issue {
+	ETHTOOL_LINK_EXT_SUBSTATE_CI_UNSUPPORTED_CABLE = 1,
+	ETHTOOL_LINK_EXT_SUBSTATE_CI_CABLE_TEST_FAILURE,
+};
+
+const char *link_ext_state_get(uint8_t link_ext_state_val)
+{
+	switch (link_ext_state_val) {
+	case ETHTOOL_LINK_EXT_STATE_AUTONEG:
+		return "Autoneg";
+	case ETHTOOL_LINK_EXT_STATE_LINK_TRAINING_FAILURE:
+		return "Link training failure";
+	case ETHTOOL_LINK_EXT_STATE_LINK_LOGICAL_MISMATCH:
+		return "Logical mismatch";
+	case ETHTOOL_LINK_EXT_STATE_BAD_SIGNAL_INTEGRITY:
+		return "Bad signal integrity";
+	case ETHTOOL_LINK_EXT_STATE_NO_CABLE:
+		return "No cable";
+	case ETHTOOL_LINK_EXT_STATE_CABLE_ISSUE:
+		return "Cable issue";
+	case ETHTOOL_LINK_EXT_STATE_EEPROM_ISSUE:
+		return "EEPROM issue";
+	case ETHTOOL_LINK_EXT_STATE_CALIBRATION_FAILURE:
+		return "Calibration failure";
+	case ETHTOOL_LINK_EXT_STATE_POWER_BUDGET_EXCEEDED:
+		return "Power budget exceeded";
+	case ETHTOOL_LINK_EXT_STATE_OVERHEAT:
+		return "Overheat";
+	default:
+		return NULL;
+	}
+}
+
+static const char *autoneg_link_ext_substate_get(uint8_t link_ext_substate_val)
+{
+	switch (link_ext_substate_val) {
+	case ETHTOOL_LINK_EXT_SUBSTATE_AN_NO_PARTNER_DETECTED:
+		return "No partner detected";
+	case ETHTOOL_LINK_EXT_SUBSTATE_AN_ACK_NOT_RECEIVED:
+		return "Ack not received";
+	case ETHTOOL_LINK_EXT_SUBSTATE_AN_NEXT_PAGE_EXCHANGE_FAILED:
+		return "Next page exchange failed";
+	case ETHTOOL_LINK_EXT_SUBSTATE_AN_NO_PARTNER_DETECTED_FORCE_MODE:
+		return "No partner detected during force mode";
+	case ETHTOOL_LINK_EXT_SUBSTATE_AN_FEC_MISMATCH_DURING_OVERRIDE:
+		return "FEC mismatch during override";
+	case ETHTOOL_LINK_EXT_SUBSTATE_AN_NO_HCD:
+		return "No HCD";
+	default:
+		return NULL;
+	}
+}
+
+static const char *link_training_link_ext_substate_get(uint8_t link_ext_substate_val)
+{
+	switch (link_ext_substate_val) {
+	case ETHTOOL_LINK_EXT_SUBSTATE_LT_KR_FRAME_LOCK_NOT_ACQUIRED:
+		return "KR frame lock not acquired";
+	case ETHTOOL_LINK_EXT_SUBSTATE_LT_KR_LINK_INHIBIT_TIMEOUT:
+		return "KR link inhibit timeout";
+	case ETHTOOL_LINK_EXT_SUBSTATE_LT_KR_LINK_PARTNER_DID_NOT_SET_RECEIVER_READY:
+		return "KR Link partner did not set receiver ready";
+	case ETHTOOL_LINK_EXT_SUBSTATE_LT_REMOTE_FAULT:
+		return "Remote side is not ready yet";
+	default:
+		return NULL;
+	}
+}
+
+static const char *link_logical_mismatch_link_ext_substate_get(uint8_t link_ext_substate_val)
+{
+	switch (link_ext_substate_val) {
+	case ETHTOOL_LINK_EXT_SUBSTATE_LLM_PCS_DID_NOT_ACQUIRE_BLOCK_LOCK:
+		return "PCS did not acquire block lock";
+	case ETHTOOL_LINK_EXT_SUBSTATE_LLM_PCS_DID_NOT_ACQUIRE_AM_LOCK:
+		return "PCS did not acquire AM lock";
+	case ETHTOOL_LINK_EXT_SUBSTATE_LLM_PCS_DID_NOT_GET_ALIGN_STATUS:
+		return "PCS did not get align_status";
+	case ETHTOOL_LINK_EXT_SUBSTATE_LLM_FC_FEC_IS_NOT_LOCKED:
+		return "FC FEC is not locked";
+	case ETHTOOL_LINK_EXT_SUBSTATE_LLM_RS_FEC_IS_NOT_LOCKED:
+		return "RS FEC is not locked";
+	default:
+		return NULL;
+	}
+}
+
+static const char *bad_signal_integrity_link_ext_substate_get(uint8_t link_ext_substate_val)
+{
+	switch (link_ext_substate_val) {
+	case ETHTOOL_LINK_EXT_SUBSTATE_BSI_LARGE_NUMBER_OF_PHYSICAL_ERRORS:
+		return "Large number of physical errors";
+	case ETHTOOL_LINK_EXT_SUBSTATE_BSI_UNSUPPORTED_RATE:
+		return "Unsupported rate";
+	default:
+		return NULL;
+	}
+}
+
+static const char *cable_issue_link_ext_substate_get(uint8_t link_ext_substate_val)
+{
+	switch (link_ext_substate_val) {
+	case ETHTOOL_LINK_EXT_SUBSTATE_CI_UNSUPPORTED_CABLE:
+		return "Unsupported cable";
+	case ETHTOOL_LINK_EXT_SUBSTATE_CI_CABLE_TEST_FAILURE:
+		return "Cable test failure";
+	default:
+		return NULL;
+	}
+}
+
+const char *link_ext_substate_get(uint8_t link_ext_state_val, uint8_t link_ext_substate_val)
+{
+	switch (link_ext_state_val) {
+	case ETHTOOL_LINK_EXT_STATE_AUTONEG:
+		return autoneg_link_ext_substate_get(link_ext_substate_val);
+	case ETHTOOL_LINK_EXT_STATE_LINK_TRAINING_FAILURE:
+		return link_training_link_ext_substate_get(link_ext_substate_val);
+	case ETHTOOL_LINK_EXT_STATE_LINK_LOGICAL_MISMATCH:
+		return link_logical_mismatch_link_ext_substate_get(link_ext_substate_val);
+	case ETHTOOL_LINK_EXT_STATE_BAD_SIGNAL_INTEGRITY:
+		return bad_signal_integrity_link_ext_substate_get(link_ext_substate_val);
+	case ETHTOOL_LINK_EXT_STATE_CABLE_ISSUE:
+		return cable_issue_link_ext_substate_get(link_ext_substate_val);
+	default:
+		return NULL;
+	}
+}
+
 int dump_wol(struct ethtool_wolinfo *wol)
 {
 	fprintf(stdout, "	Supports Wake-on: %s\n",
diff --git a/common.h b/common.h
index b74fdfa..26bec2f 100644
--- a/common.h
+++ b/common.h
@@ -39,6 +39,8 @@ struct off_flag_def {
 extern const struct off_flag_def off_flag_def[OFF_FLAG_DEF_SIZE];
 
 void print_flags(const struct flag_info *info, unsigned int n_info, u32 value);
+const char *link_ext_state_get(uint8_t link_ext_state_val);
+const char *link_ext_substate_get(uint8_t link_ext_state_val, uint8_t link_ext_substate_val);
 int dump_wol(struct ethtool_wolinfo *wol);
 void dump_mdix(u8 mdix, u8 mdix_ctrl);
 
-- 
2.20.1

[PATCH ethtool 3/3] netlink: settings: expand linkstate_reply_cb() to support link extended state

[Amit Cohen]

Print extended state in addition to link state.

In case that extended state is not provided, print state only.
If extended substate is provided in addition to the extended state,
print it also.

Signed-off-by: Amit Cohen <amitc@mellanox.com>
---
 netlink/settings.c | 59 +++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 56 insertions(+), 3 deletions(-)

diff --git a/netlink/settings.c b/netlink/settings.c
index 35ba2f5..a4d1908 100644
--- a/netlink/settings.c
+++ b/netlink/settings.c
@@ -604,6 +604,57 @@ int linkinfo_reply_cb(const struct nlmsghdr *nlhdr, void *data)
 	return MNL_CB_OK;
 }
 
+int linkstate_link_ext_substate_print(const struct nlattr *tb[],
+				      struct nl_context *nlctx, uint8_t link_val,
+				      uint8_t link_ext_state_val,
+				      const char *link_ext_state_str)
+{
+	uint8_t link_ext_substate_val;
+	const char *link_ext_substate_str;
+
+	if (!tb[ETHTOOL_A_LINKSTATE_EXT_SUBSTATE])
+		return -ENODATA;
+
+	link_ext_substate_val = mnl_attr_get_u8(tb[ETHTOOL_A_LINKSTATE_EXT_SUBSTATE]);
+
+	link_ext_substate_str = link_ext_substate_get(link_ext_state_val, link_ext_substate_val);
+	if (!link_ext_substate_str)
+		return -ENODATA;
+
+	print_banner(nlctx);
+	printf("\tLink detected: %s (%s, %s)\n", link_val ? "yes" : "no",
+	       link_ext_state_str, link_ext_substate_str);
+
+	return 0;
+}
+
+int linkstate_link_ext_state_print(const struct nlattr *tb[],
+				   struct nl_context *nlctx, uint8_t link_val)
+{
+	uint8_t link_ext_state_val;
+	const char *link_ext_state_str;
+	int ret;
+
+	if (!tb[ETHTOOL_A_LINKSTATE_EXT_STATE])
+		return -ENODATA;
+
+	link_ext_state_val = mnl_attr_get_u8(tb[ETHTOOL_A_LINKSTATE_EXT_STATE]);
+
+	link_ext_state_str = link_ext_state_get(link_ext_state_val);
+	if (!link_ext_state_str)
+		return -ENODATA;
+
+	ret = linkstate_link_ext_substate_print(tb, nlctx, link_val, link_ext_state_val,
+						link_ext_state_str);
+	if (ret < 0) {
+		print_banner(nlctx);
+		printf("\tLink detected: %s (%s)\n", link_val ? "yes" : "no",
+		       link_ext_state_str);
+	}
+
+	return 0;
+}
+
 int linkstate_reply_cb(const struct nlmsghdr *nlhdr, void *data)
 {
 	const struct nlattr *tb[ETHTOOL_A_LINKSTATE_MAX + 1] = {};
@@ -622,9 +673,11 @@ int linkstate_reply_cb(const struct nlmsghdr *nlhdr, void *data)
 
 	if (tb[ETHTOOL_A_LINKSTATE_LINK]) {
 		uint8_t val = mnl_attr_get_u8(tb[ETHTOOL_A_LINKSTATE_LINK]);
-
-		print_banner(nlctx);
-		printf("\tLink detected: %s\n", val ? "yes" : "no");
+		ret = linkstate_link_ext_state_print(tb, nlctx, val);
+		if (ret < 0) {
+			print_banner(nlctx);
+			printf("\tLink detected: %s\n", val ? "yes" : "no");
+		}
 	}
 
 	if (tb[ETHTOOL_A_LINKSTATE_SQI]) {
-- 
2.20.1

Re: [PATCH ethtool 1/3] netlink: expand ETHTOOL_LINKSTATE with extended state attributes

[Michal Kubecek]

On Tue, Jun 30, 2020 at 12:24:10PM +0300, Amit Cohen wrote:
> Add ETHTOOL_A_LINKSTATE_EXT_STATE to expose general extended state.
> 
> Add ETHTOOL_A_LINKSTATE_EXT_SUBSTATE to expose more information in
> addition to the extended state.
> 
> Signed-off-by: Amit Cohen <amitc@mellanox.com>
> ---
>  netlink/desc-ethtool.c       | 2 ++
>  uapi/linux/ethtool_netlink.h | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/netlink/desc-ethtool.c b/netlink/desc-ethtool.c
> index 98b898e..bce22e2 100644
> --- a/netlink/desc-ethtool.c
> +++ b/netlink/desc-ethtool.c
> @@ -95,6 +95,8 @@ static const struct pretty_nla_desc __linkstate_desc[] = {
>  	NLATTR_DESC_BOOL(ETHTOOL_A_LINKSTATE_LINK),
>  	NLATTR_DESC_U32(ETHTOOL_A_LINKSTATE_SQI),
>  	NLATTR_DESC_U32(ETHTOOL_A_LINKSTATE_SQI_MAX),
> +	NLATTR_DESC_U8(ETHTOOL_A_LINKSTATE_EXT_STATE),
> +	NLATTR_DESC_U8(ETHTOOL_A_LINKSTATE_EXT_SUBSTATE),
>  };
>  
>  static const struct pretty_nla_desc __debug_desc[] = {
> diff --git a/uapi/linux/ethtool_netlink.h b/uapi/linux/ethtool_netlink.h
> index b18e7bc..0922ca6 100644
> --- a/uapi/linux/ethtool_netlink.h
> +++ b/uapi/linux/ethtool_netlink.h
> @@ -236,6 +236,8 @@ enum {
>  	ETHTOOL_A_LINKSTATE_LINK,		/* u8 */
>  	ETHTOOL_A_LINKSTATE_SQI,		/* u32 */
>  	ETHTOOL_A_LINKSTATE_SQI_MAX,		/* u32 */
> +	ETHTOOL_A_LINKSTATE_EXT_STATE,		/* u8 */
> +	ETHTOOL_A_LINKSTATE_EXT_SUBSTATE,	/* u8 */
>  
>  	/* add new constants above here */
>  	__ETHTOOL_A_LINKSTATE_CNT,

Please do not mix uapi header updates with other changes. Once the
kernel counterpart is in net-next, update all headers in uapi/ to
a net-next snapshot as described at

  https://www.kernel.org/pub/software/network/ethtool/devel.html

and use that as first commit of your series.

Michal

Re: [PATCH ethtool 2/3] common: add infrastructure to convert kernel values to userspace strings

[Michal Kubecek]

On Tue, Jun 30, 2020 at 12:24:11PM +0300, Amit Cohen wrote:
> Add enums and functions to covert extended state values sent from kernel to
> appropriate strings to expose in userspace.
> 
> Signed-off-by: Amit Cohen <amitc@mellanox.com>
> ---
>  common.c | 171 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  common.h |   2 +
>  2 files changed, 173 insertions(+)
> 
> diff --git a/common.c b/common.c
> index 2630e73..ac848f7 100644
> --- a/common.c
> +++ b/common.c
> @@ -127,6 +127,177 @@ static char *unparse_wolopts(int wolopts)
>  	return buf;
>  }
>  
> +enum link_ext_state {
> +	ETHTOOL_LINK_EXT_STATE_AUTONEG,
> +	ETHTOOL_LINK_EXT_STATE_LINK_TRAINING_FAILURE,
> +	ETHTOOL_LINK_EXT_STATE_LINK_LOGICAL_MISMATCH,
> +	ETHTOOL_LINK_EXT_STATE_BAD_SIGNAL_INTEGRITY,
> +	ETHTOOL_LINK_EXT_STATE_NO_CABLE,
> +	ETHTOOL_LINK_EXT_STATE_CABLE_ISSUE,
> +	ETHTOOL_LINK_EXT_STATE_EEPROM_ISSUE,
> +	ETHTOOL_LINK_EXT_STATE_CALIBRATION_FAILURE,
> +	ETHTOOL_LINK_EXT_STATE_POWER_BUDGET_EXCEEDED,
> +	ETHTOOL_LINK_EXT_STATE_OVERHEAT,
> +};
> +
> +enum link_ext_substate_autoneg {
> +	ETHTOOL_LINK_EXT_SUBSTATE_AN_NO_PARTNER_DETECTED = 1,
> +	ETHTOOL_LINK_EXT_SUBSTATE_AN_ACK_NOT_RECEIVED,
> +	ETHTOOL_LINK_EXT_SUBSTATE_AN_NEXT_PAGE_EXCHANGE_FAILED,
> +	ETHTOOL_LINK_EXT_SUBSTATE_AN_NO_PARTNER_DETECTED_FORCE_MODE,
> +	ETHTOOL_LINK_EXT_SUBSTATE_AN_FEC_MISMATCH_DURING_OVERRIDE,
> +	ETHTOOL_LINK_EXT_SUBSTATE_AN_NO_HCD,
> +};
> +
> +enum link_ext_substate_link_training {
> +	ETHTOOL_LINK_EXT_SUBSTATE_LT_KR_FRAME_LOCK_NOT_ACQUIRED = 1,
> +	ETHTOOL_LINK_EXT_SUBSTATE_LT_KR_LINK_INHIBIT_TIMEOUT,
> +	ETHTOOL_LINK_EXT_SUBSTATE_LT_KR_LINK_PARTNER_DID_NOT_SET_RECEIVER_READY,
> +	ETHTOOL_LINK_EXT_SUBSTATE_LT_REMOTE_FAULT,
> +};
> +
> +enum link_ext_substate_link_logical_mismatch {
> +	ETHTOOL_LINK_EXT_SUBSTATE_LLM_PCS_DID_NOT_ACQUIRE_BLOCK_LOCK = 1,
> +	ETHTOOL_LINK_EXT_SUBSTATE_LLM_PCS_DID_NOT_ACQUIRE_AM_LOCK,
> +	ETHTOOL_LINK_EXT_SUBSTATE_LLM_PCS_DID_NOT_GET_ALIGN_STATUS,
> +	ETHTOOL_LINK_EXT_SUBSTATE_LLM_FC_FEC_IS_NOT_LOCKED,
> +	ETHTOOL_LINK_EXT_SUBSTATE_LLM_RS_FEC_IS_NOT_LOCKED,
> +};
> +
> +enum link_ext_substate_bad_signal_integrity {
> +	ETHTOOL_LINK_EXT_SUBSTATE_BSI_LARGE_NUMBER_OF_PHYSICAL_ERRORS = 1,
> +	ETHTOOL_LINK_EXT_SUBSTATE_BSI_UNSUPPORTED_RATE,
> +};
> +
> +enum ethtool_link_ext_substate_cable_issue {
> +	ETHTOOL_LINK_EXT_SUBSTATE_CI_UNSUPPORTED_CABLE = 1,
> +	ETHTOOL_LINK_EXT_SUBSTATE_CI_CABLE_TEST_FAILURE,
> +};

Once you update uapi/linux/ethtool.h, these constants will be defined
there so that there is no need to duplicate them here.

> +
> +const char *link_ext_state_get(uint8_t link_ext_state_val)
> +{
> +	switch (link_ext_state_val) {
> +	case ETHTOOL_LINK_EXT_STATE_AUTONEG:
> +		return "Autoneg";
> +	case ETHTOOL_LINK_EXT_STATE_LINK_TRAINING_FAILURE:
> +		return "Link training failure";
> +	case ETHTOOL_LINK_EXT_STATE_LINK_LOGICAL_MISMATCH:
> +		return "Logical mismatch";
> +	case ETHTOOL_LINK_EXT_STATE_BAD_SIGNAL_INTEGRITY:
> +		return "Bad signal integrity";
> +	case ETHTOOL_LINK_EXT_STATE_NO_CABLE:
> +		return "No cable";
> +	case ETHTOOL_LINK_EXT_STATE_CABLE_ISSUE:
> +		return "Cable issue";
> +	case ETHTOOL_LINK_EXT_STATE_EEPROM_ISSUE:
> +		return "EEPROM issue";
> +	case ETHTOOL_LINK_EXT_STATE_CALIBRATION_FAILURE:
> +		return "Calibration failure";
> +	case ETHTOOL_LINK_EXT_STATE_POWER_BUDGET_EXCEEDED:
> +		return "Power budget exceeded";
> +	case ETHTOOL_LINK_EXT_STATE_OVERHEAT:
> +		return "Overheat";
> +	default:
> +		return NULL;
> +	}
> +}
> +
> +static const char *autoneg_link_ext_substate_get(uint8_t link_ext_substate_val)
> +{
> +	switch (link_ext_substate_val) {
> +	case ETHTOOL_LINK_EXT_SUBSTATE_AN_NO_PARTNER_DETECTED:
> +		return "No partner detected";
> +	case ETHTOOL_LINK_EXT_SUBSTATE_AN_ACK_NOT_RECEIVED:
> +		return "Ack not received";
> +	case ETHTOOL_LINK_EXT_SUBSTATE_AN_NEXT_PAGE_EXCHANGE_FAILED:
> +		return "Next page exchange failed";
> +	case ETHTOOL_LINK_EXT_SUBSTATE_AN_NO_PARTNER_DETECTED_FORCE_MODE:
> +		return "No partner detected during force mode";
> +	case ETHTOOL_LINK_EXT_SUBSTATE_AN_FEC_MISMATCH_DURING_OVERRIDE:
> +		return "FEC mismatch during override";
> +	case ETHTOOL_LINK_EXT_SUBSTATE_AN_NO_HCD:
> +		return "No HCD";
> +	default:
> +		return NULL;
> +	}
> +}
> +
> +static const char *link_training_link_ext_substate_get(uint8_t link_ext_substate_val)
> +{
> +	switch (link_ext_substate_val) {
> +	case ETHTOOL_LINK_EXT_SUBSTATE_LT_KR_FRAME_LOCK_NOT_ACQUIRED:
> +		return "KR frame lock not acquired";
> +	case ETHTOOL_LINK_EXT_SUBSTATE_LT_KR_LINK_INHIBIT_TIMEOUT:
> +		return "KR link inhibit timeout";
> +	case ETHTOOL_LINK_EXT_SUBSTATE_LT_KR_LINK_PARTNER_DID_NOT_SET_RECEIVER_READY:
> +		return "KR Link partner did not set receiver ready";
> +	case ETHTOOL_LINK_EXT_SUBSTATE_LT_REMOTE_FAULT:
> +		return "Remote side is not ready yet";
> +	default:
> +		return NULL;
> +	}
> +}
> +
> +static const char *link_logical_mismatch_link_ext_substate_get(uint8_t link_ext_substate_val)
> +{
> +	switch (link_ext_substate_val) {
> +	case ETHTOOL_LINK_EXT_SUBSTATE_LLM_PCS_DID_NOT_ACQUIRE_BLOCK_LOCK:
> +		return "PCS did not acquire block lock";
> +	case ETHTOOL_LINK_EXT_SUBSTATE_LLM_PCS_DID_NOT_ACQUIRE_AM_LOCK:
> +		return "PCS did not acquire AM lock";
> +	case ETHTOOL_LINK_EXT_SUBSTATE_LLM_PCS_DID_NOT_GET_ALIGN_STATUS:
> +		return "PCS did not get align_status";
> +	case ETHTOOL_LINK_EXT_SUBSTATE_LLM_FC_FEC_IS_NOT_LOCKED:
> +		return "FC FEC is not locked";
> +	case ETHTOOL_LINK_EXT_SUBSTATE_LLM_RS_FEC_IS_NOT_LOCKED:
> +		return "RS FEC is not locked";
> +	default:
> +		return NULL;
> +	}
> +}
> +
> +static const char *bad_signal_integrity_link_ext_substate_get(uint8_t link_ext_substate_val)
> +{
> +	switch (link_ext_substate_val) {
> +	case ETHTOOL_LINK_EXT_SUBSTATE_BSI_LARGE_NUMBER_OF_PHYSICAL_ERRORS:
> +		return "Large number of physical errors";
> +	case ETHTOOL_LINK_EXT_SUBSTATE_BSI_UNSUPPORTED_RATE:
> +		return "Unsupported rate";
> +	default:
> +		return NULL;
> +	}
> +}
> +
> +static const char *cable_issue_link_ext_substate_get(uint8_t link_ext_substate_val)
> +{
> +	switch (link_ext_substate_val) {
> +	case ETHTOOL_LINK_EXT_SUBSTATE_CI_UNSUPPORTED_CABLE:
> +		return "Unsupported cable";
> +	case ETHTOOL_LINK_EXT_SUBSTATE_CI_CABLE_TEST_FAILURE:
> +		return "Cable test failure";
> +	default:
> +		return NULL;
> +	}
> +}
> +
> +const char *link_ext_substate_get(uint8_t link_ext_state_val, uint8_t link_ext_substate_val)
> +{
> +	switch (link_ext_state_val) {
> +	case ETHTOOL_LINK_EXT_STATE_AUTONEG:
> +		return autoneg_link_ext_substate_get(link_ext_substate_val);
> +	case ETHTOOL_LINK_EXT_STATE_LINK_TRAINING_FAILURE:
> +		return link_training_link_ext_substate_get(link_ext_substate_val);
> +	case ETHTOOL_LINK_EXT_STATE_LINK_LOGICAL_MISMATCH:
> +		return link_logical_mismatch_link_ext_substate_get(link_ext_substate_val);
> +	case ETHTOOL_LINK_EXT_STATE_BAD_SIGNAL_INTEGRITY:
> +		return bad_signal_integrity_link_ext_substate_get(link_ext_substate_val);
> +	case ETHTOOL_LINK_EXT_STATE_CABLE_ISSUE:
> +		return cable_issue_link_ext_substate_get(link_ext_substate_val);
> +	default:
> +		return NULL;
> +	}
> +}

Files common.c and common.h contain common code used by both ioctl and
netlink implementations. These helpers are only going to be used by
netlink code so that you can put them into netlink/settings.c

Personally, I would find string tables easier to read and update than
switch statements but that's a matter of taste so I don't feel strong
about it.

Michal

> diff --git a/common.h b/common.h
> index b74fdfa..26bec2f 100644
> --- a/common.h
> +++ b/common.h
> @@ -39,6 +39,8 @@ struct off_flag_def {
>  extern const struct off_flag_def off_flag_def[OFF_FLAG_DEF_SIZE];
>  
>  void print_flags(const struct flag_info *info, unsigned int n_info, u32 value);
> +const char *link_ext_state_get(uint8_t link_ext_state_val);
> +const char *link_ext_substate_get(uint8_t link_ext_state_val, uint8_t link_ext_substate_val);
>  int dump_wol(struct ethtool_wolinfo *wol);
>  void dump_mdix(u8 mdix, u8 mdix_ctrl);
>  
> -- 
> 2.20.1
> 

Re: [PATCH ethtool 3/3] netlink: settings: expand linkstate_reply_cb() to support link extended state

[Michal Kubecek]

On Tue, Jun 30, 2020 at 12:24:12PM +0300, Amit Cohen wrote:
> Print extended state in addition to link state.
> 
> In case that extended state is not provided, print state only.
> If extended substate is provided in addition to the extended state,
> print it also.
> 
> Signed-off-by: Amit Cohen <amitc@mellanox.com>
> ---
>  netlink/settings.c | 59 +++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 56 insertions(+), 3 deletions(-)
> 
> diff --git a/netlink/settings.c b/netlink/settings.c
> index 35ba2f5..a4d1908 100644
> --- a/netlink/settings.c
> +++ b/netlink/settings.c
> @@ -604,6 +604,57 @@ int linkinfo_reply_cb(const struct nlmsghdr *nlhdr, void *data)
>  	return MNL_CB_OK;
>  }
>  
> +int linkstate_link_ext_substate_print(const struct nlattr *tb[],
> +				      struct nl_context *nlctx, uint8_t link_val,
> +				      uint8_t link_ext_state_val,
> +				      const char *link_ext_state_str)
> +{
> +	uint8_t link_ext_substate_val;
> +	const char *link_ext_substate_str;
> +
> +	if (!tb[ETHTOOL_A_LINKSTATE_EXT_SUBSTATE])
> +		return -ENODATA;
> +
> +	link_ext_substate_val = mnl_attr_get_u8(tb[ETHTOOL_A_LINKSTATE_EXT_SUBSTATE]);
> +
> +	link_ext_substate_str = link_ext_substate_get(link_ext_state_val, link_ext_substate_val);
> +	if (!link_ext_substate_str)
> +		return -ENODATA;

This does not distinguish between missing attribute (substate not
provided by kernel) and unknown value which can happen when older
ethtool version is used on a newer kernel which returns a new substate
not known by ethtool. IMHO we should fall back to reporting the numeric
value in such case rather than behaving as if the information were not
provided.

> +
> +	print_banner(nlctx);
> +	printf("\tLink detected: %s (%s, %s)\n", link_val ? "yes" : "no",
> +	       link_ext_state_str, link_ext_substate_str);
> +
> +	return 0;
> +}
> +
> +int linkstate_link_ext_state_print(const struct nlattr *tb[],
> +				   struct nl_context *nlctx, uint8_t link_val)
> +{
> +	uint8_t link_ext_state_val;
> +	const char *link_ext_state_str;
> +	int ret;
> +
> +	if (!tb[ETHTOOL_A_LINKSTATE_EXT_STATE])
> +		return -ENODATA;
> +
> +	link_ext_state_val = mnl_attr_get_u8(tb[ETHTOOL_A_LINKSTATE_EXT_STATE]);
> +
> +	link_ext_state_str = link_ext_state_get(link_ext_state_val);
> +	if (!link_ext_state_str)
> +		return -ENODATA;

The same problem as above.

> +
> +	ret = linkstate_link_ext_substate_print(tb, nlctx, link_val, link_ext_state_val,
> +						link_ext_state_str);
> +	if (ret < 0) {
> +		print_banner(nlctx);
> +		printf("\tLink detected: %s (%s)\n", link_val ? "yes" : "no",
> +		       link_ext_state_str);
> +	}
> +
> +	return 0;
> +}
> +
>  int linkstate_reply_cb(const struct nlmsghdr *nlhdr, void *data)
>  {
>  	const struct nlattr *tb[ETHTOOL_A_LINKSTATE_MAX + 1] = {};
> @@ -622,9 +673,11 @@ int linkstate_reply_cb(const struct nlmsghdr *nlhdr, void *data)
>  
>  	if (tb[ETHTOOL_A_LINKSTATE_LINK]) {
>  		uint8_t val = mnl_attr_get_u8(tb[ETHTOOL_A_LINKSTATE_LINK]);
> -
> -		print_banner(nlctx);
> -		printf("\tLink detected: %s\n", val ? "yes" : "no");
> +		ret = linkstate_link_ext_state_print(tb, nlctx, val);
> +		if (ret < 0) {
> +			print_banner(nlctx);
> +			printf("\tLink detected: %s\n", val ? "yes" : "no");
> +		}
>  	}
>  
>  	if (tb[ETHTOOL_A_LINKSTATE_SQI]) {

It's a bit impractical to have the banner and first part of the line
printed in three differente places. How about this:

  - linkstate_reply_cb() calls print_banner() and prints what it does
    now, only without the newline
  - linkstate_link_ext_state_print() prints " (%s" or " (%u" with
    extended state if it is provided or bails out if not
  - linkstate_link_ext_substate_print() prints ", %s" or ", %u" with
    extended substate if it is provided
  - linkstate_link_ext_state_print() prints ")"
  - linkstate_reply_cb() prints the newline

Michal