Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 2 Jul 2022 21:56:52 +0300
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc:     Hauke Mehrtens <hauke@hauke-m.de>, netdev@vger.kernel.org,
        andrew@lunn.ch, vivien.didelot@gmail.com, davem@davemloft.net,
        edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
        linux-kernel@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH net v1] net: dsa: lantiq_gswip: Fix FDB add/remove on the
 CPU port
Message-ID: <20220702185652.dpzrxuitacqp6m3t@skbuf>
References: <20220630212703.3280485-1-martin.blumenstingl@googlemail.com>
 <20220701130157.bwepfw2oeco6teyv@skbuf>
 <CAFBinCDqgQ1WWWPmfXykeZPsiwLNu+fPg6nCN7TMNNR_JL3gxQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAFBinCDqgQ1WWWPmfXykeZPsiwLNu+fPg6nCN7TMNNR_JL3gxQ@mail.gmail.com>
Precedence: bulk

On Sat, Jul 02, 2022 at 07:43:11PM +0200, Martin Blumenstingl wrote:
> Hi Vladimir,
> 
> On Fri, Jul 1, 2022 at 3:02 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> [...]
> > > Use FID 0 (which is also the "default" FID) when adding/removing an FDB
> > > entry for the CPU port.
> >
> > What does "default" FID even mean, and why is the default FID relevant?
> The GSW140 datasheet [0] (which is for a newer IP than the one we are
> targeting currently with the GSWIP driver - but I am not aware of any
> older datasheets)

Thanks for the document! Really useful.

> page 78 mentions: "By default the FID is zero and all entries belong
> to shared VLAN learning."

Not talking about the hardware defaults when it's obvious the driver
changes those, in an attempt to comply to Linux networking expectations...

> > In any case, I recommend you to first set up a test bench where you
> > actually see a difference between packets being flooded to the CPU vs
> > matching an FDB entry targeting it. Then read up a bit what the provided
> > dsa_db argument wants from port_fdb_add(). This conversation with Alvin
> > should explain a few things.
> > https://patchwork.kernel.org/project/netdevbpf/cover/20220302191417.1288145-1-vladimir.oltean@nxp.com/#24763870
> I previously asked Hauke whether the RX tag (net/dsa/tag_gswip.c) has
> some bit to indicate whether traffic is flooded - but to his knowledge
> the switch doesn't provide this information.

Yeah, you generally won't find quite that level of detail even in more
advanced switches. Not that you need it...

> So I am not sure what I can do in this case - do you have any pointers for me?

Yes, I do.

gswip_setup has:

	/* Default unknown Broadcast/Multicast/Unicast port maps */
	gswip_switch_w(priv, BIT(cpu_port), GSWIP_PCE_PMAP1);
	gswip_switch_w(priv, BIT(cpu_port), GSWIP_PCE_PMAP2);
	gswip_switch_w(priv, BIT(cpu_port), GSWIP_PCE_PMAP3); <- replace BIT(cpu_port) with 0

If you can no longer ping, it means that flooding was how packets
reached the system.

It appears that what goes on is interesting.
The switch is configured to flood traffic that's unknown to the FDB only
to the CPU (notably not to other bridged ports).
In software, the packet reaches tag_gswip.c, where unlike the majority
of other DSA tagging protocols, we do not call dsa_default_offload_fwd_mark(skb).
Then, the packet reaches the software bridge, and the switch has
informed the bridge (via skb->offload_fwd_mark == 0) that the packet
hasn't been already flooded in hardware, so the software bridge needs to
do it (only if necessary, of course).

The software bridge floods the packet according to its own FDB. In your
case, the software bridge recognizes the MAC DA of the packet as being
equal to the MAC address of br0 itself, and so, it doesn't flood it,
just terminates it locally. This is true whether or not the switch
learned that address in its FDB on the CPU port.

> Also apologies if all of this is very obvious. So far I have only been
> working on the xMII part of Ethernet drivers, meaning: I am totally
> new to the FDB part.
> 
> > Then have a patch (set) lifting the "return -EINVAL" from gswip *properly*.
> > And only then do we get to ask the questions "how bad are things for
> > linux-5.18.y? how bad are they for linux-5.15.y? what do we need to do?".
> agreed
> 
> 
> Thanks again for your time and all these valuable hints Vladimir!
> Martin
> 
> 
> [0] https://assets.maxlinear.com/web/documents/617930_gsw140_ds_rev1.11.pdf

So if I'm right, the state of facts is quite "not broken" (quite the
other way around, I'm really impressed), although there are still
improvements to be made. Flooding could be offloaded to hardware, then
flooding to CPU could be turned off and controlled via port promiscuity.
This would save quite a few CPU cycles.