diff mbox

[PATCH/RFC,net-next] rocker: forward packets to CPU when a port in promiscuous mode

Message ID 20150715044521.GA7603@vergenet.net
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Simon Horman July 15, 2015, 4:45 a.m. UTC 
Hi Scott,

On Mon, Jul 13, 2015 at 11:37:59PM -0700, Scott Feldman wrote:
> On Wed, Jul 8, 2015 at 9:25 PM, Simon Horman <simon.horman@netronome.com> wrote:
> > This change allows the CPU to see all packets seen by a port when the
> > netdev associated with the port is in promiscuous mode.
> >
> > This change was previously posted as part of a larger patch and in turn
> > patchset which also aimed to allow rocker interfaces to receive packets
> > when not bridged. That problem has subsequently been addressed in a
> > different way by Scott Feldman.
> >
> > When this change was previously posted Scott indicated that he had some
> > reservations about sending all packets from a switch to the CPU. The
> > purpose of posting this patch is to start discussion of weather this
> > approach is appropriate and if not how else we might move forwards.
> >
> > In my opinion if host doesn't want all packets its shouldn't put a port
> > in promiscuous mode. But perhaps that is an overly naïve view to take.
> >
> > My main motivation for this change at this time is to allow rocker to
> > work with Open vSwitch and it appears that this change is sufficient to
> > reach that goal. Another approach might be to teach
> > rocker_port_master_changed() about Open vSwitch.
> >
> > In the longer term I believe Open vSwitch should be able to program
> > flows into rocker 'hardware' and thus not all packets would reach the CPU.
> 
> Hi Simon,
> 
> I like your alternate approach to teach rocker about Open vSwitch
> using rocker_port_master_change() and only when port is captured by
> OVS would we install the "promisc" filter to pass all traffic up.
> (Maybe call it ROCKER_CTRL_DFLT_OVS rule?).
> 
> Putting a non-bridged, non-ovs port into promisc is kind of weird for
> a switch port.  I think of the port in L3 mode by default, where the
> port is locked down for all but some selective mcasts, and only opened
> up by installing explicit routes.  (Unlike a bridged port where we
> flood everything L2 we don't understand).
> 
> So maybe first pass is to pass up everything when port is captured by
> OVS, and then later refine what's passed up per ovs flows on that
> port.

That sounds reasonable to me. Its pretty clear to me from the responses
from John and yourself that my approach to the promiscuous flag isn't
as clean-cut as I had hoped. And it seems that we have a nice way to
move forwards on supporting Open vSwitch.

How about this?

-- >8 --

From: Simon Horman <simon.horman@netronome.com>

Subject: rocker: forward packets to CPU when port is joined to openvswitch

Teach rocker to forward packets to CPU when a port is joined to Open vSwitch.
There is scope to later refine what is passed up as per Open vSwitch flows
on a port.

This does not change the behaviour of rocker ports that are
not joined to Open vSwitch.

Signed-off-by: Simon Horman <simon.horman@netronome.com>
---
 drivers/net/ethernet/rocker/rocker.c | 55 ++++++++++++++++++++++++++++++++----
 1 file changed, 49 insertions(+), 6 deletions(-)

Comments

Scott Feldman July 15, 2015, 5:32 a.m. UTC | #1 
On Tue, Jul 14, 2015 at 9:45 PM, Simon Horman
<simon.horman@netronome.com> wrote:
> Hi Scott,
>
> On Mon, Jul 13, 2015 at 11:37:59PM -0700, Scott Feldman wrote:
>> On Wed, Jul 8, 2015 at 9:25 PM, Simon Horman <simon.horman@netronome.com> wrote:
>> > This change allows the CPU to see all packets seen by a port when the
>> > netdev associated with the port is in promiscuous mode.
>> >
>> > This change was previously posted as part of a larger patch and in turn
>> > patchset which also aimed to allow rocker interfaces to receive packets
>> > when not bridged. That problem has subsequently been addressed in a
>> > different way by Scott Feldman.
>> >
>> > When this change was previously posted Scott indicated that he had some
>> > reservations about sending all packets from a switch to the CPU. The
>> > purpose of posting this patch is to start discussion of weather this
>> > approach is appropriate and if not how else we might move forwards.
>> >
>> > In my opinion if host doesn't want all packets its shouldn't put a port
>> > in promiscuous mode. But perhaps that is an overly naïve view to take.
>> >
>> > My main motivation for this change at this time is to allow rocker to
>> > work with Open vSwitch and it appears that this change is sufficient to
>> > reach that goal. Another approach might be to teach
>> > rocker_port_master_changed() about Open vSwitch.
>> >
>> > In the longer term I believe Open vSwitch should be able to program
>> > flows into rocker 'hardware' and thus not all packets would reach the CPU.
>>
>> Hi Simon,
>>
>> I like your alternate approach to teach rocker about Open vSwitch
>> using rocker_port_master_change() and only when port is captured by
>> OVS would we install the "promisc" filter to pass all traffic up.
>> (Maybe call it ROCKER_CTRL_DFLT_OVS rule?).
>>
>> Putting a non-bridged, non-ovs port into promisc is kind of weird for
>> a switch port.  I think of the port in L3 mode by default, where the
>> port is locked down for all but some selective mcasts, and only opened
>> up by installing explicit routes.  (Unlike a bridged port where we
>> flood everything L2 we don't understand).
>>
>> So maybe first pass is to pass up everything when port is captured by
>> OVS, and then later refine what's passed up per ovs flows on that
>> port.
>
> That sounds reasonable to me. Its pretty clear to me from the responses
> from John and yourself that my approach to the promiscuous flag isn't
> as clean-cut as I had hoped. And it seems that we have a nice way to
> move forwards on supporting Open vSwitch.
>
> How about this?

Looks good, some inline comments...

> From: Simon Horman <simon.horman@netronome.com>
>
> Subject: rocker: forward packets to CPU when port is joined to openvswitch
>
> Teach rocker to forward packets to CPU when a port is joined to Open vSwitch.
> There is scope to later refine what is passed up as per Open vSwitch flows
> on a port.
>
> This does not change the behaviour of rocker ports that are
> not joined to Open vSwitch.
>
> Signed-off-by: Simon Horman <simon.horman@netronome.com>
> ---
>  drivers/net/ethernet/rocker/rocker.c | 55 ++++++++++++++++++++++++++++++++----
>  1 file changed, 49 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
> index c0051673c9fa..8c53d6839260 100644
> --- a/drivers/net/ethernet/rocker/rocker.c
> +++ b/drivers/net/ethernet/rocker/rocker.c
> @@ -202,6 +202,7 @@ enum {
>         ROCKER_CTRL_IPV4_MCAST,
>         ROCKER_CTRL_IPV6_MCAST,
>         ROCKER_CTRL_DFLT_BRIDGING,
> +       ROCKER_CTRL_DFLT_OVS,
>         ROCKER_CTRL_MAX,
>  };
>
> @@ -321,9 +322,21 @@ static u16 rocker_port_vlan_to_vid(const struct rocker_port *rocker_port,
>         return ntohs(vlan_id);
>  }
>
> +static bool rocker_port_is_bridged__(const struct rocker_port *rocker_port,
> +                                    const char *kind)

Maybe use __rocker_port_is_bridged?  (leading '__'; I've not seen use
of trailing '__').  Or __rocker_port_is_slave(port, kind)?

> +{
> +       return rocker_port->bridge_dev &&
> +               !strcmp(rocker_port->bridge_dev->rtnl_link_ops->kind, kind);
> +}
> +
>  static bool rocker_port_is_bridged(const struct rocker_port *rocker_port)
>  {
> -       return !!rocker_port->bridge_dev;
> +       return rocker_port_is_bridged__(rocker_port, "bridge");
> +}
> +
> +static bool rocker_port_is_ovs(const struct rocker_port *rocker_port)

rocker_port_is_ovsed?  Just to be consistent with is_bridged.

> +{
> +       return rocker_port_is_bridged__(rocker_port, "openvswitch");
>  }
>
>  #define ROCKER_OP_FLAG_REMOVE          BIT(0)
> @@ -3275,6 +3288,12 @@ static struct rocker_ctrl {
>                 .bridge = true,
>                 .copy_to_cpu = true,
>         },
> +       [ROCKER_CTRL_DFLT_OVS] = {
> +               /* pass all pkts up to CPU */
> +               .eth_dst = zero_mac,
> +               .eth_dst_mask = zero_mac,
> +               .acl = true,
> +       },
>  };
>
>  static int rocker_port_ctrl_vlan_acl(struct rocker_port *rocker_port,
> @@ -3787,11 +3806,14 @@ static int rocker_port_stp_update(struct rocker_port *rocker_port,
>                 break;
>         case BR_STATE_LEARNING:
>         case BR_STATE_FORWARDING:
> -               want[ROCKER_CTRL_LINK_LOCAL_MCAST] = true;
> +               if (!rocker_port_is_ovs(rocker_port))
> +                       want[ROCKER_CTRL_LINK_LOCAL_MCAST] = true;
>                 want[ROCKER_CTRL_IPV4_MCAST] = true;
>                 want[ROCKER_CTRL_IPV6_MCAST] = true;
>                 if (rocker_port_is_bridged(rocker_port))
>                         want[ROCKER_CTRL_DFLT_BRIDGING] = true;
> +               else if (rocker_port_is_ovs(rocker_port))
> +                       want[ROCKER_CTRL_DFLT_OVS] = true;
>                 else
>                         want[ROCKER_CTRL_LOCAL_ARP] = true;
>                 break;
> @@ -5251,6 +5273,22 @@ static int rocker_port_bridge_leave(struct rocker_port *rocker_port)
>         return err;
>  }
>
> +
> +static int rocker_port_ovs_changed(struct rocker_port *rocker_port,
> +                                  struct net_device *master)
> +{
> +       int err;
> +
> +       rocker_port->bridge_dev = master;
> +
> +       err = rocker_port_fwd_disable(rocker_port, SWITCHDEV_TRANS_NONE, 0);
> +       if (err)
> +               return err;
> +       err = rocker_port_fwd_enable(rocker_port, SWITCHDEV_TRANS_NONE, 0);
> +
> +       return err;
> +}
> +
>  static int rocker_port_master_changed(struct net_device *dev)
>  {
>         struct rocker_port *rocker_port = netdev_priv(dev);
> @@ -5263,11 +5301,16 @@ static int rocker_port_master_changed(struct net_device *dev)
>          * 3. Other, e.g. being added to or removed from a bond or openvswitch,
>          *    in which case nothing is done
>          */

Maybe comment above needs adjusting?

> -       if (master && master->rtnl_link_ops &&
> -           !strcmp(master->rtnl_link_ops->kind, "bridge"))
> -               err = rocker_port_bridge_join(rocker_port, master);
> -       else if (rocker_port_is_bridged(rocker_port))
> +       if (master && master->rtnl_link_ops) {
> +               if (!strcmp(master->rtnl_link_ops->kind, "bridge"))
> +                       err = rocker_port_bridge_join(rocker_port, master);
> +               else if (!strcmp(master->rtnl_link_ops->kind, "openvswitch"))
> +                       err = rocker_port_ovs_changed(rocker_port, master);
> +       } else if (rocker_port_is_bridged(rocker_port)) {
>                 err = rocker_port_bridge_leave(rocker_port);
> +       } else if (rocker_port_is_ovs(rocker_port)) {
> +               err = rocker_port_ovs_changed(rocker_port, NULL);
> +       }
>
>         return err;
>  }
> --
> 2.1.4
>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Simon Horman July 15, 2015, 6:34 a.m. UTC | #2 
On Tue, Jul 14, 2015 at 10:32:54PM -0700, Scott Feldman wrote:
> On Tue, Jul 14, 2015 at 9:45 PM, Simon Horman

[snip]

> > How about this?
> 
> Looks good, some inline comments...

[snip]

> > @@ -321,9 +322,21 @@ static u16 rocker_port_vlan_to_vid(const struct rocker_port *rocker_port,
> >         return ntohs(vlan_id);
> >  }
> >
> > +static bool rocker_port_is_bridged__(const struct rocker_port *rocker_port,
> > +                                    const char *kind)
> 
> Maybe use __rocker_port_is_bridged?  (leading '__'; I've not seen use
> of trailing '__').  Or __rocker_port_is_slave(port, kind)?

Perhaps rocker_port_is_slave() would be a good choice?

> > +{
> > +       return rocker_port->bridge_dev &&
> > +               !strcmp(rocker_port->bridge_dev->rtnl_link_ops->kind, kind);
> > +}
> > +
> >  static bool rocker_port_is_bridged(const struct rocker_port *rocker_port)
> >  {
> > -       return !!rocker_port->bridge_dev;
> > +       return rocker_port_is_bridged__(rocker_port, "bridge");
> > +}
> > +
> > +static bool rocker_port_is_ovs(const struct rocker_port *rocker_port)
> 
> rocker_port_is_ovsed?  Just to be consistent with is_bridged.

Sure, for the sake of consistency I'll change things as you suggest.

[snip]

> > @@ -5263,11 +5301,16 @@ static int rocker_port_master_changed(struct net_device *dev)
> >          * 3. Other, e.g. being added to or removed from a bond or openvswitch,
> >          *    in which case nothing is done
> >          */
> 
> Maybe comment above needs adjusting?

Indeed, sorry for missing that.
How about this?


        /* There are currently five cases handled here:
         * 1. Joining a bridge
         * 2. Joining a Open vSwitch datapath
         * 3. Leaving a previously joined bridge
         * 4. Leaving a previously joined Open vSwitch datapath
         * 5. Other, e.g. being added to or removed from a bond,
         *    in which case nothing is done
         */

[snip]
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Scott Feldman July 15, 2015, 7:18 a.m. UTC | #3 
On Tue, Jul 14, 2015 at 11:34 PM, Simon Horman
<simon.horman@netronome.com> wrote:
> On Tue, Jul 14, 2015 at 10:32:54PM -0700, Scott Feldman wrote:

>> > @@ -5263,11 +5301,16 @@ static int rocker_port_master_changed(struct net_device *dev)
>> >          * 3. Other, e.g. being added to or removed from a bond or openvswitch,
>> >          *    in which case nothing is done
>> >          */
>>
>> Maybe comment above needs adjusting?
>
> Indeed, sorry for missing that.
> How about this?
>
>
>         /* There are currently five cases handled here:
>          * 1. Joining a bridge
>          * 2. Joining a Open vSwitch datapath
>          * 3. Leaving a previously joined bridge
>          * 4. Leaving a previously joined Open vSwitch datapath
>          * 5. Other, e.g. being added to or removed from a bond,
>          *    in which case nothing is done
>          */

Seems like one of those comments that needs adjusting each time the
code changes, which isn't good.  Maybe just kill the comment or write
in a generic way saying what code is doing.  Code seems obvious enough
to me to not require a comment.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Simon Horman July 15, 2015, 7:54 a.m. UTC | #4 
On Wed, Jul 15, 2015 at 12:18:20AM -0700, Scott Feldman wrote:
> On Tue, Jul 14, 2015 at 11:34 PM, Simon Horman
> <simon.horman@netronome.com> wrote:
> > On Tue, Jul 14, 2015 at 10:32:54PM -0700, Scott Feldman wrote:
> 
> >> > @@ -5263,11 +5301,16 @@ static int rocker_port_master_changed(struct net_device *dev)
> >> >          * 3. Other, e.g. being added to or removed from a bond or openvswitch,
> >> >          *    in which case nothing is done
> >> >          */
> >>
> >> Maybe comment above needs adjusting?
> >
> > Indeed, sorry for missing that.
> > How about this?
> >
> >
> >         /* There are currently five cases handled here:
> >          * 1. Joining a bridge
> >          * 2. Joining a Open vSwitch datapath
> >          * 3. Leaving a previously joined bridge
> >          * 4. Leaving a previously joined Open vSwitch datapath
> >          * 5. Other, e.g. being added to or removed from a bond,
> >          *    in which case nothing is done
> >          */
> 
> Seems like one of those comments that needs adjusting each time the
> code changes, which isn't good.  Maybe just kill the comment or write
> in a generic way saying what code is doing.  Code seems obvious enough
> to me to not require a comment.

My purpose in adding the comment in the first place was
to document the "other" case, as it wasn't handled and thus
didn't seem obvious.

Perhaps something like this would work.

	/* N.B: Do nothing if the type of master is not supported */
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Scott Feldman July 15, 2015, 2:50 p.m. UTC | #5 
On Wed, Jul 15, 2015 at 12:54 AM, Simon Horman
<simon.horman@netronome.com> wrote:
> On Wed, Jul 15, 2015 at 12:18:20AM -0700, Scott Feldman wrote:
>> On Tue, Jul 14, 2015 at 11:34 PM, Simon Horman
>> <simon.horman@netronome.com> wrote:
>> > On Tue, Jul 14, 2015 at 10:32:54PM -0700, Scott Feldman wrote:
>>
>> >> > @@ -5263,11 +5301,16 @@ static int rocker_port_master_changed(struct net_device *dev)
>> >> >          * 3. Other, e.g. being added to or removed from a bond or openvswitch,
>> >> >          *    in which case nothing is done
>> >> >          */
>> >>
>> >> Maybe comment above needs adjusting?
>> >
>> > Indeed, sorry for missing that.
>> > How about this?
>> >
>> >
>> >         /* There are currently five cases handled here:
>> >          * 1. Joining a bridge
>> >          * 2. Joining a Open vSwitch datapath
>> >          * 3. Leaving a previously joined bridge
>> >          * 4. Leaving a previously joined Open vSwitch datapath
>> >          * 5. Other, e.g. being added to or removed from a bond,
>> >          *    in which case nothing is done
>> >          */
>>
>> Seems like one of those comments that needs adjusting each time the
>> code changes, which isn't good.  Maybe just kill the comment or write
>> in a generic way saying what code is doing.  Code seems obvious enough
>> to me to not require a comment.
>
> My purpose in adding the comment in the first place was
> to document the "other" case, as it wasn't handled and thus
> didn't seem obvious.
>
> Perhaps something like this would work.
>
>         /* N.B: Do nothing if the type of master is not supported */

Ack
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Simon Horman July 16, 2015, 1:41 a.m. UTC | #6 
On Wed, Jul 15, 2015 at 07:50:32AM -0700, Scott Feldman wrote:
> On Wed, Jul 15, 2015 at 12:54 AM, Simon Horman
> <simon.horman@netronome.com> wrote:
> > On Wed, Jul 15, 2015 at 12:18:20AM -0700, Scott Feldman wrote:
> >> On Tue, Jul 14, 2015 at 11:34 PM, Simon Horman
> >> <simon.horman@netronome.com> wrote:
> >> > On Tue, Jul 14, 2015 at 10:32:54PM -0700, Scott Feldman wrote:
> >>
> >> >> > @@ -5263,11 +5301,16 @@ static int rocker_port_master_changed(struct net_device *dev)
> >> >> >          * 3. Other, e.g. being added to or removed from a bond or openvswitch,
> >> >> >          *    in which case nothing is done
> >> >> >          */
> >> >>
> >> >> Maybe comment above needs adjusting?
> >> >
> >> > Indeed, sorry for missing that.
> >> > How about this?
> >> >
> >> >
> >> >         /* There are currently five cases handled here:
> >> >          * 1. Joining a bridge
> >> >          * 2. Joining a Open vSwitch datapath
> >> >          * 3. Leaving a previously joined bridge
> >> >          * 4. Leaving a previously joined Open vSwitch datapath
> >> >          * 5. Other, e.g. being added to or removed from a bond,
> >> >          *    in which case nothing is done
> >> >          */
> >>
> >> Seems like one of those comments that needs adjusting each time the
> >> code changes, which isn't good.  Maybe just kill the comment or write
> >> in a generic way saying what code is doing.  Code seems obvious enough
> >> to me to not require a comment.
> >
> > My purpose in adding the comment in the first place was
> > to document the "other" case, as it wasn't handled and thus
> > didn't seem obvious.
> >
> > Perhaps something like this would work.
> >
> >         /* N.B: Do nothing if the type of master is not supported */
> 
> Ack

Thanks. I've made a formal submission of the path with the
changes you suggested earlier in this thread.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
index c0051673c9fa..8c53d6839260 100644
--- a/drivers/net/ethernet/rocker/rocker.c
+++ b/drivers/net/ethernet/rocker/rocker.c
@@ -202,6 +202,7 @@  enum {
 	ROCKER_CTRL_IPV4_MCAST,
 	ROCKER_CTRL_IPV6_MCAST,
 	ROCKER_CTRL_DFLT_BRIDGING,
+	ROCKER_CTRL_DFLT_OVS,
 	ROCKER_CTRL_MAX,
 };
 
@@ -321,9 +322,21 @@  static u16 rocker_port_vlan_to_vid(const struct rocker_port *rocker_port,
 	return ntohs(vlan_id);
 }
 
+static bool rocker_port_is_bridged__(const struct rocker_port *rocker_port,
+				     const char *kind)
+{
+	return rocker_port->bridge_dev &&
+		!strcmp(rocker_port->bridge_dev->rtnl_link_ops->kind, kind);
+}
+
 static bool rocker_port_is_bridged(const struct rocker_port *rocker_port)
 {
-	return !!rocker_port->bridge_dev;
+	return rocker_port_is_bridged__(rocker_port, "bridge");
+}
+
+static bool rocker_port_is_ovs(const struct rocker_port *rocker_port)
+{
+	return rocker_port_is_bridged__(rocker_port, "openvswitch");
 }
 
 #define ROCKER_OP_FLAG_REMOVE		BIT(0)
@@ -3275,6 +3288,12 @@  static struct rocker_ctrl {
 		.bridge = true,
 		.copy_to_cpu = true,
 	},
+	[ROCKER_CTRL_DFLT_OVS] = {
+		/* pass all pkts up to CPU */
+		.eth_dst = zero_mac,
+		.eth_dst_mask = zero_mac,
+		.acl = true,
+	},
 };
 
 static int rocker_port_ctrl_vlan_acl(struct rocker_port *rocker_port,
@@ -3787,11 +3806,14 @@  static int rocker_port_stp_update(struct rocker_port *rocker_port,
 		break;
 	case BR_STATE_LEARNING:
 	case BR_STATE_FORWARDING:
-		want[ROCKER_CTRL_LINK_LOCAL_MCAST] = true;
+		if (!rocker_port_is_ovs(rocker_port))
+			want[ROCKER_CTRL_LINK_LOCAL_MCAST] = true;
 		want[ROCKER_CTRL_IPV4_MCAST] = true;
 		want[ROCKER_CTRL_IPV6_MCAST] = true;
 		if (rocker_port_is_bridged(rocker_port))
 			want[ROCKER_CTRL_DFLT_BRIDGING] = true;
+		else if (rocker_port_is_ovs(rocker_port))
+			want[ROCKER_CTRL_DFLT_OVS] = true;
 		else
 			want[ROCKER_CTRL_LOCAL_ARP] = true;
 		break;
@@ -5251,6 +5273,22 @@  static int rocker_port_bridge_leave(struct rocker_port *rocker_port)
 	return err;
 }
 
+
+static int rocker_port_ovs_changed(struct rocker_port *rocker_port,
+				   struct net_device *master)
+{
+	int err;
+
+	rocker_port->bridge_dev = master;
+
+	err = rocker_port_fwd_disable(rocker_port, SWITCHDEV_TRANS_NONE, 0);
+	if (err)
+		return err;
+	err = rocker_port_fwd_enable(rocker_port, SWITCHDEV_TRANS_NONE, 0);
+
+	return err;
+}
+
 static int rocker_port_master_changed(struct net_device *dev)
 {
 	struct rocker_port *rocker_port = netdev_priv(dev);
@@ -5263,11 +5301,16 @@  static int rocker_port_master_changed(struct net_device *dev)
 	 * 3. Other, e.g. being added to or removed from a bond or openvswitch,
 	 *    in which case nothing is done
 	 */
-	if (master && master->rtnl_link_ops &&
-	    !strcmp(master->rtnl_link_ops->kind, "bridge"))
-		err = rocker_port_bridge_join(rocker_port, master);
-	else if (rocker_port_is_bridged(rocker_port))
+	if (master && master->rtnl_link_ops) {
+		if (!strcmp(master->rtnl_link_ops->kind, "bridge"))
+			err = rocker_port_bridge_join(rocker_port, master);
+		else if (!strcmp(master->rtnl_link_ops->kind, "openvswitch"))
+			err = rocker_port_ovs_changed(rocker_port, master);
+	} else if (rocker_port_is_bridged(rocker_port)) {
 		err = rocker_port_bridge_leave(rocker_port);
+	} else if (rocker_port_is_ovs(rocker_port)) {
+		err = rocker_port_ovs_changed(rocker_port, NULL);
+	}
 
 	return err;
 }