| Message ID | 20210210091445.741269-1-olteanv@gmail.com |
|---|---|
| Headers | show |
| Series | Cleanup in brport flags switchdev offload for DSA | expand |
On 10/02/2021 11:14, Vladimir Oltean wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > The initial goal of this series was to have better support for > standalone ports mode and multiple bridges on the DSA drivers like > ocelot/felix and sja1105. Proper support for standalone mode requires > disabling address learning, which in turn requires interaction with the > switchdev notifier, which is actually where most of the patches are. > > I also noticed that most of the drivers are actually talking either to > firmware or SPI/MDIO connected devices from the brport flags switchdev > attribute handler, so it makes sense to actually make it sleepable > instead of atomic. > Hi Vladimir, Let's take a step back for a moment and discuss the bridge unlock/lock sequences that come with this set. I'd really like to avoid those as they're a recipe for future problems. The only good way to achieve that currently is to keep the PRE_FLAGS call and do that in unsleepable context but move the FLAGS call after the flags have been changed (if they have changed obviously). That would make the code read much easier since we'll have all our lock/unlock sequences in the same code blocks and won't play games to get sleepable context. Please let's think and work in that direction, rather than having: + spin_lock_bh(&p->br->lock); + if (err) { + netdev_err(p->dev, "%s\n", extack._msg); + return err; } + which immediately looks like a bug even though after some code checking we can verify it's ok. WDYT? I plan to get rid of most of the br->lock since it's been abused for a very long time because it's essentially STP lock, but people have started using it for other things and I plan to fix that when I get more time. Thanks, Nik
On 10/02/2021 12:45, Vladimir Oltean wrote: > Hi Nikolay, > > On Wed, Feb 10, 2021 at 12:31:43PM +0200, Nikolay Aleksandrov wrote: >> Hi Vladimir, >> Let's take a step back for a moment and discuss the bridge unlock/lock sequences >> that come with this set. I'd really like to avoid those as they're a recipe >> for future problems. The only good way to achieve that currently is to keep >> the PRE_FLAGS call and do that in unsleepable context but move the FLAGS call >> after the flags have been changed (if they have changed obviously). That would >> make the code read much easier since we'll have all our lock/unlock sequences >> in the same code blocks and won't play games to get sleepable context. >> Please let's think and work in that direction, rather than having: >> + spin_lock_bh(&p->br->lock); >> + if (err) { >> + netdev_err(p->dev, "%s\n", extack._msg); >> + return err; >> } >> + >> >> which immediately looks like a bug even though after some code checking we can >> verify it's ok. WDYT? >> >> I plan to get rid of most of the br->lock since it's been abused for a very long >> time because it's essentially STP lock, but people have started using it for other >> things and I plan to fix that when I get more time. > > This won't make the sysfs codepath any nicer, will it? > Currently we'll have to live with a hack that checks if the flags have changed. I agree it won't be pretty, but we won't have to unlock and lock again in the middle of the called function and we'll have all our locking in the same place, easier to verify and later easier to remove. Once I get rid of most of the br->lock usage we can revisit the drop of PRE_FLAGS if it's a problem. The alternative is to change the flags, then send the switchdev notification outside of the lock and revert the flags if it doesn't go through which doesn't sound much better. I'm open to any other suggestions, but definitely would like to avoid playing locking games. Even if it means casing out flag setting from all other store_ functions for sysfs.
On Wed, Feb 10, 2021 at 12:52:33PM +0200, Nikolay Aleksandrov wrote: > On 10/02/2021 12:45, Vladimir Oltean wrote: > > Hi Nikolay, > > > > On Wed, Feb 10, 2021 at 12:31:43PM +0200, Nikolay Aleksandrov wrote: > >> Hi Vladimir, > >> Let's take a step back for a moment and discuss the bridge unlock/lock sequences > >> that come with this set. I'd really like to avoid those as they're a recipe > >> for future problems. The only good way to achieve that currently is to keep > >> the PRE_FLAGS call and do that in unsleepable context but move the FLAGS call > >> after the flags have been changed (if they have changed obviously). That would > >> make the code read much easier since we'll have all our lock/unlock sequences > >> in the same code blocks and won't play games to get sleepable context. > >> Please let's think and work in that direction, rather than having: > >> + spin_lock_bh(&p->br->lock); > >> + if (err) { > >> + netdev_err(p->dev, "%s\n", extack._msg); > >> + return err; > >> } > >> + > >> > >> which immediately looks like a bug even though after some code checking we can > >> verify it's ok. WDYT? > >> > >> I plan to get rid of most of the br->lock since it's been abused for a very long > >> time because it's essentially STP lock, but people have started using it for other > >> things and I plan to fix that when I get more time. > > > > This won't make the sysfs codepath any nicer, will it? > > > > Currently we'll have to live with a hack that checks if the flags have changed. I agree > it won't be pretty, but we won't have to unlock and lock again in the middle of the > called function and we'll have all our locking in the same place, easier to verify and > later easier to remove. Once I get rid of most of the br->lock usage we can revisit > the drop of PRE_FLAGS if it's a problem. The alternative is to change the flags, then > send the switchdev notification outside of the lock and revert the flags if it doesn't > go through which doesn't sound much better. > I'm open to any other suggestions, but definitely would like to avoid playing locking games. > Even if it means casing out flag setting from all other store_ functions for sysfs. By casing out flag settings you mean something like this? #define BRPORT_ATTR(_name, _mode, _show, _store) \ const struct brport_attribute brport_attr_##_name = { \ .attr = {.name = __stringify(_name), \ .mode = _mode }, \ .show = _show, \ .store_unlocked = _store, \ }; #define BRPORT_ATTR_FLAG(_name, _mask) \ static ssize_t show_##_name(struct net_bridge_port *p, char *buf) \ { \ return sprintf(buf, "%d\n", !!(p->flags & _mask)); \ } \ static int store_##_name(struct net_bridge_port *p, unsigned long v) \ { \ return store_flag(p, v, _mask); \ } \ static BRPORT_ATTR(_name, 0644, \ show_##_name, store_##_name) static ssize_t brport_store(struct kobject *kobj, struct attribute *attr, const char *buf, size_t count) { ... } else if (brport_attr->store_unlocked) { val = simple_strtoul(buf, &endp, 0); if (endp == buf) goto out_unlock; ret = brport_attr->store_unlocked(p, val); }
On Wed, Feb 10, 2021 at 01:05:57PM +0200, Nikolay Aleksandrov wrote: > On 10/02/2021 13:01, Vladimir Oltean wrote: > > On Wed, Feb 10, 2021 at 12:52:33PM +0200, Nikolay Aleksandrov wrote: > >> On 10/02/2021 12:45, Vladimir Oltean wrote: > >>> Hi Nikolay, > >>> > >>> On Wed, Feb 10, 2021 at 12:31:43PM +0200, Nikolay Aleksandrov wrote: > >>>> Hi Vladimir, > >>>> Let's take a step back for a moment and discuss the bridge unlock/lock sequences > >>>> that come with this set. I'd really like to avoid those as they're a recipe > >>>> for future problems. The only good way to achieve that currently is to keep > >>>> the PRE_FLAGS call and do that in unsleepable context but move the FLAGS call > >>>> after the flags have been changed (if they have changed obviously). That would > >>>> make the code read much easier since we'll have all our lock/unlock sequences > >>>> in the same code blocks and won't play games to get sleepable context. > >>>> Please let's think and work in that direction, rather than having: > >>>> + spin_lock_bh(&p->br->lock); > >>>> + if (err) { > >>>> + netdev_err(p->dev, "%s\n", extack._msg); > >>>> + return err; > >>>> } > >>>> + > >>>> > >>>> which immediately looks like a bug even though after some code checking we can > >>>> verify it's ok. WDYT? > >>>> > >>>> I plan to get rid of most of the br->lock since it's been abused for a very long > >>>> time because it's essentially STP lock, but people have started using it for other > >>>> things and I plan to fix that when I get more time. > >>> > >>> This won't make the sysfs codepath any nicer, will it? > >>> > >> > >> Currently we'll have to live with a hack that checks if the flags have changed. I agree > >> it won't be pretty, but we won't have to unlock and lock again in the middle of the > >> called function and we'll have all our locking in the same place, easier to verify and > >> later easier to remove. Once I get rid of most of the br->lock usage we can revisit > >> the drop of PRE_FLAGS if it's a problem. The alternative is to change the flags, then > >> send the switchdev notification outside of the lock and revert the flags if it doesn't > >> go through which doesn't sound much better. > >> I'm open to any other suggestions, but definitely would like to avoid playing locking games. > >> Even if it means casing out flag setting from all other store_ functions for sysfs. > > > > By casing out flag settings you mean something like this? > > > > > > #define BRPORT_ATTR(_name, _mode, _show, _store) \ > > const struct brport_attribute brport_attr_##_name = { \ > > .attr = {.name = __stringify(_name), \ > > .mode = _mode }, \ > > .show = _show, \ > > .store_unlocked = _store, \ > > }; > > > > #define BRPORT_ATTR_FLAG(_name, _mask) \ > > static ssize_t show_##_name(struct net_bridge_port *p, char *buf) \ > > { \ > > return sprintf(buf, "%d\n", !!(p->flags & _mask)); \ > > } \ > > static int store_##_name(struct net_bridge_port *p, unsigned long v) \ > > { \ > > return store_flag(p, v, _mask); \ > > } \ > > static BRPORT_ATTR(_name, 0644, \ > > show_##_name, store_##_name) > > > > static ssize_t brport_store(struct kobject *kobj, > > struct attribute *attr, > > const char *buf, size_t count) > > { > > ... > > > > } else if (brport_attr->store_unlocked) { > > val = simple_strtoul(buf, &endp, 0); > > if (endp == buf) > > goto out_unlock; > > ret = brport_attr->store_unlocked(p, val); > > } > > > > Yes, this can work but will need a bit more changes because of br_port_flags_change(). > Then the netlink side can be modeled in a similar way. What I just don't understand is how others can get away with doing sleepable work in atomic context but I can't make the notifier blocking by dropping a spinlock which isn't needed there, because it looks ugly :D
On Wed, Feb 10, 2021 at 02:01:06PM +0200, Vladimir Oltean wrote: > On Wed, Feb 10, 2021 at 01:05:57PM +0200, Nikolay Aleksandrov wrote: > > On 10/02/2021 13:01, Vladimir Oltean wrote: > > > On Wed, Feb 10, 2021 at 12:52:33PM +0200, Nikolay Aleksandrov wrote: > > >> On 10/02/2021 12:45, Vladimir Oltean wrote: > > >>> Hi Nikolay, > > >>> > > >>> On Wed, Feb 10, 2021 at 12:31:43PM +0200, Nikolay Aleksandrov wrote: > > >>>> Hi Vladimir, > > >>>> Let's take a step back for a moment and discuss the bridge unlock/lock sequences > > >>>> that come with this set. I'd really like to avoid those as they're a recipe > > >>>> for future problems. The only good way to achieve that currently is to keep > > >>>> the PRE_FLAGS call and do that in unsleepable context but move the FLAGS call > > >>>> after the flags have been changed (if they have changed obviously). That would > > >>>> make the code read much easier since we'll have all our lock/unlock sequences > > >>>> in the same code blocks and won't play games to get sleepable context. > > >>>> Please let's think and work in that direction, rather than having: > > >>>> + spin_lock_bh(&p->br->lock); > > >>>> + if (err) { > > >>>> + netdev_err(p->dev, "%s\n", extack._msg); > > >>>> + return err; > > >>>> } > > >>>> + > > >>>> > > >>>> which immediately looks like a bug even though after some code checking we can > > >>>> verify it's ok. WDYT? > > >>>> > > >>>> I plan to get rid of most of the br->lock since it's been abused for a very long > > >>>> time because it's essentially STP lock, but people have started using it for other > > >>>> things and I plan to fix that when I get more time. > > >>> > > >>> This won't make the sysfs codepath any nicer, will it? > > >>> > > >> > > >> Currently we'll have to live with a hack that checks if the flags have changed. I agree > > >> it won't be pretty, but we won't have to unlock and lock again in the middle of the > > >> called function and we'll have all our locking in the same place, easier to verify and > > >> later easier to remove. Once I get rid of most of the br->lock usage we can revisit > > >> the drop of PRE_FLAGS if it's a problem. The alternative is to change the flags, then > > >> send the switchdev notification outside of the lock and revert the flags if it doesn't > > >> go through which doesn't sound much better. > > >> I'm open to any other suggestions, but definitely would like to avoid playing locking games. > > >> Even if it means casing out flag setting from all other store_ functions for sysfs. > > > > > > By casing out flag settings you mean something like this? > > > > > > > > > #define BRPORT_ATTR(_name, _mode, _show, _store) \ > > > const struct brport_attribute brport_attr_##_name = { \ > > > .attr = {.name = __stringify(_name), \ > > > .mode = _mode }, \ > > > .show = _show, \ > > > .store_unlocked = _store, \ > > > }; > > > > > > #define BRPORT_ATTR_FLAG(_name, _mask) \ > > > static ssize_t show_##_name(struct net_bridge_port *p, char *buf) \ > > > { \ > > > return sprintf(buf, "%d\n", !!(p->flags & _mask)); \ > > > } \ > > > static int store_##_name(struct net_bridge_port *p, unsigned long v) \ > > > { \ > > > return store_flag(p, v, _mask); \ > > > } \ > > > static BRPORT_ATTR(_name, 0644, \ > > > show_##_name, store_##_name) > > > > > > static ssize_t brport_store(struct kobject *kobj, > > > struct attribute *attr, > > > const char *buf, size_t count) > > > { > > > ... > > > > > > } else if (brport_attr->store_unlocked) { > > > val = simple_strtoul(buf, &endp, 0); > > > if (endp == buf) > > > goto out_unlock; > > > ret = brport_attr->store_unlocked(p, val); > > > } > > > > > > > Yes, this can work but will need a bit more changes because of br_port_flags_change(). > > Then the netlink side can be modeled in a similar way. > > What I just don't understand is how others can get away with doing > sleepable work in atomic context but I can't make the notifier blocking > by dropping a spinlock which isn't needed there, because it looks ugly :D Can you please point to the bug? I'm not following
On Wed, Feb 10, 2021 at 02:29:36PM +0200, Vladimir Oltean wrote: > On Wed, Feb 10, 2021 at 02:21:05PM +0200, Ido Schimmel wrote: > > On Wed, Feb 10, 2021 at 02:01:06PM +0200, Vladimir Oltean wrote: > > > On Wed, Feb 10, 2021 at 01:05:57PM +0200, Nikolay Aleksandrov wrote: > > > > On 10/02/2021 13:01, Vladimir Oltean wrote: > > > > > On Wed, Feb 10, 2021 at 12:52:33PM +0200, Nikolay Aleksandrov wrote: > > > > >> On 10/02/2021 12:45, Vladimir Oltean wrote: > > > > >>> Hi Nikolay, > > > > >>> > > > > >>> On Wed, Feb 10, 2021 at 12:31:43PM +0200, Nikolay Aleksandrov wrote: > > > > >>>> Hi Vladimir, > > > > >>>> Let's take a step back for a moment and discuss the bridge unlock/lock sequences > > > > >>>> that come with this set. I'd really like to avoid those as they're a recipe > > > > >>>> for future problems. The only good way to achieve that currently is to keep > > > > >>>> the PRE_FLAGS call and do that in unsleepable context but move the FLAGS call > > > > >>>> after the flags have been changed (if they have changed obviously). That would > > > > >>>> make the code read much easier since we'll have all our lock/unlock sequences > > > > >>>> in the same code blocks and won't play games to get sleepable context. > > > > >>>> Please let's think and work in that direction, rather than having: > > > > >>>> + spin_lock_bh(&p->br->lock); > > > > >>>> + if (err) { > > > > >>>> + netdev_err(p->dev, "%s\n", extack._msg); > > > > >>>> + return err; > > > > >>>> } > > > > >>>> + > > > > >>>> > > > > >>>> which immediately looks like a bug even though after some code checking we can > > > > >>>> verify it's ok. WDYT? > > > > >>>> > > > > >>>> I plan to get rid of most of the br->lock since it's been abused for a very long > > > > >>>> time because it's essentially STP lock, but people have started using it for other > > > > >>>> things and I plan to fix that when I get more time. > > > > >>> > > > > >>> This won't make the sysfs codepath any nicer, will it? > > > > >>> > > > > >> > > > > >> Currently we'll have to live with a hack that checks if the flags have changed. I agree > > > > >> it won't be pretty, but we won't have to unlock and lock again in the middle of the > > > > >> called function and we'll have all our locking in the same place, easier to verify and > > > > >> later easier to remove. Once I get rid of most of the br->lock usage we can revisit > > > > >> the drop of PRE_FLAGS if it's a problem. The alternative is to change the flags, then > > > > >> send the switchdev notification outside of the lock and revert the flags if it doesn't > > > > >> go through which doesn't sound much better. > > > > >> I'm open to any other suggestions, but definitely would like to avoid playing locking games. > > > > >> Even if it means casing out flag setting from all other store_ functions for sysfs. > > > > > > > > > > By casing out flag settings you mean something like this? > > > > > > > > > > > > > > > #define BRPORT_ATTR(_name, _mode, _show, _store) \ > > > > > const struct brport_attribute brport_attr_##_name = { \ > > > > > .attr = {.name = __stringify(_name), \ > > > > > .mode = _mode }, \ > > > > > .show = _show, \ > > > > > .store_unlocked = _store, \ > > > > > }; > > > > > > > > > > #define BRPORT_ATTR_FLAG(_name, _mask) \ > > > > > static ssize_t show_##_name(struct net_bridge_port *p, char *buf) \ > > > > > { \ > > > > > return sprintf(buf, "%d\n", !!(p->flags & _mask)); \ > > > > > } \ > > > > > static int store_##_name(struct net_bridge_port *p, unsigned long v) \ > > > > > { \ > > > > > return store_flag(p, v, _mask); \ > > > > > } \ > > > > > static BRPORT_ATTR(_name, 0644, \ > > > > > show_##_name, store_##_name) > > > > > > > > > > static ssize_t brport_store(struct kobject *kobj, > > > > > struct attribute *attr, > > > > > const char *buf, size_t count) > > > > > { > > > > > ... > > > > > > > > > > } else if (brport_attr->store_unlocked) { > > > > > val = simple_strtoul(buf, &endp, 0); > > > > > if (endp == buf) > > > > > goto out_unlock; > > > > > ret = brport_attr->store_unlocked(p, val); > > > > > } > > > > > > > > > > > > > Yes, this can work but will need a bit more changes because of br_port_flags_change(). > > > > Then the netlink side can be modeled in a similar way. > > > > > > What I just don't understand is how others can get away with doing > > > sleepable work in atomic context but I can't make the notifier blocking > > > by dropping a spinlock which isn't needed there, because it looks ugly :D > > > > Can you please point to the bug? I'm not following > > For example, mlxsw eventually calls mlxsw_sp_fid_flood_set from the > SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS handling data path, and this > function allocates memory with GFP_KERNEL. > > Another example is prestera which eventually calls prestera_fw_send_req > which takes a mutex_lock. > > Yet another example are mv88e6xxx and b53 which use MDIO and SPI > from their .port_egress_floods implementation, buses which have > might_sleep() in them. Right, but see the code: ``` attr.id = SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS; attr.flags = SWITCHDEV_F_DEFER; attr.u.brport_flags = flags; err = switchdev_port_attr_set(p->dev, &attr); ``` And check how SWITCHDEV_F_DEFER is used. We can squash SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS and SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS into one blocking notification by reducing the scope of the bridge lock like Nik suggested. Currently it's just blindly taken around br_setport().
On 2/10/2021 1:14 AM, Vladimir Oltean wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > When a struct switchdev_attr is notified through switchdev, there is no > way to report informational messages, unlike for struct switchdev_obj. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > Reviewed-by: Ido Schimmel <idosch@nvidia.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> -- Florian
On 2/10/2021 1:14 AM, Vladimir Oltean wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > For a DSA switch port operating in standalone mode, address learning > doesn't make much sense since that is a bridge function. In fact, > address learning even breaks setups such as this one: > > +---------------------------------------------+ > | | > | +-------------------+ | > | | br0 | send receive | > | +--------+-+--------+ +--------+ +--------+ | > | | | | | | | | | | > | | swp0 | | swp1 | | swp2 | | swp3 | | > | | | | | | | | | | > +-+--------+-+--------+-+--------+-+--------+-+ > | ^ | ^ > | | | | > | +-----------+ | > | | > +--------------------------------+ > > because if the switch has a single FDB (can offload a single bridge) > then source address learning on swp3 can "steal" the source MAC address > of swp2 from br0's FDB, because learning frames coming from swp2 will be > done twice: first on the swp1 ingress port, second on the swp3 ingress > port. So the hardware FDB will become out of sync with the software > bridge, and when swp2 tries to send one more packet towards swp1, the > ASIC will attempt to short-circuit the forwarding path and send it > directly to swp3 (since that's the last port it learned that address on), > which it obviously can't, because swp3 operates in standalone mode. > > So DSA drivers operating in standalone mode should still configure a > list of bridge port flags even when they are standalone. Currently DSA > attempts to call dsa_port_bridge_flags with 0, which disables egress > flooding of unknown unicast and multicast, something which doesn't make > much sense. For the switches that implement .port_egress_floods - b53 > and mv88e6xxx, it probably doesn't matter too much either, since they > can possibly inject traffic from the CPU into a standalone port, > regardless of MAC DA, even if egress flooding is turned off for that > port, but certainly not all DSA switches can do that - sja1105, for > example, can't. So it makes sense to use a better common default there, > such as "flood everything". > > It should also be noted that what DSA calls "dsa_port_bridge_flags()" > is a degenerate name for just calling .port_egress_floods(), since > nothing else is implemented - not learning, in particular. But disabling > address learning, something that this driver is also coding up for, will > be supported by individual drivers once .port_egress_floods is replaced > with a more generic .port_bridge_flags. > > Previous attempts to code up this logic have been in the common bridge > layer, but as pointed out by Ido Schimmel, there are corner cases that > are missed when doing that: > https://patchwork.kernel.org/project/netdevbpf/patch/20210209151936.97382-5-olteanv@gmail.com/ > > So, at least for now, let's leave DSA in charge of setting port flags > before and after the bridge join and leave. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> -- Florian
On 2/10/2021 1:14 AM, Vladimir Oltean wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > The bridge offloads the port flags through a single bit mask using > switchdev, which among others, contains learning and flooding settings. > > The commit 57652796aa97 ("net: dsa: add support for bridge flags") > missed one crucial aspect of the SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS API > when designing the API one level lower, towards the drivers. > This is that the bitmask of passed brport flags never has more than one > bit set at a time. On the other hand, the prototype passed to the driver > is .port_egress_floods(int port, bool unicast, bool multicast), which > configures two flags at a time. > > DSA currently checks if .port_egress_floods is implemented, and if it > is, reports both BR_FLOOD and BR_MCAST_FLOOD as supported. So the driver > has no choice if it wants to inform the bridge that, for example, it > can't configure unicast flooding independently of multicast flooding - > the DSA mid layer is standing in the way. Or the other way around: a new > driver wants to start configuring BR_BCAST_FLOOD separately, but what do > we do with the rest, which only support unicast and multicast flooding? > Do we report broadcast flooding configuration as supported for those > too, and silently do nothing? > > Secondly, currently DSA deems the driver too dumb to deserve knowing that > a SWITCHDEV_ATTR_ID_BRIDGE_MROUTER attribute was offloaded, because it > just calls .port_egress_floods for the CPU port. When we'll add support > for the plain SWITCHDEV_ATTR_ID_PORT_MROUTER, that will become a real > problem because the flood settings will need to be held statefully in > the DSA middle layer, otherwise changing the mrouter port attribute will > impact the flooding attribute. And that's _assuming_ that the underlying > hardware doesn't have anything else to do when a multicast router > attaches to a port than flood unknown traffic to it. If it does, there > will need to be a dedicated .port_set_mrouter anyway. > > Lastly, we have DSA drivers that have a backlink into a pure switchdev > driver (felix -> ocelot). It seems reasonable that the other switchdev > drivers should not have to suffer from the oddities of DSA overengineering, > so keeping DSA a pass-through layer makes more sense there. > > To simplify the brport flags situation we just delete .port_egress_floods > and we introduce a simple .port_bridge_flags which is passed to the > driver. Also, the logic from dsa_port_mrouter is removed and a > .port_set_mrouter is created. > > Functionally speaking, we simply move the calls to .port_egress_floods > one step lower, in the two drivers that implement it: mv88e6xxx and b53, > so things should work just as before. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> -- Florian
On 2/10/2021 1:14 AM, Vladimir Oltean wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > In preparation of offloading the bridge port flags which have > independent settings for unknown multicast and for broadcast, we should > also start reserving one destination Port Group ID for the flooding of > broadcast packets, to allow configuring it individually. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> -- Florian
From: Vladimir Oltean <vladimir.oltean@nxp.com> The initial goal of this series was to have better support for standalone ports mode and multiple bridges on the DSA drivers like ocelot/felix and sja1105. Proper support for standalone mode requires disabling address learning, which in turn requires interaction with the switchdev notifier, which is actually where most of the patches are. I also noticed that most of the drivers are actually talking either to firmware or SPI/MDIO connected devices from the brport flags switchdev attribute handler, so it makes sense to actually make it sleepable instead of atomic. Vladimir Oltean (11): net: switchdev: propagate extack to port attributes net: bridge: offload all port flags at once in br_setport net: bridge: don't print in br_switchdev_set_port_flag net: dsa: configure proper brport flags when ports leave the bridge net: squash switchdev attributes PRE_BRIDGE_FLAGS and BRIDGE_FLAGS net: dsa: kill .port_egress_floods overengineering net: prep switchdev drivers for concurrent SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS net: bridge: put SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS on the blocking call chain net: mscc: ocelot: use separate flooding PGID for broadcast net: mscc: ocelot: offload bridge port flags to device net: dsa: sja1105: offload bridge port flags to device drivers/net/dsa/b53/b53_common.c | 20 +- drivers/net/dsa/mv88e6xxx/chip.c | 21 +- drivers/net/dsa/ocelot/felix.c | 10 + drivers/net/dsa/sja1105/sja1105.h | 2 + drivers/net/dsa/sja1105/sja1105_main.c | 212 +++++++++++++++++- drivers/net/dsa/sja1105/sja1105_spi.c | 6 + .../marvell/prestera/prestera_switchdev.c | 54 +++-- .../mellanox/mlxsw/spectrum_switchdev.c | 90 ++++---- drivers/net/ethernet/mscc/ocelot.c | 72 +++++- drivers/net/ethernet/mscc/ocelot_net.c | 7 +- drivers/net/ethernet/rocker/rocker.h | 2 +- drivers/net/ethernet/rocker/rocker_main.c | 24 +- drivers/net/ethernet/rocker/rocker_ofdpa.c | 26 ++- drivers/net/ethernet/ti/cpsw_switchdev.c | 35 ++- drivers/staging/fsl-dpaa2/ethsw/ethsw.c | 43 ++-- include/net/dsa.h | 7 +- include/net/switchdev.h | 14 +- include/soc/mscc/ocelot.h | 18 +- net/bridge/br_netlink.c | 162 ++++++------- net/bridge/br_private.h | 6 +- net/bridge/br_switchdev.c | 33 ++- net/bridge/br_sysfs_if.c | 21 +- net/dsa/dsa_priv.h | 8 +- net/dsa/port.c | 76 ++++--- net/dsa/slave.c | 10 +- net/switchdev/switchdev.c | 11 +- 26 files changed, 654 insertions(+), 336 deletions(-)