Date:   Tue, 18 Jun 2019 11:16:23 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Benedikt Spranger <b.spranger@...utronix.de>,
        netdev@...r.kernel.org,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Kurt Kanzenbach <kurt@...utronix.de>
Subject: Re: [RFC PATCH 0/2] enable broadcom tagging for bcm531x5 switches

On 6/18/19 10:57 AM, Benedikt Spranger wrote:
> Hi,
> 
> while puting a Banana Pi R1 board into operation I faced network hickups
> and get into serious trouble from my coworkers:
> 
> Banana Pi network setup:
> PC (eth1) <--> BPi R1 (wan) / BPi R1 (lan4) <--> DUT (eth0)
> 10.0.32.1      10.0.32.2      172.16.0.1         172.16.0.2
> ---8<---
> #! /bin/bash
> 
> # create VLANs
> ip link add link eth0 name eth0.1 type vlan id 1
> ip link add link eth0 name eth0.100 type vlan id 100
> 
> # create bridges
> ip link add br0 type bridge
> ip link set dev br0 type bridge vlan_filtering 1
> 
> ip link add br1 type bridge
> ip link set dev br1 type bridge vlan_filtering 1
> 
> # add interfaces to bridges
> ip link set lan1 master br0
> ip link set lan2 master br0
> ip link set lan3 master br0
> ip link set lan4 master br0
> ip link set eth0.100 master br0
> 
> ip link set wan master br1
> 
> # adjust tagging
> bridge vlan add dev lan1 vid 100 pvid untagged
> bridge vlan add dev lan2 vid 100 pvid untagged
> bridge vlan add dev lan3 vid 100 pvid untagged
> bridge vlan add dev lan4 vid 100 pvid untagged
> 
> bridge vlan del dev lan1 vid 1
> bridge vlan del dev lan2 vid 1
> bridge vlan del dev lan3 vid 1
> bridge vlan del dev lan4 vid 1
> 
> # set IPs
> ip addr add 10.0.32.2/24 dev eth0.1
> ip addr add 172.16.0.1/24 dev br0
> 
> # up and run
> ip link set eth0 up
> ip link set eth0.1 up
> ip link set eth0.100 up
> 
> ip link set br0 up
> ip link set br1 up
> 
> ip link set wan up
> 
> ip link set lan1 up
> ip link set lan2 up
> ip link set lan3 up
> ip link set lan4 up
> ---8<---
> 
> While trying to ping a non existing host 10.0.32.111 result in
> doublication of ARP broadcasts:
> 
> On the BPi R1:
> # tshark -i br0
>     1 8.157471671 DavicomS_43:18:fc → Broadcast    ARP 60 Who has 10.0.32.111?
>  Tell 10.0.32.1
>     2 9.167563213 DavicomS_43:18:fc → Broadcast    ARP 60 Who has 10.0.32.111?
>  Tell 10.0.32.1
>     3 10.191703047 DavicomS_43:18:fc → Broadcast    ARP 60 Who has 10.0.32.111
> ? Tell 10.0.32.1
>     4 11.215905797 DavicomS_43:18:fc → Broadcast    ARP 60 Who has 10.0.32.111
> ? Tell 10.0.32.1
>     5 12.243932964 DavicomS_43:18:fc → Broadcast    ARP 60 Who has 10.0.32.111
> ? Tell 10.0.32.1
>     6 13.264033298 DavicomS_43:18:fc → Broadcast    ARP 60 Who has 10.0.32.111
> ? Tell 10.0.32.1
> 
> # tshark -i wan0
>     1 8.157421295 DavicomS_43:18:fc → Broadcast    ARP 60 Who has 10.0.32.111?
>  Tell 10.0.32.1
>     2 9.167510087 DavicomS_43:18:fc → Broadcast    ARP 60 Who has 10.0.32.111?
>  Tell 10.0.32.1
>     3 10.191649213 DavicomS_43:18:fc → Broadcast    ARP 60 Who has 10.0.32.111
> ? Tell 10.0.32.1
>     4 11.215856838 DavicomS_43:18:fc → Broadcast    ARP 60 Who has 10.0.32.111
> ? Tell 10.0.32.1
>     5 12.243881922 DavicomS_43:18:fc → Broadcast    ARP 60 Who has 10.0.32.111
> ? Tell 10.0.32.1
>     6 13.263981506 DavicomS_43:18:fc → Broadcast    ARP 60 Who has 10.0.32.111
> ? Tell 10.0.32.1
> 
> On the PC:
> # tshark -i eth1
>   116 272.660717059 DavicomS_43:18:fc → Broadcast    ARP 42 Who has 10.0.32.111? Tell 10.0.32.1
>   117 272.661062292 DavicomS_43:18:fc → Broadcast    ARP 60 Who has 10.0.32.111? Tell 10.0.32.1
>   118 273.670690338 DavicomS_43:18:fc → Broadcast    ARP 42 Who has 10.0.32.111? Tell 10.0.32.1
>   119 273.671058605 DavicomS_43:18:fc → Broadcast    ARP 60 Who has 10.0.32.111? Tell 10.0.32.1
>   120 274.694720511 DavicomS_43:18:fc → Broadcast    ARP 42 Who has 10.0.32.111? Tell 10.0.32.1
>   121 274.695250723 DavicomS_43:18:fc → Broadcast    ARP 60 Who has 10.0.32.111? Tell 10.0.32.1
>   122 275.718813538 DavicomS_43:18:fc → Broadcast    ARP 42 Who has 10.0.32.111? Tell 10.0.32.1
>   123 275.719285852 DavicomS_43:18:fc → Broadcast    ARP 60 Who has 10.0.32.111? Tell 10.0.32.1
>   124 276.746729795 DavicomS_43:18:fc → Broadcast    ARP 42 Who has 10.0.32.111? Tell 10.0.32.1
>   125 276.747236341 DavicomS_43:18:fc → Broadcast    ARP 60 Who has 10.0.32.111? Tell 10.0.32.1
>   126 277.766722429 DavicomS_43:18:fc → Broadcast    ARP 42 Who has 10.0.32.111? Tell 10.0.32.1
>   127 277.767233098 DavicomS_43:18:fc → Broadcast    ARP 60 Who has 10.0.32.111? Tell 10.0.32.1
> 
> Choosing between network disturbance by mirrored broadcast packages (and angry
> coworkers) or lack of multicast, I choose the later. 

How is that a problem for other machines? Does that lead to some kind of
broadcast storm because there are machines that keep trying to respond
to ARP solicitations?

The few aspects that bother me, not in any particular order, are that:

- you might be able to not change anything and just get away with the
one line patch below that sets skb->offload_fwd_mark to 1 to indicate to
the bridge, not to bother with sending a copy of the packet, since the
HW took care of that already

- the patch from me that you included was part of a larger series that
also addressed multicast while in management mode, such we would not
have to chose like you did, have you tested it, did it somehow not work?

- you have not copied the DSA maintainers on all of the patches, so you
get a C grade for your patch submission

diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
index 9c3114179690..9169b63a89e3 100644
--- a/net/dsa/tag_brcm.c
+++ b/net/dsa/tag_brcm.c
@@ -140,6 +140,8 @@ static struct sk_buff *brcm_tag_rcv_ll(struct
sk_buff *skb,
        /* Remove Broadcom tag and update checksum */
        skb_pull_rcsum(skb, BRCM_TAG_LEN);

+       skb->offload_fwd_mark = 1;
+
        return skb;
 }
 #endif


-- 
Florian

Powered by blists - more mailing lists