[PATCH] ip link: hsr: Add support for passing information about INTERLINK device

[PATCH] ip link: hsr: Add support for passing information about INTERLINK device

[Lukasz Majewski]

The HSR capable device can operate in two modes of operations -
Doubly Attached Node for HSR (DANH) and RedBOX.

The latter one allows connection of non-HSR aware device to HSR network.
This node is called SAN (Singly Attached Network) and is connected via
INTERLINK network device.

This patch adds support for passing information about the INTERLINK device,
so the Linux driver can properly setup it.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
 include/uapi/linux/if_link.h |  1 +
 ip/iplink_hsr.c              | 22 +++++++++++++++++++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 41708e2..aa70ed6 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -1122,6 +1122,7 @@ enum {
 	IFLA_HSR_PROTOCOL,		/* Indicate different protocol than
 					 * HSR. For example PRP.
 					 */
+	IFLA_HSR_INTERLINK,		/* HSR interlink network device */
 	__IFLA_HSR_MAX,
 };
 
diff --git a/ip/iplink_hsr.c b/ip/iplink_hsr.c
index da2d03d..1f048fd 100644
--- a/ip/iplink_hsr.c
+++ b/ip/iplink_hsr.c
@@ -25,12 +25,15 @@ static void print_usage(FILE *f)
 {
 	fprintf(f,
 		"Usage:\tip link add name NAME type hsr slave1 SLAVE1-IF slave2 SLAVE2-IF\n"
-		"\t[ supervision ADDR-BYTE ] [version VERSION] [proto PROTOCOL]\n"
+		"\t[ interlink INTERLINK-IF ] [ supervision ADDR-BYTE ] [ version VERSION ]\n"
+		"\t[ proto PROTOCOL ]\n"
 		"\n"
 		"NAME\n"
 		"	name of new hsr device (e.g. hsr0)\n"
 		"SLAVE1-IF, SLAVE2-IF\n"
 		"	the two slave devices bound to the HSR device\n"
+		"INTERLINK-IF\n"
+		"	the interlink device bound to the HSR network to connect SAN device\n"
 		"ADDR-BYTE\n"
 		"	0-255; the last byte of the multicast address used for HSR supervision\n"
 		"	frames (default = 0)\n"
@@ -86,6 +89,12 @@ static int hsr_parse_opt(struct link_util *lu, int argc, char **argv,
 			if (ifindex == 0)
 				invarg("No such interface", *argv);
 			addattr_l(n, 1024, IFLA_HSR_SLAVE2, &ifindex, 4);
+		} else if (matches(*argv, "interlink") == 0) {
+			NEXT_ARG();
+			ifindex = ll_name_to_index(*argv);
+			if (ifindex == 0)
+				invarg("No such interface", *argv);
+			addattr_l(n, 1024, IFLA_HSR_INTERLINK, &ifindex, 4);
 		} else if (matches(*argv, "help") == 0) {
 			usage();
 			return -1;
@@ -113,6 +122,9 @@ static void hsr_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 	if (tb[IFLA_HSR_SLAVE2] &&
 	    RTA_PAYLOAD(tb[IFLA_HSR_SLAVE2]) < sizeof(__u32))
 		return;
+	if (tb[IFLA_HSR_INTERLINK] &&
+	    RTA_PAYLOAD(tb[IFLA_HSR_INTERLINK]) < sizeof(__u32))
+		return;
 	if (tb[IFLA_HSR_SEQ_NR] &&
 	    RTA_PAYLOAD(tb[IFLA_HSR_SEQ_NR]) < sizeof(__u16))
 		return;
@@ -136,6 +148,14 @@ static void hsr_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 	else
 		print_null(PRINT_ANY, "slave2", "slave2 %s ", "<none>");
 
+	if (tb[IFLA_HSR_INTERLINK])
+		print_string(PRINT_ANY,
+			     "interlink",
+			     "interlink %s ",
+			     ll_index_to_name(rta_getattr_u32(tb[IFLA_HSR_INTERLINK])));
+	else
+		print_null(PRINT_ANY, "interlink", "interlink %s ", "<none>");
+
 	if (tb[IFLA_HSR_SEQ_NR])
 		print_int(PRINT_ANY,
 			  "seq_nr",
-- 
2.20.1

Re: [PATCH] ip link: hsr: Add support for passing information about INTERLINK device

[Lukasz Majewski]

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

Dear Community,

> The HSR capable device can operate in two modes of operations -
> Doubly Attached Node for HSR (DANH) and RedBOX.
> 
> The latter one allows connection of non-HSR aware device to HSR
> network. This node is called SAN (Singly Attached Network) and is
> connected via INTERLINK network device.
> 
> This patch adds support for passing information about the INTERLINK
> device, so the Linux driver can properly setup it.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> ---
>  include/uapi/linux/if_link.h |  1 +
>  ip/iplink_hsr.c              | 22 +++++++++++++++++++++-
>  2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/if_link.h
> b/include/uapi/linux/if_link.h index 41708e2..aa70ed6 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -1122,6 +1122,7 @@ enum {
>  	IFLA_HSR_PROTOCOL,		/* Indicate different
> protocol than
>  					 * HSR. For example PRP.
>  					 */
> +	IFLA_HSR_INTERLINK,		/* HSR interlink network
> device */ __IFLA_HSR_MAX,
>  };
>  
> diff --git a/ip/iplink_hsr.c b/ip/iplink_hsr.c
> index da2d03d..1f048fd 100644
> --- a/ip/iplink_hsr.c
> +++ b/ip/iplink_hsr.c
> @@ -25,12 +25,15 @@ static void print_usage(FILE *f)
>  {
>  	fprintf(f,
>  		"Usage:\tip link add name NAME type hsr slave1
> SLAVE1-IF slave2 SLAVE2-IF\n"
> -		"\t[ supervision ADDR-BYTE ] [version VERSION]
> [proto PROTOCOL]\n"
> +		"\t[ interlink INTERLINK-IF ] [ supervision
> ADDR-BYTE ] [ version VERSION ]\n"
> +		"\t[ proto PROTOCOL ]\n"
>  		"\n"
>  		"NAME\n"
>  		"	name of new hsr device (e.g. hsr0)\n"
>  		"SLAVE1-IF, SLAVE2-IF\n"
>  		"	the two slave devices bound to the HSR
> device\n"
> +		"INTERLINK-IF\n"
> +		"	the interlink device bound to the HSR
> network to connect SAN device\n" "ADDR-BYTE\n"
>  		"	0-255; the last byte of the multicast
> address used for HSR supervision\n" "	frames (default = 0)\n"
> @@ -86,6 +89,12 @@ static int hsr_parse_opt(struct link_util *lu, int
> argc, char **argv, if (ifindex == 0)
>  				invarg("No such interface", *argv);
>  			addattr_l(n, 1024, IFLA_HSR_SLAVE2,
> &ifindex, 4);
> +		} else if (matches(*argv, "interlink") == 0) {
> +			NEXT_ARG();
> +			ifindex = ll_name_to_index(*argv);
> +			if (ifindex == 0)
> +				invarg("No such interface", *argv);
> +			addattr_l(n, 1024, IFLA_HSR_INTERLINK,
> &ifindex, 4); } else if (matches(*argv, "help") == 0) {
>  			usage();
>  			return -1;
> @@ -113,6 +122,9 @@ static void hsr_print_opt(struct link_util *lu,
> FILE *f, struct rtattr *tb[]) if (tb[IFLA_HSR_SLAVE2] &&
>  	    RTA_PAYLOAD(tb[IFLA_HSR_SLAVE2]) < sizeof(__u32))
>  		return;
> +	if (tb[IFLA_HSR_INTERLINK] &&
> +	    RTA_PAYLOAD(tb[IFLA_HSR_INTERLINK]) < sizeof(__u32))
> +		return;
>  	if (tb[IFLA_HSR_SEQ_NR] &&
>  	    RTA_PAYLOAD(tb[IFLA_HSR_SEQ_NR]) < sizeof(__u16))
>  		return;
> @@ -136,6 +148,14 @@ static void hsr_print_opt(struct link_util *lu,
> FILE *f, struct rtattr *tb[]) else
>  		print_null(PRINT_ANY, "slave2", "slave2 %s ",
> "<none>"); 
> +	if (tb[IFLA_HSR_INTERLINK])
> +		print_string(PRINT_ANY,
> +			     "interlink",
> +			     "interlink %s ",
> +
> ll_index_to_name(rta_getattr_u32(tb[IFLA_HSR_INTERLINK])));
> +	else
> +		print_null(PRINT_ANY, "interlink", "interlink %s ",
> "<none>"); +
>  	if (tb[IFLA_HSR_SEQ_NR])
>  		print_int(PRINT_ANY,
>  			  "seq_nr",

Any feedback if this approach is acceptable?


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

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

Re: [PATCH] ip link: hsr: Add support for passing information about INTERLINK device

[Stephen Hemminger]

On Fri, 16 Feb 2024 14:21:14 +0100
Lukasz Majewski <lukma@denx.de> wrote:

> The HSR capable device can operate in two modes of operations -
> Doubly Attached Node for HSR (DANH) and RedBOX.
> 
> The latter one allows connection of non-HSR aware device to HSR network.
> This node is called SAN (Singly Attached Network) and is connected via
> INTERLINK network device.
> 
> This patch adds support for passing information about the INTERLINK device,
> so the Linux driver can properly setup it.
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>

Patch should target iproute2-next since headers come from upstream.
And kernel side needs to be accepted first.

When it is merged to net-next, Dave Ahern can pickup the headers
from there.

>  
> diff --git a/ip/iplink_hsr.c b/ip/iplink_hsr.c
> index da2d03d..1f048fd 100644
> --- a/ip/iplink_hsr.c
> +++ b/ip/iplink_hsr.c
> @@ -25,12 +25,15 @@ static void print_usage(FILE *f)
>  {
>  	fprintf(f,
>  		"Usage:\tip link add name NAME type hsr slave1 SLAVE1-IF slave2 SLAVE2-IF\n"
> -		"\t[ supervision ADDR-BYTE ] [version VERSION] [proto PROTOCOL]\n"
> +		"\t[ interlink INTERLINK-IF ] [ supervision ADDR-BYTE ] [ version VERSION ]\n"
> +		"\t[ proto PROTOCOL ]\n"
>  		"\n"
>  		"NAME\n"
>  		"	name of new hsr device (e.g. hsr0)\n"
>  		"SLAVE1-IF, SLAVE2-IF\n"
>  		"	the two slave devices bound to the HSR device\n"
> +		"INTERLINK-IF\n"
> +		"	the interlink device bound to the HSR network to connect SAN device\n"
>  		"ADDR-BYTE\n"
>  		"	0-255; the last byte of the multicast address used for HSR supervision\n"
>  		"	frames (default = 0)\n"
> @@ -86,6 +89,12 @@ static int hsr_parse_opt(struct link_util *lu, int argc, char **argv,
>  			if (ifindex == 0)
>  				invarg("No such interface", *argv);
>  			addattr_l(n, 1024, IFLA_HSR_SLAVE2, &ifindex, 4);
> +		} else if (matches(*argv, "interlink") == 0) {

No new uses of matches() allowed in iproute2.

> +			NEXT_ARG();
> +			ifindex = ll_name_to_index(*argv);
> +			if (ifindex == 0)
> +				invarg("No such interface", *argv);
> +			addattr_l(n, 1024, IFLA_HSR_INTERLINK, &ifindex, 4);
>  		} else if (matches(*argv, "help") == 0) {
>  			usage();
>  			return -1;
> @@ -113,6 +122,9 @@ static void hsr_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
>  	if (tb[IFLA_HSR_SLAVE2] &&
>  	    RTA_PAYLOAD(tb[IFLA_HSR_SLAVE2]) < sizeof(__u32))
>  		return;
> +	if (tb[IFLA_HSR_INTERLINK] &&
> +	    RTA_PAYLOAD(tb[IFLA_HSR_INTERLINK]) < sizeof(__u32))
> +		return;
>  	if (tb[IFLA_HSR_SEQ_NR] &&
>  	    RTA_PAYLOAD(tb[IFLA_HSR_SEQ_NR]) < sizeof(__u16))
>  		return;
> @@ -136,6 +148,14 @@ static void hsr_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
>  	else
>  		print_null(PRINT_ANY, "slave2", "slave2 %s ", "<none>");
>  
> +	if (tb[IFLA_HSR_INTERLINK])
> +		print_string(PRINT_ANY,
> +			     "interlink",
> +			     "interlink %s ",
> +			     ll_index_to_name(rta_getattr_u32(tb[IFLA_HSR_INTERLINK])));

Better to print that in color and pack args on line up to 100 characters.

	if (tb[IFLA_HSR_INTERLINK])
		print_color_string(PRINT_ANY, COLOR_IFNAME, "interlink", "interlink %s ",
				   ll_index_to_name(rta_getattr_u32(tb[IFLA_HSR_INTERLINK])));

> +	else
> +		print_null(PRINT_ANY, "interlink", "interlink %s ", "<none>");

The output from ip show commands should resemble inputs to configuration.
Therefore the else clause should not be there. Only print if interlink is configured.

Re: [PATCH] ip link: hsr: Add support for passing information about INTERLINK device

[Lukasz Majewski]

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

Hi Stephen,

> On Fri, 16 Feb 2024 14:21:14 +0100
> Lukasz Majewski <lukma@denx.de> wrote:
> 
> > The HSR capable device can operate in two modes of operations -
> > Doubly Attached Node for HSR (DANH) and RedBOX.
> > 
> > The latter one allows connection of non-HSR aware device to HSR
> > network. This node is called SAN (Singly Attached Network) and is
> > connected via INTERLINK network device.
> > 
> > This patch adds support for passing information about the INTERLINK
> > device, so the Linux driver can properly setup it.
> > 
> > Signed-off-by: Lukasz Majewski <lukma@denx.de>  
> 
> Patch should target iproute2-next since headers come from upstream.
> And kernel side needs to be accepted first.
> 

Ok. I'm going to post RFC for it soon.

> When it is merged to net-next, Dave Ahern can pickup the headers
> from there.

Ok.

> 
> >  
> > diff --git a/ip/iplink_hsr.c b/ip/iplink_hsr.c
> > index da2d03d..1f048fd 100644
> > --- a/ip/iplink_hsr.c
> > +++ b/ip/iplink_hsr.c
> > @@ -25,12 +25,15 @@ static void print_usage(FILE *f)
> >  {
> >  	fprintf(f,
> >  		"Usage:\tip link add name NAME type hsr slave1
> > SLAVE1-IF slave2 SLAVE2-IF\n"
> > -		"\t[ supervision ADDR-BYTE ] [version VERSION]
> > [proto PROTOCOL]\n"
> > +		"\t[ interlink INTERLINK-IF ] [ supervision
> > ADDR-BYTE ] [ version VERSION ]\n"
> > +		"\t[ proto PROTOCOL ]\n"
> >  		"\n"
> >  		"NAME\n"
> >  		"	name of new hsr device (e.g. hsr0)\n"
> >  		"SLAVE1-IF, SLAVE2-IF\n"
> >  		"	the two slave devices bound to the HSR
> > device\n"
> > +		"INTERLINK-IF\n"
> > +		"	the interlink device bound to the HSR
> > network to connect SAN device\n" "ADDR-BYTE\n"
> >  		"	0-255; the last byte of the multicast
> > address used for HSR supervision\n" "	frames (default = 0)\n"
> > @@ -86,6 +89,12 @@ static int hsr_parse_opt(struct link_util *lu,
> > int argc, char **argv, if (ifindex == 0)
> >  				invarg("No such interface", *argv);
> >  			addattr_l(n, 1024, IFLA_HSR_SLAVE2,
> > &ifindex, 4);
> > +		} else if (matches(*argv, "interlink") == 0) {  
> 
> No new uses of matches() allowed in iproute2.

Could you be more specific here? Is there any other function (or idiom)
to be used?

> 
> > +			NEXT_ARG();
> > +			ifindex = ll_name_to_index(*argv);
> > +			if (ifindex == 0)
> > +				invarg("No such interface", *argv);
> > +			addattr_l(n, 1024, IFLA_HSR_INTERLINK,
> > &ifindex, 4); } else if (matches(*argv, "help") == 0) {
> >  			usage();
> >  			return -1;
> > @@ -113,6 +122,9 @@ static void hsr_print_opt(struct link_util *lu,
> > FILE *f, struct rtattr *tb[]) if (tb[IFLA_HSR_SLAVE2] &&
> >  	    RTA_PAYLOAD(tb[IFLA_HSR_SLAVE2]) < sizeof(__u32))
> >  		return;
> > +	if (tb[IFLA_HSR_INTERLINK] &&
> > +	    RTA_PAYLOAD(tb[IFLA_HSR_INTERLINK]) < sizeof(__u32))
> > +		return;
> >  	if (tb[IFLA_HSR_SEQ_NR] &&
> >  	    RTA_PAYLOAD(tb[IFLA_HSR_SEQ_NR]) < sizeof(__u16))
> >  		return;
> > @@ -136,6 +148,14 @@ static void hsr_print_opt(struct link_util
> > *lu, FILE *f, struct rtattr *tb[]) else
> >  		print_null(PRINT_ANY, "slave2", "slave2 %s ",
> > "<none>"); 
> > +	if (tb[IFLA_HSR_INTERLINK])
> > +		print_string(PRINT_ANY,
> > +			     "interlink",
> > +			     "interlink %s ",
> > +
> > ll_index_to_name(rta_getattr_u32(tb[IFLA_HSR_INTERLINK])));  
> 
> Better to print that in color and pack args on line up to 100
> characters.
> 
> 	if (tb[IFLA_HSR_INTERLINK])
> 		print_color_string(PRINT_ANY, COLOR_IFNAME,
> "interlink", "interlink %s ",
> ll_index_to_name(rta_getattr_u32(tb[IFLA_HSR_INTERLINK])));
> 

Ok.

> > +	else
> > +		print_null(PRINT_ANY, "interlink", "interlink %s
> > ", "<none>");  
> 
> The output from ip show commands should resemble inputs to
> configuration. Therefore the else clause should not be there. Only
> print if interlink is configured.
> 

Ok. I will adjust it.

> 


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

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

Re: [PATCH] ip link: hsr: Add support for passing information about INTERLINK device

[Stephen Hemminger]

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

On Wed, 28 Feb 2024 15:20:27 +0100
Lukasz Majewski <lukma@denx.de> wrote:

> > No new uses of matches() allowed in iproute2.  
> 
> Could you be more specific here? Is there any other function (or idiom)
> to be used?

Use strcmp.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]