[2/2] iproute2: add support for link set

Add support for link set useful to set CPU port for dsa drivers.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 ...-iplink_allow_to_change_iplink_value.patch | 94 +++++++++++++++++++
 1 file changed, 94 insertions(+)
 create mode 100644 package/network/utils/iproute2/patches/191-iplink_allow_to_change_iplink_value.patch

Hi, Ansuel,

On Thu, 20 Jan 2022 at 15:11, Ansuel Smith <ansuelsmth@gmail.com> wrote:
>
> Add support for link set useful to set CPU port for dsa drivers.
>
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  ...-iplink_allow_to_change_iplink_value.patch | 94 +++++++++++++++++++
>  1 file changed, 94 insertions(+)
>  create mode 100644 package/network/utils/iproute2/patches/191-iplink_allow_to_change_iplink_value.patch
>
> diff --git a/package/network/utils/iproute2/patches/191-iplink_allow_to_change_iplink_value.patch b/package/network/utils/iproute2/patches/191-iplink_allow_to_change_iplink_value.patch
> new file mode 100644
> index 0000000000..1a8bad9119
> --- /dev/null
> +++ b/package/network/utils/iproute2/patches/191-iplink_allow_to_change_iplink_value.patch
> @@ -0,0 +1,94 @@
> +From:   Marek Behún <marek.behun@nic.cz>
> +Subject: [PATCH RFC iproute2-next] iplink: allow to change iplink value
> +Date:   Sat, 24 Aug 2019 04:42:51 +0200
> +
> +Allow to change the interface to which a given interface is linked to.
> +This is useful in the case of multi-CPU port DSA, for changing the CPU
> +port of a given user port.
> +
> +Signed-off-by: Marek Behún <marek.behun@nic.cz>
> +Cc: David Ahern <dsahern@gmail.com>
> +Cc: Stephen Hemminger <stephen@networkplumber.org>
> +Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> +---
> + ip/iplink.c           | 16 +++++-----------
> + man/man8/ip-link.8.in |  7 +++++++
> + 2 files changed, 12 insertions(+), 11 deletions(-)
> +
> +diff --git a/ip/iplink.c b/ip/iplink.c
> +index 212a0885..d52c0aaf 100644
> +--- a/ip/iplink.c
> ++++ b/ip/iplink.c
> +@@ -579,7 +579,6 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req, char **type)
> + {
> +       char *name = NULL;
> +       char *dev = NULL;
> +-      char *link = NULL;
> +       int ret, len;
> +       char abuf[32];
> +       int qlen = -1;
> +@@ -590,6 +589,7 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req, char **type)
> +       int numrxqueues = -1;
> +       int link_netnsid = -1;
> +       int index = 0;
> ++      int link = -1;
> +       int group = -1;
> +       int addr_len = 0;
> +
> +@@ -620,7 +620,10 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req, char **type)
> +                               invarg("Invalid \"index\" value", *argv);
> +               } else if (matches(*argv, "link") == 0) {
> +                       NEXT_ARG();
> +-                      link = *argv;
> ++                      link = ll_name_to_index(*argv);
> ++                      if (!link)
> ++                              return nodev(*argv);
> ++                      addattr32(&req->n, sizeof(*req), IFLA_LINK, link);
> +               } else if (matches(*argv, "address") == 0) {
> +                       NEXT_ARG();
> +                       addr_len = ll_addr_a2n(abuf, sizeof(abuf), *argv);
> +@@ -1004,15 +1007,6 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req, char **type)
> +                       exit(-1);
> +               }
> +
> +-              if (link) {
> +-                      int ifindex;
> +-
> +-                      ifindex = ll_name_to_index(link);
> +-                      if (!ifindex)
> +-                              return nodev(link);
> +-                      addattr32(&req->n, sizeof(*req), IFLA_LINK, ifindex);
> +-              }
> +-
> +               req->i.ifi_index = index;
> +       }
> +
> +diff --git a/man/man8/ip-link.8.in b/man/man8/ip-link.8.in
> +index a8ae72d2..800aed05 100644
> +--- a/man/man8/ip-link.8.in
> ++++ b/man/man8/ip-link.8.in
> +@@ -149,6 +149,9 @@ ip-link \- network device configuration
> + .br
> + .RB "[ " nomaster " ]"
> + .br
> ++.RB "[ " link
> ++.IR DEVICE " ]"
> ++.br
> + .RB "[ " vrf
> + .IR NAME " ]"
> + .br
> +@@ -2131,6 +2134,10 @@ set master device of the device (enslave device).
> + .BI nomaster
> + unset master device of the device (release device).
> +
> ++.TP
> ++.BI link " DEVICE"
> ++set device to which this device is linked to.
> ++
> + .TP
> + .BI addrgenmode " eui64|none|stable_secret|random"
> + set the IPv6 address generation mode
> +--
> +2.21.0
> +
> +
> --
> 2.30.2.windows.1
>

