[PATCH v2] ethtool: add support for ETHTOOL_A_CABLE_FAULT_LENGTH_SRC and ETHTOOL_A_CABLE_RESULT_SRC

[PATCH v2] ethtool: add support for ETHTOOL_A_CABLE_FAULT_LENGTH_SRC and ETHTOOL_A_CABLE_RESULT_SRC

[Oleksij Rempel]

Extend cable test output to include source information, supporting
diagnostic technologies like TDR (Time Domain Reflectometry) and ALCD
(Active Link Cable Diagnostic). The source is displayed optionally at
the end of each result or fault length line.

TDR requires interrupting the active link to measure parameters like
fault location, while ALCD can operate on an active link to provide
details like cable length without disruption.

Example output:
Pair B code Open Circuit, source: TDR
Pair B, fault length: 8.00m, source: TDR

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
changes v2:
- s/NLATTR_DESC_U8/NLATTR_DESC_U32
---
 netlink/cable_test.c   | 39 +++++++++++++++++++++++++++++++++------
 netlink/desc-ethtool.c |  2 ++
 2 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/netlink/cable_test.c b/netlink/cable_test.c
index ba21c6cd31e4..0a1c42010114 100644
--- a/netlink/cable_test.c
+++ b/netlink/cable_test.c
@@ -18,7 +18,7 @@ struct cable_test_context {
 };
 
 static int nl_get_cable_test_result(const struct nlattr *nest, uint8_t *pair,
-				    uint16_t *code)
+				    uint16_t *code, uint32_t *src)
 {
 	const struct nlattr *tb[ETHTOOL_A_CABLE_RESULT_MAX+1] = {};
 	DECLARE_ATTR_TB_INFO(tb);
@@ -32,12 +32,15 @@ static int nl_get_cable_test_result(const struct nlattr *nest, uint8_t *pair,
 
 	*pair = mnl_attr_get_u8(tb[ETHTOOL_A_CABLE_RESULT_PAIR]);
 	*code = mnl_attr_get_u8(tb[ETHTOOL_A_CABLE_RESULT_CODE]);
+	if (tb[ETHTOOL_A_CABLE_RESULT_CODE])
+		*src = mnl_attr_get_u32(tb[ETHTOOL_A_CABLE_RESULT_SRC]);
 
 	return 0;
 }
 
 static int nl_get_cable_test_fault_length(const struct nlattr *nest,
-					  uint8_t *pair, unsigned int *cm)
+					  uint8_t *pair, unsigned int *cm,
+					  uint32_t *src)
 {
 	const struct nlattr *tb[ETHTOOL_A_CABLE_FAULT_LENGTH_MAX+1] = {};
 	DECLARE_ATTR_TB_INFO(tb);
@@ -51,6 +54,8 @@ static int nl_get_cable_test_fault_length(const struct nlattr *nest,
 
 	*pair = mnl_attr_get_u8(tb[ETHTOOL_A_CABLE_FAULT_LENGTH_PAIR]);
 	*cm = mnl_attr_get_u32(tb[ETHTOOL_A_CABLE_FAULT_LENGTH_CM]);
+	if (tb[ETHTOOL_A_CABLE_FAULT_LENGTH_CM])
+		*src = mnl_attr_get_u32(tb[ETHTOOL_A_CABLE_FAULT_LENGTH_SRC]);
 
 	return 0;
 }
@@ -88,33 +93,55 @@ static char *nl_pair2txt(uint8_t pair)
 	}
 }
 
