Re: [PATCH net-next 0/9] DSA changes for multiple CPU ports (part 4)

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Sat, Sep 03, 2022 at 04:48:32AM +0200, Marek Behún wrote:
> My opinion is that it would be better to add new DSA specific netlink
> operations instead of using the existing iflink as I did in the that
> patch.

Ok, I'll send out the iproute2 patch today.

> I think that DSA should have it's own IP subcommands. Using the
> standard, already existing API, is not sufficient for more complex
> configurations/DSA routing settings. Consider DSA where there are
> multiple switches and the switches are connected via multiple ports:
> 
> +----------+   +---------------+   +---------+
> |     eth0 <---> sw0p0   sw0p2 <---> sw1p0
> | cpu      |   |               |   |        ....
> |     eth1 <---> sw0p1   s20p3 <---> sw1p1
> +----------+   +---------------+   +---------+
> 
> The routing is more complicated in this scenario.

This is so problematic that I don't even know where to start.

Does anyone do this? What do they want to achieve? How do they want the
system to behave?

Let's push your example even one step further:

 +----------+   +---------------+   +---------------+   +---------+
 |     eth0 <---> sw0p0   sw0p2 <---> sw1p0   sw1p2 <---> sw2p0
 | cpu      |   |               |   |               |   |        ....
 |     eth1 <---> sw0p1   sw0p3 <---> sw1p1   sw1p3 <---> sw2p1
 +----------+   +---------------+   +---------------+   +---------+

With our current DT bindings, DSA (cascade) ports would need to contain
links to all DSA ports of indirectly connected switches, because this is
what the dst->rtable expects.

	switch@0 {
		reg = <0>;
		dsa,member = <0 0>;

		ethernet-ports {
			port@0 {
				reg = <0>;
				ethernet = <&eth0>;
			};

			port@1 {
				reg = <1>;
				ethernet = <&eth1>;
			};

			sw0p2: port@2 {
				reg = <2>;
				link = <&sw1p0>, <&sw2p0>, <&sw2p1>;
			};

			sw0p3: port@3 {
				reg = <3>;
				link = <&sw1p1>, <&sw2p0>, <&sw2p1>;
			};
		};
	};

	switch@1 {
		reg = <1>;
		dsa,member = <0 1>;

		ethernet-ports {
			sw1p0: port@0 {
				reg = <0>;
				link = <&sw0p2>;
			};

			sw1p1: port@1 {
				reg = <1>;
				link = <&sw0p3>;
			};

			sw1p2: port@2 {
				reg = <2>;
				link = <&sw2p0>;
			};

			sw1p3: port@3 {
				reg = <3>;
				link = <&sw2p1>;
			};
		};
	};

	switch@2 {
		reg = <2>;
		dsa,member = <0 2>;

		ethernet-ports {
			port@0 {
				reg = <0>;
				link = <&sw1p2>, <&sw0p2>, <&sw0p3>;
			};

			port@1 {
				reg = <1>;
				link = <&sw1p3>, <&sw0p2>, <&sw0p3>;
			};
		};
	};

> The old API is not sufficient.

There is no "old API" to speak of, because none of this is exposed.
So, 'insufficient' is an understatement. Furthermore, the rtnl_link_ops
are exposed per *user* port, we still gain zero control of the
dst->rtable.

The main consumer of the dst->rtable is this:

/* Return the local port used to reach an arbitrary switch device */
static inline unsigned int dsa_routing_port(struct dsa_switch *ds, int device)
{
	struct dsa_switch_tree *dst = ds->dst;
	struct dsa_link *dl;

	list_for_each_entry(dl, &dst->rtable, list)
		if (dl->dp->ds == ds && dl->link_dp->ds->index == device)
			return dl->dp->index;

	return ds->num_ports;
}

Notice the singular *the* local port. So it would currently return the
local cascade port from the first dsa_link added to the rtable (in turn,
the first port which has a 'link' OF node description to a cascade port
of @device). If there are any further dsa_links between a cascade port
of this switch and a cascade port of that switch, they are ignored
(which is a good thing in terms of compatibility between old kernels and
new device trees, but still raises questions).

For each of the functions in the call graph below, we need to determine
exactly what we need to make behave differently (consider a potential
second, third, fourth dsa_link, and how to expose this structure):

                             dsa_port_host_address_match
                              |
                              |  dsa_port_host_vlan_match
                              |   |
                              v   v
                  dsa_switch_is_upstream_of
                  |                        |
                  |                        |
                  | mv88e6xxx_lag_sync_map |
                  v            |           |
          dsa_is_upstream_port |           |
                          |    |           |
   mv88e6xxx_port_vlan    |    |           |
              |           |    |           |
              v           |    |           |
dsa_switch_upstream_port  |    |           |
                     |    |    |           |
                     |    |    |           |
 plenty of drivers   |    |    |           |
               |     |    |    |           |
               v     v    v    |           |
           dsa_upstream_port   |           |
                       |       |           |
                       v       v           |
                     dsa_towards_port      |
                                   |       |
                                   |       |
                                   v       v
                              dsa_routing_port