Nice, but we'll need to do the same for BusyBox's ip applet. Let's not
force everyone who needs multi-CPU DSA to install iproute2's ip just
for this case. ;)

Cheers,
Rui

Hi again,

On Thu, 20 Jan 2022 at 15:56, Rui Salvaterra <rsalvaterra@gmail.com> wrote:
>
> Nice, but we'll need to do the same for BusyBox's ip applet. Let's not
> force everyone who needs multi-CPU DSA to install iproute2's ip just
> for this case. ;)

Just to clarify, I'll take care of the BusyBox side of things, don't
worry about it (and it's optional functionality, anyway).

Cheers,
Rui

[Apologies for resending, Gmail turned my message into HTML behind my back.]

Hi, Ansuel,

After reading the patch more carefully, I have to say I'm really not
fond of it in its current form, especially considering that the
original feedback from Stephen Hemminger and Vladimir Oltean [1]
hasn't been addressed. To be honest, overloading the IFLA_LINK
attribute for this purpose also doesn't feel right to me.

Cc'ing the original author of the patch. Marek?

[1] https://lore.kernel.org/netdev/20210411170939.cxmva5vdcpqu4bmi@skbuf/

Cheers,
Rui

>
> Hi, Ansuel,
>
> After reading the patch more carefully, I have to say I'm really not fond of it in its current form, especially considering that the original feedback from Stephen Hemminger and Vladimir Oltean [1] hasn't been addressed. To be honest, overloading the IFLA_LINK attribute for this purpose also doesn't feel right to me.
>

Should we follow the suggestion and add a specific attribute just for DSA?

> Cc'ing the original author of the patch. Marek?
>
> [1] https://lore.kernel.org/netdev/20210411170939.cxmva5vdcpqu4bmi@skbuf/
>
> Cheers,
> Rui
>

On Fri, 21 Jan 2022 at 21:11, Ansuel Smith <ansuelsmth@gmail.com> wrote:
>
>
> Should we follow the suggestion and add a specific attribute just for DSA?

I believe that would be the right thing to do, but I'd like to know
what Daniel and Marek think about it too.

On Fri, Jan 21, 2022 at 09:14:50PM +0000, Rui Salvaterra wrote:
> On Fri, 21 Jan 2022 at 21:11, Ansuel Smith <ansuelsmth@gmail.com> wrote:
> >
> > Should we follow the suggestion and add a specific attribute just for DSA?
> I believe that would be the right thing to do, but I'd like to know
> what Daniel and Marek think about it too.

While adding a new attribute for the DSA CPU port linked means more
added complexity, I still believe it's the right thing to do if that's
what would be acceptable upstream and hence significantly reduce the
burden of having to maintain and forward-port the patch locally in the
long run.
Personally I perceive the use of the existing 'set link' attribute to
be sound and aligned with it's use in other contexts, but that's most
certainly a matter of taste.

>
> On Fri, Jan 21, 2022 at 09:14:50PM +0000, Rui Salvaterra wrote:
> > On Fri, 21 Jan 2022 at 21:11, Ansuel Smith <ansuelsmth@gmail.com> wrote:
> > >
> > > Should we follow the suggestion and add a specific attribute just for DSA?
> > I believe that would be the right thing to do, but I'd like to know
> > what Daniel and Marek think about it too.
>
> While adding a new attribute for the DSA CPU port linked means more
> added complexity, I still believe it's the right thing to do if that's
> what would be acceptable upstream and hence significantly reduce the
> burden of having to maintain and forward-port the patch locally in the
> long run.
> Personally I perceive the use of the existing 'set link' attribute to
> be sound and aligned with it's use in other contexts, but that's most
> certainly a matter of taste.

So.. how should we proceed with this? From what I understand the idea
is to merge this ASAP. Think we have to change this with the DSA specific
attribute.

Hi, Ansuel,

On Sat, 22 Jan 2022 at 01:08, Ansuel Smith <ansuelsmth@gmail.com> wrote:
>
>
> So.. how should we proceed with this? From what I understand the idea is to merge this ASAP.

But not a moment sooner. ;) I'm sure we agree that this patch won't be
merged upstream in its current form, according to the comments
received, and the less we diverge from upstream, the better for
maintenance.