+static char *nl_src2txt(uint32_t src)
+{
+	switch (src) {
+	case ETHTOOL_A_CABLE_INF_SRC_TDR:
+		return "TDR";
+	case ETHTOOL_A_CABLE_INF_SRC_ALCD:
+		return "ALCD";
+	default:
+		return "Unknown";
+	}
+}
+
 static int nl_cable_test_ntf_attr(struct nlattr *evattr)
 {
 	unsigned int cm;
+	uint32_t src = UINT32_MAX;
 	uint16_t code;
 	uint8_t pair;
 	int ret;
 
 	switch (mnl_attr_get_type(evattr)) {
 	case ETHTOOL_A_CABLE_NEST_RESULT:
-		ret = nl_get_cable_test_result(evattr, &pair, &code);
+		ret = nl_get_cable_test_result(evattr, &pair, &code, &src);
 		if (ret < 0)
 			return ret;
 
 		open_json_object(NULL);
 		print_string(PRINT_ANY, "pair", "%s ", nl_pair2txt(pair));
-		print_string(PRINT_ANY, "code", "code %s\n", nl_code2txt(code));
+		print_string(PRINT_ANY, "code", "code %s", nl_code2txt(code));
+		if (src != UINT32_MAX)
+			print_string(PRINT_ANY, "src", ", source: %s",
+				     nl_src2txt(src));
+		print_nl();
 		close_json_object();
 		break;
 
 	case ETHTOOL_A_CABLE_NEST_FAULT_LENGTH:
-		ret = nl_get_cable_test_fault_length(evattr, &pair, &cm);
+		ret = nl_get_cable_test_fault_length(evattr, &pair, &cm, &src);
 		if (ret < 0)
 			return ret;
 		open_json_object(NULL);
 		print_string(PRINT_ANY, "pair", "%s, ", nl_pair2txt(pair));
-		print_float(PRINT_ANY, "length", "fault length: %0.2fm\n",
+		print_float(PRINT_ANY, "length", "fault length: %0.2fm",
 			    (float)cm / 100);
+		if (src != UINT32_MAX)
+			print_string(PRINT_ANY, "src", ", source: %s",
+				     nl_src2txt(src));
+		print_nl();
+
 		close_json_object();
 		break;
 	}
diff --git a/netlink/desc-ethtool.c b/netlink/desc-ethtool.c
index 5c0e1c6f433d..32a9eb36bde6 100644
--- a/netlink/desc-ethtool.c
+++ b/netlink/desc-ethtool.c
@@ -252,12 +252,14 @@ static const struct pretty_nla_desc __cable_test_result_desc[] = {
 	NLATTR_DESC_INVALID(ETHTOOL_A_CABLE_RESULT_UNSPEC),
 	NLATTR_DESC_U8(ETHTOOL_A_CABLE_RESULT_PAIR),
 	NLATTR_DESC_U8(ETHTOOL_A_CABLE_RESULT_CODE),
+	NLATTR_DESC_U32(ETHTOOL_A_CABLE_RESULT_SRC),
 };
 
 static const struct pretty_nla_desc __cable_test_flength_desc[] = {
 	NLATTR_DESC_INVALID(ETHTOOL_A_CABLE_FAULT_LENGTH_UNSPEC),
 	NLATTR_DESC_U8(ETHTOOL_A_CABLE_FAULT_LENGTH_PAIR),
 	NLATTR_DESC_U32(ETHTOOL_A_CABLE_FAULT_LENGTH_CM),
+	NLATTR_DESC_U32(ETHTOOL_A_CABLE_FAULT_LENGTH_SRC),
 };
 
 static const struct pretty_nla_desc __cable_nest_desc[] = {
-- 
2.39.5

Re: [PATCH ethtool v2] ethtool: add support for ETHTOOL_A_CABLE_FAULT_LENGTH_SRC and ETHTOOL_A_CABLE_RESULT_SRC

[Oleksij Rempel]

Sorry, I forgot to add ethtool notation to the subject.

On Thu, Nov 28, 2024 at 10:01:11AM +0100, Oleksij Rempel wrote:
> Extend cable test output to include source information, supporting
> diagnostic technologies like TDR (Time Domain Reflectometry) and ALCD
> (Active Link Cable Diagnostic). The source is displayed optionally at
> the end of each result or fault length line.
> 
> TDR requires interrupting the active link to measure parameters like
> fault location, while ALCD can operate on an active link to provide
> details like cable length without disruption.
> 
> Example output:
> Pair B code Open Circuit, source: TDR
> Pair B, fault length: 8.00m, source: TDR
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

-- 
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 |

Re: [PATCH v2] ethtool: add support for ETHTOOL_A_CABLE_FAULT_LENGTH_SRC and ETHTOOL_A_CABLE_RESULT_SRC

[Andrew Lunn]

On Thu, Nov 28, 2024 at 10:01:11AM +0100, Oleksij Rempel wrote:
> Extend cable test output to include source information, supporting
> diagnostic technologies like TDR (Time Domain Reflectometry) and ALCD
> (Active Link Cable Diagnostic). The source is displayed optionally at
> the end of each result or fault length line.
> 
> TDR requires interrupting the active link to measure parameters like
> fault location, while ALCD can operate on an active link to provide
> details like cable length without disruption.
> 
> Example output:
> Pair B code Open Circuit, source: TDR
> Pair B, fault length: 8.00m, source: TDR
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
> changes v2:
> - s/NLATTR_DESC_U8/NLATTR_DESC_U32
> ---
>  netlink/cable_test.c   | 39 +++++++++++++++++++++++++++++++++------
>  netlink/desc-ethtool.c |  2 ++
>  2 files changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/netlink/cable_test.c b/netlink/cable_test.c
> index ba21c6cd31e4..0a1c42010114 100644
> --- a/netlink/cable_test.c
> +++ b/netlink/cable_test.c
> @@ -18,7 +18,7 @@ struct cable_test_context {
>  };
>  
>  static int nl_get_cable_test_result(const struct nlattr *nest, uint8_t *pair,
> -				    uint16_t *code)
> +				    uint16_t *code, uint32_t *src)
>  {
>  	const struct nlattr *tb[ETHTOOL_A_CABLE_RESULT_MAX+1] = {};
>  	DECLARE_ATTR_TB_INFO(tb);
> @@ -32,12 +32,15 @@ static int nl_get_cable_test_result(const struct nlattr *nest, uint8_t *pair,
>  
>  	*pair = mnl_attr_get_u8(tb[ETHTOOL_A_CABLE_RESULT_PAIR]);
>  	*code = mnl_attr_get_u8(tb[ETHTOOL_A_CABLE_RESULT_CODE]);
> +	if (tb[ETHTOOL_A_CABLE_RESULT_CODE])
> +		*src = mnl_attr_get_u32(tb[ETHTOOL_A_CABLE_RESULT_SRC]);

ETHTOOL_A_CABLE_RESULT_SRC is a new property, so only newer kernels
will report it. I think you need an
if (tb[ETHTOOL_A_CABLE_RESULT_SRC]) here, and anywhere else you look for
this property?

	Andrew

Re: [PATCH v2] ethtool: add support for ETHTOOL_A_CABLE_FAULT_LENGTH_SRC and ETHTOOL_A_CABLE_RESULT_SRC

[Michal Kubecek]

[-- Attachment #1: Type: text/plain, Size: 2346 bytes --]

On Thu, Nov 28, 2024 at 05:58:09PM +0100, Andrew Lunn wrote:
> On Thu, Nov 28, 2024 at 10:01:11AM +0100, Oleksij Rempel wrote:
> > Extend cable test output to include source information, supporting
> > diagnostic technologies like TDR (Time Domain Reflectometry) and ALCD
> > (Active Link Cable Diagnostic). The source is displayed optionally at
> > the end of each result or fault length line.
> > 
> > TDR requires interrupting the active link to measure parameters like
> > fault location, while ALCD can operate on an active link to provide
> > details like cable length without disruption.
> > 
> > Example output:
> > Pair B code Open Circuit, source: TDR
> > Pair B, fault length: 8.00m, source: TDR
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> > changes v2:
> > - s/NLATTR_DESC_U8/NLATTR_DESC_U32
> > ---
> >  netlink/cable_test.c   | 39 +++++++++++++++++++++++++++++++++------
> >  netlink/desc-ethtool.c |  2 ++
> >  2 files changed, 35 insertions(+), 6 deletions(-)
> > 
> > diff --git a/netlink/cable_test.c b/netlink/cable_test.c
> > index ba21c6cd31e4..0a1c42010114 100644
> > --- a/netlink/cable_test.c
> > +++ b/netlink/cable_test.c
> > @@ -18,7 +18,7 @@ struct cable_test_context {
> >  };
> >  
> >  static int nl_get_cable_test_result(const struct nlattr *nest, uint8_t *pair,
> > -				    uint16_t *code)
> > +				    uint16_t *code, uint32_t *src)
> >  {
> >  	const struct nlattr *tb[ETHTOOL_A_CABLE_RESULT_MAX+1] = {};
> >  	DECLARE_ATTR_TB_INFO(tb);
> > @@ -32,12 +32,15 @@ static int nl_get_cable_test_result(const struct nlattr *nest, uint8_t *pair,
> >  
> >  	*pair = mnl_attr_get_u8(tb[ETHTOOL_A_CABLE_RESULT_PAIR]);
> >  	*code = mnl_attr_get_u8(tb[ETHTOOL_A_CABLE_RESULT_CODE]);
> > +	if (tb[ETHTOOL_A_CABLE_RESULT_CODE])
> > +		*src = mnl_attr_get_u32(tb[ETHTOOL_A_CABLE_RESULT_SRC]);
> 
> ETHTOOL_A_CABLE_RESULT_SRC is a new property, so only newer kernels
> will report it. I think you need an
> if (tb[ETHTOOL_A_CABLE_RESULT_SRC]) here, and anywhere else you look for
> this property?

Looks like a forgotten edit of copy&pasted text as the
!tb[ETHTOOL_A_CABLE_RESULT_CODE] case is already handled by a bail out
few lines before. (And the same for ETHTOOL_A_CABLE_FAULT_LENGTH_CM in
nl_get_cable_test_fault_length().)

Michal

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] ethtool: add support for ETHTOOL_A_CABLE_FAULT_LENGTH_SRC and ETHTOOL_A_CABLE_RESULT_SRC

[Andrew Lunn]

> > ETHTOOL_A_CABLE_RESULT_SRC is a new property, so only newer kernels
> > will report it. I think you need an
> > if (tb[ETHTOOL_A_CABLE_RESULT_SRC]) here, and anywhere else you look for
> > this property?
> 
> Looks like a forgotten edit of copy&pasted text

Duh! Yes, i did forget.

     Andrew

Re: [PATCH v2] ethtool: add support for ETHTOOL_A_CABLE_FAULT_LENGTH_SRC and ETHTOOL_A_CABLE_RESULT_SRC

[Oleksij Rempel]

On Thu, Nov 28, 2024 at 06:41:11PM +0100, Andrew Lunn wrote:
> > > ETHTOOL_A_CABLE_RESULT_SRC is a new property, so only newer kernels
> > > will report it. I think you need an
> > > if (tb[ETHTOOL_A_CABLE_RESULT_SRC]) here, and anywhere else you look for
> > > this property?
> > 
> > Looks like a forgotten edit of copy&pasted text
> 
> Duh! Yes, i did forget.

Arrr... i check existence of wrong field...  (-‸ლ)

-- 
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 |

Re: [PATCH v2] ethtool: add support for ETHTOOL_A_CABLE_FAULT_LENGTH_SRC and ETHTOOL_A_CABLE_RESULT_SRC

[Andrew Lunn]

On Fri, Nov 29, 2024 at 07:11:39AM +0100, Oleksij Rempel wrote:
> On Thu, Nov 28, 2024 at 06:41:11PM +0100, Andrew Lunn wrote:
> > > > ETHTOOL_A_CABLE_RESULT_SRC is a new property, so only newer kernels
> > > > will report it. I think you need an
> > > > if (tb[ETHTOOL_A_CABLE_RESULT_SRC]) here, and anywhere else you look for
> > > > this property?
> > > 
> > > Looks like a forgotten edit of copy&pasted text
> > 
> > Duh! Yes, i did forget.
> 
> Arrr... i check existence of wrong field...  (-‸ლ)

We all failed the copy/paste test :-(

	Andrew