diff mbox

[RFC,7/9] net: dsa: mv88e6352: lock CPU port from learning addresses

Message ID 1433208470-25338-8-git-send-email-vivien.didelot@savoirfairelinux.com
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Vivien Didelot June 2, 2015, 1:27 a.m. UTC 
This commit disables SA learning and refreshing for the CPU port.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx.c | 8 +++++---
 drivers/net/dsa/mv88e6xxx.h | 1 +
 2 files changed, 6 insertions(+), 3 deletions(-)

Comments

Guenter Roeck June 2, 2015, 2:24 p.m. UTC | #1 
On 06/01/2015 06:27 PM, Vivien Didelot wrote:
> This commit disables SA learning and refreshing for the CPU port.
>

Hi Vivien,

This patch also seems to be unrelated to the rest of the series.

Can you add an explanation why it is needed ?

With this in place, how does the CPU port SA find its way into the fdb ?
Do we assume that it will be configured statically ?
An explanation might be useful.

Thanks,
Guenter

> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> ---
>   drivers/net/dsa/mv88e6xxx.c | 8 +++++---
>   drivers/net/dsa/mv88e6xxx.h | 1 +
>   2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
> index d2beb10..ed49bd8 100644
> --- a/drivers/net/dsa/mv88e6xxx.c
> +++ b/drivers/net/dsa/mv88e6xxx.c
> @@ -1761,10 +1761,12 @@ static int mv88e6xxx_setup_port(struct dsa_switch *ds, int port)
>   	/* Port Association Vector: when learning source addresses
>   	 * of packets, add the address to the address database using
>   	 * a port bitmap that has only the bit for this port set and
> -	 * the other bits clear.
> +	 * the other bits clear, except for the CPU port.
>   	 */
> -	ret = _mv88e6xxx_reg_write(ds, REG_PORT(port), PORT_ASSOC_VECTOR,
> -				   1 << port);
> +	reg = BIT(port);
> +	if (dsa_is_cpu_port(ds, port))
> +		reg |= PORT_ASSOC_VECTOR_LOCKED_PORT;
> +	ret = _mv88e6xxx_reg_write(ds, REG_PORT(port), PORT_ASSOC_VECTOR, reg);
>   	if (ret)
>   		goto abort;
>
> diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
> index 412d14e..e26eb0c 100644
> --- a/drivers/net/dsa/mv88e6xxx.h
> +++ b/drivers/net/dsa/mv88e6xxx.h
> @@ -144,6 +144,7 @@
>   #define PORT_RATE_CONTROL	0x09
>   #define PORT_RATE_CONTROL_2	0x0a
>   #define PORT_ASSOC_VECTOR	0x0b
> +#define PORT_ASSOC_VECTOR_LOCKED_PORT	BIT(13)
>   #define PORT_ATU_CONTROL	0x0c
>   #define PORT_PRI_OVERRIDE	0x0d
>   #define PORT_ETH_TYPE		0x0f
>

--
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
Vivien Didelot June 3, 2015, 1:06 a.m. UTC | #2 
Hi Guenter,

On Jun 2, 2015, at 10:24 AM, Guenter Roeck linux@roeck-us.net wrote:
On 06/01/2015 06:27 PM, Vivien Didelot wrote:
>> This commit disables SA learning and refreshing for the CPU port.
>>
> 
> Hi Vivien,
> 
> This patch also seems to be unrelated to the rest of the series.
> 
> Can you add an explanation why it is needed ?
> 
> With this in place, how does the CPU port SA find its way into the fdb ?
> Do we assume that it will be configured statically ?
> An explanation might be useful.

Without this patch, I noticed the CPU port was stealing the SA of a PC
behind a switch port. this happened when the port was a bridge member,
as the bridge was relaying broadcast coming from one switch port to the
other switch ports in the same vlan.

Thanks,
-v
--
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
Guenter Roeck June 3, 2015, 2:24 a.m. UTC | #3 
On Tue, Jun 02, 2015 at 09:06:15PM -0400, Vivien Didelot wrote:
> Hi Guenter,
> 
> On Jun 2, 2015, at 10:24 AM, Guenter Roeck linux@roeck-us.net wrote:
> On 06/01/2015 06:27 PM, Vivien Didelot wrote:
> >> This commit disables SA learning and refreshing for the CPU port.
> >>
> > 
> > Hi Vivien,
> > 
> > This patch also seems to be unrelated to the rest of the series.
> > 
> > Can you add an explanation why it is needed ?
> > 
> > With this in place, how does the CPU port SA find its way into the fdb ?
> > Do we assume that it will be configured statically ?
> > An explanation might be useful.
> 
> Without this patch, I noticed the CPU port was stealing the SA of a PC
> behind a switch port. this happened when the port was a bridge member,
> as the bridge was relaying broadcast coming from one switch port to the
> other switch ports in the same vlan.
> 
Makes me feel really uncomfortable. I think we may be going into the wrong
direction. The whole point of offloading bridging is to have the switch handle
forwarding, and that includes multicasts and broadcasts. Instead of doing that,
it looks like we put more and more workarounds in place.

Maybe the software bridge code needs to understand that it isn't support to
forward broadcasts to ports of an offloaded bridge, and we should let the
switch chip handle it ?

Thanks,
Guenter
--
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
Guenter Roeck June 3, 2015, 4:17 a.m. UTC | #4 
On 06/02/2015 07:31 PM, Chris Healy wrote:
> Guenter,
>
> That's a very valid concern.  I have a configuration with a 6352 controlled by a low end ARM core with a 100mbps connection on the CPU port.  This switch needs to support passing multicast streams that are more than 100mbps on GigE links.  (The ARM does not need to consume the multicast, it just manages the switch.)
>

