| Message ID | 20210210091445.741269-1-olteanv@gmail.com (mailing list archive) |
|---|---|
| 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
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?
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 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.
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 10/02/2021 14:01, 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 > That's a bug that's gone unnoticed, surely not an argument to make error-prone changes. It's not because of ugliness, rather for easier reasoning when people want to work with that code, easier to maintain and later easier to verify when the lock gets removed. We'll reduce the chance for new bugs by having code that can be understood easier, especially for locking it's never a good idea to play games, we must try to avoid it when we can.
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: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.
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 Wed, Feb 10, 2021 at 02:38:23PM +0200, Ido Schimmel wrote: > 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(). Okay, so the deferred attr_set propagates just a possible ENOMEM from the deferred work enqueue, not the actual failure if that occurred. I can leave alone the piece that sends two notifications for now, but I would still need to deliver the full struct switchdev_brport_flags with both the flags and the mask to both the PRE_BRIDGE_FLAGS and the BRIDGE_FLAGS, because I need to deliver an extack from the sja1105 driver that BR_FLOOD should always have the same value as BR_MCAST_FLOOD.
On Wed, Feb 10, 2021 at 02:55:01PM +0200, Vladimir Oltean wrote: > On Wed, Feb 10, 2021 at 02:38:23PM +0200, Ido Schimmel wrote: > > 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(). > > Okay, so the deferred attr_set propagates just a possible ENOMEM from > the deferred work enqueue, not the actual failure if that occurred. > > I can leave alone the piece that sends two notifications for now, but I > would still need to deliver the full struct switchdev_brport_flags with > both the flags and the mask to both the PRE_BRIDGE_FLAGS and the > BRIDGE_FLAGS, because I need to deliver an extack from the sja1105 driver > that BR_FLOOD should always have the same value as BR_MCAST_FLOOD. OK
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(-)