| Message ID | 20210120063019.1989081-1-paweldembicki@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | dsa: vsc73xx: add support for vlan filtering | expand |
Hi Pawel, On Wed, Jan 20, 2021 at 07:30:18AM +0100, Pawel Dembicki wrote: > This patch adds support for vlan filtering in vsc73xx driver. > > After vlan filtering enable, CPU_PORT is configured as trunk, without > non-tagged frames. This allows to avoid problems with transmit untagged > frames because vsc73xx is DSA_TAG_PROTO_NONE. > > Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com> What are the issues that are preventing you from getting rid of DSA_TAG_PROTO_NONE? Not saying that making the driver VLAN aware is a bad idea, but maybe also adding a tagging driver should really be the path going forward. If there are hardware issues surrounding the native tagging support, then DSA can make use of your VLAN features by transforming them into a software-defined tagger, see net/dsa/tag_8021q.c. But using a trunk CPU port with 8021q uppers on top of the DSA master is a poor job of achieving that. > --- > +static int > +vsc73xx_port_read_vlan_table_entry(struct dsa_switch *ds, u16 vid, u8 *portmap) > +{ > + struct vsc73xx *vsc = ds->priv; > + u32 val; > + int ret; > + > + if (vid > 4095) > + return -EPERM; This is a paranoid check and should be removed (not only here but everywhere). > +static int vsc73xx_port_vlan_prepare(struct dsa_switch *ds, int port, > + const struct switchdev_obj_port_vlan *vlan) > +{ > + /* nothing needed */ > + return 0; > +} Can you please rebase your work on top of the net-next/master branch? You will see that the API has changed. https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git > + > +static void vsc73xx_port_vlan_add(struct dsa_switch *ds, int port, > + const struct switchdev_obj_port_vlan *vlan) > +{ > + bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED; > + bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID; > + struct vsc73xx *vsc = ds->priv; > + int ret; > + u32 tmp; > + > + if (!dsa_port_is_vlan_filtering(dsa_to_port(ds, port))) > + return; Sorry, but no. You need to support the case where the bridge (or 8021q module) adds a VLAN even when the port is not enforcing VLAN filtering. See commit: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=0ee2af4ebbe3c4364429859acd571018ebfb3424 > + > + ret = vsc73xx_port_update_vlan_table(ds, port, vlan->vid_begin, > + vlan->vid_end, 1); > + if (ret) > + return; > + > + if (untagged && port != CPU_PORT) { > + /* VSC73xx can have only one untagged vid per port. */ > + vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, > + VSC73XX_TXUPDCFG, &tmp); > + > + if (tmp & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA) > + dev_warn(vsc->dev, > + "Chip support only one untagged VID per port. Overwriting...\n"); Just return an error, don't overwrite, this leaves the bridge VLAN information out of sync with the hardware otherwise, which is not a great idea. FWIW the drivers/net/dsa/ocelot/felix.c and drivers/net/mscc/ocelot.c files support switching chips from the same vendor. The VSC73XX family is much older, but some of the limitations apply to both architectures nonetheless (like this one), you can surely borrow some ideas from ocelot - in this case search for ocelot_vlan_prepare. > + > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, > + VSC73XX_TXUPDCFG, > + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID, > + (vlan->vid_end << > + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT) & > + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID); > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, > + VSC73XX_TXUPDCFG, > + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA, > + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA); > + } > + if (pvid && port != CPU_PORT) { > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, > + VSC73XX_CAT_DROP, > + VSC73XX_CAT_DROP_UNTAGGED_ENA, > + ~VSC73XX_CAT_DROP_UNTAGGED_ENA); > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, > + VSC73XX_CAT_PORT_VLAN, > + VSC73XX_CAT_PORT_VLAN_VLAN_VID, > + vlan->vid_end & > + VSC73XX_CAT_PORT_VLAN_VLAN_VID); > + } > +}
On Thu, Jan 21, 2021 at 11:45 PM Vladimir Oltean <olteanv@gmail.com> wrote: > On Wed, Jan 20, 2021 at 07:30:18AM +0100, Pawel Dembicki wrote: > > This patch adds support for vlan filtering in vsc73xx driver. > > > > After vlan filtering enable, CPU_PORT is configured as trunk, without > > non-tagged frames. This allows to avoid problems with transmit untagged > > frames because vsc73xx is DSA_TAG_PROTO_NONE. > > > > Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com> > > What are the issues that are preventing you from getting rid of > DSA_TAG_PROTO_NONE? Not saying that making the driver VLAN aware is a > bad idea, but maybe also adding a tagging driver should really be the > path going forward. This is due to the internal architecture of the switch, while it does have an internal tagging format, this is stripped off before letting it exit through the CPU port, and tagged on by the hardware whenever the CPU transmits something. So these tags are invisible to the CPU. Itr would be neat if there was some bit in the switch we could flick and then the internal tagging format would come out on the CPU port, but sadly this does not exist. The vendors idea is that the switch should be programmed internally as it contains an 8051 processor that can indeed see the internal tags. This makes a lot of sense when the chips are used for a hardware switch, i.e. a box with several ethernet ports on it. Sadly it is not very well adopted for the usecase of smart operating system like linux hogging into the CPU port and using it as a managed switch. :/ We currently have the 8051 processor in the switch disabled. Yours, Linus Walleij
On Mon, Jan 25, 2021 at 12:19:55AM +0100, Linus Walleij wrote: > This is due to the internal architecture of the switch, while it does > have an internal tagging format, this is stripped off before letting > it exit through the CPU port, and tagged on by the hardware > whenever the CPU transmits something. So these tags are > invisible to the CPU. > > Itr would be neat if there was some bit in the switch we could > flick and then the internal tagging format would come out on > the CPU port, but sadly this does not exist. > > The vendors idea is that the switch should be programmed > internally as it contains an 8051 processor that can indeed see > the internal tags. This makes a lot of sense when the chips are > used for a hardware switch, i.e. a box with several ethernet ports > on it. Sadly it is not very well adopted for the usecase of smart > operating system like linux hogging into the CPU port and > using it as a managed switch. :/ > > We currently have the 8051 processor in the switch disabled. The sad part of me not having access to any Sparx-G5e documentation other than product briefs is that I can't actually be fully convinced that this is true without seeing it. Other Vitesse switches support DSA tagging towards an external CPU, so if these ones don't, the Node Processor Interface feature must have been added later. Anyhow, you did not approve or disprove the tag_8021q idea. With VLAN trunking on the CPU port, how would per-port traffic be managed? Would it be compatible with hardware-accelerated bridging (which this driver still does not support)?
czw., 21 sty 2021 o 23:45 Vladimir Oltean <olteanv@gmail.com> napisał(a): > > Hi Pawel, > Hi Vladimir, Thank You for Your answer. > On Wed, Jan 20, 2021 at 07:30:18AM +0100, Pawel Dembicki wrote: > > This patch adds support for vlan filtering in vsc73xx driver. > > > > After vlan filtering enable, CPU_PORT is configured as trunk, without > > non-tagged frames. This allows to avoid problems with transmit untagged > > frames because vsc73xx is DSA_TAG_PROTO_NONE. > > > > Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com> > > What are the issues that are preventing you from getting rid of > DSA_TAG_PROTO_NONE? Not saying that making the driver VLAN aware is a > bad idea, but maybe also adding a tagging driver should really be the > path going forward. If there are hardware issues surrounding the native > tagging support, then DSA can make use of your VLAN features by > transforming them into a software-defined tagger, see > net/dsa/tag_8021q.c. But using a trunk CPU port with 8021q uppers on top > of the DSA master is a poor job of achieving that. > I was planning to prepare tagging for the next step. Without VLAN filtering and/or tagging it is usable only as a full bridge. Vsc73xx devices support QinQ. I can use double tagging for port separation, but then it's impossible to filter vlans. So, I'm planning to start working with tagging based on net/dsa/tag_8021q.c. Should I wait with this patch and send the corrected version with tagging support? > > --- > > +static int > > +vsc73xx_port_read_vlan_table_entry(struct dsa_switch *ds, u16 vid, u8 *portmap) > > +{ > > + struct vsc73xx *vsc = ds->priv; > > + u32 val; > > + int ret; > > + > > + if (vid > 4095) > > + return -EPERM; > > This is a paranoid check and should be removed (not only here but > everywhere). > > > +static int vsc73xx_port_vlan_prepare(struct dsa_switch *ds, int port, > > + const struct switchdev_obj_port_vlan *vlan) > > +{ > > + /* nothing needed */ > > + return 0; > > +} > > Can you please rebase your work on top of the net-next/master branch? > You will see that the API has changed. > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git > > > + > > +static void vsc73xx_port_vlan_add(struct dsa_switch *ds, int port, > > + const struct switchdev_obj_port_vlan *vlan) > > +{ > > + bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED; > > + bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID; > > + struct vsc73xx *vsc = ds->priv; > > + int ret; > > + u32 tmp; > > + > > + if (!dsa_port_is_vlan_filtering(dsa_to_port(ds, port))) > > + return; > > Sorry, but no. You need to support the case where the bridge (or 8021q > module) adds a VLAN even when the port is not enforcing VLAN filtering. > See commit: > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=0ee2af4ebbe3c4364429859acd571018ebfb3424 > > > + > > + ret = vsc73xx_port_update_vlan_table(ds, port, vlan->vid_begin, > > + vlan->vid_end, 1); > > + if (ret) > > + return; > > + > > + if (untagged && port != CPU_PORT) { > > + /* VSC73xx can have only one untagged vid per port. */ > > + vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, > > + VSC73XX_TXUPDCFG, &tmp); > > + > > + if (tmp & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA) > > + dev_warn(vsc->dev, > > + "Chip support only one untagged VID per port. Overwriting...\n"); > > Just return an error, don't overwrite, this leaves the bridge VLAN > information out of sync with the hardware otherwise, which is not a > great idea. > But it's a void return function. It always will be out of sync after the second untagged VID attemption. Should I give warning without changing untagged vlan? > FWIW the drivers/net/dsa/ocelot/felix.c and drivers/net/mscc/ocelot.c > files support switching chips from the same vendor. The VSC73XX family > is much older, but some of the limitations apply to both architectures > nonetheless (like this one), you can surely borrow some ideas from > ocelot - in this case search for ocelot_vlan_prepare. > As Linus said, It's impossible because vsc73xx doesn't support tags for external cpu via rgmii or regular port. This chip is very limited. > > + > > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, > > + VSC73XX_TXUPDCFG, > > + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID, > > + (vlan->vid_end << > > + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT) & > > + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID); > > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, > > + VSC73XX_TXUPDCFG, > > + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA, > > + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA); > > + } > > + if (pvid && port != CPU_PORT) { > > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, > > + VSC73XX_CAT_DROP, > > + VSC73XX_CAT_DROP_UNTAGGED_ENA, > > + ~VSC73XX_CAT_DROP_UNTAGGED_ENA); > > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, > > + VSC73XX_CAT_PORT_VLAN, > > + VSC73XX_CAT_PORT_VLAN_VLAN_VID, > > + vlan->vid_end & > > + VSC73XX_CAT_PORT_VLAN_VLAN_VID); > > + } > > +} Best Regards, Pawel Dembicki
On Mon, Jan 25, 2021 at 12:45 AM Vladimir Oltean <olteanv@gmail.com> wrote: > Anyhow, you did not approve or disprove the tag_8021q idea. I just don't understand it well enough so I didn't know what to say about that... > With VLAN trunking on the CPU port, how would per-port traffic be > managed? Would it be compatible with hardware-accelerated bridging > (which this driver still does not support)? I think in vendor code it is done like it is done on the RTL8366RB: one VLAN per port, so the filtering inside the switch isolate the ports by assigning them one VLAN each and the PTAG of the packets make sure they can only reach the desired port. (+/- my half-assed knowledge of how VLAN actually works, one day I think I understand it, next day I don't understand it anymore) Yours, Linus Walleij
Hi Pawel, On Mon, Jan 25, 2021 at 08:17:04AM +0100, Paweł Dembicki wrote: > > What are the issues that are preventing you from getting rid of > > DSA_TAG_PROTO_NONE? Not saying that making the driver VLAN aware is a > > bad idea, but maybe also adding a tagging driver should really be the > > path going forward. If there are hardware issues surrounding the native > > tagging support, then DSA can make use of your VLAN features by > > transforming them into a software-defined tagger, see > > net/dsa/tag_8021q.c. But using a trunk CPU port with 8021q uppers on top > > of the DSA master is a poor job of achieving that. > > I was planning to prepare tagging for the next step. Without VLAN > filtering and/or tagging it is usable only as a full bridge. > Vsc73xx devices support QinQ. I can use double tagging for port > separation, but then it's impossible to filter vlans. > So, I'm planning to start working with tagging based on > net/dsa/tag_8021q.c. Should I wait with this patch and send the > corrected version with tagging support? I was going to send a patch some time to the DSA documentation, maybe it clarifies some of your options. Dealing with switches without hardware support for DSA tags =========================================================== DSA registers a network interface for each user port, and the tagging protocol allows the network stack to redirect Ethernet frames sent and received by the host port into Ethernet frames sent and received by each individual switch port. In lack of a tagging protocol, DSA will register network interfaces that are incapable of sending or receiving traffic, and are 'dead' from the perspective of a user space process trying to open a socket on them. This should be avoided as much as possible for new drivers. Individual addressing of switch ports is a necessity for: - supporting the default configuration where ports operate as standalone with no bridging offload - implementing link-layer protocols that send PDUs (STP, IEEE 1588, etc) when the ports offload the Linux bridge In lack of hardware support for a tagging protocol, the switch can be configured in other ways that uniquely mark the frames sent to the CPU with source port information (and steer them towards the correct destination port on transmission). There is no standardized methodology for this approach, since it depends highly on what the hardware is able to do. The most basic approach is to assign each port a unique pvid (port-based VLAN, i.e. the VLAN ID that a bridge classifies untagged traffic to) and make the CPU port a VLAN trunk. The benefit is that each port will now be isolated from every other port in terms of forwarding, so unique pvids per port can also be used to implement the standalone ports mode. This is also the drawback: when offloading the Linux bridge, forwarding will not be allowed between ports because of the VLAN restriction imposed by the unique pvids. A workaround to this is to add all the other ports that are members in the same bridge into the VLAN membership of this port's pvid, but care must be taken such that the switch is configured to operate in shared VLAN learning mode (i.e. FDB lookup is performed ignoring the VLAN ID, just looking at the destination MAC). There are more drawbacks to the basic approach: (a) it would work only for VLAN-untagged traffic. VLAN tagged traffic would be classified by the hardware to the VID derived from the VLAN header, if that is present, unless the port is configured in VLAN-unaware mode. (b) if the switch port is configured in VLAN-unaware mode, it cannot offload a Linux bridge with vlan_filtering=1, i.e. it cannot use its hardware VLAN support for the bridge VLANs. Further refinements to the basic approach are possible. VLAN retagging rules can be used, which match on an ingress port equal to the front panel port, an egress port equal to the CPU port, a VLAN ID equal to the ingress VLAN ID, and which produce as a result a packet tagged with a remapped VLAN ID that encodes not only the original VLAN, but also the source port information. On transmission, VLAN-tagged traffic is basically not a problem because most hardware is happy to transmit double-tagged traffic. The drawbacks with VLAN retagging are usually related to the limited number of retagging rules (which means that less than the full 4096 VLAN ID range can typically be installed per port) and, more importantly, the fact that encoding the source port information in the 12-bit VLAN ID sent to the CPU will consume bits that are used to encode the number of the original VLAN ID. With enhancements to the Linux bridge code and to DSA, this limitation can be somewhat alleviated by not adding a VLAN retagging rule for every VLAN ID inserted into the hardware database, but just for those VLAN IDs that are also present in the software VLAN database (installed with "bridge vlan add dev br0 self"). The other VLANs will just allow autonomous forwarding and no packet delivery to the CPU, which works because a VLAN-aware bridge would drop VLAN-tagged packets if their VLAN ID is not present in the software database anyway. In short, the basic approach relies on port VLAN membership, and unless VLAN retagging is used, means that a choice needs to be made regarding whether the offload for a VLAN-aware bridge is to be supported, or DSA tagging is to be supported. With even more enhancements to the bridge and DSA data path, the source port information might not even matter for the network stack when the port is bridged, since the packet will end up in the data path of the bridge anyway, regardless of which bridged port the packet came in through (a notable exception to this is link-local traffic like bridge PDUs - some switches treat link-local packets differently than normal data ones). On transmission, the imprecise steering to the correct egress port poses further complications, because of the flooding implementation in the Linux bridge: a packet that is to be flooded towards swp0, swp1 and swp2 will be cloned by the bridge once per egress port, and each skb will be individually delivered to each net_device driver for xmit. The bridge does not know that the packet transmission through a DSA driver with no tagging protocol is imprecise, and instead of delivering the packet just towards the requested egress port, that the switch will likely flood the packet. So each packet will end up flooded once by the software bridge, and twice by the hardware bridge. This can be modeled as a TX-side offload for packet flooding, and could be used to prevent the bridge from cloning the packets in the first place, and just deliver them once to a randomly chosen port which is bridged. The enhancements discussed earlier make one assumption: that link-local traffic is to be treated separately, and that the hardware has provisioning for decoding the source port at least for those. In fact, link-local traffic is an issue also on transmission, since BPDUs need to be transmitted by the network stack even towards ports that are in the BLOCKING STP state. A tagging protocol implementation based purely on VLAN port membership will not help with that. A radically different implementation approach based still on VLAN IDs is to make use of packet processing pipelines in the switch that are not coupled with the bridging service, but work independently on a port's ingress and egress pipeline. Typically, these are configured by the user as an offload to tc-flower filters. Identifying the ingress switch port can be achieved by pushing the desired VLAN as a second, outer tag, on egress towards the CPU port. In tc-flower pseudocode it would look like this: $ tc qdisc add dev <cpu-port> clsact $ for eth in swp0 swp1 swp2 swp3; do \ tc filter add dev <cpu-port> egress flower indev ${eth} \ action vlan push id <rxvlan> protocol 802.1ad; \ done Other ways of pushing an outer VLAN, such as based on 802.1ad bridging, are useless because they basically require that the port is VLAN-unaware, which lands us in the same spot as the basic approach with port-based VLAN membership, being unable to offload a VLAN-aware bridge. As for steering traffic injected into the switch from the network stack, it can be done using a combination of a redirect rule that matches on VLAN ID, plus another VLAN pop action chained to it. $ tc qdisc add dev <cpu-port> clsact $ for eth in swp0 swp1 swp2 swp3; do \ tc filter add dev <cpu-port> ingress protocol 802.1Q flower vlan_id <txvlan> action vlan pop \ action mirred egress redirect dev ${eth}; \ done But since DSA does not register network interfaces for the CPU port, this configuration would be impossible for the user to do. So this configuration, if supported by the hardware, should be done privately by the driver. The advantage is that since the packet processing pipeline is not coupled with the bridging service, then a VLAN-aware Linux bridge can still be offloaded. The drawback is that chances are low for a switch to not have hardware support for a tagging protocol, but to support a programmable packet pipeline, which may make the idea impractical. ---------------------------- So there you have it, this is my trainwreck of thoughts, you should think about the use cases you need and choose appropriately. It is unfortunately not easy and you will need to find compromises. Finally, it may be that the switch is not really a DSA switch (you can be certain that this is the case if you can't even decode link-local traffic, or inject link-local traffic into a BLOCKING port) and you are doing it a disservice by treating it like one. Non-DSA switches are devices that have non-Ethernet based methods of sending and receiving packets. > > Can you please rebase your work on top of the net-next/master branch? > > You will see that the API has changed. > > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git > > > > > + > > > +static void vsc73xx_port_vlan_add(struct dsa_switch *ds, int port, > > > + const struct switchdev_obj_port_vlan *vlan) > > > +{ > > > + bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED; > > > + bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID; > > > + struct vsc73xx *vsc = ds->priv; > > > + int ret; > > > + u32 tmp; > > > + > > > + if (!dsa_port_is_vlan_filtering(dsa_to_port(ds, port))) > > > + return; > > > > Sorry, but no. You need to support the case where the bridge (or 8021q > > module) adds a VLAN even when the port is not enforcing VLAN filtering. > > See commit: > > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=0ee2af4ebbe3c4364429859acd571018ebfb3424 > > > > > + > > > + ret = vsc73xx_port_update_vlan_table(ds, port, vlan->vid_begin, > > > + vlan->vid_end, 1); > > > + if (ret) > > > + return; > > > + > > > + if (untagged && port != CPU_PORT) { > > > + /* VSC73xx can have only one untagged vid per port. */ > > > + vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, > > > + VSC73XX_TXUPDCFG, &tmp); > > > + > > > + if (tmp & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA) > > > + dev_warn(vsc->dev, > > > + "Chip support only one untagged VID per port. Overwriting...\n"); > > > > Just return an error, don't overwrite, this leaves the bridge VLAN > > information out of sync with the hardware otherwise, which is not a > > great idea. > > > > But it's a void return function. It always will be out of sync after > the second untagged VID attemption. > Should I give warning without changing untagged vlan? If you had checked the updated API, you'd have noticed that the function no longer returns void.
Hi Pawel, On Thu, Jan 28, 2021 at 02:37:55AM +0200, Vladimir Oltean wrote: > With even more enhancements to the bridge and DSA data path, the source > port information might not even matter for the network stack when the > port is bridged, since the packet will end up in the data path of the > bridge anyway, regardless of which bridged port the packet came in > through (a notable exception to this is link-local traffic like bridge > PDUs - some switches treat link-local packets differently than normal > data ones). On transmission, the imprecise steering to the correct > egress port poses further complications, because of the flooding > implementation in the Linux bridge: a packet that is to be flooded > towards swp0, swp1 and swp2 will be cloned by the bridge once per egress > port, and each skb will be individually delivered to each net_device > driver for xmit. The bridge does not know that the packet transmission > through a DSA driver with no tagging protocol is imprecise, and instead > of delivering the packet just towards the requested egress port, that > the switch will likely flood the packet. So each packet will end up > flooded once by the software bridge, and twice by the hardware bridge. > This can be modeled as a TX-side offload for packet flooding, and could > be used to prevent the bridge from cloning the packets in the first > place, and just deliver them once to a randomly chosen port which is > bridged. Did you make any progress with getting rid of DSA_TAG_PROTO_NONE for vsc73xx? Just FYI, the bridge and DSA enhancement that I was talking about above got accepted and you should be able to make use of it. https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=356ae88f8322066a2cd1aee831b7fb768ff2905c https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=beeee08ca1d432891f9e1f6188eea85ffac68527 Assuming you haven't solved your issue with link-local packets, you should at least be able to configure the switch as follows: - in standalone mode and under a VLAN-unaware bridge: enable Shared VLAN Learning, set VLAN_TCI_IGNORE_ENA to always classify packets to the port-based VLAN and not look at the VLAN headers, reserve the 1024-3071 VID range for tag_8021q, and call dsa_tag_8021q_register(). - under a VLAN-aware bridge: same as the above except enable Independent VLAN Learning and disable VLAN_TCI_IGNORE_ENA. Packets from ports under a VLAN-aware bridge will come tagged with the bridge VLAN, so you can draw inspiration from net/dsa/tag_sja1105.c to see how to perform reception for those (dsa_find_designated_bridge_port_by_vid). This should give you the ability to expose the switch as an STP-incapable bridge accelerator with port isolation and VLAN support, which frankly seems about all that the hardware can offer.
diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c index 19ce4aa0973b..bf805eb9d3a6 100644 --- a/drivers/net/dsa/vitesse-vsc73xx-core.c +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c @@ -39,6 +39,7 @@ #define VSC73XX_BLOCK_SYSTEM 0x7 /* Only subblock 0 */ #define CPU_PORT 6 /* CPU port */ +#define VLAN_TABLE_ATTEMPTS 10 /* MAC Block registers */ #define VSC73XX_MAC_CFG 0x00 @@ -62,6 +63,8 @@ #define VSC73XX_CAT_DROP 0x6e #define VSC73XX_CAT_PR_MISC_L2 0x6f #define VSC73XX_CAT_PR_USR_PRIO 0x75 +#define VSC73XX_CAT_VLAN_MISC 0x79 +#define VSC73XX_CAT_PORT_VLAN 0x7a #define VSC73XX_Q_MISC_CONF 0xdf /* MAC_CFG register bits */ @@ -122,6 +125,17 @@ #define VSC73XX_ADVPORTM_IO_LOOPBACK BIT(1) #define VSC73XX_ADVPORTM_HOST_LOOPBACK BIT(0) +/* TXUPDCFG transmit modify setup bits */ +#define VSC73XX_TXUPDCFG_DSCP_REWR_MODE GENMASK(20, 19) +#define VSC73XX_TXUPDCFG_DSCP_REWR_ENA BIT(18) +#define VSC73XX_TXUPDCFG_TX_INT_TO_USRPRIO_ENA BIT(17) +#define VSC73XX_TXUPDCFG_TX_UNTAGGED_VID GENMASK(15, 4) +#define VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA BIT(3) +#define VSC73XX_TXUPDCFG_TX_UPDATE_CRC_CPU_ENA BIT(1) +#define VSC73XX_TXUPDCFG_TX_INSERT_TAG BIT(0) + +#define VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT 4 + /* CAT_DROP categorizer frame dropping register bits */ #define VSC73XX_CAT_DROP_DROP_MC_SMAC_ENA BIT(6) #define VSC73XX_CAT_DROP_FWD_CTRL_ENA BIT(4) @@ -135,6 +149,15 @@ #define VSC73XX_Q_MISC_CONF_EARLY_TX_512 (1 << 1) #define VSC73XX_Q_MISC_CONF_MAC_PAUSE_MODE BIT(0) +/* CAT_VLAN_MISC categorizer VLAN miscellaneous bits*/ +#define VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA BIT(8) +#define VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA BIT(7) + +/* CAT_PORT_VLAN categorizer port VLAN*/ +#define VSC73XX_CAT_PORT_VLAN_VLAN_CFI BIT(15) +#define VSC73XX_CAT_PORT_VLAN_VLAN_USR_PRIO GENMASK(14, 12) +#define VSC73XX_CAT_PORT_VLAN_VLAN_VID GENMASK(11, 0) + /* Frame analyzer block 2 registers */ #define VSC73XX_STORMLIMIT 0x02 #define VSC73XX_ADVLEARN 0x03 @@ -185,7 +208,8 @@ #define VSC73XX_VLANACCESS_VLAN_MIRROR BIT(29) #define VSC73XX_VLANACCESS_VLAN_SRC_CHECK BIT(28) #define VSC73XX_VLANACCESS_VLAN_PORT_MASK GENMASK(9, 2) -#define VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK GENMASK(2, 0) +#define VSC73XX_VLANACCESS_VLAN_PORT_MASK_SHIFT 2 +#define VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK GENMASK(1, 0) #define VSC73XX_VLANACCESS_VLAN_TBL_CMD_IDLE 0 #define VSC73XX_VLANACCESS_VLAN_TBL_CMD_READ_ENTRY 1 #define VSC73XX_VLANACCESS_VLAN_TBL_CMD_WRITE_ENTRY 2 @@ -557,6 +581,287 @@ static enum dsa_tag_protocol vsc73xx_get_tag_protocol(struct dsa_switch *ds, return DSA_TAG_PROTO_NONE; } +static int +vsc73xx_port_wait_for_vlan_table_cmd(struct vsc73xx *vsc, int attempts) +{ + u32 val; + int i; + + for (i = 0; i <= attempts; i++) { + vsc73xx_read(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANACCESS, + &val); + if ((val & VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK) == + VSC73XX_VLANACCESS_VLAN_TBL_CMD_IDLE) + return 0; + } + return -EBUSY; +} + +static int +vsc73xx_port_read_vlan_table_entry(struct dsa_switch *ds, u16 vid, u8 *portmap) +{ + struct vsc73xx *vsc = ds->priv; + u32 val; + int ret; + + if (vid > 4095) + return -EPERM; + vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANTIDX, vid); + ret = vsc73xx_port_wait_for_vlan_table_cmd(vsc, VLAN_TABLE_ATTEMPTS); + if (ret) + return ret; + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANACCESS, + VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK, + VSC73XX_VLANACCESS_VLAN_TBL_CMD_READ_ENTRY); + ret = vsc73xx_port_wait_for_vlan_table_cmd(vsc, VLAN_TABLE_ATTEMPTS); + if (ret) + return ret; + vsc73xx_read(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANACCESS, &val); + *portmap = + (val & VSC73XX_VLANACCESS_VLAN_PORT_MASK) >> + VSC73XX_VLANACCESS_VLAN_PORT_MASK_SHIFT; + return 0; +} + +static int +vsc73xx_port_write_vlan_table_entry(struct dsa_switch *ds, u16 vid, u8 portmap) +{ + struct vsc73xx *vsc = ds->priv; + int ret; + + if (vid > 4095) + return -EPERM; + vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANTIDX, vid); + ret = vsc73xx_port_wait_for_vlan_table_cmd(vsc, VLAN_TABLE_ATTEMPTS); + if (ret) + return ret; + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANACCESS, + VSC73XX_VLANACCESS_VLAN_SRC_CHECK | + VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK | + VSC73XX_VLANACCESS_VLAN_PORT_MASK, + VSC73XX_VLANACCESS_VLAN_SRC_CHECK | + VSC73XX_VLANACCESS_VLAN_TBL_CMD_WRITE_ENTRY | + (portmap << + VSC73XX_VLANACCESS_VLAN_PORT_MASK_SHIFT)); + ret = vsc73xx_port_wait_for_vlan_table_cmd(vsc, VLAN_TABLE_ATTEMPTS); + if (ret) + return ret; + return 0; +} + +static int +vsc73xx_port_update_vlan_table(struct dsa_switch *ds, int port, u16 vid_begin, + u16 vid_end, bool set) +{ + u8 portmap; + int ret; + u16 i; + + if (vid_begin > 4095 || vid_end > 4095 || vid_begin > vid_end) + return -EPERM; + + for (i = vid_begin; i <= vid_end; i++) { + ret = vsc73xx_port_read_vlan_table_entry(ds, i, &portmap); + if (ret) + return ret; + if (set) + portmap |= BIT(port); + else + portmap &= ~BIT(port); + + ret = vsc73xx_port_write_vlan_table_entry(ds, i, portmap); + if (ret) + return ret; + } + return 0; +} + +static int vsc73xx_port_set_vlan_unaware(struct dsa_switch *ds, int port) +{ + struct vsc73xx *vsc = ds->priv; + int ret; + + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG, + VSC73XX_MAC_CFG_VLAN_AWR, + ~VSC73XX_MAC_CFG_VLAN_AWR); + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG, + VSC73XX_MAC_CFG_VLAN_DBLAWR, + ~VSC73XX_MAC_CFG_VLAN_DBLAWR); + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_VLAN_MISC, + VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA, + VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA); + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_VLAN_MISC, + VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA, + VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA); + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP, + VSC73XX_CAT_DROP_TAGGED_ENA, + ~VSC73XX_CAT_DROP_TAGGED_ENA); + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP, + VSC73XX_CAT_DROP_UNTAGGED_ENA, + ~VSC73XX_CAT_DROP_UNTAGGED_ENA); + + ret = vsc73xx_port_update_vlan_table(ds, port, 0, 4095, 0); + return ret; +} + +static int vsc73xx_port_set_vlan_aware(struct dsa_switch *ds, int port) +{ + struct vsc73xx *vsc = ds->priv; + int ret; + + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG, + VSC73XX_MAC_CFG_VLAN_AWR, VSC73XX_MAC_CFG_VLAN_AWR); + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG, + VSC73XX_MAC_CFG_VLAN_DBLAWR, + ~VSC73XX_MAC_CFG_VLAN_DBLAWR); + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_VLAN_MISC, + VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA, + ~VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA); + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_VLAN_MISC, + VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA, + ~VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA); + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP, + VSC73XX_CAT_DROP_TAGGED_ENA, + ~VSC73XX_CAT_DROP_TAGGED_ENA); + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP, + VSC73XX_CAT_DROP_UNTAGGED_ENA, + VSC73XX_CAT_DROP_UNTAGGED_ENA); + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG, + VSC73XX_TXUPDCFG_TX_INSERT_TAG, + VSC73XX_TXUPDCFG_TX_INSERT_TAG); + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG, + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA, + ~VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA); + + if (port == CPU_PORT) + ret = vsc73xx_port_update_vlan_table(ds, port, 0, 4095, 1); + else + ret = vsc73xx_port_update_vlan_table(ds, port, 0, 4095, 0); + return ret; +} + +static int +vsc73xx_port_vlan_filtering(struct dsa_switch *ds, int port, + bool vlan_filtering, struct switchdev_trans *trans) +{ + int ret; + + if (switchdev_trans_ph_prepare(trans)) + return 0; + + if (vlan_filtering) { + ret = vsc73xx_port_set_vlan_aware(ds, port); + if (ret) + return ret; + ret = vsc73xx_port_set_vlan_aware(ds, CPU_PORT); + } else { + ret = vsc73xx_port_set_vlan_unaware(ds, port); + } + return ret; +} + +static int vsc73xx_port_vlan_prepare(struct dsa_switch *ds, int port, + const struct switchdev_obj_port_vlan *vlan) +{ + /* nothing needed */ + return 0; +} + +static void vsc73xx_port_vlan_add(struct dsa_switch *ds, int port, + const struct switchdev_obj_port_vlan *vlan) +{ + bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED; + bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID; + struct vsc73xx *vsc = ds->priv; + int ret; + u32 tmp; + + if (!dsa_port_is_vlan_filtering(dsa_to_port(ds, port))) + return; + + ret = vsc73xx_port_update_vlan_table(ds, port, vlan->vid_begin, + vlan->vid_end, 1); + if (ret) + return; + + if (untagged && port != CPU_PORT) { + /* VSC73xx can have only one untagged vid per port. */ + vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, + VSC73XX_TXUPDCFG, &tmp); + + if (tmp & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA) + dev_warn(vsc->dev, + "Chip support only one untagged VID per port. Overwriting...\n"); + + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, + VSC73XX_TXUPDCFG, + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID, + (vlan->vid_end << + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT) & + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID); + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, + VSC73XX_TXUPDCFG, + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA, + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA); + } + if (pvid && port != CPU_PORT) { + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, + VSC73XX_CAT_DROP, + VSC73XX_CAT_DROP_UNTAGGED_ENA, + ~VSC73XX_CAT_DROP_UNTAGGED_ENA); + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, + VSC73XX_CAT_PORT_VLAN, + VSC73XX_CAT_PORT_VLAN_VLAN_VID, + vlan->vid_end & + VSC73XX_CAT_PORT_VLAN_VLAN_VID); + } +} + +static int vsc73xx_port_vlan_del(struct dsa_switch *ds, int port, + const struct switchdev_obj_port_vlan *vlan) +{ + bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED; + bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID; + struct vsc73xx *vsc = ds->priv; + u32 tmp, untagged_vid; + int ret; + + if (!dsa_port_is_vlan_filtering(dsa_to_port(ds, port))) + return -EINVAL; + + ret = + vsc73xx_port_update_vlan_table(ds, port, vlan->vid_begin, + vlan->vid_end, 0); + if (ret) + return ret; + + /* VSC73xx can have only one untagged vid per port. Check if match. */ + vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, + VSC73XX_TXUPDCFG, &tmp); + untagged_vid = (tmp & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID) + >> VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT; + + if (untagged && untagged_vid == vlan->vid_end) { + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, + VSC73XX_TXUPDCFG, + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA, + ~VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA); + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, + VSC73XX_TXUPDCFG, + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID, 0); + } + if (pvid) { + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, + VSC73XX_CAT_DROP, + VSC73XX_CAT_DROP_UNTAGGED_ENA, + VSC73XX_CAT_DROP_UNTAGGED_ENA); + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, + VSC73XX_CAT_PORT_VLAN, + VSC73XX_CAT_PORT_VLAN_VLAN_VID, 0); + } + return 0; +} + static int vsc73xx_setup(struct dsa_switch *ds) { struct vsc73xx *vsc = ds->priv; @@ -1051,6 +1356,10 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = { .port_disable = vsc73xx_port_disable, .port_change_mtu = vsc73xx_change_mtu, .port_max_mtu = vsc73xx_get_max_mtu, + .port_vlan_filtering = vsc73xx_port_vlan_filtering, + .port_vlan_prepare = vsc73xx_port_vlan_prepare, + .port_vlan_add = vsc73xx_port_vlan_add, + .port_vlan_del = vsc73xx_port_vlan_del, }; static int vsc73xx_gpio_get(struct gpio_chip *chip, unsigned int offset)
This patch adds support for vlan filtering in vsc73xx driver. After vlan filtering enable, CPU_PORT is configured as trunk, without non-tagged frames. This allows to avoid problems with transmit untagged frames because vsc73xx is DSA_TAG_PROTO_NONE. Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com> --- drivers/net/dsa/vitesse-vsc73xx-core.c | 311 ++++++++++++++++++++++++- 1 file changed, 310 insertions(+), 1 deletion(-)