Messages in this thread | | | From | Vladimir Oltean <> | Subject | Re: [PATCH net-next 0/9] DSA changes for multiple CPU ports (part 4) | Date | Sun, 4 Sep 2022 15:41:15 +0000 |
| |
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 = <ð0>; };
port@1 { reg = <1>; ethernet = <ð1>; };
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
| |