| Message ID | 20220705173114.2004386-4-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Delete ds->configure_vlan_while_not_filtering | expand |
Hi Vladimir, On Tue, 2022-07-05 at 20:31 +0300, Vladimir Oltean wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > Stop protecting DSA drivers from switchdev VLAN notifications emitted > while the bridge has vlan_filtering 0, by deleting the deprecated > bool > ds->configure_vlan_while_not_filtering opt-in. Now all DSA drivers > see > all notifications and should save the bridge PVID until they need to > commit it to hardware. > > The 2 remaining unconverted drivers are the gswip and the Microchip > KSZ > family. They are both probably broken and would need changing as far > as > I can see: > > - For lantiq_gswip, after the initial call path > -> gswip_port_bridge_join > -> gswip_vlan_add_unaware > -> gswip_switch_w(priv, 0, GSWIP_PCE_DEFPVID(port)); > nobody seems to prevent a future call path > -> gswip_port_vlan_add > -> gswip_vlan_add_aware > -> gswip_switch_w(priv, idx, GSWIP_PCE_DEFPVID(port)); > > - For ksz9477, REG_PORT_DEFAULT_VID seems to be only modified by the > bridge VLAN add/del handlers, so there is no logic to update it on > VLAN awareness change. I don't know exactly how the switch behaves > after ksz9477_port_vlan_filtering() is called with "false". > If packets are classified to the REG_PORT_DEFAULT_VID, then it is > broken. Similar thing can be said for KSZ8. When ksz9477_port_vlan_filtering is set to false, then it clears the 802.1Q vlan enable bit in the switch. So the packets are not classified based on vlan tag. Only if the vlan bit is set, packets are classified based on pvid. > > In any case, see commit 8b6836d82470 ("net: dsa: mv88e6xxx: keep the > pvid at 0 when VLAN-unaware") for an example of how to deal with the > problem, and test pvid_change() in > tools/testing/selftests/drivers/net/dsa/bridge_vlan_unaware.sh for > how > to check whether the driver behaves correctly. I don't have the > hardware > to test any changes. I can test it and report the observation. But I haven't run the selftests before. I looked in the scripts today, but couldn't find out how to compile it as part of kernel and program it to the target & test. I infer that, this scripts should be run on target (I have SAMA5D3 + KSZ9477) with 4 switch ports. Can you guide me on testing this. > > Cc: Arun Ramadoss <arun.ramadoss@microchip.com> > Cc: Hauke Mehrtens <hauke@hauke-m.de> > Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- > drivers/net/dsa/lantiq_gswip.c | 2 -- > drivers/net/dsa/microchip/ksz_common.c | 2 -- > include/net/dsa.h | 7 ------- > net/dsa/dsa2.c | 2 -- > net/dsa/dsa_priv.h | 1 - > net/dsa/port.c | 14 -------------- > net/dsa/slave.c | 22 +--------------------- > 7 files changed, 1 insertion(+), 49 deletions(-) > > diff --git a/drivers/net/dsa/lantiq_gswip.c > b/drivers/net/dsa/lantiq_gswip.c > index e531b93f3cb2..86acf8a4e53c 100644 > --- a/drivers/net/dsa/lantiq_gswip.c > +++ b/drivers/net/dsa/lantiq_gswip.c > @@ -893,8 +893,6 @@ static int gswip_setup(struct dsa_switch *ds) > > gswip_port_enable(ds, cpu_port, NULL); > > - ds->configure_vlan_while_not_filtering = false; > - > return 0; > } > > diff --git a/drivers/net/dsa/microchip/ksz_common.c > b/drivers/net/dsa/microchip/ksz_common.c > index 28d7cb2ce98f..561a515c7cb8 100644 > --- a/drivers/net/dsa/microchip/ksz_common.c > +++ b/drivers/net/dsa/microchip/ksz_common.c > @@ -927,8 +927,6 @@ static int ksz_setup(struct dsa_switch *ds) > > ksz_init_mib_timer(dev); > > - ds->configure_vlan_while_not_filtering = false; > - > if (dev->dev_ops->setup) { > ret = dev->dev_ops->setup(ds); > if (ret) > diff --git a/include/net/dsa.h b/include/net/dsa.h > index b902b31bebce..2ed1c2db4037 100644 > --- a/include/net/dsa.h > +++ b/include/net/dsa.h > @@ -400,13 +400,6 @@ struct dsa_switch { > /* Keep VLAN filtering enabled on ports not offloading any > upper */ > u32 needs_standalone_vlan_filtering:1; > > - /* Pass .port_vlan_add and .port_vlan_del to drivers even for > bridges > - * that have vlan_filtering=0. All drivers should ideally set > this (and > - * then the option would get removed), but it is unknown > whether this > - * would break things or not. > - */ > - u32 configure_vlan_while_not_filtering:1; > - > /* If the switch driver always programs the CPU port as > egress tagged > * despite the VLAN configuration indicating otherwise, then > setting > * @untag_bridge_pvid will force the DSA receive path to pop > the > diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c > index cac48a741f27..47a2e60b6080 100644 > --- a/net/dsa/dsa2.c > +++ b/net/dsa/dsa2.c > @@ -890,8 +890,6 @@ static int dsa_switch_setup(struct dsa_switch > *ds) > if (err) > goto unregister_devlink_ports; > > - ds->configure_vlan_while_not_filtering = true; > - > err = ds->ops->setup(ds); > if (err < 0) > goto unregister_notifier; > diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h > index d9722e49864b..63191db45d02 100644 > --- a/net/dsa/dsa_priv.h > +++ b/net/dsa/dsa_priv.h > @@ -224,7 +224,6 @@ void dsa_port_pre_lag_leave(struct dsa_port *dp, > struct net_device *lag_dev); > void dsa_port_lag_leave(struct dsa_port *dp, struct net_device > *lag_dev); > int dsa_port_vlan_filtering(struct dsa_port *dp, bool > vlan_filtering, > struct netlink_ext_ack *extack); > -bool dsa_port_skip_vlan_configuration(struct dsa_port *dp); > int dsa_port_ageing_time(struct dsa_port *dp, clock_t ageing_clock); > int dsa_port_mst_enable(struct dsa_port *dp, bool on, > struct netlink_ext_ack *extack); > diff --git a/net/dsa/port.c b/net/dsa/port.c > index 3738f2d40a0b..79853f673b65 100644 > --- a/net/dsa/port.c > +++ b/net/dsa/port.c > @@ -834,20 +834,6 @@ int dsa_port_vlan_filtering(struct dsa_port *dp, > bool vlan_filtering, > return err; > } > > -/* This enforces legacy behavior for switch drivers which assume > they can't > - * receive VLAN configuration when enslaved to a bridge with > vlan_filtering=0 > - */ > -bool dsa_port_skip_vlan_configuration(struct dsa_port *dp) > -{ > - struct net_device *br = dsa_port_bridge_dev_get(dp); > - struct dsa_switch *ds = dp->ds; > - > - if (!br) > - return false; > - > - return !ds->configure_vlan_while_not_filtering && > !br_vlan_enabled(br); > -} > - > int dsa_port_ageing_time(struct dsa_port *dp, clock_t ageing_clock) > { > unsigned long ageing_jiffies = > clock_t_to_jiffies(ageing_clock); > diff --git a/net/dsa/slave.c b/net/dsa/slave.c > index ad6a6663feeb..d1284f8684d8 100644 > --- a/net/dsa/slave.c > +++ b/net/dsa/slave.c > @@ -533,11 +533,6 @@ static int dsa_slave_vlan_add(struct net_device > *dev, > struct switchdev_obj_port_vlan *vlan; > int err; > > - if (dsa_port_skip_vlan_configuration(dp)) { > - NL_SET_ERR_MSG_MOD(extack, "skipping configuration of > VLAN"); > - return 0; > - } > - > vlan = SWITCHDEV_OBJ_PORT_VLAN(obj); > > /* Deny adding a bridge VLAN when there is already an 802.1Q > upper with > @@ -571,11 +566,6 @@ static int dsa_slave_host_vlan_add(struct > net_device *dev, > if (!dp->bridge) > return -EOPNOTSUPP; > > - if (dsa_port_skip_vlan_configuration(dp)) { > - NL_SET_ERR_MSG_MOD(extack, "skipping configuration of > VLAN"); > - return 0; > - } > - > vlan = *SWITCHDEV_OBJ_PORT_VLAN(obj); > > /* Even though drivers often handle CPU membership in special > ways, > @@ -642,9 +632,6 @@ static int dsa_slave_vlan_del(struct net_device > *dev, > struct dsa_port *dp = dsa_slave_to_port(dev); > struct switchdev_obj_port_vlan *vlan; > > - if (dsa_port_skip_vlan_configuration(dp)) > - return 0; > - > vlan = SWITCHDEV_OBJ_PORT_VLAN(obj); > > return dsa_port_vlan_del(dp, vlan); > @@ -660,9 +647,6 @@ static int dsa_slave_host_vlan_del(struct > net_device *dev, > if (!dp->bridge) > return -EOPNOTSUPP; > > - if (dsa_port_skip_vlan_configuration(dp)) > - return 0; > - > vlan = SWITCHDEV_OBJ_PORT_VLAN(obj); > > return dsa_port_host_vlan_del(dp, vlan); > @@ -1655,11 +1639,7 @@ static int dsa_slave_clear_vlan(struct > net_device *vdev, int vid, void *arg) > * - no VLAN (any 8021q upper is a software VLAN) > * > * - If under a vlan_filtering=0 bridge which it offload: > - * - if ds->configure_vlan_while_not_filtering = true (default): > - * - the bridge VLANs. These VLANs are committed to hardware > but inactive. > - * - else (deprecated): > - * - no VLAN. The bridge VLANs are not restored when VLAN > awareness is > - * enabled, so this behavior is broken and discouraged. > + * - the bridge VLANs. These VLANs are committed to hardware but > inactive. > * > * - If under a vlan_filtering=1 bridge which it offload: > * - the bridge VLANs > -- > 2.25.1 >
Hi Arun, On Wed, Jul 06, 2022 at 10:51:34AM +0000, Arun.Ramadoss@microchip.com wrote: > Hi Vladimir, > > On Tue, 2022-07-05 at 20:31 +0300, Vladimir Oltean wrote: > > - For ksz9477, REG_PORT_DEFAULT_VID seems to be only modified by the > > bridge VLAN add/del handlers, so there is no logic to update it on > > VLAN awareness change. I don't know exactly how the switch behaves > > after ksz9477_port_vlan_filtering() is called with "false". > > If packets are classified to the REG_PORT_DEFAULT_VID, then it is > > broken. Similar thing can be said for KSZ8. > > When ksz9477_port_vlan_filtering is set to false, then it clears the > 802.1Q vlan enable bit in the switch. So the packets are not classified > based on vlan tag. Only if the vlan bit is set, packets are classified > based on pvid. So bit 7 of the global Switch Lookup Engine Control 0 Register says: 802.1Q VLAN Enable This is the master enable for VLAN forwarding and filtering. Note that the VLAN Table must be set up before VLAN mode is enabled. 1 = VLAN mode enabled 0 = VLAN mode disabled I'm not completely clear, personally, from the chosen wording. More interesting is the definition for the Port Control 1 Register, where it says Port VLAN Membership Each bit corresponds to a device port. This feature does not utilize VLAN tags or the VLAN Table, and is unrelated to tag-based VLAN functions. Also refer to bit 1 in the Queue Management Control 0 Register. Bit 0 is for port 1 Bit 1 is for port 2, etc. 1 = Frames may be forwarded to the corresponding port 0 = Frames are blocked from being forwarded to corresponding port So you may be right, if the VLAN mode is disabled, the packet is forwarded within the "port VLAN". It would be good to check, nonetheless. > > In any case, see commit 8b6836d82470 ("net: dsa: mv88e6xxx: keep the > > pvid at 0 when VLAN-unaware") for an example of how to deal with the > > problem, and test pvid_change() in > > tools/testing/selftests/drivers/net/dsa/bridge_vlan_unaware.sh for > > how > > to check whether the driver behaves correctly. I don't have the > > hardware > > to test any changes. > > I can test it and report the observation. But I haven't run the > selftests before. I looked in the scripts today, but couldn't find out > how to compile it as part of kernel and program it to the target & > test. I infer that, this scripts should be run on target (I have > SAMA5D3 + KSZ9477) with 4 switch ports. Can you guide me on testing > this. To be honest I've no idea how to run the kselftests "correctly" either, I just rsync -avr tools/testing/selftests/ root@<board-ip>:selftests/, then install the packages I need (running the selftest usually shows what those are), then run the test itself: $ cd selftests/drivers/net/dsa/ocelot $ ./bridge_vlan_unaware.sh lan1 lan2 lan3 lan4 The topology for most of these selftests is pretty standard. There are 2 host interfaces $h1 and $h2 (the first and the last one), and 2 switch interfaces $swp1 and $swp2 (the middle 2). There is supposed to be a loopback cable between $h1 and $swp1, and a cable between $swp2 and $h2. The $h1 and $h2 can be any physical Ethernet interfaces, $swp1 and $swp2 need to be switchdev.
Hi Vladimir, On Tue, Jul 5, 2022 at 7:32 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > > Stop protecting DSA drivers from switchdev VLAN notifications emitted > while the bridge has vlan_filtering 0, by deleting the deprecated bool > ds->configure_vlan_while_not_filtering opt-in. Now all DSA drivers see > all notifications and should save the bridge PVID until they need to > commit it to hardware. > > The 2 remaining unconverted drivers are the gswip and the Microchip KSZ > family. They are both probably broken and would need changing as far as > I can see: > > - For lantiq_gswip, after the initial call path > -> gswip_port_bridge_join > -> gswip_vlan_add_unaware > -> gswip_switch_w(priv, 0, GSWIP_PCE_DEFPVID(port)); > nobody seems to prevent a future call path > -> gswip_port_vlan_add > -> gswip_vlan_add_aware > -> gswip_switch_w(priv, idx, GSWIP_PCE_DEFPVID(port)); Thanks for bringing this to my attention! I tried to reproduce this issue with the selftest script you provided (patch #1 in this series). Unfortunately not even the ping_ipv4 and ping_ipv6 tests from bridge_vlan_unaware.sh are working for me, nor are the tests from router_bridge.sh. I suspect that this is an issue with OpenWrt: I already enabled bash, jq and the full ip package, vrf support in the kernel. OpenWrt's ping command doesn't like a ping interval of 0.1s so I replaced that with an 1s interval. I will try to get the selftests to work here but I think that shouldn't block this patch. Best regards, Martin
Hi Martin, On Wed, Jul 06, 2022 at 06:33:18PM +0200, Martin Blumenstingl wrote: > Hi Vladimir, > > On Tue, Jul 5, 2022 at 7:32 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > > > > Stop protecting DSA drivers from switchdev VLAN notifications emitted > > while the bridge has vlan_filtering 0, by deleting the deprecated bool > > ds->configure_vlan_while_not_filtering opt-in. Now all DSA drivers see > > all notifications and should save the bridge PVID until they need to > > commit it to hardware. > > > > The 2 remaining unconverted drivers are the gswip and the Microchip KSZ > > family. They are both probably broken and would need changing as far as > > I can see: > > > > - For lantiq_gswip, after the initial call path > > -> gswip_port_bridge_join > > -> gswip_vlan_add_unaware > > -> gswip_switch_w(priv, 0, GSWIP_PCE_DEFPVID(port)); > > nobody seems to prevent a future call path > > -> gswip_port_vlan_add > > -> gswip_vlan_add_aware > > -> gswip_switch_w(priv, idx, GSWIP_PCE_DEFPVID(port)); > Thanks for bringing this to my attention! > > I tried to reproduce this issue with the selftest script you provided > (patch #1 in this series). > Unfortunately not even the ping_ipv4 and ping_ipv6 tests from > bridge_vlan_unaware.sh are working for me, nor are the tests from > router_bridge.sh. > I suspect that this is an issue with OpenWrt: I already enabled bash, > jq and the full ip package, vrf support in the kernel. OpenWrt's ping > command doesn't like a ping interval of 0.1s so I replaced that with > an 1s interval. > > I will try to get the selftests to work here but I think that > shouldn't block this patch. Thanks for the willingness to test! Somehow we should do something to make sure that the OpenWRT devices are able to run the selftests, because there's a large number of DSA switches intended for that segment and we should all be onboard (easily). I wonder, would it be possible to set up a debian chroot?
Hi Vladimir, On Wed, Jul 6, 2022 at 6:45 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote: [...] > Somehow we should do something to make sure that the OpenWRT devices are > able to run the selftests, because there's a large number of DSA switches > intended for that segment and we should all be onboard (easily). I could work on this on the OpenWrt side. But this would require me to get any test working at all... I am struggling with not seeing any ping responses. Instead of adding VLANs and others to the mix I started with seemingly simple commands while connecting an Ethernet cable between lan1 and lan2 and another cable between lan3 and lan4: 1) give each port a unique MAC address, which is not the default on my device under test ip link set dev lan1 address 6a:88:f1:99:e1:81 ip link set dev lan2 address 6a:88:f1:99:e1:82 ip link set dev lan3 address 6a:88:f1:99:e1:83 ip link set dev lan4 address 6a:88:f1:99:e1:84 2) set up IP addresses on LAN1 and LAN2 ip addr add 192.168.2.1/24 brd + dev lan1 ip addr add 192.168.2.2/24 brd + dev lan2 3) bring up the interfaces ip link set up dev lan1 ip link set up dev lan2 4) start tcpdump on LAN1 tcpdump -i lan1 & 5) start ping from LAN2 to LAN1 ping -I lan2 192.168.2.1 result: PING 192.168.2.1 (192.168.2.1): 56 data bytes 20:02:01.522977 ARP, Request who-has 192.168.2.1 tell 192.168.2.2, length 46 20:02:02.569234 ARP, Request who-has 192.168.2.1 tell 192.168.2.2, length 46 20:02:03.609132 ARP, Request who-has 192.168.2.1 tell 192.168.2.2, length 46 20:02:05.524200 ARP, Request who-has 192.168.2.1 tell 192.168.2.2, length 46 20:02:06.569226 ARP, Request who-has 192.168.2.1 tell 192.168.2.2, length 46 ...repeats... So LAN1 can see the ARP request from the ping on LAN2. But I am not seeing Linux trying to send a reply. I already verified that nftables doesn't have any rules active (if it was I think it would rather result in tcpdump not seeing the who-has request): # nft list tables # # grep "" /proc/sys/net/ipv4/icmp_echo_ignore_* /proc/sys/net/ipv4/icmp_echo_ignore_all:0 /proc/sys/net/ipv4/icmp_echo_ignore_broadcasts:1 # ps | grep netif 3014 root 1308 S grep netif # To make things worse: if I let OpenWrt's netifd configure LAN1 and LAN2 as single ports with above IP addresses ping works. Something is odd and I am not sure how to find out what's wrong. > I wonder, would it be possible to set up a debian chroot? I'm thinking of packaging the selftests as OpenWrt package and also providing all needed dependencies as OpenWrt packages. I think (or I hope, not sure yet) the ping interval is just a matter of a busybox config option. Best regards, Martin
On 7/6/22 18:45, Vladimir Oltean wrote: > Hi Martin, > > On Wed, Jul 06, 2022 at 06:33:18PM +0200, Martin Blumenstingl wrote: >> Hi Vladimir, >> >> On Tue, Jul 5, 2022 at 7:32 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote: >>> >>> Stop protecting DSA drivers from switchdev VLAN notifications emitted >>> while the bridge has vlan_filtering 0, by deleting the deprecated bool >>> ds->configure_vlan_while_not_filtering opt-in. Now all DSA drivers see >>> all notifications and should save the bridge PVID until they need to >>> commit it to hardware. >>> >>> The 2 remaining unconverted drivers are the gswip and the Microchip KSZ >>> family. They are both probably broken and would need changing as far as >>> I can see: >>> >>> - For lantiq_gswip, after the initial call path >>> -> gswip_port_bridge_join >>> -> gswip_vlan_add_unaware >>> -> gswip_switch_w(priv, 0, GSWIP_PCE_DEFPVID(port)); >>> nobody seems to prevent a future call path >>> -> gswip_port_vlan_add >>> -> gswip_vlan_add_aware >>> -> gswip_switch_w(priv, idx, GSWIP_PCE_DEFPVID(port)); >> Thanks for bringing this to my attention! >> >> I tried to reproduce this issue with the selftest script you provided >> (patch #1 in this series). >> Unfortunately not even the ping_ipv4 and ping_ipv6 tests from >> bridge_vlan_unaware.sh are working for me, nor are the tests from >> router_bridge.sh. >> I suspect that this is an issue with OpenWrt: I already enabled bash, >> jq and the full ip package, vrf support in the kernel. OpenWrt's ping >> command doesn't like a ping interval of 0.1s so I replaced that with >> an 1s interval. >> >> I will try to get the selftests to work here but I think that >> shouldn't block this patch. > > Thanks for the willingness to test! > > Somehow we should do something to make sure that the OpenWRT devices are > able to run the selftests, because there's a large number of DSA switches > intended for that segment and we should all be onboard (easily). > > I wonder, would it be possible to set up a debian chroot? OpenWrt takes many packages like ping from busybox, but OpenWrt can also install the full versions. Adding a package which packs the self tests from the kernel and has the needed applications as dependencies would be nice. It would be nice to have such a thing in the OpenWrt package feed then we can easily test switches. A debian chroot should be possible, but Debian supports MIPS32 BE only till Debian 10, I do not know if this recent enough. The GSWIP driver only works on SoCs with a MIPS32 BE CPU, I think this is similar for some other switches too. There are some old manuals in the Internet on how to run Debian on such systems. Hauke
Hi Martin, On Wed, Jul 06, 2022 at 09:57:40PM +0200, Martin Blumenstingl wrote: > Hi Vladimir, > > On Wed, Jul 6, 2022 at 6:45 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > [...] > > Somehow we should do something to make sure that the OpenWRT devices are > > able to run the selftests, because there's a large number of DSA switches > > intended for that segment and we should all be onboard (easily). > > I could work on this on the OpenWrt side. > But this would require me to get any test working at all... > > I am struggling with not seeing any ping responses. > Instead of adding VLANs and others to the mix I started with seemingly > simple commands while connecting an Ethernet cable between lan1 and > lan2 and another cable between lan3 and lan4: > 1) give each port a unique MAC address, which is not the default on my > device under test > ip link set dev lan1 address 6a:88:f1:99:e1:81 > ip link set dev lan2 address 6a:88:f1:99:e1:82 > ip link set dev lan3 address 6a:88:f1:99:e1:83 > ip link set dev lan4 address 6a:88:f1:99:e1:84 > > 2) set up IP addresses on LAN1 and LAN2 > ip addr add 192.168.2.1/24 brd + dev lan1 > ip addr add 192.168.2.2/24 brd + dev lan2 > > 3) bring up the interfaces > ip link set up dev lan1 > ip link set up dev lan2 > > 4) start tcpdump on LAN1 > tcpdump -i lan1 & > > 5) start ping from LAN2 to LAN1 > ping -I lan2 192.168.2.1 > > result: > PING 192.168.2.1 (192.168.2.1): 56 data bytes > 20:02:01.522977 ARP, Request who-has 192.168.2.1 tell 192.168.2.2, length 46 > 20:02:02.569234 ARP, Request who-has 192.168.2.1 tell 192.168.2.2, length 46 > 20:02:03.609132 ARP, Request who-has 192.168.2.1 tell 192.168.2.2, length 46 > 20:02:05.524200 ARP, Request who-has 192.168.2.1 tell 192.168.2.2, length 46 > 20:02:06.569226 ARP, Request who-has 192.168.2.1 tell 192.168.2.2, length 46 > ...repeats... > > So LAN1 can see the ARP request from the ping on LAN2. > But I am not seeing Linux trying to send a reply. It won't reply, you either need a network namespace or a VRF to do loopback IP networking. A VRF is a bit more complicated to do, here's a netns setup: ip netns add ns0 ip link set lan2 netns ns0 ip -n ns0 link set lan2 up ip -n ns0 addr add 192.168.2.2/24 dev lan2 ip netns exec ns0 tcpdump -i lan2 -e -n ping 192.168.2.2 > I already verified that nftables doesn't have any rules active (if it > was I think it would rather result in tcpdump not seeing the who-has > request): > # nft list tables > # > > # grep "" /proc/sys/net/ipv4/icmp_echo_ignore_* > /proc/sys/net/ipv4/icmp_echo_ignore_all:0 > /proc/sys/net/ipv4/icmp_echo_ignore_broadcasts:1 > > # ps | grep netif > 3014 root 1308 S grep netif > # > > To make things worse: if I let OpenWrt's netifd configure LAN1 and > LAN2 as single ports with above IP addresses ping works. > Something is odd and I am not sure how to find out what's wrong. I'm not familiar with OpenWrt, sorry, I don't know what netifd does. Also, it's curious that this works, are you sure that the ARP responses and ICMP replies actually exit through the Ethernet port? ethtool -S should show if the physical counters increment. > > I wonder, would it be possible to set up a debian chroot? > > I'm thinking of packaging the selftests as OpenWrt package and also > providing all needed dependencies as OpenWrt packages. > I think (or I hope, not sure yet) the ping interval is just a matter > of a busybox config option. I think it depends on busybox version. At least the latest one https://github.com/mirror/busybox/blob/master/networking/ping.c#L970 seems to support fractions of a second as intervals, I didn't see any restriction to sub-second values. In any case, the iputils version certainly does work.
Hi Hauke, On Wed, Jul 06, 2022 at 10:04:19PM +0200, Hauke Mehrtens wrote: > OpenWrt takes many packages like ping from busybox, but OpenWrt can also > install the full versions. Adding a package which packs the self tests from > the kernel and has the needed applications as dependencies would be nice. It > would be nice to have such a thing in the OpenWrt package feed then we can > easily test switches. I didn't want to insist on that because I know that OpenWrt devices are generally strapped for storage. Also, I never managed to build an OpenWrt by myself, plus my only OpenWrt device is helping me send this email, so I can't be of very much help on that front, I'm afraid. But I'm happy to see Martin take the initiative, and the encouragement from you. > A debian chroot should be possible, but Debian supports MIPS32 BE only till > Debian 10, I do not know if this recent enough. The GSWIP driver only works > on SoCs with a MIPS32 BE CPU, I think this is similar for some other > switches too. There are some old manuals in the Internet on how to run > Debian on such systems. I was definitely suggesting this as the easy way out, as you can put Debian on an ext4 formatted USB stick. Debian 10 should definitely be enough, as these selftests don't use the latest and greatest features added to iproute2 IIRC, but it's concerning on the other hand that maintainership for the debian-mips port was dropped. Are you closely involved with the topic, do you know what's blocking the upgrade?
Hi Vladimir, On Fri, Jul 8, 2022 at 12:31 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote: [...] > > So LAN1 can see the ARP request from the ping on LAN2. > > But I am not seeing Linux trying to send a reply. > > It won't reply, you either need a network namespace or a VRF to do > loopback IP networking. A VRF is a bit more complicated to do, here's a > netns setup: > > ip netns add ns0 > ip link set lan2 netns ns0 > ip -n ns0 link set lan2 up > ip -n ns0 addr add 192.168.2.2/24 dev lan2 > ip netns exec ns0 tcpdump -i lan2 -e -n > ping 192.168.2.2 This does indeed work! That made me look at another selftest and indeed: most of the local_termination.sh tests are passing (albeit after having to make some changes to the selftest scripts, I'll provide patches for these soon) None (zero) of the tests from bridge_vlan_unaware.sh and only a single test from bridge_vlan_aware.sh ("Externally learned FDB entry - ageing & roaming") are passing for me on GSWIP. Also most of the ethtool.sh tests are failing (ping always reports "Destination Host Unreachable"). I guess most (or at least more) of these are supposed to pass? Do you want me to open another thread for this or is it fine to reply here? [...] > I'm not familiar with OpenWrt, sorry, I don't know what netifd does. netifd is the network configuration daemon, it takes the network configuration (from OpenWrts configuration files/format) and sets up the corresponding interfaces and manages things like pppoe. > Also, it's curious that this works, are you sure that the ARP responses > and ICMP replies actually exit through the Ethernet port? ethtool -S > should show if the physical counters increment. Since it works with your example and I got the first selftests to pass I'll skip further investigation here > > > I wonder, would it be possible to set up a debian chroot? > > > > I'm thinking of packaging the selftests as OpenWrt package and also > > providing all needed dependencies as OpenWrt packages. > > I think (or I hope, not sure yet) the ping interval is just a matter > > of a busybox config option. > > I think it depends on busybox version. At least the latest one > https://github.com/mirror/busybox/blob/master/networking/ping.c#L970 > seems to support fractions of a second as intervals, I didn't see any > restriction to sub-second values. In any case, the iputils version > certainly does work. Yes, there's a duration library inside busybox which by default only takes integer values (it can be configured to use floats though). I pushed my work in progress OpenWrt package to a branch, making use of the iputils version: [0] Compressed initramfs size is below 10M and uncompressed at 22M. My device under test has 128M of RAM and that seems to be enough to run mausezahn as well as any other tool that was run so far. So I am not particularly concerned about storage size (anything with 32M flash or more will do - 16M could be a bit tight in the end but will still work I guess). Best regards, Martin [0] https://github.com/xdarklight/openwrt-packages/commits/wip-kernel-selftests-net-forwarding-20220707
On Fri, Jul 08, 2022 at 12:00:33PM +0200, Martin Blumenstingl wrote: > That made me look at another selftest and indeed: most of the > local_termination.sh tests are passing (albeit after having to make > some changes to the selftest scripts, I'll provide patches for these > soon) > > None (zero) of the tests from bridge_vlan_unaware.sh and only a single > test from bridge_vlan_aware.sh ("Externally learned FDB entry - ageing > & roaming") are passing for me on GSWIP. > Also most of the ethtool.sh tests are failing (ping always reports > "Destination Host Unreachable"). I don't recall having run ethtool.sh, so I don't know what's the status there. > I guess most (or at least more) of these are supposed to pass? Do you > want me to open another thread for this or is it fine to reply here? So yes, the intention is for the selftests to pass, but I wouldn't be surprised if they don't. They didn't when I started this effort for the ocelot/felix DSA driver either, it's most likely that individual drivers will need changes, that's kind of the whole point of having selftests, to have implementations that are uniform in terms of behavior. For the ocelot driver, the tests symlinked in the DSA folder do pass (with the exception of the locked port test, which isn't implemented, and the bridge local_termination.sh tests, but that's a bridge problem and not a driver problem). I should have a remote setup and I should be able to repeat some tests if necessary. I think it would be a good idea to create a new email thread with the relevant DSA maintainers for selftest status on GSWIP. You'll need to gather some information on what exactly fails when things fail. The way I prefer to do this is to run the test itself with "bash -x ./bridge_vlan_unaware.sh swp0 swp1 swp2 swp3", analyze a bit where things get stuck, then edit the script, put a "bash" command after the failing portion, and run the selftest again; this gives me a subshell with all the VRFs configured from which I have more control and can re-run the commands that just failed (I copy them from right above, they're visible when run with bash -x). Then I try to manually check counters, tcpdump, things like that. > > I'm not familiar with OpenWrt, sorry, I don't know what netifd does. > netifd is the network configuration daemon, it takes the network > configuration (from OpenWrts configuration files/format) and sets up > the corresponding interfaces and manages things like pppoe. Thanks for the summary, I meant that I don't know what netifd does to make loopback IP networking work in this case. Anyway it's not the center point of the topic, as long as the interfaces used by the kselftests are not managed by netifd or another network management program. > > > I'm thinking of packaging the selftests as OpenWrt package and also > > > providing all needed dependencies as OpenWrt packages. > > > I think (or I hope, not sure yet) the ping interval is just a matter > > > of a busybox config option. > > > > I think it depends on busybox version. At least the latest one > > https://github.com/mirror/busybox/blob/master/networking/ping.c#L970 > > seems to support fractions of a second as intervals, I didn't see any > > restriction to sub-second values. In any case, the iputils version > > certainly does work. > Yes, there's a duration library inside busybox which by default only > takes integer values (it can be configured to use floats though). > I pushed my work in progress OpenWrt package to a branch, making use > of the iputils version: [0] > Compressed initramfs size is below 10M and uncompressed at 22M. My > device under test has 128M of RAM and that seems to be enough to run > mausezahn as well as any other tool that was run so far. So I am not > particularly concerned about storage size (anything with 32M flash or > more will do - 16M could be a bit tight in the end but will still work > I guess). > > > Best regards, > Martin > > > [0] https://github.com/xdarklight/openwrt-packages/commits/wip-kernel-selftests-net-forwarding-20220707 Sounds good then.
Hi Vladimir, On Fri, Jul 8, 2022 at 2:09 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > > On Fri, Jul 08, 2022 at 12:00:33PM +0200, Martin Blumenstingl wrote: > > That made me look at another selftest and indeed: most of the > > local_termination.sh tests are passing (albeit after having to make > > some changes to the selftest scripts, I'll provide patches for these > > soon) > > > > None (zero) of the tests from bridge_vlan_unaware.sh and only a single > > test from bridge_vlan_aware.sh ("Externally learned FDB entry - ageing > > & roaming") are passing for me on GSWIP. > > Also most of the ethtool.sh tests are failing (ping always reports > > "Destination Host Unreachable"). > > I don't recall having run ethtool.sh, so I don't know what's the status > there. OK, no worries there > > I guess most (or at least more) of these are supposed to pass? Do you > > want me to open another thread for this or is it fine to reply here? > > So yes, the intention is for the selftests to pass, but I wouldn't be > surprised if they don't. They didn't when I started this effort for the > ocelot/felix DSA driver either, it's most likely that individual drivers > will need changes, that's kind of the whole point of having selftests, > to have implementations that are uniform in terms of behavior. > For the ocelot driver, the tests symlinked in the DSA folder do pass > (with the exception of the locked port test, which isn't implemented, > and the bridge local_termination.sh tests, but that's a bridge problem > and not a driver problem). I should have a remote setup and I should be > able to repeat some tests if necessary. > > I think it would be a good idea to create a new email thread with the > relevant DSA maintainers for selftest status on GSWIP. You'll need to > gather some information on what exactly fails when things fail. > The way I prefer to do this is to run the test itself with "bash -x > ./bridge_vlan_unaware.sh swp0 swp1 swp2 swp3", analyze a bit where > things get stuck, then edit the script, put a "bash" command after the > failing portion, and run the selftest again; this gives me a subshell > with all the VRFs configured from which I have more control and can > re-run the commands that just failed (I copy them from right above, > they're visible when run with bash -x). Then I try to manually check > counters, tcpdump, things like that. I already found "bash -x" and used a similar trick (launching tcpdump before the failing portion). But it's good to have it written down! Thanks a lot again for all your detailed explanations and the time you've taken to help me out! I'll start a new thread on this in the next few days. Best regards, Martin
Hi Vladimir, We couldn't able to setup the selftests and failed during installation of packages. In the mean time, We tried the following things Setup - Host1 --> lan1 --> lan2 --> Host2. Packet transmitted from Host1 and received by Host2. Scenario-1: Vlan aware system and both lan1 & lan2 are in same vid ip link set dev br0 type bridge vlan_filtering 1 bridge vlan add dev lan2 vid 10 pvid untagged bridge vlan add dev lan1 vid 10 pvid untagged Packet transmitted from Host1 with vid 10 is received by the Host2. Packet transmitted from Host1 with vid 5 is not received by the Host2. Scenario-2: Vlan unaware system ip link set dev br0 type bridge vlan_filtering 0 Now, irrespective of the vid, the packets are received by Host2 Packet transmitted from Host1 with vid 10 is received by the Host2. Packet transmitted from Host1 with vid 5 is received by the Host2. Whether the above approach is correct or do we need to test anything further. Thanks Arun On Sat, 2022-07-09 at 00:27 +0200, Martin Blumenstingl wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > Hi Vladimir, > > On Fri, Jul 8, 2022 at 2:09 PM Vladimir Oltean < > vladimir.oltean@nxp.com> wrote: > > > > On Fri, Jul 08, 2022 at 12:00:33PM +0200, Martin Blumenstingl > > wrote: > > > That made me look at another selftest and indeed: most of the > > > local_termination.sh tests are passing (albeit after having to > > > make > > > some changes to the selftest scripts, I'll provide patches for > > > these > > > soon) > > > > > > None (zero) of the tests from bridge_vlan_unaware.sh and only a > > > single > > > test from bridge_vlan_aware.sh ("Externally learned FDB entry - > > > ageing > > > & roaming") are passing for me on GSWIP. > > > Also most of the ethtool.sh tests are failing (ping always > > > reports > > > "Destination Host Unreachable"). > > > > I don't recall having run ethtool.sh, so I don't know what's the > > status > > there. > > OK, no worries there > > > > I guess most (or at least more) of these are supposed to pass? Do > > > you > > > want me to open another thread for this or is it fine to reply > > > here? > > > > So yes, the intention is for the selftests to pass, but I wouldn't > > be > > surprised if they don't. They didn't when I started this effort for > > the > > ocelot/felix DSA driver either, it's most likely that individual > > drivers > > will need changes, that's kind of the whole point of having > > selftests, > > to have implementations that are uniform in terms of behavior. > > For the ocelot driver, the tests symlinked in the DSA folder do > > pass > > (with the exception of the locked port test, which isn't > > implemented, > > and the bridge local_termination.sh tests, but that's a bridge > > problem > > and not a driver problem). I should have a remote setup and I > > should be > > able to repeat some tests if necessary. > > > > I think it would be a good idea to create a new email thread with > > the > > relevant DSA maintainers for selftest status on GSWIP. You'll need > > to > > gather some information on what exactly fails when things fail. > > The way I prefer to do this is to run the test itself with "bash -x > > ./bridge_vlan_unaware.sh swp0 swp1 swp2 swp3", analyze a bit where > > things get stuck, then edit the script, put a "bash" command after > > the > > failing portion, and run the selftest again; this gives me a > > subshell > > with all the VRFs configured from which I have more control and can > > re-run the commands that just failed (I copy them from right above, > > they're visible when run with bash -x). Then I try to manually > > check > > counters, tcpdump, things like that. > > I already found "bash -x" and used a similar trick (launching tcpdump > before the failing portion). But it's good to have it written down! > Thanks a lot again for all your detailed explanations and the time > you've taken to help me out! > I'll start a new thread on this in the next few days. > > > Best regards, > Martin
Hi Arun, On Thu, Jul 14, 2022 at 10:46:02AM +0000, Arun.Ramadoss@microchip.com wrote: > Hi Vladimir, > We couldn't able to setup the selftests and failed during installation > of packages. In the mean time, We tried the following things > > Setup - Host1 --> lan1 --> lan2 --> Host2. Packet transmitted from > Host1 and received by Host2. > > Scenario-1: Vlan aware system and both lan1 & lan2 are in same vid > ip link set dev br0 type bridge vlan_filtering 1 > bridge vlan add dev lan2 vid 10 pvid untagged > bridge vlan add dev lan1 vid 10 pvid untagged > > Packet transmitted from Host1 with vid 10 is received by the Host2. > Packet transmitted from Host1 with vid 5 is not received by the Host2. > > Scenario-2: Vlan unaware system > ip link set dev br0 type bridge vlan_filtering 0 > > Now, irrespective of the vid, the packets are received by Host2 > Packet transmitted from Host1 with vid 10 is received by the Host2. > Packet transmitted from Host1 with vid 5 is received by the Host2. > > Whether the above approach is correct or do we need to test anything > further. > > Thanks > Arun The above is correct to the extent that it is a valid configuration, but isn't what my pvid_change() selftest was intended to capture. The pvid_change() selftest from patch 1/3 https://patchwork.kernel.org/project/netdevbpf/patch/20220705173114.2004386-2-vladimir.oltean@nxp.com/ checks that VLAN-unaware forwarding still takes place after this array of operations: ip link add br0 type bridge vlan_filtering 0 # notice the 0 instead of 1 ip link set $swp1 master br0 ip link set $swp2 master br0 bridge vlan add vid 3 dev $swp1 pvid untagged # notice how VID 3 is absent on $swp2 If you let me know if this works, I can continue and resend this patch set while you figure out the kselftest setup issues.
Hi Vladimir, On Thu, 2022-07-14 at 15:12 +0000, Vladimir Oltean wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > Hi Arun, > > On Thu, Jul 14, 2022 at 10:46:02AM +0000, Arun.Ramadoss@microchip.com > wrote: > > Hi Vladimir, > > We couldn't able to setup the selftests and failed during > > installation > > of packages. In the mean time, We tried the following things > > > > Setup - Host1 --> lan1 --> lan2 --> Host2. Packet transmitted from > > Host1 and received by Host2. > > > > Scenario-1: Vlan aware system and both lan1 & lan2 are in same vid > > ip link set dev br0 type bridge vlan_filtering 1 > > bridge vlan add dev lan2 vid 10 pvid untagged > > bridge vlan add dev lan1 vid 10 pvid untagged > > > > Packet transmitted from Host1 with vid 10 is received by the Host2. > > Packet transmitted from Host1 with vid 5 is not received by the > > Host2. > > > > Scenario-2: Vlan unaware system > > ip link set dev br0 type bridge vlan_filtering 0 > > > > Now, irrespective of the vid, the packets are received by Host2 > > Packet transmitted from Host1 with vid 10 is received by the Host2. > > Packet transmitted from Host1 with vid 5 is received by the Host2. > > > > Whether the above approach is correct or do we need to test > > anything > > further. > > > > Thanks > > Arun > > The above is correct to the extent that it is a valid configuration, > but isn't what my pvid_change() selftest was intended to capture. > > The pvid_change() selftest from patch 1/3 > https://patchwork.kernel.org/project/netdevbpf/patch/20220705173114.2004386-2-vladimir.oltean@nxp.com/ > checks that VLAN-unaware forwarding still takes place after this > array > of operations: > > ip link add br0 type bridge vlan_filtering 0 # notice the 0 instead > of 1 > ip link set $swp1 master br0 > ip link set $swp2 master br0 > bridge vlan add vid 3 dev $swp1 pvid untagged # notice how VID 3 is > absent on $swp2 > > If you let me know if this works, I can continue and resend this > patch > set while you figure out the kselftest setup issues. We tried the following test ip link set dev br0 type bridge vlan_filtering 0 ip link set lan1 master br0 ip link set lan2 master br0 bridge vlan add vid 10 dev lan1 pvid untagged ==> Packet transmitted from Host1 with vid 5 is not received by the Host2 Packet transmitted from Host1 with vid 10 is not received by the Host2 ==> bridge vlan add vid 10 dev lan2 pvid untagged ==> Packet transmitted from Host1 with vid 5 is received by the Host2 Pa cket transmitted from Host1 with vid 10 is received by the Host2 ==> bridge vlan del vid 10 dev lan2 ==> Packet transmitted from Host1 with vid 5 is not received by the Host2 Packet transmitted from Host1 with vid 10 is not received by the Host2 ==> Tried this test before and after applying this patch series. And got the same result. In summary, packets are dropped when pvid is added to vlan unaware bridge. Let me know if anything need to performed on this. Thanks Arun
On Fri, Jul 15, 2022 at 09:23:19AM +0000, Arun.Ramadoss@microchip.com wrote: > Hi Vladimir, > > We tried the following test > > ip link set dev br0 type bridge vlan_filtering 0 > > ip link set lan1 master br0 > ip link set lan2 master br0 > > bridge vlan add vid 10 dev lan1 pvid untagged > > ==> > Packet transmitted from Host1 with vid 5 is not received by the Host2 > Packet transmitted from Host1 with vid 10 is not received by the Host2 > ==> > > bridge vlan add vid 10 dev lan2 pvid untagged > > ==> > Packet transmitted from Host1 with vid 5 is received by the Host2 > Pa > cket transmitted from Host1 with vid 10 is received by the Host2 > ==> > > bridge vlan del vid 10 dev lan2 > > ==> > Packet transmitted from Host1 with vid 5 is not received by the Host2 > Packet transmitted from Host1 with vid 10 is not received by the Host2 > ==> > > Tried this test before and after applying this patch series. And got > the same result. > > In summary, packets are dropped when pvid is added to vlan unaware > bridge. Let me know if anything need to performed on this. I'm not surprised that forwarding is broken after removing "ds->configure_vlan_while_not_filtering = false", but I'm surprised that it's broken even without the change. That suggests that either the flag wasn't effective in the first place, or that the breakage is caused by other code paths (not sure which). Do you get the "skipping configuration of VLAN" warning extack when you run the "bridge vlan add" command without the patches here? Does ksz_port_vlan_add() get called at all with VID 10?
Hi Vladimir, On Fri, 2022-07-15 at 15:26 +0000, Vladimir Oltean wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Fri, Jul 15, 2022 at 09:23:19AM +0000, Arun.Ramadoss@microchip.com > wrote: > > Hi Vladimir, > > > > We tried the following test > > > > ip link set dev br0 type bridge vlan_filtering 0 > > > > ip link set lan1 master br0 > > ip link set lan2 master br0 > > > > bridge vlan add vid 10 dev lan1 pvid untagged > > > > ==> > > Packet transmitted from Host1 with vid 5 is not received by the > > Host2 > > Packet transmitted from Host1 with vid 10 is not received by the > > Host2 > > ==> > > > > bridge vlan add vid 10 dev lan2 pvid untagged > > > > ==> > > Packet transmitted from Host1 with vid 5 is received by the Host2 > > Pa > > cket transmitted from Host1 with vid 10 is received by the Host2 > > ==> > > > > bridge vlan del vid 10 dev lan2 > > > > ==> > > Packet transmitted from Host1 with vid 5 is not received by the > > Host2 > > Packet transmitted from Host1 with vid 10 is not received by the > > Host2 > > ==> > > > > Tried this test before and after applying this patch series. And > > got > > the same result. > > > > In summary, packets are dropped when pvid is added to vlan unaware > > bridge. Let me know if anything need to performed on this. > > I'm not surprised that forwarding is broken after removing > "ds->configure_vlan_while_not_filtering = false", but I'm surprised > that > it's broken even without the change. That suggests that either the > flag > wasn't effective in the first place, or that the breakage is caused > by > other code paths (not sure which). > > Do you get the "skipping configuration of VLAN" warning extack when > you > run the "bridge vlan add" command without the patches here? Does > ksz_port_vlan_add() get called at all with VID 10? There was a mistake in our testing on the latest code base of net-next. Today we tried in the latest net-next and following are the observation. Scenario 1: Before applying the patch ------------------------------ ip link set dev br0 type bridge vlan_filtering 0 bridge vlan add vid 10 dev lan1 pvid untagged bridge vlan add vid 10 dev lan2 pvid untagged We got warning skipping configuration of VLAN and ksz_port_vlan_add() is not called. Packet is received in Host2 when transmitted from Host1. So there is no breakage in the forwarding. Scenario 2: After applying the patch ---------------------------- ip link set dev br0 type bridge vlan_filtering 0 bridge vlan add vid 10 dev lan1 pvid untagged --> Packet is not received in the Host2 bridge vlan add vid 10 dev lan2 pvid untagged --> packet is received in the Host2 bridge vlan del vid 10 dev lan1 --> packet is received in the Host2 bridge vlan del vid 10 dev lan2 --> packet is received in the Host2 * Let us know, do we need to test anything further on this. Thanks Arun
Hi Arun, On Mon, Jul 18, 2022 at 02:34:33PM +0000, Arun.Ramadoss@microchip.com wrote: > Hi Vladimir, > There was a mistake in our testing on the latest code base of net-next. > Today we tried in the latest net-next and following are the > observation. > > Scenario 1: Before applying the patch > ------------------------------ > ip link set dev br0 type bridge vlan_filtering 0 > > bridge vlan add vid 10 dev lan1 pvid untagged > bridge vlan add vid 10 dev lan2 pvid untagged "bridge vlan add" on lan2 was never part of the test instructions. > > We got warning skipping configuration of VLAN and ksz_port_vlan_add() > is not called. > > Packet is received in Host2 when transmitted from Host1. So there is no > breakage in the forwarding. Nonetheless it's good to know that the control scenario behaves as expected, i.e. the VLANs are skipped while VLAN-unaware (and therefore, the port hardware PVID remains what it was). > Scenario 2: After applying the patch > ---------------------------- > ip link set dev br0 type bridge vlan_filtering 0 > > bridge vlan add vid 10 dev lan1 pvid untagged > > --> Packet is not received in the Host2 The problem is exactly here. The driver should pass packets at this stage. This is because the bridge is still VLAN-unaware, despite its VLAN-aware pvid VLAN having been changed from 1 (vlan_default_pvid) to 10. > bridge vlan add vid 10 dev lan2 pvid untagged > > --> packet is received in the Host2 This only goes to prove that the VLAN in which the switch processes traffic while VLAN-unaware is the PVID of the port. So when the PVID on the ingress and egress ports matches, forwarding is naturally restored. > bridge vlan del vid 10 dev lan1 > > --> packet is received in the Host2 > > bridge vlan del vid 10 dev lan2 > > --> packet is received in the Host2 > > * Let us know, do we need to test anything further on this. Yes, now you need to go fix the driver :) Please read the comments from this patch and the series in general (including cover letter), they point to similar issues in other drivers and to commits which have solved them. I need the ksz driver to work properly before I can delete the configure_vlan_while_not_filtering workaround. Generally speaking, the PVID needs to be committed to hardware based on a smarter logic, see this for example (and all the places from which it is called): static int mv88e6xxx_port_commit_pvid(struct mv88e6xxx_chip *chip, int port) { struct dsa_port *dp = dsa_to_port(chip->ds, port); struct net_device *br = dsa_port_bridge_dev_get(dp); struct mv88e6xxx_port *p = &chip->ports[port]; u16 pvid = MV88E6XXX_VID_STANDALONE; // Dedicated PVID for standalone mode bool drop_untagged = false; int err; if (br) { if (br_vlan_enabled(br)) { pvid = p->bridge_pvid.vid; // PVID is inherited from bridge only if the bridge is *currently* VLAN-aware drop_untagged = !p->bridge_pvid.valid; } else { pvid = MV88E6XXX_VID_BRIDGED; // Dedicated PVID for VLAN-unaware bridging } } err = mv88e6xxx_port_set_pvid(chip, port, pvid); if (err) return err; return mv88e6xxx_port_drop_untagged(chip, port, drop_untagged); } Additionally, DaveM has just merged some DSA documentation updates which I think are very relevant to this discussion. See the new "Address databases" chapter for a review of how things are supposed to actually work when done carefully: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/tree/Documentation/networking/dsa/dsa.rst#n730 Thanks!
Hi Vladimir, On Mon, 2022-07-18 at 16:24 +0000, Vladimir Oltean wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > Hi Arun, > > On Mon, Jul 18, 2022 at 02:34:33PM +0000, Arun.Ramadoss@microchip.com > wrote: > > Hi Vladimir, > > There was a mistake in our testing on the latest code base of net- > > next. > > Today we tried in the latest net-next and following are the > > observation. > > > > Scenario 1: Before applying the patch > > ------------------------------ > > ip link set dev br0 type bridge vlan_filtering 0 > > > > bridge vlan add vid 10 dev lan1 pvid untagged > > bridge vlan add vid 10 dev lan2 pvid untagged > > "bridge vlan add" on lan2 was never part of the test instructions. > > > > > We got warning skipping configuration of VLAN and > > ksz_port_vlan_add() > > is not called. > > > > Packet is received in Host2 when transmitted from Host1. So there > > is no > > breakage in the forwarding. > > Nonetheless it's good to know that the control scenario behaves as > expected, i.e. the VLANs are skipped while VLAN-unaware (and > therefore, > the port hardware PVID remains what it was). > > > Scenario 2: After applying the patch > > ---------------------------- > > ip link set dev br0 type bridge vlan_filtering 0 > > > > bridge vlan add vid 10 dev lan1 pvid untagged > > > > --> Packet is not received in the Host2 > > The problem is exactly here. The driver should pass packets at this > stage. > This is because the bridge is still VLAN-unaware, despite its VLAN- > aware > pvid VLAN having been changed from 1 (vlan_default_pvid) to 10. > > > bridge vlan add vid 10 dev lan2 pvid untagged > > > > --> packet is received in the Host2 > > This only goes to prove that the VLAN in which the switch processes > traffic while VLAN-unaware is the PVID of the port. So when the PVID > on > the ingress and egress ports matches, forwarding is naturally > restored. > > > bridge vlan del vid 10 dev lan1 > > > > --> packet is received in the Host2 > > > > bridge vlan del vid 10 dev lan2 > > > > --> packet is received in the Host2 > > > > * Let us know, do we need to test anything further on this. > > Yes, now you need to go fix the driver :) Please read the comments > from > this patch and the series in general (including cover letter), they > point to similar issues in other drivers and to commits which have > solved them. I need the ksz driver to work properly before I can > delete > the configure_vlan_while_not_filtering workaround. Generally > speaking, > the PVID needs to be committed to hardware based on a smarter logic, > see > this for example (and all the places from which it is called): > > static int mv88e6xxx_port_commit_pvid(struct mv88e6xxx_chip *chip, > int port) > { > struct dsa_port *dp = dsa_to_port(chip->ds, port); > struct net_device *br = dsa_port_bridge_dev_get(dp); > struct mv88e6xxx_port *p = &chip->ports[port]; > u16 pvid = MV88E6XXX_VID_STANDALONE; // Dedicated PVID for > standalone mode > bool drop_untagged = false; > int err; > > if (br) { > if (br_vlan_enabled(br)) { > pvid = p->bridge_pvid.vid; // PVID is > inherited from bridge only if the bridge is *currently* VLAN-aware > drop_untagged = !p->bridge_pvid.valid; > } else { > pvid = MV88E6XXX_VID_BRIDGED; // Dedicated > PVID for VLAN-unaware bridging > } > } > > err = mv88e6xxx_port_set_pvid(chip, port, pvid); > if (err) > return err; > > return mv88e6xxx_port_drop_untagged(chip, port, > drop_untagged); > } > > Additionally, DaveM has just merged some DSA documentation updates > which > I think are very relevant to this discussion. See the new "Address > databases" > chapter for a review of how things are supposed to actually work when > done carefully: > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/tree/Documentation/networking/dsa/dsa.rst#n730 > > Thanks! I tried to update the ksz code and tested after applying this patch series. Following are the observation, Scenario 1: bridge_vlan_unaware default pvid = 1 --------------------------------- ip link set dev br0 type bridge vlan_filtering 0 --> packet is received in the Host 2 bridge vlan add vid 10 dev lan1 pvid untagged --> packet is received in the Host 2 bridge vlan add vid 10 dev lan2 pvid untagged --> packet is received in the Host 2 bridge vlan del vid 10 dev lan1 --> packet is received in the Host 2 bridge vlan del vid 10 dev lan2 --> packet is received in the Host2 Scenario 2: bridge_vlan_unaware default pvid other than 1 --------------------------------- ip link set dev br0 type bridge vlan_filtering 0 --> packet is not received in the Host 2. Only after enabling the vlan_filtering and bridge vlan add packets are receiving. ip link set dev br0 type bridge vlan_filtering 1 bridge vlan add vid 10 dev lan2 pvid untagged In summary, only for pvid 1 below patch is working. Initially I tried with pvid 0, 21, 4095, it were not working, only for pvid 1 it is working. Kindly suggest whether any changes to be done in patch or testing methodology. Updated code patch ------------------ diff --git a/drivers/net/dsa/microchip/ksz8.h b/drivers/net/dsa/microchip/ksz8.h index 42c50cc4d853..e8e1590a78c0 100644 --- a/drivers/net/dsa/microchip/ksz8.h +++ b/drivers/net/dsa/microchip/ksz8.h @@ -43,6 +43,7 @@ int ksz8_port_vlan_add(struct ksz_device *dev, int port, struct netlink_ext_ack *extack); int ksz8_port_vlan_del(struct ksz_device *dev, int port, const struct switchdev_obj_port_vlan *vlan); +void ksz8_port_enable_pvid(struct ksz_device *dev, int port, bool state); int ksz8_port_mirror_add(struct ksz_device *dev, int port, struct dsa_mall_mirror_tc_entry *mirror, bool ingress, struct netlink_ext_ack *extack); diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c index c79a5128235f..4f4e26001957 100644 --- a/drivers/net/dsa/microchip/ksz8795.c +++ b/drivers/net/dsa/microchip/ksz8795.c @@ -952,7 +952,7 @@ int ksz8_port_vlan_filtering(struct ksz_device *dev, int port, bool flag, return 0; } -static void ksz8_port_enable_pvid(struct ksz_device *dev, int port, bool state) +void ksz8_port_enable_pvid(struct ksz_device *dev, int port, bool state) { if (ksz_is_ksz88x3(dev)) { ksz_cfg(dev, REG_SW_INSERT_SRC_PVID, @@ -968,8 +968,8 @@ int ksz8_port_vlan_add(struct ksz_device *dev, int port, { bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED; struct ksz_port *p = &dev->ports[port]; - u16 data, new_pvid = 0; u8 fid, member, valid; + u16 data; if (ksz_is_ksz88x3(dev)) return -ENOTSUPP; @@ -1016,36 +1016,18 @@ int ksz8_port_vlan_add(struct ksz_device *dev, int port, ksz8_to_vlan(dev, fid, member, valid, &data); ksz8_w_vlan_table(dev, vlan->vid, data); - /* change PVID */ - if (vlan->flags & BRIDGE_VLAN_INFO_PVID) - new_pvid = vlan->vid; - - if (new_pvid) { - u16 vid; - - ksz_pread16(dev, port, REG_PORT_CTRL_VID, &vid); - vid &= ~VLAN_VID_MASK; - vid |= new_pvid; - ksz_pwrite16(dev, port, REG_PORT_CTRL_VID, vid); - - ksz8_port_enable_pvid(dev, port, true); - } - return 0; } int ksz8_port_vlan_del(struct ksz_device *dev, int port, const struct switchdev_obj_port_vlan *vlan) { - u16 data, pvid; + u16 data; u8 fid, member, valid; if (ksz_is_ksz88x3(dev)) return -ENOTSUPP; - ksz_pread16(dev, port, REG_PORT_CTRL_VID, &pvid); - pvid = pvid & 0xFFF; - ksz8_r_vlan_table(dev, vlan->vid, &data); ksz8_from_vlan(dev, data, &fid, &member, &valid); @@ -1060,9 +1042,6 @@ int ksz8_port_vlan_del(struct ksz_device *dev, int port, ksz8_to_vlan(dev, fid, member, valid, &data); ksz8_w_vlan_table(dev, vlan->vid, data); - if (pvid == vlan->vid) - ksz8_port_enable_pvid(dev, port, false); - return 0; } diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c index 4b14d80d27ed..6a39d6893142 100644 --- a/drivers/net/dsa/microchip/ksz9477.c +++ b/drivers/net/dsa/microchip/ksz9477.c @@ -354,6 +354,12 @@ void ksz9477_flush_dyn_mac_table(struct ksz_device *dev, int port) } } +void ksz9477_port_drop_untagged(struct ksz_device *dev, int port, bool state) +{ + ksz_port_cfg(dev, port, REG_PORT_MRI_MAC_CTRL, PORT_DROP_NON_VLAN, + state); +} + int ksz9477_port_vlan_filtering(struct ksz_device *dev, int port, bool flag, struct netlink_ext_ack *extack) { @@ -399,10 +405,6 @@ int ksz9477_port_vlan_add(struct ksz_device *dev, int port, return err; } - /* change PVID */ - if (vlan->flags & BRIDGE_VLAN_INFO_PVID) - ksz_pwrite16(dev, port, REG_PORT_DEFAULT_VID, vlan- >vid); - return 0; } @@ -411,10 +413,6 @@ int ksz9477_port_vlan_del(struct ksz_device *dev, int port, { bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED; u32 vlan_table[3]; - u16 pvid; - - ksz_pread16(dev, port, REG_PORT_DEFAULT_VID, &pvid); - pvid = pvid & 0xFFF; if (ksz9477_get_vlan_table(dev, vlan->vid, vlan_table)) { dev_dbg(dev->dev, "Failed to get vlan table\n"); @@ -423,9 +421,6 @@ int ksz9477_port_vlan_del(struct ksz_device *dev, int port, vlan_table[2] &= ~BIT(port); - if (pvid == vlan->vid) - pvid = 1; - if (untagged) vlan_table[1] &= ~BIT(port); @@ -434,8 +429,6 @@ int ksz9477_port_vlan_del(struct ksz_device *dev, int port, return -ETIMEDOUT; } - ksz_pwrite16(dev, port, REG_PORT_DEFAULT_VID, pvid); - return 0; } diff --git a/drivers/net/dsa/microchip/ksz9477.h b/drivers/net/dsa/microchip/ksz9477.h index cd278b307b3c..005c510ee6c2 100644 --- a/drivers/net/dsa/microchip/ksz9477.h +++ b/drivers/net/dsa/microchip/ksz9477.h @@ -30,6 +30,7 @@ int ksz9477_port_vlan_add(struct ksz_device *dev, int port, struct netlink_ext_ack *extack); int ksz9477_port_vlan_del(struct ksz_device *dev, int port, const struct switchdev_obj_port_vlan *vlan); +void ksz9477_port_drop_untagged(struct ksz_device *dev, int port, bool state); int ksz9477_port_mirror_add(struct ksz_device *dev, int port, struct dsa_mall_mirror_tc_entry *mirror, bool ingress, struct netlink_ext_ack *extack); diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c index ed7d137cba99..8711be6155e6 100644 --- a/drivers/net/dsa/microchip/ksz_common.c +++ b/drivers/net/dsa/microchip/ksz_common.c @@ -14,6 +14,7 @@ #include <linux/phy.h> #include <linux/etherdevice.h> #include <linux/if_bridge.h> +#include <linux/if_vlan.h> #include <linux/of_device.h> #include <linux/of_net.h> #include <linux/micrel_phy.h> @@ -160,6 +161,7 @@ static const struct ksz_dev_ops ksz8_dev_ops = { .vlan_filtering = ksz8_port_vlan_filtering, .vlan_add = ksz8_port_vlan_add, .vlan_del = ksz8_port_vlan_del, + .drop_untagged = ksz8_port_enable_pvid, .mirror_add = ksz8_port_mirror_add, .mirror_del = ksz8_port_mirror_del, .get_caps = ksz8_get_caps, @@ -186,6 +188,7 @@ static const struct ksz_dev_ops ksz9477_dev_ops = { .vlan_filtering = ksz9477_port_vlan_filtering, .vlan_add = ksz9477_port_vlan_add, .vlan_del = ksz9477_port_vlan_del, + .drop_untagged = ksz9477_port_drop_untagged, .mirror_add = ksz9477_port_mirror_add, .mirror_del = ksz9477_port_mirror_del, .get_caps = ksz9477_get_caps, @@ -219,6 +222,7 @@ static const struct ksz_dev_ops lan937x_dev_ops = { .vlan_filtering = ksz9477_port_vlan_filtering, .vlan_add = ksz9477_port_vlan_add, .vlan_del = ksz9477_port_vlan_del, + .drop_untagged = ksz9477_port_drop_untagged, .mirror_add = ksz9477_port_mirror_add, .mirror_del = ksz9477_port_mirror_del, .get_caps = lan937x_phylink_get_caps, @@ -258,6 +262,7 @@ static const u16 ksz8795_regs[] = { [S_MULTICAST_CTRL] = 0x04, [P_XMII_CTRL_0] = 0x06, [P_XMII_CTRL_1] = 0x56, + [P_DEFAULT_PVID] = 0x03, }; static const u32 ksz8795_masks[] = { @@ -330,6 +335,7 @@ static const u16 ksz8863_regs[] = { [S_START_CTRL] = 0x01, [S_BROADCAST_CTRL] = 0x06, [S_MULTICAST_CTRL] = 0x04, + [P_DEFAULT_PVID] = 0x03, }; static const u32 ksz8863_masks[] = { @@ -372,6 +378,7 @@ static const u16 ksz9477_regs[] = { [S_MULTICAST_CTRL] = 0x0331, [P_XMII_CTRL_0] = 0x0300, [P_XMII_CTRL_1] = 0x0301, + [P_DEFAULT_PVID] = 0x0000, }; static const u32 ksz9477_masks[] = { @@ -1333,38 +1340,120 @@ static enum dsa_tag_protocol ksz_get_tag_protocol(struct dsa_switch *ds, return proto; } +static void ksz_get_pvid(struct ksz_device *dev, int port, u16 *pvid) +{ + const u16 *regs = dev->info->regs; + u16 val; + + ksz_pread16(dev, port, regs[P_DEFAULT_PVID], &val); + + *pvid = val & VLAN_VID_MASK; +} + +static void ksz_set_pvid(struct ksz_device *dev, int port, u16 pvid) +{ + const u16 *regs = dev->info->regs; + + ksz_prmw16(dev, port, regs[P_DEFAULT_PVID], VLAN_VID_MASK, pvid); +} + +static int ksz_commit_pvid(struct dsa_switch *ds, int port) +{ + struct dsa_port *dp = dsa_to_port(ds, port); + struct net_device *br = dsa_port_bridge_dev_get(dp); + struct ksz_device *dev = ds->priv; + bool drop_untagged = false; + struct ksz_port *p; + u16 pvid = 1; /* bridge vlan unaware pvid */ + + p = &dev->ports[port]; + + if (br && br_vlan_enabled(br)) { + pvid = p->bridge_pvid.vid; + drop_untagged = !p->bridge_pvid.valid; + } + + ksz_set_pvid(dev, port, pvid); + + if (dev->dev_ops->drop_untagged) + dev->dev_ops->drop_untagged(dev, port, drop_untagged); + + return 0; +} + static int ksz_port_vlan_filtering(struct dsa_switch *ds, int port, bool flag, struct netlink_ext_ack *extack) { struct ksz_device *dev = ds->priv; + int ret; if (!dev->dev_ops->vlan_filtering) return -EOPNOTSUPP; - return dev->dev_ops->vlan_filtering(dev, port, flag, extack); + ret = dev->dev_ops->vlan_filtering(dev, port, flag, extack); + if (ret) + return ret; + + return ksz_commit_pvid(ds, port); } static int ksz_port_vlan_add(struct dsa_switch *ds, int port, const struct switchdev_obj_port_vlan *vlan, struct netlink_ext_ack *extack) { + bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID; struct ksz_device *dev = ds->priv; + struct ksz_port *p; + int ret; + + p = &dev->ports[port]; if (!dev->dev_ops->vlan_add) return -EOPNOTSUPP; - return dev->dev_ops->vlan_add(dev, port, vlan, extack); + ret = dev->dev_ops->vlan_add(dev, port, vlan, extack); + if (ret) + return ret; + + if (pvid) { + p->bridge_pvid.vid = vlan->vid; + p->bridge_pvid.valid = true; + } else if (vlan->vid && p->bridge_pvid.vid == vlan->vid) { + /* The old pvid was reinstalled as a non-pvid VLAN */ + p->bridge_pvid.valid = false; + } + + return ksz_commit_pvid(ds, port); } static int ksz_port_vlan_del(struct dsa_switch *ds, int port, const struct switchdev_obj_port_vlan *vlan) { struct ksz_device *dev = ds->priv; + struct ksz_port *p; + u16 pvid; + int ret; + + p = &dev->ports[port]; if (!dev->dev_ops->vlan_del) return -EOPNOTSUPP; - return dev->dev_ops->vlan_del(dev, port, vlan); + ksz_get_pvid(dev, port, &pvid); + + ret = dev->dev_ops->vlan_del(dev, port, vlan); + if (ret) + return ret; + + if (vlan->vid == pvid) { + p->bridge_pvid.valid = false; + + ret = ksz_commit_pvid(ds, port); + if (ret) + return ret; + } + + return 0; } static int ksz_port_mirror_add(struct dsa_switch *ds, int port, diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h index 764ada3a0f42..c583cda6cc3d 100644 --- a/drivers/net/dsa/microchip/ksz_common.h +++ b/drivers/net/dsa/microchip/ksz_common.h @@ -63,6 +63,11 @@ struct ksz_chip_data { bool internal_phy[KSZ_MAX_NUM_PORTS]; }; +struct ksz_vlan { + u16 vid; + bool valid; +}; + struct ksz_port { bool remove_tag; /* Remove Tag flag set, for ksz8795 only */ int stp_state; @@ -81,6 +86,7 @@ struct ksz_port { u16 max_frame; u32 rgmii_tx_val; u32 rgmii_rx_val; + struct ksz_vlan bridge_pvid; }; struct ksz_device { @@ -175,6 +181,7 @@ enum ksz_regs { S_MULTICAST_CTRL, P_XMII_CTRL_0, P_XMII_CTRL_1, + P_DEFAULT_PVID, }; enum ksz_masks { @@ -272,6 +279,7 @@ struct ksz_dev_ops { struct netlink_ext_ack *extack); int (*vlan_del)(struct ksz_device *dev, int port, const struct switchdev_obj_port_vlan *vlan); + void (*drop_untagged)(struct ksz_device *dev, int port, bool untagged); int (*mirror_add)(struct ksz_device *dev, int port, struct dsa_mall_mirror_tc_entry *mirror, bool ingress, struct netlink_ext_ack *extack); @@ -434,6 +442,14 @@ static inline void ksz_prmw8(struct ksz_device *dev, int port, int offset, mask, val); } +static inline void ksz_prmw16(struct ksz_device *dev, int port, int offset, + u16 mask, u16 val) +{ + regmap_update_bits(dev->regmap[1], + dev->dev_ops->get_port_addr(port, offset), + mask, val); +} + static inline void ksz_regmap_lock(void *__mtx) { struct mutex *mtx = __mtx;
Hi Arun, On Tue, Jul 26, 2022 at 03:10:24PM +0000, Arun.Ramadoss@microchip.com wrote: > I tried to update the ksz code and tested after applying this patch > series. Following are the observation, > (...) > In summary, only for pvid 1 below patch is working. Initially I tried > with pvid 0, 21, 4095, it were not working, only for pvid 1 it is > working. Kindly suggest whether any changes to be done in patch or > testing methodology. What are you saying exactly that you tried with pvid 0, 21, 4095? Do you mean (a) you changed the vlan_default_pvid of the bridge to these values, or (b) you edited "u16 pvid = 1" in ksz_commit_pvid() to "u16 pvid = 0" (or 21, 4095 etc)? Either way, the fundamental reason why neither was going to work is the same, although the explanation is going to be slightly different. What vlan_default_pvid means is what the bridge layer uses as a pvid value for VLAN-aware ports. The value of 0 is special and it means "don't add a PVID at all". It's the same as if you compiled your kernel with CONFIG_BRIDGE_VLAN_FILTERING=n. The problem is that you're not making a difference between the bridge PVID and the hardware PVID. See, things don't work due to the line highlighted below: static int ksz_commit_pvid(struct dsa_switch *ds, int port) { struct dsa_port *dp = dsa_to_port(ds, port); struct net_device *br = dsa_port_bridge_dev_get(dp); struct ksz_device *dev = ds->priv; bool drop_untagged = false; struct ksz_port *p; u16 pvid = 1; /* bridge vlan unaware pvid */ <--- this line p = &dev->ports[port]; if (br && br_vlan_enabled(br)) { pvid = p->bridge_pvid.vid; drop_untagged = !p->bridge_pvid.valid; } ksz_set_pvid(dev, port, pvid); if (dev->dev_ops->drop_untagged) dev->dev_ops->drop_untagged(dev, port, drop_untagged); return 0; } You can't just gingerly say that the PVID is 1, then rely on some other code (the bridge layer) to actually _add_ VLAN 1 to your VLAN database, and expect things to work (as you've noticed, they don't, when the bridge doesn't add this VLAN). If I'm following along properly, and *there is some guesswork involved*, VLAN-unaware bridging only works with the KSZ driver if you have CONFIG_BRIDGE_VLAN_FILTERING enabled. If you don't, nobody adds any VLAN to your VLAN table, and (this is the guessing part) the VLAN table of these switches is by default empty. You need to pick a VLAN that you use for the address database of a VLAN unaware bridge, and manage it by yourself. That means, add logic to the driver to add said VLAN ID to the VLAN table, and deny other layers (DSA's port_vlan_add) from touching it. That's why some other drivers use VLAN 4095 and/or 0 for that purpose, because they don't need to deny these VIDs from the bridge layer, because 'bridge vlan add' already fails for VID 0 and 4095 (IEEE 802.1Q defines them as having reserved values). This is done for example in mt7530_setup_vlan0() in mt7530, managed by tag_8021q for sja1105, or ocelot_port_bridge_join() -> ocelot_add_vlan_unaware_pvid() -> ocelot_vlan_unaware_pvid() in the case of the felix driver. mt7530 has nothing to reject, while sja1105 rejects private VLANs in sja1105_bridge_vlan_add() and felix in ocelot_vlan_prepare() - see OCELOT_RSV_VLAN_RANGE_START. Now don't take anything of what I've said too literal, because I'm a complete non-expert for KSZ switches, and the way in which you enforce isolation of address databases is completely hardware-specific. Of course what I've told you above relies on cropping some VLANs from what the bridge layer can use (and then you need to make sure that somebody from the outside can't 'attack' you by sending a packet with VLAN 4095 on a port from a VLAN-aware bridge, and your switch thinks that it should process it in the forwarding domain of a VLAN-unaware bridge that *actually* uses VID 4095 as part of your private driver implementation). So the approach of cropping VLANs to use privately is not optimal, and does need care to implement properly. I advise you to read back starting from this thread with Qingfang about how address database isolation was implemented for mt7530. There, we ended up using transparent mode for standalone ports (VLAN table is not looked up at all, packets default to one FID), and fallback mode for VLAN-unaware bridge ports (packets default to another FID). So that can be extended to allocate a FID for each VLAN unaware bridge, without cropping more and more VLANs, just mapping the ports of each VLAN unaware bridge to a different FID. https://lore.kernel.org/all/20210730175139.518992-1-dqfext@gmail.com/ Some experimentation needs to be done to find the optimum configuration for this hardware. At the very least, what I care to see is, in this order: (1) a driver that makes sure the private VLANs it uses are present in the VLAN table (2) a driver that only changes the hardware PVID when it should, i.e. not when it's VLAN-unaware and the bridge changes the VLAN-aware PVID (3) a driver that separates the address databases of standalone ports from that of VLAN-unaware bridge ports (4) a driver which separates the address databases of *each* VLAN-unaware bridge from each other For the purposes of my change, I only need (1) and (2), but I strongly suggest you to look into (3) and (4) as well. Some selftests (which I'd very much like for the KSZ driver to pass!) rely on (3) working properly. Sorry for not diving into KSZ hardware documentation, I wrote this email in a bit of a hurry.
On Tue, 2022-07-26 at 17:21 +0000, Vladimir Oltean wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe Hi Vladimir, I am trying to bringup the kselftest for bridge_vlan_aware.sh, in that I am facing problem during the ping test and all the tests are failing. #ip vrf exec vlan1 ping 192.0.2.2 Cannot open network namespace: No such file or directory Failed to get name of network namespace: No such file or directory Is there any configurations need to be enabled in the linux kernel, can you suggest/help me out in resolving it. -- Arun > > Hi Arun, > > On Tue, Jul 26, 2022 at 03:10:24PM +0000, Arun.Ramadoss@microchip.com > wrote: > > I tried to update the ksz code and tested after applying this patch > > series. Following are the observation, > > > > (...) > > In summary, only for pvid 1 below patch is working. Initially I > > tried > > with pvid 0, 21, 4095, it were not working, only for pvid 1 it is > > working. Kindly suggest whether any changes to be done in patch or > > testing methodology. > > What are you saying exactly that you tried with pvid 0, 21, 4095? > Do you mean > (a) you changed the vlan_default_pvid of the bridge to these values, > or > (b) you edited "u16 pvid = 1" in ksz_commit_pvid() to "u16 pvid = 0" > (or 21, 4095 etc)? > > Either way, the fundamental reason why neither was going to work is > the > same, although the explanation is going to be slightly different. > > What vlan_default_pvid means is what the bridge layer uses as a pvid > value for VLAN-aware ports. The value of 0 is special and it means > "don't add a PVID at all". It's the same as if you compiled your > kernel > with CONFIG_BRIDGE_VLAN_FILTERING=n. > > The problem is that you're not making a difference between the bridge > PVID and the hardware PVID. > > See, things don't work due to the line highlighted below: > > static int ksz_commit_pvid(struct dsa_switch *ds, int port) > { > struct dsa_port *dp = dsa_to_port(ds, port); > struct net_device *br = dsa_port_bridge_dev_get(dp); > struct ksz_device *dev = ds->priv; > bool drop_untagged = false; > struct ksz_port *p; > u16 pvid = 1; /* bridge vlan unaware pvid */ <--- > this line > > p = &dev->ports[port]; > > if (br && br_vlan_enabled(br)) { > pvid = p->bridge_pvid.vid; > drop_untagged = !p->bridge_pvid.valid; > } > > ksz_set_pvid(dev, port, pvid); > > if (dev->dev_ops->drop_untagged) > dev->dev_ops->drop_untagged(dev, port, > drop_untagged); > > return 0; > } > >
On Mon, Sep 12, 2022 at 03:30:18PM +0000, Arun.Ramadoss@microchip.com wrote: > On Tue, 2022-07-26 at 17:21 +0000, Vladimir Oltean wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you > > know the content is safe > Hi Vladimir, > I am trying to bringup the kselftest for bridge_vlan_aware.sh, in that > I am facing problem during the ping test and all the tests are failing. > > #ip vrf exec vlan1 ping 192.0.2.2 > Cannot open network namespace: No such file or directory > Failed to get name of network namespace: No such file or directory > > Is there any configurations need to be enabled in the linux kernel, can > you suggest/help me out in resolving it. > > -- > Arun Yes, quite a few, in fact. Note that I'm not quite sure why it says "cannot open network namespace" when the command accesses a VRF instead. Try these: CONFIG_IP_ADVANCED_ROUTER=y CONFIG_IP_MULTIPLE_TABLES=y CONFIG_NET_L3_MASTER_DEV=y CONFIG_IPV6_MULTIPLE_TABLES=y CONFIG_NET_VRF=y CONFIG_BPF_SYSCALL=y CONFIG_CGROUP_BPF=y
Hi Vladimir, Thanks for reply. On Mon, 2022-09-12 at 15:42 +0000, Vladimir Oltean wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Mon, Sep 12, 2022 at 03:30:18PM +0000, Arun.Ramadoss@microchip.com > wrote: > > On Tue, 2022-07-26 at 17:21 +0000, Vladimir Oltean wrote: > > > EXTERNAL EMAIL: Do not click links or open attachments unless you > > > know the content is safe > > > > Hi Vladimir, > > I am trying to bringup the kselftest for bridge_vlan_aware.sh, in > > that > > I am facing problem during the ping test and all the tests are > > failing. > > > > #ip vrf exec vlan1 ping 192.0.2.2 > > Cannot open network namespace: No such file or directory > > Failed to get name of network namespace: No such file or directory > > > > Is there any configurations need to be enabled in the linux kernel, > > can > > you suggest/help me out in resolving it. > > > > -- > > Arun > > Yes, quite a few, in fact. > Note that I'm not quite sure why it says "cannot open network > namespace" > when the command accesses a VRF instead. > > Try these: > > CONFIG_IP_ADVANCED_ROUTER=y > CONFIG_IP_MULTIPLE_TABLES=y > CONFIG_NET_L3_MASTER_DEV=y > CONFIG_IPV6_MULTIPLE_TABLES=y > CONFIG_NET_VRF=y > CONFIG_BPF_SYSCALL=y > CONFIG_CGROUP_BPF=y In addition to above config, I had set CONFIG_NAMESPACES=y, then the above error message disappered. But the ping is not successful. If I change the setup like Linux laptop 1 --> DUT1 (Lan2) --> DUT1 (Lan3) --> Linux laptop 2 then ping is successful. If I use the standard kselftest setup lan1 --> lan2 --> lan3 --> lan4, ping is not success. I went through the comments given in this thread to bring up ping for openwrt, is that applicable to kselftest also. ip netns add ns0 ip link set lan2 netns ns0 ip -n ns0 link set lan2 up ip -n ns0 addr add 192.168.2.2/24 dev lan2 ip netns exec ns0 tcpdump -i lan2 -e -n ping 192.168.2.2 I am struck with the ping test bringup. It would be helpful, if you can give some suggestion to bring it up.
On Tue, Sep 13, 2022 at 10:57:44AM +0000, Arun.Ramadoss@microchip.com wrote: > In addition to above config, I had set CONFIG_NAMESPACES=y, then the > above error message disappered. > But the ping is not successful. > > If I change the setup like > Linux laptop 1 --> DUT1 (Lan2) --> DUT1 (Lan3) --> Linux laptop 2 > then ping is successful. > > If I use the standard kselftest setup > lan1 --> lan2 --> lan3 --> lan4, ping is not success. > > I went through the comments given in this thread to bring up ping for > openwrt, is that applicable to kselftest also. > > ip netns add ns0 > ip link set lan2 netns ns0 > ip -n ns0 link set lan2 up > ip -n ns0 addr add 192.168.2.2/24 dev lan2 > ip netns exec ns0 tcpdump -i lan2 -e -n > ping 192.168.2.2 You meant to put lan4 in a namespace and ping it and not lan2, since lan2 will be a bridge port, right? > I am struck with the ping test bringup. It would be helpful, if you can > give some suggestion to bring it up. Well, do you have unique MAC addresses for lan1, lan2, lan3, lan4? The kselftests will ensure you do, via the STABLE_MAC_ADDRS variable, but otherwise you may not. Then, can you tell us what packets you do see reaching lan4 in the ping setup? If none, can you show us the output of ethtool -S lan2 | grep -v ': 0' to figure out what is the drop reason (supposing the packets are dropped at the ingress of lan2)? Or even tcpdump -i lan2, maybe the packets are forwarded to the termination plane of the bridge, instead of being autonomously forwarded to lan3.
diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c index e531b93f3cb2..86acf8a4e53c 100644 --- a/drivers/net/dsa/lantiq_gswip.c +++ b/drivers/net/dsa/lantiq_gswip.c @@ -893,8 +893,6 @@ static int gswip_setup(struct dsa_switch *ds) gswip_port_enable(ds, cpu_port, NULL); - ds->configure_vlan_while_not_filtering = false; - return 0; } diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c index 28d7cb2ce98f..561a515c7cb8 100644 --- a/drivers/net/dsa/microchip/ksz_common.c +++ b/drivers/net/dsa/microchip/ksz_common.c @@ -927,8 +927,6 @@ static int ksz_setup(struct dsa_switch *ds) ksz_init_mib_timer(dev); - ds->configure_vlan_while_not_filtering = false; - if (dev->dev_ops->setup) { ret = dev->dev_ops->setup(ds); if (ret) diff --git a/include/net/dsa.h b/include/net/dsa.h index b902b31bebce..2ed1c2db4037 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -400,13 +400,6 @@ struct dsa_switch { /* Keep VLAN filtering enabled on ports not offloading any upper */ u32 needs_standalone_vlan_filtering:1; - /* Pass .port_vlan_add and .port_vlan_del to drivers even for bridges - * that have vlan_filtering=0. All drivers should ideally set this (and - * then the option would get removed), but it is unknown whether this - * would break things or not. - */ - u32 configure_vlan_while_not_filtering:1; - /* If the switch driver always programs the CPU port as egress tagged * despite the VLAN configuration indicating otherwise, then setting * @untag_bridge_pvid will force the DSA receive path to pop the diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c index cac48a741f27..47a2e60b6080 100644 --- a/net/dsa/dsa2.c +++ b/net/dsa/dsa2.c @@ -890,8 +890,6 @@ static int dsa_switch_setup(struct dsa_switch *ds) if (err) goto unregister_devlink_ports; - ds->configure_vlan_while_not_filtering = true; - err = ds->ops->setup(ds); if (err < 0) goto unregister_notifier; diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h index d9722e49864b..63191db45d02 100644 --- a/net/dsa/dsa_priv.h +++ b/net/dsa/dsa_priv.h @@ -224,7 +224,6 @@ void dsa_port_pre_lag_leave(struct dsa_port *dp, struct net_device *lag_dev); void dsa_port_lag_leave(struct dsa_port *dp, struct net_device *lag_dev); int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering, struct netlink_ext_ack *extack); -bool dsa_port_skip_vlan_configuration(struct dsa_port *dp); int dsa_port_ageing_time(struct dsa_port *dp, clock_t ageing_clock); int dsa_port_mst_enable(struct dsa_port *dp, bool on, struct netlink_ext_ack *extack); diff --git a/net/dsa/port.c b/net/dsa/port.c index 3738f2d40a0b..79853f673b65 100644 --- a/net/dsa/port.c +++ b/net/dsa/port.c @@ -834,20 +834,6 @@ int dsa_port_vlan_filtering(struct dsa_port *dp, bool vlan_filtering, return err; } -/* This enforces legacy behavior for switch drivers which assume they can't - * receive VLAN configuration when enslaved to a bridge with vlan_filtering=0 - */ -bool dsa_port_skip_vlan_configuration(struct dsa_port *dp) -{ - struct net_device *br = dsa_port_bridge_dev_get(dp); - struct dsa_switch *ds = dp->ds; - - if (!br) - return false; - - return !ds->configure_vlan_while_not_filtering && !br_vlan_enabled(br); -} - int dsa_port_ageing_time(struct dsa_port *dp, clock_t ageing_clock) { unsigned long ageing_jiffies = clock_t_to_jiffies(ageing_clock); diff --git a/net/dsa/slave.c b/net/dsa/slave.c index ad6a6663feeb..d1284f8684d8 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -533,11 +533,6 @@ static int dsa_slave_vlan_add(struct net_device *dev, struct switchdev_obj_port_vlan *vlan; int err; - if (dsa_port_skip_vlan_configuration(dp)) { - NL_SET_ERR_MSG_MOD(extack, "skipping configuration of VLAN"); - return 0; - } - vlan = SWITCHDEV_OBJ_PORT_VLAN(obj); /* Deny adding a bridge VLAN when there is already an 802.1Q upper with @@ -571,11 +566,6 @@ static int dsa_slave_host_vlan_add(struct net_device *dev, if (!dp->bridge) return -EOPNOTSUPP; - if (dsa_port_skip_vlan_configuration(dp)) { - NL_SET_ERR_MSG_MOD(extack, "skipping configuration of VLAN"); - return 0; - } - vlan = *SWITCHDEV_OBJ_PORT_VLAN(obj); /* Even though drivers often handle CPU membership in special ways, @@ -642,9 +632,6 @@ static int dsa_slave_vlan_del(struct net_device *dev, struct dsa_port *dp = dsa_slave_to_port(dev); struct switchdev_obj_port_vlan *vlan; - if (dsa_port_skip_vlan_configuration(dp)) - return 0; - vlan = SWITCHDEV_OBJ_PORT_VLAN(obj); return dsa_port_vlan_del(dp, vlan); @@ -660,9 +647,6 @@ static int dsa_slave_host_vlan_del(struct net_device *dev, if (!dp->bridge) return -EOPNOTSUPP; - if (dsa_port_skip_vlan_configuration(dp)) - return 0; - vlan = SWITCHDEV_OBJ_PORT_VLAN(obj); return dsa_port_host_vlan_del(dp, vlan); @@ -1655,11 +1639,7 @@ static int dsa_slave_clear_vlan(struct net_device *vdev, int vid, void *arg) * - no VLAN (any 8021q upper is a software VLAN) * * - If under a vlan_filtering=0 bridge which it offload: - * - if ds->configure_vlan_while_not_filtering = true (default): - * - the bridge VLANs. These VLANs are committed to hardware but inactive. - * - else (deprecated): - * - no VLAN. The bridge VLANs are not restored when VLAN awareness is - * enabled, so this behavior is broken and discouraged. + * - the bridge VLANs. These VLANs are committed to hardware but inactive. * * - If under a vlan_filtering=1 bridge which it offload: * - the bridge VLANs
Stop protecting DSA drivers from switchdev VLAN notifications emitted while the bridge has vlan_filtering 0, by deleting the deprecated bool ds->configure_vlan_while_not_filtering opt-in. Now all DSA drivers see all notifications and should save the bridge PVID until they need to commit it to hardware. The 2 remaining unconverted drivers are the gswip and the Microchip KSZ family. They are both probably broken and would need changing as far as I can see: - For lantiq_gswip, after the initial call path -> gswip_port_bridge_join -> gswip_vlan_add_unaware -> gswip_switch_w(priv, 0, GSWIP_PCE_DEFPVID(port)); nobody seems to prevent a future call path -> gswip_port_vlan_add -> gswip_vlan_add_aware -> gswip_switch_w(priv, idx, GSWIP_PCE_DEFPVID(port)); - For ksz9477, REG_PORT_DEFAULT_VID seems to be only modified by the bridge VLAN add/del handlers, so there is no logic to update it on VLAN awareness change. I don't know exactly how the switch behaves after ksz9477_port_vlan_filtering() is called with "false". If packets are classified to the REG_PORT_DEFAULT_VID, then it is broken. Similar thing can be said for KSZ8. In any case, see commit 8b6836d82470 ("net: dsa: mv88e6xxx: keep the pvid at 0 when VLAN-unaware") for an example of how to deal with the problem, and test pvid_change() in tools/testing/selftests/drivers/net/dsa/bridge_vlan_unaware.sh for how to check whether the driver behaves correctly. I don't have the hardware to test any changes. Cc: Arun Ramadoss <arun.ramadoss@microchip.com> Cc: Hauke Mehrtens <hauke@hauke-m.de> Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- drivers/net/dsa/lantiq_gswip.c | 2 -- drivers/net/dsa/microchip/ksz_common.c | 2 -- include/net/dsa.h | 7 ------- net/dsa/dsa2.c | 2 -- net/dsa/dsa_priv.h | 1 - net/dsa/port.c | 14 -------------- net/dsa/slave.c | 22 +--------------------- 7 files changed, 1 insertion(+), 49 deletions(-)