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

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

[Simon Horman]

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.

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

diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
index 2d8578cade03..9e812b85b421 100644
--- a/drivers/net/ethernet/rocker/rocker.c
+++ b/drivers/net/ethernet/rocker/rocker.c
@@ -199,6 +199,7 @@ struct rocker;
 enum {
 	ROCKER_CTRL_LINK_LOCAL_MCAST,
 	ROCKER_CTRL_LOCAL_ARP,
+	ROCKER_CTRL_PROMISC,
 	ROCKER_CTRL_IPV4_MCAST,
 	ROCKER_CTRL_IPV6_MCAST,
 	ROCKER_CTRL_DFLT_BRIDGING,
@@ -3222,6 +3223,12 @@ static struct rocker_ctrl {
 		.eth_type = htons(ETH_P_ARP),
 		.acl = true,
 	},
+	[ROCKER_CTRL_PROMISC] = {
+		/* pass all pkts up to CPU */
+		.eth_dst = zero_mac,
+		.eth_dst_mask = zero_mac,
+		.acl = true,
+	},
 	[ROCKER_CTRL_IPV4_MCAST] = {
 		/* pass IPv4 mcast pkts up to CPU, RFC 1112 */
 		.eth_dst = ipv4_mcast,
@@ -3755,13 +3762,19 @@ 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->dev->flags & IFF_PROMISC))
+			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))
+		if (rocker_port_is_bridged(rocker_port)) {
 			want[ROCKER_CTRL_DFLT_BRIDGING] = true;
-		else
-			want[ROCKER_CTRL_LOCAL_ARP] = true;
+		} else {
+			if (rocker_port->dev->flags & IFF_PROMISC) {
+				want[ROCKER_CTRL_PROMISC] = true;
+			} else {
+				want[ROCKER_CTRL_LOCAL_ARP] = true;
+			}
+		}
 		break;
 	}
 
@@ -4152,6 +4165,17 @@ static int rocker_port_set_mac_address(struct net_device *dev, void *p)
 	return 0;
 }
 
+static void rocker_port_change_rx_flags(struct net_device *dev, int change)
+{
+	struct rocker_port *rocker_port = netdev_priv(dev);
+
+	if (change & IFF_PROMISC && rocker_port->dev->flags & IFF_UP)
+		if (!rocker_port_fwd_disable(rocker_port,
+					     SWITCHDEV_TRANS_NONE, 0))
+			rocker_port_fwd_enable(rocker_port,
+					       SWITCHDEV_TRANS_NONE, 0);
+}
+
 static int rocker_port_get_phys_port_name(struct net_device *dev,
 					  char *buf, size_t len)
 {
@@ -4171,6 +4195,7 @@ static const struct net_device_ops rocker_port_netdev_ops = {
 	.ndo_open			= rocker_port_open,
 	.ndo_stop			= rocker_port_stop,
 	.ndo_start_xmit			= rocker_port_xmit,
+	.ndo_change_rx_flags		= rocker_port_change_rx_flags,
 	.ndo_set_mac_address		= rocker_port_set_mac_address,
 	.ndo_bridge_getlink		= switchdev_port_bridge_getlink,
 	.ndo_bridge_setlink		= switchdev_port_bridge_setlink,
-- 
2.1.4

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

[John Fastabend]

On 15-07-08 09:25 PM, Simon Horman 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.
> 

My opinion would be to not confuse a host concept 'promiscuous mode'
and allow it to impact how the forwarding is done by the hardware. I
think a better approach would be to put a default low priority rule in
one of the hardware match action tables to mirror all traffic to the
port you want to mirror traffic to. And if it happens to be a CPU port
then it will be pushed to the host.

My problem with the semantics here is it seems overload how we use
promiscuous mode in the NIC today. For example I don't see how to use
it in an SR-IOV case or multi-function device. How would you handle
this if more than one port was put in promiscuous mode? Mirror to both
ports? Today when we put a device in a promiscuous mode behind a
switch we don't expect to start receiving all traffic.

I think at very least we need to be very specific about how we define
it and when it applies if we overload promisc field. Also wouldn't
it break any 'bridge/vswitch' software that puts the port in promisc
mode by default.

> 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.

Agree with this point.

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

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

[Scott Feldman]

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.

-scott

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

[Simon Horman]

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(-)

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;
 }
-- 
2.1.4

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

[Scott Feldman]

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
>
>

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

[Simon Horman]

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]

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

[Scott Feldman]

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.

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

[Simon Horman]

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 */

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

[Scott Feldman]

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

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

[Simon Horman]

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.