Re: [PATCH RESEND net-next v4 3/3] net: dsa: rzn1-a5psw: add vlan support

From: "Clément Léger" <clement.leger@bootlin.com>
To: Vladimir Oltean <olteanv@gmail.com>
Cc: "Andrew Lunn" <andrew@lunn.ch>,
	"Florian Fainelli" <f.fainelli@gmail.com>,
	"David S. Miller" <davem@davemloft.net>,
	"Eric Dumazet" <edumazet@google.com>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"Paolo Abeni" <pabeni@redhat.com>,
	"Thomas Petazzoni" <thomas.petazzoni@bootlin.com>,
	"Herve Codina" <herve.codina@bootlin.com>,
	"Miquèl Raynal" <miquel.raynal@bootlin.com>,
	"Milan Stevanovic" <milan.stevanovic@se.com>,
	"Jimmy Lalande" <jimmy.lalande@se.com>,
	"Pascal Eberhard" <pascal.eberhard@se.com>,
	"Arun Ramadoss" <Arun.Ramadoss@microchip.com>,
	linux-renesas-soc@vger.kernel.org, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	"Alexis Lothore" <alexis.lothore@bootlin.com>
Subject: Re: [PATCH RESEND net-next v4 3/3] net: dsa: rzn1-a5psw: add vlan support
Date: Thu, 30 Mar 2023 11:09:59 +0200	[thread overview]
Message-ID: <20230330110959.2132cd07@fixe.home> (raw)
In-Reply-To: <20230329131613.zg4whzzoa4yna7lh@skbuf>

Le Wed, 29 Mar 2023 16:16:13 +0300,
Vladimir Oltean <olteanv@gmail.com> a écrit :

> > After thinking about the current mechasnim, let me summarize why I
> > think it almost matches what you described in this last paragraph:
> > 
> > - Port is set to match a specific matching rule which will enforce port
> >   to CPU forwarding only based on the MGMTFWD bit of PATTERN_CTRL which
> >   states the following: "When set, the frame is forwarded to the
> >   management port only (suppressing destination address lookup)"
> > 
> > This means that for the "port to CPU" path when in standalone mode, we
> > are fine. Regarding the other "CPU to port" path only:
> > 
> > - Learning will be disabled when leaving the bridge. This will allow
> >   not to have any new forwarding entries in the MAC lookup table.
> > 
> > - Port is fast aged which means it won't be targeted for packet
> >   forwarding.
> > 
> > - We remove the port from the flooding mask which means it won't be
> >   flooded after being removed from the port.
> > 
> > Based on that, the port should not be the target of any forward packet
> > from the other ports. Note that anyway, even if using per-port VLAN for
> > standalone mode, we would also end up needing to disable learning,
> > fast-age the port and disable flooding (at least from my understanding
> > if we want the port to be truly isolated).
> > 
> > Tell me if it makes sense.  
> 
> This makes sense.
> 
> However, I still spotted a bug and I don't know where to mention it
> better, so I'll mention it here:
> 
> a5psw_port_vlan_add()
> 
> 	if (pvid) {
> 		a5psw_reg_rmw(a5psw, A5PSW_VLAN_IN_MODE_ENA, BIT(port),
> 			      BIT(port));
> 		a5psw_reg_writel(a5psw, A5PSW_SYSTEM_TAGINFO(port), vid);
> 	}
> 
> You don't want a5psw_port_vlan_add() to change VLAN_IN_MODE_ENA, because
> port_vlan_add() will be called even for VLAN-unaware bridges, and you
> want all traffic to be forwarded as if untagged, and not according to
> the PVID. In other words, in a setup like this:
> 
> ip link add br0 type bridge vlan_filtering 0 && ip link set br0 up
> ip link set swp0 master br0 && ip link set swp0 up
> ip link set swp1 master br0 && ip link set swp1 up
> bridge vlan del dev swp1 vid 1
> 
> forwarding should still take place with no issues, because the entire
> VLAN table is bypassed by the software bridge when vlan_filtering=0, and
> the hardware accelerator should replicate that behavior.

Ok, we'll see how to fix that.

> 
> I suspect that the PVID handling in a5psw_port_vlan_del() is also
> incorrect:
> 
> 	/* Disable PVID if the vid is matching the port one */
> 	if (vid == a5psw_reg_readl(a5psw, A5PSW_SYSTEM_TAGINFO(port)))
> 		a5psw_reg_rmw(a5psw, A5PSW_VLAN_IN_MODE_ENA, BIT(port), 0);
> 
> VLAN-aware bridge ports without a PVID should drop untagged and VID-0-tagged
> packets. However, as per your own comments:
> 
> | > What does it mean to disable PVID?
> | 
> | It means it disable the input tagging of packets with this PVID.
> | Incoming packets will not be modified and passed as-is.
> 
> so this is not what happens.

Yes indeed, and we noticed the handling of VLANVERI and VLANDISC in
vlan_filtering() should be set according to the fact there is a PVID or
not (which is not the case right now).

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com