> Think we have to change this with the DSA specific attribute.

Ok, let's step back and take a look at our possibilities. Stephen
Hemminger suggested auditing all kernel usage of IFLA_LINK and adding
checks where needed to make sure the current users don't break [1].
This would certainly work, but that would mean sprinkling error checks
in possibly quite a number of places [2]. Vladimir Oltean, instead,
suggested creating a new netlink attribute for this specific purpose
[3] (let's call it IFLA_CPU, for example). I believe the latter is the
less intrusive of the options, with the added bonus of not having to
overload IFLA_LINK with different semantics (something I personally
dislike). I would also rename "link" to "cpu" in the ip patch
(avoiding the overload, once again).

[1] https://lore.kernel.org/netdev/20210411100411.6d16e51d@hermes.local/
[2] https://elixir.bootlin.com/linux/latest/A/ident/IFLA_LINK
[3] https://lore.kernel.org/netdev/20210411170939.cxmva5vdcpqu4bmi@skbuf/

Cheers,
Rui

On Sat, 22 Jan 2022 09:38:01 +0000
Rui Salvaterra <rsalvaterra@gmail.com> wrote:

> Hi, Ansuel,
> 
> On Sat, 22 Jan 2022 at 01:08, Ansuel Smith <ansuelsmth@gmail.com> wrote:
> >
> >
> > So.. how should we proceed with this? From what I understand the idea is to merge this ASAP.  
> 
> But not a moment sooner. ;) I'm sure we agree that this patch won't be
> merged upstream in its current form, according to the comments
> received, and the less we diverge from upstream, the better for
> maintenance.
> 
> > Think we have to change this with the DSA specific attribute.  
> 
> Ok, let's step back and take a look at our possibilities. Stephen
> Hemminger suggested auditing all kernel usage of IFLA_LINK and adding
> checks where needed to make sure the current users don't break [1].
> This would certainly work, but that would mean sprinkling error checks
> in possibly quite a number of places [2]. Vladimir Oltean, instead,
> suggested creating a new netlink attribute for this specific purpose
> [3] (let's call it IFLA_CPU, for example). I believe the latter is the
> less intrusive of the options, with the added bonus of not having to
> overload IFLA_LINK with different semantics (something I personally
> dislike). I would also rename "link" to "cpu" in the ip patch
> (avoiding the overload, once again).
> 
> [1] https://lore.kernel.org/netdev/20210411100411.6d16e51d@hermes.local/
> [2] https://elixir.bootlin.com/linux/latest/A/ident/IFLA_LINK
> [3] https://lore.kernel.org/netdev/20210411170939.cxmva5vdcpqu4bmi@skbuf/

Hello guys,

I am pessimistic about this being resolved soon in upstream, so my
suggestion to you for OpenWRT is to do something that can be used now,
for example a sysfs attribute, and create an utility for changing port's
CPU port.

Then if things get solved in upstream, you can just change the
utility's code.

Marek