Vladimir Oltean
2022-Oct-20 13:02 UTC
[Bridge] [PATCH v8 net-next 05/12] net: dsa: propagate the locked flag down through the DSA layer
On Tue, Oct 18, 2022 at 06:56:12PM +0200, Hans J. Schultz wrote:> Add a new u16 for fdb flags to propagate through the DSA layer towards the > fdb add and del functions of the drivers. > > Signed-off-by: Hans J. Schultz <netdev at kapio-technology.com> > --- > include/net/dsa.h | 2 ++ > net/dsa/dsa_priv.h | 6 ++++-- > net/dsa/port.c | 10 ++++++---- > net/dsa/slave.c | 10 ++++++++-- > net/dsa/switch.c | 16 ++++++++-------- > 5 files changed, 28 insertions(+), 16 deletions(-) > > diff --git a/include/net/dsa.h b/include/net/dsa.h > index ee369670e20e..e4b641b20713 100644 > --- a/include/net/dsa.h > +++ b/include/net/dsa.h > @@ -821,6 +821,8 @@ static inline bool dsa_port_tree_same(const struct dsa_port *a, > return a->ds->dst == b->ds->dst; > } > > +#define DSA_FDB_FLAG_LOCKED (1 << 0) > + > typedef int dsa_fdb_dump_cb_t(const unsigned char *addr, u16 vid, > bool is_static, void *data); > struct dsa_switch_ops { > diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h > index 6e65c7ffd6f3..c943e8934063 100644 > --- a/net/dsa/dsa_priv.h > +++ b/net/dsa/dsa_priv.h > @@ -65,6 +65,7 @@ struct dsa_notifier_fdb_info { > const struct dsa_port *dp; > const unsigned char *addr; > u16 vid; > + u16 fdb_flags; > struct dsa_db db; > }; > > @@ -131,6 +132,7 @@ struct dsa_switchdev_event_work { > */ > unsigned char addr[ETH_ALEN]; > u16 vid; > + u16 fdb_flags; > bool host_addr; > }; > > @@ -241,9 +243,9 @@ int dsa_port_vlan_msti(struct dsa_port *dp, > const struct switchdev_vlan_msti *msti); > int dsa_port_mtu_change(struct dsa_port *dp, int new_mtu); > int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr, > - u16 vid); > + u16 vid, u16 fdb_flags); > int dsa_port_fdb_del(struct dsa_port *dp, const unsigned char *addr, > - u16 vid); > + u16 vid, u16 fdb_flags); > int dsa_port_standalone_host_fdb_add(struct dsa_port *dp, > const unsigned char *addr, u16 vid); > int dsa_port_standalone_host_fdb_del(struct dsa_port *dp, > diff --git a/net/dsa/port.c b/net/dsa/port.c > index 208168276995..ff4f66f14d39 100644 > --- a/net/dsa/port.c > +++ b/net/dsa/port.c > @@ -304,7 +304,7 @@ static int dsa_port_inherit_brport_flags(struct dsa_port *dp, > struct netlink_ext_ack *extack) > { > const unsigned long mask = BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD | > - BR_BCAST_FLOOD | BR_PORT_LOCKED; > + BR_BCAST_FLOOD; > struct net_device *brport_dev = dsa_port_to_bridge_port(dp); > int flag, err; > > @@ -328,7 +328,7 @@ static void dsa_port_clear_brport_flags(struct dsa_port *dp) > { > const unsigned long val = BR_FLOOD | BR_MCAST_FLOOD | BR_BCAST_FLOOD; > const unsigned long mask = BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD | > - BR_BCAST_FLOOD | BR_PORT_LOCKED; > + BR_BCAST_FLOOD | BR_PORT_LOCKED | BR_PORT_MAB;Why does the mask of cleared brport flags differ from the one of set brport flags, and what/where is the explanation for this change?> int flag, err; > > for_each_set_bit(flag, &mask, 32) { > @@ -956,12 +956,13 @@ int dsa_port_mtu_change(struct dsa_port *dp, int new_mtu) > } > > int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr, > - u16 vid) > + u16 vid, u16 fdb_flags) > { > struct dsa_notifier_fdb_info info = { > .dp = dp, > .addr = addr, > .vid = vid, > + .fdb_flags = fdb_flags, > .db = { > .type = DSA_DB_BRIDGE, > .bridge = *dp->bridge, > @@ -979,12 +980,13 @@ int dsa_port_fdb_add(struct dsa_port *dp, const unsigned char *addr, > } > > int dsa_port_fdb_del(struct dsa_port *dp, const unsigned char *addr, > - u16 vid) > + u16 vid, u16 fdb_flags) > { > struct dsa_notifier_fdb_info info = { > .dp = dp, > .addr = addr, > .vid = vid, > + .fdb_flags = fdb_flags, > .db = { > .type = DSA_DB_BRIDGE, > .bridge = *dp->bridge, > diff --git a/net/dsa/slave.c b/net/dsa/slave.c > index 1a59918d3b30..65f0c578ef44 100644 > --- a/net/dsa/slave.c > +++ b/net/dsa/slave.c > @@ -3246,6 +3246,7 @@ static void dsa_slave_switchdev_event_work(struct work_struct *work) > container_of(work, struct dsa_switchdev_event_work, work); > const unsigned char *addr = switchdev_work->addr; > struct net_device *dev = switchdev_work->dev; > + u16 fdb_flags = switchdev_work->fdb_flags; > u16 vid = switchdev_work->vid; > struct dsa_switch *ds; > struct dsa_port *dp; > @@ -3261,7 +3262,7 @@ static void dsa_slave_switchdev_event_work(struct work_struct *work) > else if (dp->lag) > err = dsa_port_lag_fdb_add(dp, addr, vid); > else > - err = dsa_port_fdb_add(dp, addr, vid); > + err = dsa_port_fdb_add(dp, addr, vid, fdb_flags); > if (err) { > dev_err(ds->dev, > "port %d failed to add %pM vid %d to fdb: %d\n", > @@ -3277,7 +3278,7 @@ static void dsa_slave_switchdev_event_work(struct work_struct *work) > else if (dp->lag) > err = dsa_port_lag_fdb_del(dp, addr, vid); > else > - err = dsa_port_fdb_del(dp, addr, vid); > + err = dsa_port_fdb_del(dp, addr, vid, fdb_flags); > if (err) { > dev_err(ds->dev, > "port %d failed to delete %pM vid %d from fdb: %d\n", > @@ -3315,6 +3316,7 @@ static int dsa_slave_fdb_event(struct net_device *dev, > struct dsa_port *dp = dsa_slave_to_port(dev); > bool host_addr = fdb_info->is_local; > struct dsa_switch *ds = dp->ds; > + u16 fdb_flags = 0; > > if (ctx && ctx != dp) > return 0; > @@ -3361,6 +3363,9 @@ static int dsa_slave_fdb_event(struct net_device *dev, > orig_dev->name, fdb_info->addr, fdb_info->vid, > host_addr ? " as host address" : ""); > > + if (fdb_info->locked) > + fdb_flags |= DSA_FDB_FLAG_LOCKED;This is the bridge->driver direction. In which of the changes up until now/through which mechanism will the bridge emit a SWITCHDEV_FDB_ADD_TO_DEVICE with fdb_info->locked = true? Don't the other switchdev drivers except DSA (search for SWITCHDEV_FDB_EVENT_TO_DEVICE in the drivers/ folder) need to handle this new flag too, even if to reject it? When other drivers will want to look at fdb_info->locked, they'll have the surprise that it's impossible to maintain backwards compatibility, because they didn't use to treat the flag at all in the past (so either locked or unlocked, they did the same thing).> + > INIT_WORK(&switchdev_work->work, dsa_slave_switchdev_event_work); > switchdev_work->event = event; > switchdev_work->dev = dev; > @@ -3369,6 +3374,7 @@ static int dsa_slave_fdb_event(struct net_device *dev, > ether_addr_copy(switchdev_work->addr, fdb_info->addr); > switchdev_work->vid = fdb_info->vid; > switchdev_work->host_addr = host_addr; > + switchdev_work->fdb_flags = fdb_flags;
Ido Schimmel
2022-Oct-20 13:24 UTC
[Bridge] [PATCH v8 net-next 05/12] net: dsa: propagate the locked flag down through the DSA layer
On Thu, Oct 20, 2022 at 04:02:24PM +0300, Vladimir Oltean wrote:> On Tue, Oct 18, 2022 at 06:56:12PM +0200, Hans J. Schultz wrote: > > @@ -3315,6 +3316,7 @@ static int dsa_slave_fdb_event(struct net_device *dev, > > struct dsa_port *dp = dsa_slave_to_port(dev); > > bool host_addr = fdb_info->is_local; > > struct dsa_switch *ds = dp->ds; > > + u16 fdb_flags = 0; > > > > if (ctx && ctx != dp) > > return 0; > > @@ -3361,6 +3363,9 @@ static int dsa_slave_fdb_event(struct net_device *dev, > > orig_dev->name, fdb_info->addr, fdb_info->vid, > > host_addr ? " as host address" : ""); > > > > + if (fdb_info->locked) > > + fdb_flags |= DSA_FDB_FLAG_LOCKED; > > This is the bridge->driver direction. In which of the changes up until > now/through which mechanism will the bridge emit a > SWITCHDEV_FDB_ADD_TO_DEVICE with fdb_info->locked = true?I believe it can happen in the following call chain: br_handle_frame_finish br_fdb_update // p->flags & BR_PORT_MAB fdb_notify br_switchdev_fdb_notify This can happen with Spectrum when a packet ingresses via a locked port and incurs an FDB miss in hardware. The packet will be trapped and injected to the Rx path where it should invoke the above call chain.> Don't the other switchdev drivers except DSA (search for SWITCHDEV_FDB_EVENT_TO_DEVICE > in the drivers/ folder) need to handle this new flag too, even if to reject it?Yes, agree. At least with mlxsw it is not a big deal right now because it ignores entries with !BR_FDB_ADDED_BY_USER and locked entries are always like that, but it would be good to make it more explicit.> > When other drivers will want to look at fdb_info->locked, they'll have > the surprise that it's impossible to maintain backwards compatibility, > because they didn't use to treat the flag at all in the past (so either > locked or unlocked, they did the same thing). > > > + > > INIT_WORK(&switchdev_work->work, dsa_slave_switchdev_event_work); > > switchdev_work->event = event; > > switchdev_work->dev = dev; > > @@ -3369,6 +3374,7 @@ static int dsa_slave_fdb_event(struct net_device *dev, > > ether_addr_copy(switchdev_work->addr, fdb_info->addr); > > switchdev_work->vid = fdb_info->vid; > > switchdev_work->host_addr = host_addr; > > + switchdev_work->fdb_flags = fdb_flags;
netdev at kapio-technology.com
2022-Oct-20 19:43 UTC
[Bridge] [PATCH v8 net-next 05/12] net: dsa: propagate the locked flag down through the DSA layer
On 2022-10-20 15:02, Vladimir Oltean wrote:>> --- a/net/dsa/port.c >> +++ b/net/dsa/port.c >> @@ -304,7 +304,7 @@ static int dsa_port_inherit_brport_flags(struct >> dsa_port *dp, >> struct netlink_ext_ack *extack) >> { >> const unsigned long mask = BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD | >> - BR_BCAST_FLOOD | BR_PORT_LOCKED; >> + BR_BCAST_FLOOD; >> struct net_device *brport_dev = dsa_port_to_bridge_port(dp); >> int flag, err; >> >> @@ -328,7 +328,7 @@ static void dsa_port_clear_brport_flags(struct >> dsa_port *dp) >> { >> const unsigned long val = BR_FLOOD | BR_MCAST_FLOOD | >> BR_BCAST_FLOOD; >> const unsigned long mask = BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD | >> - BR_BCAST_FLOOD | BR_PORT_LOCKED; >> + BR_BCAST_FLOOD | BR_PORT_LOCKED | BR_PORT_MAB; > > Why does the mask of cleared brport flags differ from the one of set > brport flags, and what/where is the explanation for this change? >I guess you mean, why it differs from the inherit flag mask list? If so it is explained in the update to v7 in 00/12.