[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240228152027.295b2d82@wsk>
Date: Wed, 28 Feb 2024 15:20:27 +0100
From: Lukasz Majewski <lukma@...x.de>
To: Stephen Hemminger <stephen@...workplumber.org>
Cc: Andrew Lunn <andrew@...n.ch>, Vladimir Oltean <olteanv@...il.com>, Eric
Dumazet <edumazet@...gle.com>, Florian Fainelli <f.fainelli@...il.com>,
"David S. Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
netdev@...r.kernel.org
Subject: Re: [PATCH] ip link: hsr: Add support for passing information about
INTERLINK device
Hi Stephen,
> On Fri, 16 Feb 2024 14:21:14 +0100
> Lukasz Majewski <lukma@...x.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@...x.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@...x.de
Content of type "application/pgp-signature" skipped
Powered by blists - more mailing lists