| Message ID | 20230314163651.242259-4-clement.leger@bootlin.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | net: dsa: rzn1-a5psw: add support for vlan and .port_bridge_flags | expand |
On Tue, Mar 14, 2023 at 05:36:51PM +0100, Clément Léger wrote: > Add support for vlan operation (add, del, filtering) on the RZN1 > driver. The a5psw switch supports up to 32 VLAN IDs with filtering, > tagged/untagged VLANs and PVID for each ports. > > Signed-off-by: Clément Léger <clement.leger@bootlin.com> > --- > drivers/net/dsa/rzn1_a5psw.c | 164 +++++++++++++++++++++++++++++++++++ > drivers/net/dsa/rzn1_a5psw.h | 8 +- > 2 files changed, 169 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c > index 5059b2814cdd..a9a42a8bc7e3 100644 > --- a/drivers/net/dsa/rzn1_a5psw.c > +++ b/drivers/net/dsa/rzn1_a5psw.c > @@ -583,6 +583,144 @@ static int a5psw_port_fdb_dump(struct dsa_switch *ds, int port, > return ret; > } > > +static int a5psw_port_vlan_filtering(struct dsa_switch *ds, int port, > + bool vlan_filtering, > + struct netlink_ext_ack *extack) > +{ > + u32 mask = BIT(port + A5PSW_VLAN_VERI_SHIFT) | > + BIT(port + A5PSW_VLAN_DISC_SHIFT); I'm curious what the A5PSW_VLAN_VERI_SHIFT and A5PSW_VLAN_DISC_SHIFT bits do. Also curious in general what does this hardware do w.r.t. VLANs. There would be several things on the checklist: - can it drop a VLAN which isn't present in the port membership list? I guess this is what A5PSW_VLAN_DISC_SHIFT does. - can it use VLAN information from the packet (with a fallback on the port PVID) to determine where to send, and where *not* to send the packet? How does this relate to the flooding registers? Is the flood mask restricted by the VLAN mask? Is there a default VLAN installed in the hardware tables, which is also the PVID of all ports, and all ports are members of it? Could you implement standalone/bridged port forwarding isolation based on VLANs, rather than the flimsy and most likely buggy implementation done based on flooding domains, from this patch set? - is the FDB looked up per {MAC DA, VLAN ID} or just MAC DA? Looking at a5psw_port_fdb_add(), there's absolutely no sign of "vid" being used, so I guess it's Shared VLAN Learning. In that case, there's absolutely no hope to implement ds->fdb_isolation for this hardware. But at the *very* least, please disable address learning on standalone ports, *and* implement ds->ops->port_fast_age() so that ports quickly forget their learned MAC adddresses after leaving a bridge and become standalone again. - if the port PVID is indeed used to filter the flooding mask of untagged packets, then I speculate that when A5PSW_VLAN_VERI_SHIFT is set, the hardware searches for a VLAN tag in the packet, whereas if it's unset, all packets will be forwarded according just to the port PVID (A5PSW_SYSTEM_TAGINFO). That would be absolutely magnificent if true, but it also means that you need to be *a lot* more careful when programming this register. See the "Address databases" section from Documentation/networking/dsa/dsa.rst for an explanation of the asynchronous nature of .port_vlan_add() relative to .port_vlan_filtering(). Also see the call paths of sja1105_commit_pvid() and mv88e6xxx_port_commit_pvid() for an example of how this should be managed correctly, and how the bridge PVID should be committed to hardware only when the port is currently VLAN-aware. > + u32 val = vlan_filtering ? mask : 0; > + struct a5psw *a5psw = ds->priv; > + > + a5psw_reg_rmw(a5psw, A5PSW_VLAN_VERIFY, mask, val); > + > + return 0; > +} > + > +static int a5psw_port_vlan_del(struct dsa_switch *ds, int port, > + const struct switchdev_obj_port_vlan *vlan) > +{ > + struct a5psw *a5psw = ds->priv; > + u16 vid = vlan->vid; > + int vlan_res_id; > + > + dev_dbg(a5psw->dev, "Removing VLAN %d on port %d\n", vid, port); > + > + vlan_res_id = a5psw_find_vlan_entry(a5psw, vid); > + if (vlan_res_id < 0) > + return -EINVAL; > + > + a5psw_port_vlan_cfg(a5psw, vlan_res_id, port, false); > + a5psw_port_vlan_tagged_cfg(a5psw, vlan_res_id, port, false); > + > + /* Disable PVID if the vid is matching the port one */ What does it mean to disable PVID? > + if (vid == a5psw_reg_readl(a5psw, A5PSW_SYSTEM_TAGINFO(port))) > + a5psw_reg_rmw(a5psw, A5PSW_VLAN_IN_MODE_ENA, BIT(port), 0); > + > + return 0; > +} > + > static u64 a5psw_read_stat(struct a5psw *a5psw, u32 offset, int port) > { > u32 reg_lo, reg_hi; > @@ -700,6 +838,27 @@ static void a5psw_get_eth_ctrl_stats(struct dsa_switch *ds, int port, > ctrl_stats->MACControlFramesReceived = stat; > } > > +static void a5psw_vlan_setup(struct a5psw *a5psw, int port) > +{ > + u32 reg; > + > + /* Enable TAG always mode for the port, this is actually controlled > + * by VLAN_IN_MODE_ENA field which will be used for PVID insertion > + */ What does the "tag always" mode do, and what are the alternatives? > + reg = A5PSW_VLAN_IN_MODE_TAG_ALWAYS; > + reg <<= A5PSW_VLAN_IN_MODE_PORT_SHIFT(port); > + a5psw_reg_rmw(a5psw, A5PSW_VLAN_IN_MODE, A5PSW_VLAN_IN_MODE_PORT(port), > + reg); > + > + /* Set transparent mode for output frame manipulation, this will depend > + * on the VLAN_RES configuration mode > + */ What does the "transparent" output mode do, and how does it compare to the "dis", "strip" and "tag through" alternatives? > + reg = A5PSW_VLAN_OUT_MODE_TRANSPARENT; > + reg <<= A5PSW_VLAN_OUT_MODE_PORT_SHIFT(port); > + a5psw_reg_rmw(a5psw, A5PSW_VLAN_OUT_MODE, > + A5PSW_VLAN_OUT_MODE_PORT(port), reg); > +} Sorry for asking all these questions, but VLAN configuration on a switch such as to bring it in line with the bridge driver expectations is a rather tricky thing, so I'd like to have as clear of a mental model of this hardware as possible, if public documentation isn't available.
Le Wed, 15 Mar 2023 01:34:54 +0200, Vladimir Oltean <olteanv@gmail.com> a écrit : > On Tue, Mar 14, 2023 at 05:36:51PM +0100, Clément Léger wrote: > > Add support for vlan operation (add, del, filtering) on the RZN1 > > driver. The a5psw switch supports up to 32 VLAN IDs with filtering, > > tagged/untagged VLANs and PVID for each ports. > > > > Signed-off-by: Clément Léger <clement.leger@bootlin.com> > > --- > > drivers/net/dsa/rzn1_a5psw.c | 164 +++++++++++++++++++++++++++++++++++ > > drivers/net/dsa/rzn1_a5psw.h | 8 +- > > 2 files changed, 169 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c > > index 5059b2814cdd..a9a42a8bc7e3 100644 > > --- a/drivers/net/dsa/rzn1_a5psw.c > > +++ b/drivers/net/dsa/rzn1_a5psw.c > > @@ -583,6 +583,144 @@ static int a5psw_port_fdb_dump(struct dsa_switch *ds, int port, > > return ret; > > } > > > > +static int a5psw_port_vlan_filtering(struct dsa_switch *ds, int port, > > + bool vlan_filtering, > > + struct netlink_ext_ack *extack) > > +{ > > + u32 mask = BIT(port + A5PSW_VLAN_VERI_SHIFT) | > > + BIT(port + A5PSW_VLAN_DISC_SHIFT); > > I'm curious what the A5PSW_VLAN_VERI_SHIFT and A5PSW_VLAN_DISC_SHIFT > bits do. Also curious in general what does this hardware do w.r.t. > VLANs. There would be several things on the checklist: > > - can it drop a VLAN which isn't present in the port membership list? > I guess this is what A5PSW_VLAN_DISC_SHIFT does. Yes, A5PSW_VLAN_DISC_SHIFT stands for "discard" which means the packet is discarded if the port is not a member of the VLAN. A5PSW_VLAN_VERI_SHIFT is meant to enable VLAN lookup for packet flooding (instead of the default lookup). > > - can it use VLAN information from the packet (with a fallback on the > port PVID) to determine where to send, and where *not* to send the > packet? How does this relate to the flooding registers? Is the flood > mask restricted by the VLAN mask? Is there a default VLAN installed in > the hardware tables, which is also the PVID of all ports, and all > ports are members of it? Could you implement standalone/bridged port > forwarding isolation based on VLANs, rather than the flimsy and most > likely buggy implementation done based on flooding domains, from this > patch set? Yes, the VLAN membership is used for packet flooding. The flooding registers are used when the packets come has a src MAC that is not in the FDB. For more infiormation, see section 4.5.3.9, paragraph 3.c which describe the whole lookup process. Regarding your other question, by default, there is no default VLAN installed but indeed, I see what you mean, a default VLAN could be used to isolate each ports rather than setting the rule to forward only to root CPU port + disabling of flooding. I guess a unique VLAN ID per port should be used to isolate each of them and added to the root port to untag the input frames tagged with the PVID ? > > - is the FDB looked up per {MAC DA, VLAN ID} or just MAC DA? Looking at > a5psw_port_fdb_add(), there's absolutely no sign of "vid" being used, > so I guess it's Shared VLAN Learning. In that case, there's absolutely > no hope to implement ds->fdb_isolation for this hardware. But at the > *very* least, please disable address learning on standalone ports, > *and* implement ds->ops->port_fast_age() so that ports quickly forget > their learned MAC adddresses after leaving a bridge and become > standalone again. Indeed, the lookup table does not contain the VLAN ID and thus it is unused. We talked about it in a previous review and you already mentionned that there is no hope to implement fdb_isolation. Ok for disabling learning on standalone ports, and indeed, by default, it's enabled. Regarding ds->ops->port_fast_age(), it is already implemented. > > - if the port PVID is indeed used to filter the flooding mask of > untagged packets, then I speculate that when A5PSW_VLAN_VERI_SHIFT > is set, the hardware searches for a VLAN tag in the packet, whereas if > it's unset, all packets will be forwarded according just to the port > PVID (A5PSW_SYSTEM_TAGINFO). That would be absolutely magnificent if > true, but it also means that you need to be *a lot* more careful when > programming this register. See the "Address databases" section from > Documentation/networking/dsa/dsa.rst for an explanation of the > asynchronous nature of .port_vlan_add() relative to .port_vlan_filtering(). > Also see the call paths of sja1105_commit_pvid() and mv88e6xxx_port_commit_pvid() > for an example of how this should be managed correctly, and how the > bridge PVID should be committed to hardware only when the port is > currently VLAN-aware. The port PVID itself is not used to filter the flooding mask. But each time a PVID is set, the port must also be programmed as a membership of the PVID VLAN ID in the VLAN resolution table. So actually, the PVID is just here to tag (or not) the input packet, it does not take a role in packet forwading. This is entirely done by the VLAN resolution table content (VLAN_RES_TABLE register). Does this means I don't have to be extra careful when programming it ? > > > + u32 val = vlan_filtering ? mask : 0; > > + struct a5psw *a5psw = ds->priv; > > + > > + a5psw_reg_rmw(a5psw, A5PSW_VLAN_VERIFY, mask, val); > > + > > + return 0; > > +} > > + > > +static int a5psw_port_vlan_del(struct dsa_switch *ds, int port, > > + const struct switchdev_obj_port_vlan *vlan) > > +{ > > + struct a5psw *a5psw = ds->priv; > > + u16 vid = vlan->vid; > > + int vlan_res_id; > > + > > + dev_dbg(a5psw->dev, "Removing VLAN %d on port %d\n", vid, port); > > + > > + vlan_res_id = a5psw_find_vlan_entry(a5psw, vid); > > + if (vlan_res_id < 0) > > + return -EINVAL; > > + > > + a5psw_port_vlan_cfg(a5psw, vlan_res_id, port, false); > > + a5psw_port_vlan_tagged_cfg(a5psw, vlan_res_id, port, false); > > + > > + /* Disable PVID if the vid is matching the port one */ > > 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. > > > + if (vid == a5psw_reg_readl(a5psw, A5PSW_SYSTEM_TAGINFO(port))) > > + a5psw_reg_rmw(a5psw, A5PSW_VLAN_IN_MODE_ENA, BIT(port), 0); > > + > > + return 0; > > +} > > + > > static u64 a5psw_read_stat(struct a5psw *a5psw, u32 offset, int port) > > { > > u32 reg_lo, reg_hi; > > @@ -700,6 +838,27 @@ static void a5psw_get_eth_ctrl_stats(struct dsa_switch *ds, int port, > > ctrl_stats->MACControlFramesReceived = stat; > > } > > > > +static void a5psw_vlan_setup(struct a5psw *a5psw, int port) > > +{ > > + u32 reg; > > + > > + /* Enable TAG always mode for the port, this is actually controlled > > + * by VLAN_IN_MODE_ENA field which will be used for PVID insertion > > + */ > > What does the "tag always" mode do, and what are the alternatives? The name of the mode is probably missleading. When setting VLAN_IN_MODE with A5PSW_VLAN_IN_MODE_TAG_ALWAYS, the input packet will be tagged _only_ if VLAN_IN_MODE_ENA port bit is set. If this bit is not set for the port, packet will passthrough transparently. This bit is actually enabled in a5psw_port_vlan_add() when a PVID is set and unset when the PVID is removed. Maybe the comment above these lines was not clear enough. There are actually 3 modes (excerpt of the documentation): 0) Single Tagging with Passthrough/VID Overwrite: Insert tag if untagged frame. Leave frame unmodified if tagged and VID > 0. If tagged with VID = 0 (priority tagged), then the VID will be overwritten with the VID from SYSTEM_TAGINFO and priority is kept. 1) Single Tagging with Replace: If untagged, add the tag, if single tagged, overwrite the tag. 2) Tag always: Insert a tag always. This results in a single tagged frame when an untagged is received, and a double tagged frame, when a single tagged frame is received (or triple tagged if double-tagged received etc.). This mode is then enforced (or not) using VLAN_IN_MODE. Input manipulation can be enabled per port with register VLAN_IN_MODE_ENA and its port individual mode is configured in register VLAN_IN_MODE. Moreover, the tag that will be inserted is stored in the SYSTEM_TAGINFO[port] register. > > > + reg = A5PSW_VLAN_IN_MODE_TAG_ALWAYS; > > + reg <<= A5PSW_VLAN_IN_MODE_PORT_SHIFT(port); > > + a5psw_reg_rmw(a5psw, A5PSW_VLAN_IN_MODE, > > A5PSW_VLAN_IN_MODE_PORT(port), > > + reg); > > + > > + /* Set transparent mode for output frame manipulation, > > this will depend > > + * on the VLAN_RES configuration mode > > + */ > > What does the "transparent" output mode do, and how does it compare to > the "dis", "strip" and "tag through" alternatives? Here is a description of the 4 modes (excerpt of the documentation): 0) Disabled: No frame manipulation occurs, frame is output as-is. 1) Strip mode: All the tags (single or double) are removed from frame before sending it. 2) Tag through mode: Always removes first tag from frame only. In Tag Through mode, the inner Tag is passed through while the outer Tag is removed for a double Tagged frame. The following rules apply: ● When a single tagged frame is received, strip the tag from the frame. ● When a double tagged frame is received, strip the outer tag from the frame 3) VLAN domain mode / transparent mode: The first tag of a frame is removed (same as Mode 2) when the VLAN is defined as untagged for the port. The following rules apply: ● If frame’s VLAN id is found in the VLAN table (see Section 4.5.3.9(3)(b), VLAN Domain Resolution / VLAN Table) and the port is defined (in the table) as tagged for the VLAN, the frame is not modified. ● If frame’s VLAN id is found in the VLAN table and the port is defined as untagged for the VLAN, the first VLAN tag is removed from the frame. ● If frame’s VLAN id is not found in the VLAN table, the frame is not modified. This last mode allows for a fine grain control oveer tagged/untagged VLAN since each VLAN setup is in the VLAN table. > > > + reg = A5PSW_VLAN_OUT_MODE_TRANSPARENT; > > + reg <<= A5PSW_VLAN_OUT_MODE_PORT_SHIFT(port); > > + a5psw_reg_rmw(a5psw, A5PSW_VLAN_OUT_MODE, > > + A5PSW_VLAN_OUT_MODE_PORT(port), reg); > > +} > > Sorry for asking all these questions, but VLAN configuration on a switch > such as to bring it in line with the bridge driver expectations is a > rather tricky thing, so I'd like to have as clear of a mental model of > this hardware as possible, if public documentation isn't available. No worries, that's your "job" to make sure drivers are in line with what is expected in DSA. The documentation is public and available at [1]. Section 4.5.3 is of interest for your understanding of the VLAN filtering support. Let's hope I answered most of your questions. [1] https://www.renesas.com/us/en/document/mah/rzn1d-group-rzn1s-group-rzn1l-group-users-manual-r-engine-and-ethernet-peripherals?r=1054561
Hi Clément, I'm very sorry for the delay. On Wed, Mar 15, 2023 at 03:54:30PM +0100, Clément Léger wrote: > The documentation is public and available at [1]. Section 4.5.3 is of > interest for your understanding of the VLAN filtering support. Let's > hope I answered most of your questions. > > [1] > https://www.renesas.com/us/en/document/mah/rzn1d-group-rzn1s-group-rzn1l-group-users-manual-r-engine-and-ethernet-peripherals?r=1054561 Yes, indeed, it appears that you gave me this link before and I already had the PDF downloaded, but forgot about it... I've reorganized my documentation PDFs since then. > > - can it drop a VLAN which isn't present in the port membership list? > > I guess this is what A5PSW_VLAN_DISC_SHIFT does. > > Yes, A5PSW_VLAN_DISC_SHIFT stands for "discard" which means the packet > is discarded if the port is not a member of the VLAN. > A5PSW_VLAN_VERI_SHIFT is meant to enable VLAN lookup for packet > flooding (instead of the default lookup). OK. IMO, this driver should always enable VLANDISC and VLANVERI for all ports, no matter whether under a VLAN-aware bridge or not. But more on that at the end. > > - can it use VLAN information from the packet (with a fallback on the > > port PVID) to determine where to send, and where *not* to send the > > packet? How does this relate to the flooding registers? Is the flood > > mask restricted by the VLAN mask? Is there a default VLAN installed in > > the hardware tables, which is also the PVID of all ports, and all > > ports are members of it? Could you implement standalone/bridged port > > forwarding isolation based on VLANs, rather than the flimsy and most > > likely buggy implementation done based on flooding domains, from this > > patch set? > > Yes, the VLAN membership is used for packet flooding. The flooding > registers are used when the packets come has a src MAC that is not in s/src/destination/ > the FDB. For more infiormation, see section 4.5.3.9, paragraph 3.c > which describe the whole lookup process. Ok, got it. So UCAST_DEFAULT_MASK/MCAST_DEFAULT_MASK/BCAST_DEFAULT_MASK are only used for flooding, if the packet doesn't see any hit in the VLAN resolution table. And, I guess, if BIT(port) is unset in VLAN_IN_MODE_ENA, then untagged packets will not see any hit in the VLAN resolution table. But, if VLAN_IN_MODE_ENA contains BIT(port) and VLAN_IN_MODE is set to, say, TAG_ALWAYS for BIT(port), then all frames (including untagged frames) will get encapsulated in the VLAN from SYSTEM_TAGINFO[port]. In that case, the packets will always hit the VLAN resolution table (assuming that the VID from $SYSTEM_TAGINFO[port] was installed there), and the UCAST_DEFAULT_MASK/MCAST_DEFAULT_MASK/BCAST_DEFAULT_MASK flooding masks are never used for traffic coming from this port; but rather, only the VLAN resolution table decides the destination ports. Did I get this right? > Regarding your other question, by default, there is no default VLAN > installed but indeed, I see what you mean, a default VLAN could be used > to isolate each ports rather than setting the rule to forward only to > root CPU port + disabling of flooding. I guess a unique VLAN ID per port > should be used to isolate each of them and added to the root port to > untag the input frames tagged with the PVID ? For example, hellcreek_setup_vlan_membership() does something like this already. But your switch only has 32 VLANs. > > - is the FDB looked up per {MAC DA, VLAN ID} or just MAC DA? Looking at > > a5psw_port_fdb_add(), there's absolutely no sign of "vid" being used, > > so I guess it's Shared VLAN Learning. In that case, there's absolutely > > no hope to implement ds->fdb_isolation for this hardware. But at the > > *very* least, please disable address learning on standalone ports, > > *and* implement ds->ops->port_fast_age() so that ports quickly forget > > their learned MAC adddresses after leaving a bridge and become > > standalone again. > > Indeed, the lookup table does not contain the VLAN ID and thus it is > unused. We talked about it in a previous review and you already > mentionned that there is no hope to implement fdb_isolation. Yes, I vaguely remember. In any case, absolutely horrible, and let me explain why. AFAIU from the documentation, the (VLAN-unaware) MAC Address Lookup table always decides where the packet should go, if there is a MAC DA hit. Whereas the VLAN Resolution table decides if the packet can go there. The problem is that setups like this will not work for the a5psw: ___ br0__ / | \ / | \ (software) bond0 | \ / \ | | swp0 swp1 swp2 swp3 | | | | | | station A station B DSA has logic to support bond0 as an unoffloaded bridge port (swp0 and swp1 are standalone and pass all traffic just to/from the CPU port), in the same bridging domain with swp2 and swp3, which do offload the bridging process. Assume station B wants to ping station A. swp3 learns the MAC SA (station B's MAC address) from the ICMP request as a FDB entry towards swp3. The MAC DA for the packet is unknown, so it is flooded to swp2 and to the CPU port. From there, the software bridge delivers the packet to bond0, which delivers it to swp0, and it reaches station A. Station A sends an ICMP reply to station B's MAC DA. When swp0 receives this packet, the MAC Address Lookup table finds an FDB entry saying that the packet should go to swp3. But the VLAN Resolution table says that swp3 is unreachable from swp0. So, the packet is dropped. There is simply no way this can work if the MAC Address Lookup table is VLAN-unaware. What should have happened is that swp0 should have not been able to find the FDB entry towards swp3, because swp0 is standalone, and swp3 is under a bridge. Hmm, this makes me want to go to dsa_slave_changeupper() and to disable all the "Offloading not supported" fallback code paths, unless ds->fdb_isolation is set to true, so that people don't run into this pitfall. However, only the drivers that I maintain have FDB isolation, so that would disable the fallback for a lot of people :( > Ok for disabling learning on standalone ports, and indeed, by default, > it's enabled. Okay. Disabling address learning on standalone ports should help with some use cases, like when all ports are standalone and there is no bridging offload. > Regarding ds->ops->port_fast_age(), it is already implemented. Sorry, I didn't notice that. > The port PVID itself is not used to filter the flooding mask. But each > time a PVID is set, the port must also be programmed as a membership of > the PVID VLAN ID in the VLAN resolution table. So actually, the PVID is > just here to tag (or not) the input packet, it does not take a role in > packet forwading. This is entirely done by the VLAN resolution table > content (VLAN_RES_TABLE register). I think I've understood that now, finally. > Does this means I don't have to be extra careful when programming it ? Actually, no :) you still do. What I don't think will work in your current setup of the hardware is this: br0 (standalone) | | swp0 swp1 ip link add br0 type bridge vlan_filtering 1 && ip link set br0 up ip link set swp0 master br0 && ip link set swp0 up bridge vlan add dev swp0 vid 100 bridge fdb add 00:01:02:03:04:05 dev swp0 master static and then connect a station to swp1 and send a packet with { MAC DA 00:01:02:03:04:05, VID 100 }. It should only reach the CPU port of the switch, but it also leaks to swp0, am I right? I'm saying this because the standalone swp1 has vlan_filtering 0, so in the VLAN_VERIFY register, VLANVERI is 0 for swp1 (packets with VID 100 are accepted even if in the VLAN table, swp1 isn't a member of VID 100). [ hmm, if I'm correct about this, then I see that this situation isn't covered in tools/testing/selftests/drivers/net/dsa/no_forwarding.sh. Maybe we should add another entry to the selftest, for the "leak via FDB entry" case ] This creates the very awkward situation where you have do the hard work and do everything exactly right (to avoid forwarding domain leaks), as if you were shooting for ds->fdb_isolation = true, but still do not get ds->fdb_isolation = true in the end (because the MAC table is VLAN unaware and there's nothing you can do about that). So, the software bonding scenario won't work, but at least it will result in packet drops (or, ideally, would be denied), and not in packet leaks. That's about the best scenario we're aiming for. So frustrating. I think the UCAST_DEFAULT_MASK/MCAST_DEFAULT_MASK/BCAST_DEFAULT_MASK flooding destination masks are useless, because they are not keyed per source port, but global. This means that you need to be extraordinarily careful when you enable any port in these masks, because packets from literally any other port, which were untagged and didn't hit an entry in the MAC table, can reach there. To avoid forwarding leaks, I guess that: - each standalone port should be in a single VLAN with just the CPU port. This would be achieved by setting A5PSW_VLAN_IN_MODE_ENA=true to enable VLAN input manipulation, and to set SYSTEM_TAGINFO[port] to unique values per standalone port, together with VLAN_IN_MODE = "always" and VLAN_OUT_MODE = "tag through" (to encapsulate all traffic in the private port PVID, SYSTEM_TAGINFO, on ingress, and to decapsulate it on egress). Then, care must be taken that the values chosen for SYSTEM_TAGINFO[port] are reserved, and packets coming from other ports are never able to be classified to the same VLANs. - each port under a bridge which is currently VLAN-unaware should use the same technique as for standalone ports, which is to set SYSTEM_TAGINFO[port] to a reserved value, common for all ports under the same bridge. That value can even be the standalone PVID of the first port that joined the VLAN-unaware bridge. This way, you would need to reserve no more than 4 VLANs, and you would keep reusing them also for VLAN-unaware bridging. - each port under a VLAN-aware bridge should set its SYSTEM_TAGINFO[port] to the switchdev VLAN which has the BRIDGE_VLAN_INFO_PVID flag. If you reserve VLAN IDs of 4095, 4094, 4093, 4092 as special values to configure to SYSTEM_TAGINFO[port] when VLAN-unaware, then you must also reject port_vlan_add() of these VLANs, and you must ensure, using the VLAN Domain Verification function, that an "attacker" cannot sneak crafted packets through VLAN-aware bridge ports so that they are processed by the switch as if they were received on another ports. However, I do appreciate that 32 VLANs is not a lot, and that cropping 4 of them is already 12.5%. The hardware designers probably didn't intend the switch to be used like that. Would it be possible to hack the 802.1X functionality of this switch such as to configure all standalone ports to "require authentication"? IIUC, that would mean that all traffic received on these ports is delivered by the switch straight to the management port, and it would bypass even the MAC table lookup, which would be good considering the software bonding use case, for example. It would also mean that you don't need to allocate one private SYSTEM_TAGINFO value per port.
Le Sat, 25 Mar 2023 00:00:42 +0200, Vladimir Oltean <olteanv@gmail.com> a écrit : > Hi Clément, > > I'm very sorry for the delay. No worries ! > > On Wed, Mar 15, 2023 at 03:54:30PM +0100, Clément Léger wrote: > > The documentation is public and available at [1]. Section 4.5.3 is of > > interest for your understanding of the VLAN filtering support. Let's > > hope I answered most of your questions. > > > > [1] > > https://www.renesas.com/us/en/document/mah/rzn1d-group-rzn1s-group-rzn1l-group-users-manual-r-engine-and-ethernet-peripherals?r=1054561 > > Yes, indeed, it appears that you gave me this link before and I already > had the PDF downloaded, but forgot about it... I've reorganized my > documentation PDFs since then. That's ok, I understand this not the only IP you need to review ;) > > > > - can it drop a VLAN which isn't present in the port membership list? > > > I guess this is what A5PSW_VLAN_DISC_SHIFT does. > > > > Yes, A5PSW_VLAN_DISC_SHIFT stands for "discard" which means the packet > > is discarded if the port is not a member of the VLAN. > > A5PSW_VLAN_VERI_SHIFT is meant to enable VLAN lookup for packet > > flooding (instead of the default lookup). > > OK. IMO, this driver should always enable VLANDISC and VLANVERI for all > ports, no matter whether under a VLAN-aware bridge or not. But more on > that at the end. > > > > - can it use VLAN information from the packet (with a fallback on the > > > port PVID) to determine where to send, and where *not* to send the > > > packet? How does this relate to the flooding registers? Is the flood > > > mask restricted by the VLAN mask? Is there a default VLAN installed in > > > the hardware tables, which is also the PVID of all ports, and all > > > ports are members of it? Could you implement standalone/bridged port > > > forwarding isolation based on VLANs, rather than the flimsy and most > > > likely buggy implementation done based on flooding domains, from this > > > patch set? > > > > Yes, the VLAN membership is used for packet flooding. The flooding > > registers are used when the packets come has a src MAC that is not in > > s/src/destination/ > > > the FDB. For more infiormation, see section 4.5.3.9, paragraph 3.c > > which describe the whole lookup process. > > Ok, got it. So UCAST_DEFAULT_MASK/MCAST_DEFAULT_MASK/BCAST_DEFAULT_MASK > are only used for flooding, if the packet doesn't see any hit in the > VLAN resolution table. Yes exactly (at least that is why the documentation describes). > > And, I guess, if BIT(port) is unset in VLAN_IN_MODE_ENA, then untagged > packets will not see any hit in the VLAN resolution table. > But, if VLAN_IN_MODE_ENA contains BIT(port) and VLAN_IN_MODE is set to, > say, TAG_ALWAYS for BIT(port), then all frames (including untagged > frames) will get encapsulated in the VLAN from SYSTEM_TAGINFO[port]. > In that case, the packets will always hit the VLAN resolution table > (assuming that the VID from $SYSTEM_TAGINFO[port] was installed there), Yes, indeed and when adding a PVID, the documentation states that the port must also be a member of the VLAN ID when vlan verification is enabled: In addition, if VLAN verification is enabled for a port (see Section 4.4.5, VLAN_VERIFY — Verify VLAN Domain), the VLAN id used for insertion (SYSTEM_TAGINFO[n]) must also be configured in the global VLAN resolution table (see Section 4.4.51, VLAN_RES_TABLE[n] — 32 VLAN Domain Entries (n = 0..31)), to ensure the switch accepts frames, which contain the inserted tag. > and the UCAST_DEFAULT_MASK/MCAST_DEFAULT_MASK/BCAST_DEFAULT_MASK > flooding masks are never used for traffic coming from this port; but > rather, only the VLAN resolution table decides the destination ports. > > Did I get this right? Yes I think so. > > > Regarding your other question, by default, there is no default VLAN > > installed but indeed, I see what you mean, a default VLAN could be used > > to isolate each ports rather than setting the rule to forward only to > > root CPU port + disabling of flooding. I guess a unique VLAN ID per port > > should be used to isolate each of them and added to the root port to > > untag the input frames tagged with the PVID ? > > For example, hellcreek_setup_vlan_membership() does something like this > already. But your switch only has 32 VLANs. > > > > - is the FDB looked up per {MAC DA, VLAN ID} or just MAC DA? Looking at > > > a5psw_port_fdb_add(), there's absolutely no sign of "vid" being used, > > > so I guess it's Shared VLAN Learning. In that case, there's absolutely > > > no hope to implement ds->fdb_isolation for this hardware. But at the > > > *very* least, please disable address learning on standalone ports, > > > *and* implement ds->ops->port_fast_age() so that ports quickly forget > > > their learned MAC adddresses after leaving a bridge and become > > > standalone again. > > > > Indeed, the lookup table does not contain the VLAN ID and thus it is > > unused. We talked about it in a previous review and you already > > mentionned that there is no hope to implement fdb_isolation. > > Yes, I vaguely remember. In any case, absolutely horrible, and let me > explain why. > > AFAIU from the documentation, the (VLAN-unaware) MAC Address Lookup table > always decides where the packet should go, if there is a MAC DA hit. > Whereas the VLAN Resolution table decides if the packet can go there. > > The problem is that setups like this will not work for the a5psw: > > ___ br0__ > / | \ > / | \ > (software) bond0 | \ > / \ | | > swp0 swp1 swp2 swp3 > | | > | | > | | > station A station B > > DSA has logic to support bond0 as an unoffloaded bridge port (swp0 and > swp1 are standalone and pass all traffic just to/from the CPU port), in > the same bridging domain with swp2 and swp3, which do offload the > bridging process. > > Assume station B wants to ping station A. > > swp3 learns the MAC SA (station B's MAC address) from the ICMP request > as a FDB entry towards swp3. The MAC DA for the packet is unknown, so it > is flooded to swp2 and to the CPU port. From there, the software bridge > delivers the packet to bond0, which delivers it to swp0, and it reaches > station A. > > Station A sends an ICMP reply to station B's MAC DA. > > When swp0 receives this packet, the MAC Address Lookup table finds an > FDB entry saying that the packet should go to swp3. But the VLAN > Resolution table says that swp3 is unreachable from swp0. So, the packet > is dropped. > > There is simply no way this can work if the MAC Address Lookup table is > VLAN-unaware. What should have happened is that swp0 should have not > been able to find the FDB entry towards swp3, because swp0 is standalone, > and swp3 is under a bridge. Ok got it ! > > Hmm, this makes me want to go to dsa_slave_changeupper() and to disable > all the "Offloading not supported" fallback code paths, unless > ds->fdb_isolation is set to true, so that people don't run into this > pitfall. However, only the drivers that I maintain have FDB isolation, > so that would disable the fallback for a lot of people :( Hum indeed, that would be nice to have a way to forbid that on switches that have a vlan-unaware fdb (probably not so common though). > > > Ok for disabling learning on standalone ports, and indeed, by default, > > it's enabled. > > Okay. Disabling address learning on standalone ports should help with > some use cases, like when all ports are standalone and there is no > bridging offload. Based on my previous comment, if I remove standalone ports from the flooding mask, disable learning on them and if the port is fast aged when leaving a bridge, it seems correct to assume this port will never receive nor forward packets from other port and also thanks to the matching rule we set for standalone ports, it will only send packets to CPU port. Based on that I think I can say that the port will be truly standalone. This also allows to keep the full 32 VLANs available for stadnard operations. > > > Regarding ds->ops->port_fast_age(), it is already implemented. > > Sorry, I didn't notice that. > > > The port PVID itself is not used to filter the flooding mask. But each > > time a PVID is set, the port must also be programmed as a membership of > > the PVID VLAN ID in the VLAN resolution table. So actually, the PVID is > > just here to tag (or not) the input packet, it does not take a role in > > packet forwading. This is entirely done by the VLAN resolution table > > content (VLAN_RES_TABLE register). > > I think I've understood that now, finally. > > > Does this means I don't have to be extra careful when programming it ? > > Actually, no :) you still do. > > What I don't think will work in your current setup of the hardware is this: > > br0 (standalone) > | | > swp0 swp1 > > ip link add br0 type bridge vlan_filtering 1 && ip link set br0 up > ip link set swp0 master br0 && ip link set swp0 up > bridge vlan add dev swp0 vid 100 > bridge fdb add 00:01:02:03:04:05 dev swp0 master static > > and then connect a station to swp1 and send a packet with > { MAC DA 00:01:02:03:04:05, VID 100 }. It should only reach the CPU port > of the switch, but it also leaks to swp0, am I right? Actually, it won't leak to swp0 since, since we enable a specific matching rule (MGMTFWD) for the standalone ports which ensure all the lookup is bypassed and that the trafic coming from these ports is only forwarded to the CPU port (see my comment at the end of this mail). > > I'm saying this because the standalone swp1 has vlan_filtering 0, so in > the VLAN_VERIFY register, VLANVERI is 0 for swp1 (packets with VID 100 are > accepted even if in the VLAN table, swp1 isn't a member of VID 100). > > [ hmm, if I'm correct about this, then I see that this situation isn't > covered in tools/testing/selftests/drivers/net/dsa/no_forwarding.sh. > Maybe we should add another entry to the selftest, for the "leak via > FDB entry" case ] > > > This creates the very awkward situation where you have do the hard work > and do everything exactly right (to avoid forwarding domain leaks), as > if you were shooting for ds->fdb_isolation = true, but still do not get > ds->fdb_isolation = true in the end (because the MAC table is VLAN > unaware and there's nothing you can do about that). So, the software > bonding scenario won't work, but at least it will result in packet drops > (or, ideally, would be denied), and not in packet leaks. That's about > the best scenario we're aiming for. So frustrating. > > I think the UCAST_DEFAULT_MASK/MCAST_DEFAULT_MASK/BCAST_DEFAULT_MASK > flooding destination masks are useless, because they are not keyed per > source port, but global. This means that you need to be extraordinarily > careful when you enable any port in these masks, because packets from > literally any other port, which were untagged and didn't hit an entry in > the MAC table, can reach there. > > To avoid forwarding leaks, I guess that: > > - each standalone port should be in a single VLAN with just the CPU > port. This would be achieved by setting A5PSW_VLAN_IN_MODE_ENA=true to > enable VLAN input manipulation, and to set SYSTEM_TAGINFO[port] to > unique values per standalone port, together with VLAN_IN_MODE = > "always" and VLAN_OUT_MODE = "tag through" (to encapsulate all traffic > in the private port PVID, SYSTEM_TAGINFO, on ingress, and to > decapsulate it on egress). Then, care must be taken that the values > chosen for SYSTEM_TAGINFO[port] are reserved, and packets coming from > other ports are never able to be classified to the same VLANs. Yep that is what I envisionned first when writing the driver but thanks to the CPOU forward only rule, it was easier. > > - each port under a bridge which is currently VLAN-unaware should use > the same technique as for standalone ports, which is to set > SYSTEM_TAGINFO[port] to a reserved value, common for all ports under > the same bridge. That value can even be the standalone PVID of the > first port that joined the VLAN-unaware bridge. This way, you would > need to reserve no more than 4 VLANs, and you would keep reusing them > also for VLAN-unaware bridging. However I did not thought about this part :) Indeed makes sense and allows to use only 4 VLAN at most out of the 32s. By the way, this bridge supports only a single bridge due to some registers being common to all ports and not per bridge (flooding for instance...). > > - each port under a VLAN-aware bridge should set its SYSTEM_TAGINFO[port] > to the switchdev VLAN which has the BRIDGE_VLAN_INFO_PVID flag. > > If you reserve VLAN IDs of 4095, 4094, 4093, 4092 as special values to > configure to SYSTEM_TAGINFO[port] when VLAN-unaware, then you must also > reject port_vlan_add() of these VLANs, and you must ensure, using the > VLAN Domain Verification function, that an "attacker" cannot sneak > crafted packets through VLAN-aware bridge ports so that they are > processed by the switch as if they were received on another ports. > > However, I do appreciate that 32 VLANs is not a lot, and that cropping 4 > of them is already 12.5%. The hardware designers probably didn't intend > the switch to be used like that. > > Would it be possible to hack the 802.1X functionality of this switch > such as to configure all standalone ports to "require authentication"? > IIUC, that would mean that all traffic received on these ports is > delivered by the switch straight to the management port, and it would > bypass even the MAC table lookup, which would be good considering the > software bonding use case, for example. It would also mean that you > don't need to allocate one private SYSTEM_TAGINFO value per port. 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. Thanks for your time reviewing and explaining all of that,
On Tue, Mar 28, 2023 at 10:44:29AM +0200, Clément Léger wrote: > > And, I guess, if BIT(port) is unset in VLAN_IN_MODE_ENA, then untagged > > packets will not see any hit in the VLAN resolution table. > > But, if VLAN_IN_MODE_ENA contains BIT(port) and VLAN_IN_MODE is set to, > > say, TAG_ALWAYS for BIT(port), then all frames (including untagged > > frames) will get encapsulated in the VLAN from SYSTEM_TAGINFO[port]. > > In that case, the packets will always hit the VLAN resolution table > > (assuming that the VID from $SYSTEM_TAGINFO[port] was installed there), > > Yes, indeed and when adding a PVID, the documentation states that the > port must also be a member of the VLAN ID when vlan verification is > enabled: > > In addition, if VLAN verification is enabled for a port (see Section > 4.4.5, VLAN_VERIFY — Verify VLAN Domain), the VLAN id used for > insertion (SYSTEM_TAGINFO[n]) must also be configured in the global > VLAN resolution table (see Section 4.4.51, VLAN_RES_TABLE[n] — 32 VLAN > Domain Entries (n = 0..31)), to ensure the switch accepts frames, which > contain the inserted tag. Ok. Is VLAN verification also bypassed by the MGMTFWD mechanism of PATTERN_CTRL, or only the FDB table lookup? Asking for my general knowledge; I don't think the answer will be useful to the current state of the driver. > > There is simply no way this can work if the MAC Address Lookup table is > > VLAN-unaware. What should have happened is that swp0 should have not > > been able to find the FDB entry towards swp3, because swp0 is standalone, > > and swp3 is under a bridge. > > Ok got it ! So after learning about the MGMTFWD action of the pattern matching engine: the case described above should work. Maybe all hope is not lost. Although, small note, MGMTFWD is incompatible with RX filtering (IFF_UNICAST_FLT). Since you tell the switch to send all traffic received on standalone ports to the CPU and bypass the MAC table, then you can no longer tell it which addresses you are interested in seeing, and you cannot use the MAC table as an accelerator to selectively drop them. Interesting hardware design, and interesting how the past few years of changes made to the DSA framework don't seem to help it... > > Okay. Disabling address learning on standalone ports should help with > > some use cases, like when all ports are standalone and there is no > > bridging offload. > > Based on my previous comment, if I remove standalone ports from the > flooding mask, disable learning on them and if the port is fast aged > when leaving a bridge, it seems correct to assume this port will never > receive nor forward packets from other port and also thanks to the > matching rule we set for standalone ports, it will only send packets to > CPU port. Based on that I think I can say that the port will be truly > standalone. This also allows to keep the full 32 VLANs available for > stadnard operations. Seems correct. > > > Does this means I don't have to be extra careful when programming it ? > > > > Actually, no :) you still do. > > > > What I don't think will work in your current setup of the hardware is this: > > > > br0 (standalone) > > | | > > swp0 swp1 > > > > ip link add br0 type bridge vlan_filtering 1 && ip link set br0 up > > ip link set swp0 master br0 && ip link set swp0 up > > bridge vlan add dev swp0 vid 100 > > bridge fdb add 00:01:02:03:04:05 dev swp0 master static > > > > and then connect a station to swp1 and send a packet with > > { MAC DA 00:01:02:03:04:05, VID 100 }. It should only reach the CPU port > > of the switch, but it also leaks to swp0, am I right? > > Actually, it won't leak to swp0 since, since we enable a specific > matching rule (MGMTFWD) for the standalone ports which ensure all the > lookup is bypassed and that the trafic coming from these ports is only > forwarded to the CPU port (see my comment at the end of this mail). I agree, this makes sense. > > I think the UCAST_DEFAULT_MASK/MCAST_DEFAULT_MASK/BCAST_DEFAULT_MASK > > flooding destination masks are useless, because they are not keyed per > > source port, but global. > > > > - each port under a bridge which is currently VLAN-unaware should use > > the same technique as for standalone ports, which is to set > > SYSTEM_TAGINFO[port] to a reserved value, common for all ports under > > the same bridge. That value can even be the standalone PVID of the > > first port that joined the VLAN-unaware bridge. This way, you would > > need to reserve no more than 4 VLANs, and you would keep reusing them > > also for VLAN-unaware bridging. > > However I did not thought about this part :) Indeed makes sense and > allows to use only 4 VLAN at most out of the 32s. By the way, this > bridge supports only a single bridge due to some registers being common > to all ports and not per bridge (flooding for instance...). I searched to see whether it is possible to control the flooding per VLAN, in the off-chance that we decided to support multiple VLAN-unaware bridges by allocating one VLAN per bridge. It looks like VLAN_RES_TABLE[n] doesn't support this. Frames classified to a VLAN which don't hit any entry in the MAC table are flooded to all ports in that VLAN. Strange! I think this might be the actual insurmountable reason why the driver will never get support for multiple bridges. It would be good to even add a comment about this in the next patch set, so that any Renesas hardware design engineers who might be reading will take note. > 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. 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.
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).
On Thu, Mar 30, 2023 at 11:09:59AM +0200, Clément Léger wrote: > 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). I was thinking the other way around, that the handling of VLAN_IN_MODE_ENA should be moved to port_vlan_filtering(). The expected behavior relating to VLANs is documented in the "Bridge VLAN filtering" section of Documentation/networking/switchdev.rst btw.
diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c index 5059b2814cdd..a9a42a8bc7e3 100644 --- a/drivers/net/dsa/rzn1_a5psw.c +++ b/drivers/net/dsa/rzn1_a5psw.c @@ -583,6 +583,144 @@ static int a5psw_port_fdb_dump(struct dsa_switch *ds, int port, return ret; } +static int a5psw_port_vlan_filtering(struct dsa_switch *ds, int port, + bool vlan_filtering, + struct netlink_ext_ack *extack) +{ + u32 mask = BIT(port + A5PSW_VLAN_VERI_SHIFT) | + BIT(port + A5PSW_VLAN_DISC_SHIFT); + u32 val = vlan_filtering ? mask : 0; + struct a5psw *a5psw = ds->priv; + + a5psw_reg_rmw(a5psw, A5PSW_VLAN_VERIFY, mask, val); + + return 0; +} + +static int a5psw_find_vlan_entry(struct a5psw *a5psw, u16 vid) +{ + u32 vlan_res; + int i; + + /* Find vlan for this port */ + for (i = 0; i < A5PSW_VLAN_COUNT; i++) { + vlan_res = a5psw_reg_readl(a5psw, A5PSW_VLAN_RES(i)); + if (FIELD_GET(A5PSW_VLAN_RES_VLANID, vlan_res) == vid) + return i; + } + + return -1; +} + +static int a5psw_new_vlan_res_entry(struct a5psw *a5psw, u16 newvid) +{ + u32 vlan_res; + int i; + + /* Find a free VLAN entry */ + for (i = 0; i < A5PSW_VLAN_COUNT; i++) { + vlan_res = a5psw_reg_readl(a5psw, A5PSW_VLAN_RES(i)); + if (!(FIELD_GET(A5PSW_VLAN_RES_PORTMASK, vlan_res))) { + vlan_res = FIELD_PREP(A5PSW_VLAN_RES_VLANID, newvid); + a5psw_reg_writel(a5psw, A5PSW_VLAN_RES(i), vlan_res); + return i; + } + } + + return -1; +} + +static void a5psw_port_vlan_tagged_cfg(struct a5psw *a5psw, + unsigned int vlan_res_id, int port, + bool set) +{ + u32 mask = A5PSW_VLAN_RES_WR_PORTMASK | A5PSW_VLAN_RES_RD_TAGMASK | + BIT(port); + u32 vlan_res_off = A5PSW_VLAN_RES(vlan_res_id); + u32 val = A5PSW_VLAN_RES_WR_TAGMASK, reg; + + if (set) + val |= BIT(port); + + /* Toggle tag mask read */ + a5psw_reg_writel(a5psw, vlan_res_off, A5PSW_VLAN_RES_RD_TAGMASK); + reg = a5psw_reg_readl(a5psw, vlan_res_off); + a5psw_reg_writel(a5psw, vlan_res_off, A5PSW_VLAN_RES_RD_TAGMASK); + + reg &= ~mask; + reg |= val; + a5psw_reg_writel(a5psw, vlan_res_off, reg); +} + +static void a5psw_port_vlan_cfg(struct a5psw *a5psw, unsigned int vlan_res_id, + int port, bool set) +{ + u32 mask = A5PSW_VLAN_RES_WR_TAGMASK | BIT(port); + u32 reg = A5PSW_VLAN_RES_WR_PORTMASK; + + if (set) + reg |= BIT(port); + + a5psw_reg_rmw(a5psw, A5PSW_VLAN_RES(vlan_res_id), mask, reg); +} + +static int a5psw_port_vlan_add(struct dsa_switch *ds, int port, + const struct switchdev_obj_port_vlan *vlan, + struct netlink_ext_ack *extack) +{ + bool tagged = !(vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED); + bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID; + struct a5psw *a5psw = ds->priv; + u16 vid = vlan->vid; + int vlan_res_id; + + dev_dbg(a5psw->dev, "Add VLAN %d on port %d, %s, %s\n", + vid, port, tagged ? "tagged" : "untagged", + pvid ? "PVID" : "no PVID"); + + vlan_res_id = a5psw_find_vlan_entry(a5psw, vid); + if (vlan_res_id < 0) { + vlan_res_id = a5psw_new_vlan_res_entry(a5psw, vid); + if (vlan_res_id < 0) + return -ENOSPC; + } + + a5psw_port_vlan_cfg(a5psw, vlan_res_id, port, true); + if (tagged) + a5psw_port_vlan_tagged_cfg(a5psw, vlan_res_id, port, true); + + if (pvid) { + a5psw_reg_rmw(a5psw, A5PSW_VLAN_IN_MODE_ENA, BIT(port), + BIT(port)); + a5psw_reg_writel(a5psw, A5PSW_SYSTEM_TAGINFO(port), vid); + } + + return 0; +} + +static int a5psw_port_vlan_del(struct dsa_switch *ds, int port, + const struct switchdev_obj_port_vlan *vlan) +{ + struct a5psw *a5psw = ds->priv; + u16 vid = vlan->vid; + int vlan_res_id; + + dev_dbg(a5psw->dev, "Removing VLAN %d on port %d\n", vid, port); + + vlan_res_id = a5psw_find_vlan_entry(a5psw, vid); + if (vlan_res_id < 0) + return -EINVAL; + + a5psw_port_vlan_cfg(a5psw, vlan_res_id, port, false); + a5psw_port_vlan_tagged_cfg(a5psw, vlan_res_id, port, false); + + /* 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); + + return 0; +} + static u64 a5psw_read_stat(struct a5psw *a5psw, u32 offset, int port) { u32 reg_lo, reg_hi; @@ -700,6 +838,27 @@ static void a5psw_get_eth_ctrl_stats(struct dsa_switch *ds, int port, ctrl_stats->MACControlFramesReceived = stat; } +static void a5psw_vlan_setup(struct a5psw *a5psw, int port) +{ + u32 reg; + + /* Enable TAG always mode for the port, this is actually controlled + * by VLAN_IN_MODE_ENA field which will be used for PVID insertion + */ + reg = A5PSW_VLAN_IN_MODE_TAG_ALWAYS; + reg <<= A5PSW_VLAN_IN_MODE_PORT_SHIFT(port); + a5psw_reg_rmw(a5psw, A5PSW_VLAN_IN_MODE, A5PSW_VLAN_IN_MODE_PORT(port), + reg); + + /* Set transparent mode for output frame manipulation, this will depend + * on the VLAN_RES configuration mode + */ + reg = A5PSW_VLAN_OUT_MODE_TRANSPARENT; + reg <<= A5PSW_VLAN_OUT_MODE_PORT_SHIFT(port); + a5psw_reg_rmw(a5psw, A5PSW_VLAN_OUT_MODE, + A5PSW_VLAN_OUT_MODE_PORT(port), reg); +} + static int a5psw_setup(struct dsa_switch *ds) { struct a5psw *a5psw = ds->priv; @@ -772,6 +931,8 @@ static int a5psw_setup(struct dsa_switch *ds) /* Enable management forward only for user ports */ if (dsa_port_is_user(dp)) a5psw_port_mgmtfwd_set(a5psw, port, true); + + a5psw_vlan_setup(a5psw, port); } return 0; @@ -801,6 +962,9 @@ static const struct dsa_switch_ops a5psw_switch_ops = { .port_bridge_flags = a5psw_port_bridge_flags, .port_stp_state_set = a5psw_port_stp_state_set, .port_fast_age = a5psw_port_fast_age, + .port_vlan_filtering = a5psw_port_vlan_filtering, + .port_vlan_add = a5psw_port_vlan_add, + .port_vlan_del = a5psw_port_vlan_del, .port_fdb_add = a5psw_port_fdb_add, .port_fdb_del = a5psw_port_fdb_del, .port_fdb_dump = a5psw_port_fdb_dump, diff --git a/drivers/net/dsa/rzn1_a5psw.h b/drivers/net/dsa/rzn1_a5psw.h index c67abd49c013..2bad2e3edc2a 100644 --- a/drivers/net/dsa/rzn1_a5psw.h +++ b/drivers/net/dsa/rzn1_a5psw.h @@ -50,7 +50,9 @@ #define A5PSW_VLAN_IN_MODE_TAG_ALWAYS 0x2 #define A5PSW_VLAN_OUT_MODE 0x2C -#define A5PSW_VLAN_OUT_MODE_PORT(port) (GENMASK(1, 0) << ((port) * 2)) +#define A5PSW_VLAN_OUT_MODE_PORT_SHIFT(port) ((port) * 2) +#define A5PSW_VLAN_OUT_MODE_PORT(port) (GENMASK(1, 0) << \ + A5PSW_VLAN_OUT_MODE_PORT_SHIFT(port)) #define A5PSW_VLAN_OUT_MODE_DIS 0x0 #define A5PSW_VLAN_OUT_MODE_STRIP 0x1 #define A5PSW_VLAN_OUT_MODE_TAG_THROUGH 0x2 @@ -59,7 +61,7 @@ #define A5PSW_VLAN_IN_MODE_ENA 0x30 #define A5PSW_VLAN_TAG_ID 0x34 -#define A5PSW_SYSTEM_TAGINFO(port) (0x200 + A5PSW_PORT_OFFSET(port)) +#define A5PSW_SYSTEM_TAGINFO(port) (0x200 + 4 * (port)) #define A5PSW_AUTH_PORT(port) (0x240 + 4 * (port)) #define A5PSW_AUTH_PORT_AUTHORIZED BIT(0) @@ -68,7 +70,7 @@ #define A5PSW_VLAN_RES_WR_PORTMASK BIT(30) #define A5PSW_VLAN_RES_WR_TAGMASK BIT(29) #define A5PSW_VLAN_RES_RD_TAGMASK BIT(28) -#define A5PSW_VLAN_RES_ID GENMASK(16, 5) +#define A5PSW_VLAN_RES_VLANID GENMASK(16, 5) #define A5PSW_VLAN_RES_PORTMASK GENMASK(4, 0) #define A5PSW_RXMATCH_CONFIG(port) (0x3e80 + 4 * (port))
Add support for vlan operation (add, del, filtering) on the RZN1 driver. The a5psw switch supports up to 32 VLAN IDs with filtering, tagged/untagged VLANs and PVID for each ports. Signed-off-by: Clément Léger <clement.leger@bootlin.com> --- drivers/net/dsa/rzn1_a5psw.c | 164 +++++++++++++++++++++++++++++++++++ drivers/net/dsa/rzn1_a5psw.h | 8 +- 2 files changed, 169 insertions(+), 3 deletions(-)