Le Wed, 29 Mar 2023 16:16:13 +0300,
Vladimir Oltean <olteanv@xxxxxxxxx> 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
