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