Tobias Waldekranz
2022-Mar-10 08:54 UTC
[Bridge] [PATCH v2 net-next 07/10] net: dsa: Pass MST state changes to driver
On Fri, Mar 04, 2022 at 00:20, Vladimir Oltean <olteanv at gmail.com> wrote:> On Tue, Mar 01, 2022 at 11:03:18AM +0100, Tobias Waldekranz wrote: >> Add the usual trampoline functionality from the generic DSA layer down >> to the drivers for MST state changes. >> >> Signed-off-by: Tobias Waldekranz <tobias at waldekranz.com> >> --- >> include/net/dsa.h | 2 ++ >> net/dsa/dsa_priv.h | 2 ++ >> net/dsa/port.c | 30 ++++++++++++++++++++++++++++++ >> net/dsa/slave.c | 6 ++++++ >> 4 files changed, 40 insertions(+) >> >> diff --git a/include/net/dsa.h b/include/net/dsa.h >> index cc8acb01bd9b..096e6e3a8e1e 100644 >> --- a/include/net/dsa.h >> +++ b/include/net/dsa.h >> @@ -943,6 +943,8 @@ struct dsa_switch_ops { >> struct dsa_bridge bridge); >> void (*port_stp_state_set)(struct dsa_switch *ds, int port, >> u8 state); >> + int (*port_mst_state_set)(struct dsa_switch *ds, int port, >> + const struct switchdev_mst_state *state); >> void (*port_fast_age)(struct dsa_switch *ds, int port); >> int (*port_pre_bridge_flags)(struct dsa_switch *ds, int port, >> struct switchdev_brport_flags flags, >> diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h >> index 87ec0697e92e..a620e079ebc5 100644 >> --- a/net/dsa/dsa_priv.h >> +++ b/net/dsa/dsa_priv.h >> @@ -198,6 +198,8 @@ static inline struct net_device *dsa_master_find_slave(struct net_device *dev, >> void dsa_port_set_tag_protocol(struct dsa_port *cpu_dp, >> const struct dsa_device_ops *tag_ops); >> int dsa_port_set_state(struct dsa_port *dp, u8 state, bool do_fast_age); >> +int dsa_port_set_mst_state(struct dsa_port *dp, >> + const struct switchdev_mst_state *state); >> int dsa_port_enable_rt(struct dsa_port *dp, struct phy_device *phy); >> int dsa_port_enable(struct dsa_port *dp, struct phy_device *phy); >> void dsa_port_disable_rt(struct dsa_port *dp); >> diff --git a/net/dsa/port.c b/net/dsa/port.c >> index 5f45cb7d70ba..26cfbc8ab499 100644 >> --- a/net/dsa/port.c >> +++ b/net/dsa/port.c >> @@ -108,6 +108,36 @@ int dsa_port_set_state(struct dsa_port *dp, u8 state, bool do_fast_age) >> return 0; >> } >> >> +int dsa_port_set_mst_state(struct dsa_port *dp, >> + const struct switchdev_mst_state *state) >> +{ >> + struct dsa_switch *ds = dp->ds; >> + int err, port = dp->index; >> + >> + if (!ds->ops->port_mst_state_set) >> + return -EOPNOTSUPP; >> + >> + err = ds->ops->port_mst_state_set(ds, port, state); >> + if (err) >> + return err; >> + >> + if (!dsa_port_can_configure_learning(dp) || dp->learning) { >> + switch (state->state) { >> + case BR_STATE_DISABLED: >> + case BR_STATE_BLOCKING: >> + case BR_STATE_LISTENING: >> + /* Ideally we would only fast age entries >> + * belonging to VLANs controlled by this >> + * MST. >> + */ >> + dsa_port_fast_age(dp); > > Does mv88e6xxx support this? If it does, you might just as well > introduce another variant of ds->ops->port_fast_age() for an msti.You can limit ATU operations to a particular FID. So the way I see it we could either have: int (*port_vlan_fast_age)(struct dsa_switch *ds, int port, u16 vid) + Maybe more generic. You could imagine there being a way to trigger this operation from userspace for example. - We would have to keep the VLAN<->MSTI mapping in the DSA layer in order to be able to do the fan-out in dsa_port_set_mst_state. or: int (*port_msti_fast_age)(struct dsa_switch *ds, int port, u16 msti) + Let's the mapping be an internal affair in the driver. - Perhaps, less generically useful. Which one do you prefer? Or is there a hidden third option? :)> And since it is new code, you could require that drivers _do_ support > configuring learning before they could support MSTP. After all, we don't > want to keep legacy mechanisms in place forever.By "configuring learning", do you mean this new fast-age-per-vid/msti, or being able to enable/disable learning per port? If it's the latter, I'm not sure I understand how those two are related.
Vladimir Oltean
2022-Mar-10 10:35 UTC
[Bridge] [PATCH v2 net-next 07/10] net: dsa: Pass MST state changes to driver
On Thu, Mar 10, 2022 at 09:54:34AM +0100, Tobias Waldekranz wrote:> >> + if (!dsa_port_can_configure_learning(dp) || dp->learning) { > >> + switch (state->state) { > >> + case BR_STATE_DISABLED: > >> + case BR_STATE_BLOCKING: > >> + case BR_STATE_LISTENING: > >> + /* Ideally we would only fast age entries > >> + * belonging to VLANs controlled by this > >> + * MST. > >> + */ > >> + dsa_port_fast_age(dp); > > > > Does mv88e6xxx support this? If it does, you might just as well > > introduce another variant of ds->ops->port_fast_age() for an msti. > > You can limit ATU operations to a particular FID. So the way I see it we > could either have: > > int (*port_vlan_fast_age)(struct dsa_switch *ds, int port, u16 vid) > > + Maybe more generic. You could imagine there being a way to trigger > this operation from userspace for example. > - We would have to keep the VLAN<->MSTI mapping in the DSA layer in > order to be able to do the fan-out in dsa_port_set_mst_state. > > or: > > int (*port_msti_fast_age)(struct dsa_switch *ds, int port, u16 msti) > > + Let's the mapping be an internal affair in the driver. > - Perhaps, less generically useful. > > Which one do you prefer? Or is there a hidden third option? :)Yes, I was thinking of "port_msti_fast_age". I don't see a cheap way of keeping VLAN to MSTI associations in the DSA layer. Only if we could retrieve this mapping from the bridge layer - maybe with something analogous to br_vlan_get_info(), but br_mst_get_info(), and this gets passed a VLAN_N_VID sized bitmap, which the bridge populates with ones and zeroes. The reason why I asked for this is because I'm not sure of the implications of flushing the entire FDB of the port for a single MSTP state change. It would trigger temporary useless flooding in other MSTIs at the very least. There isn't any backwards compatibility concern to speak of, so we can at least try from the beginning to limit the flushing to the required VLANs. What I didn't think about, and will be a problem, is dsa_port_notify_bridge_fdb_flush() - we don't know the vid to flush. The easy way out here would be to export dsa_port_notify_bridge_fdb_flush(), add a "vid" argument to it, and let drivers call it. Thoughts? Alternatively, if you think that cross-flushing FDBs of multiple MSTIs isn't a real problem, I suppose we could keep the "port_fast_age" method.> > And since it is new code, you could require that drivers _do_ support > > configuring learning before they could support MSTP. After all, we don't > > want to keep legacy mechanisms in place forever. > > By "configuring learning", do you mean this new fast-age-per-vid/msti, > or being able to enable/disable learning per port? If it's the latter, > I'm not sure I understand how those two are related.The code from dsa_port_set_state() which you've copied: if (!dsa_port_can_configure_learning(dp) || (do_fast_age && dp->learning)) { has this explanation: 1. DSA keeps standalone ports in the FORWARDING state. 2. DSA also disables address learning on standalone ports, where this is possible (dsa_port_can_configure_learning(dp) == true). 3. When a port joins a bridge, it leaves its FORWARDING state from standalone mode and inherits the bridge port's BLOCKING state 4. dsa_port_set_state() treats a port transition from FORWARDING to BLOCKING as a transition requiring an FDB flush 5. due to (2), the FDB flush at stage (4) is in fact not needed, because the FDB of that port should already be empty. Flushing the FDB may be a costly operation for some drivers, so it is avoided if possible. So this is why the "dsa_port_can_configure_learning()" check is there - for compatibility with drivers that can't configure learning => they keep learning enabled also in standalone mode => they need an FDB flush when a standalone port joins a bridge. What I'm saying is: for drivers that offload MSTP, let's force them to get the basics right first (have configurable learning), rather than go forward forever with a backwards compatibility mode.