Possibly, but Vivien didn't answer my question (how the local SA address finds
its way into the switch fdb). I'll check it myself.

Thanks,
Guenter

> On Jun 3, 2015 3:24 AM, "Guenter Roeck" <linux@roeck-us.net <mailto:linux@roeck-us.net>> wrote:
>
>     On Tue, Jun 02, 2015 at 09:06:15PM -0400, Vivien Didelot wrote:
>      > Hi Guenter,
>      >
>      > On Jun 2, 2015, at 10:24 AM, Guenter Roeck linux@roeck-us.net <mailto:linux@roeck-us.net> wrote:
>      > On 06/01/2015 06:27 PM, Vivien Didelot wrote:
>      > >> This commit disables SA learning and refreshing for the CPU port.
>      > >>
>      > >
>      > > Hi Vivien,
>      > >
>      > > This patch also seems to be unrelated to the rest of the series.
>      > >
>      > > Can you add an explanation why it is needed ?
>      > >
>      > > With this in place, how does the CPU port SA find its way into the fdb ?
>      > > Do we assume that it will be configured statically ?
>      > > An explanation might be useful.
>      >
>      > Without this patch, I noticed the CPU port was stealing the SA of a PC
>      > behind a switch port. this happened when the port was a bridge member,
>      > as the bridge was relaying broadcast coming from one switch port to the
>      > other switch ports in the same vlan.
>      >
>     Makes me feel really uncomfortable. I think we may be going into the wrong
>     direction. The whole point of offloading bridging is to have the switch handle
>     forwarding, and that includes multicasts and broadcasts. Instead of doing that,
>     it looks like we put more and more workarounds in place.
>
>     Maybe the software bridge code needs to understand that it isn't support to
>     forward broadcasts to ports of an offloaded bridge, and we should let the
>     switch chip handle it ?
>
>     Thanks,
>     Guenter
>

--
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
Andrew Lunn June 3, 2015, 8:51 p.m. UTC | #5 
On Tue, Jun 02, 2015 at 07:31:56PM -0700, Chris Healy wrote:
> Guenter,
> 
> That's a very valid concern.  I have a configuration with a 6352 controlled
> by a low end ARM core with a 100mbps connection on the CPU port.  This
> switch needs to support passing multicast streams that are more than
> 100mbps on GigE links.  (The ARM does not need to consume the multicast, it
> just manages the switch.)

Hi Chris

Thinking out load here...

There are two use cases:

1) Without bridging. The switch ports are seen as host interfaces.
   Host interfaces are expected to accept packets for there own MAC
   address and the broadcast address. Additional multicast addresses
   can be added and the ndo_set_rx_mode() method of the driver is
   called to get to hardware to accept these MAC addresses. DSA has an
   implementation of ndo_set_rx_mode(), but all it does is ask the
   kernel to do the filtering. We need to extend it to program the
   hardware to only pass frames which match the addresses on the
   lists.  This should be just adding some static forwarding
   entries. Then, so long as an application on the host does not join
   any of the multicast groups, the frames should never be passed to
   the host.

2) With bridging, things are a bit different. Interfaces in a bridge
   are expected to be in promiscuous mode, receiving everything and
   passing it to the bridge. With the hardware bridging support
   Guenter added, we can off load unicast forwarding to the hardware.
   However, we currently don't have full support for off-loading of
   multicast. This falls into at a few different pieces:

   a) Get the hardware to do a dumb flood to all ports in the bridge
      group. However, the host is a member of the bridge, so it will
      still get a copy of all the packets. It has to, there could be
      members of the multicast groups on interfaces not accelerated by
      the hardware.

   b) Add limited IGMP snooping, so that the host bridge knows if it
      needs to see multicast frames for a specific MAC address from
      DSA interfaces or not, and program this into the hardware to
      reduce the load on the host.

   c) Add full IGMP snooping, so that the hardware no longer performs
      dumb flooding, but only forwards out ports where there has been
      an interest in the frames.

   Until we get at least b) implemented, i would expect all multicast
   packets to hit the host. In order to be correct in the general
   case, they have to.

   Andrew
--
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/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index d2beb10..ed49bd8 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -1761,10 +1761,12 @@  static int mv88e6xxx_setup_port(struct dsa_switch *ds, int port)
 	/* Port Association Vector: when learning source addresses
 	 * of packets, add the address to the address database using
 	 * a port bitmap that has only the bit for this port set and
-	 * the other bits clear.
+	 * the other bits clear, except for the CPU port.
 	 */
-	ret = _mv88e6xxx_reg_write(ds, REG_PORT(port), PORT_ASSOC_VECTOR,
-				   1 << port);
+	reg = BIT(port);
+	if (dsa_is_cpu_port(ds, port))
+		reg |= PORT_ASSOC_VECTOR_LOCKED_PORT;
+	ret = _mv88e6xxx_reg_write(ds, REG_PORT(port), PORT_ASSOC_VECTOR, reg);
 	if (ret)
 		goto abort;
 
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index 412d14e..e26eb0c 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -144,6 +144,7 @@ 
 #define PORT_RATE_CONTROL	0x09
 #define PORT_RATE_CONTROL_2	0x0a
 #define PORT_ASSOC_VECTOR	0x0b
+#define PORT_ASSOC_VECTOR_LOCKED_PORT	BIT(13)
 #define PORT_ATU_CONTROL	0x0c
 #define PORT_PRI_OVERRIDE	0x0d
 #define PORT_ETH_TYPE		0x0f