| Message ID | 20240619205220.965844-9-paweldembicki@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: vsc73xx: Implement VLAN operations | expand |
On Wed, Jun 19, 2024 at 10:52:14PM +0200, Pawel Dembicki wrote: > +static int vsc73xx_tag_8021q_vlan_add(struct dsa_switch *ds, int port, u16 vid, > + u16 flags) > +{ > + bool untagged = flags & BRIDGE_VLAN_INFO_UNTAGGED; > + bool pvid = flags & BRIDGE_VLAN_INFO_PVID; > + struct vsc73xx_portinfo *portinfo; > + struct vsc73xx *vsc = ds->priv; > + bool commit_to_hardware; > + int ret; > + > + portinfo = &vsc->portinfo[port]; > + > + if (untagged) { > + portinfo->untagged_tag_8021q_configured = true; > + portinfo->untagged_tag_8021q = vid; > + } This is something I don't really like, but I understand why you're doing it. dsa_tag_8021q_bridge_join() first adds the bridge_vid and only then deletes the standalone_vid. Both are egress-untagged. The fact that you have storage in this driver for a single egress-tagged tag_8021q VLAN makes it awkward if not impossible to track the deletion properly from tag_8021q_vlan_del(). You just record the last added VID in tag_8021q_vlan_add() and silently discard any previous one during the VSC73XX_TXUPDCFG_TX_UNTAGGED_VID setting, earlier than the deletion API. I'm wondering if this isn't actually a self-inflicted problem created by the data structures in use. I see that when the port is in VSC73XX_VLAN_IGNORE (which it is for tag_8021q), VSC73XX_TXUPDCFG_TX_INSERT_TAG will be unset. So the port is egress-untagged for all VIDs, and there's little point in tracking any one single egress untagged VID coming from tag_8021q. No? Wouldn't it be better, assuming that is the case, to just return an error if this is a user port and tag_8021q is requesting a VID which is _not_ egress-untagged? If you also respond positively to my comment about returning early in vsc73xx_vlan_commit_untagged() without doing anything in the tag_8021q case, then you can actually get rid of untagged_tag_8021q_configured and untagged_tag_8021q from portinfo. > + if (pvid) { > + portinfo->pvid_tag_8021q_configured = true; > + portinfo->pvid_tag_8021q = vid; > + } > + > + commit_to_hardware = vsc73xx_tag_8021q_active(dsa_to_port(ds, port)); > + if (commit_to_hardware) { > + ret = vsc73xx_vlan_commit_untagged(vsc, port); > + if (ret) > + return ret; > + > + ret = vsc73xx_vlan_commit_pvid(vsc, port); > + if (ret) > + return ret; Hmm, here I notice that the vsc73xx_vlan_commit_conf() call is missing. Am I right that if you add it here, then the entire vsc73xx_vlan_commit_*() string of 3 calls from vsc73xx_port_enable() becomes unnecessary? The tag_8021q VLAN add operations are initiated by you when you call dsa_tag_8021q_register(). So you have good control over when the VLAN configuration of the port for standalone mode is done. > + } > + > + return vsc73xx_update_vlan_table(vsc, port, vid, true); > +} > + > +static int vsc73xx_tag_8021q_vlan_del(struct dsa_switch *ds, int port, u16 vid) > +{ > + struct vsc73xx *vsc = ds->priv; > + As a matter of API compliance, I guess you could add a snippet: if (portinfo->pvid_tag_8021q_configured && portinfo->pvid_tag_8021q == vid) { portinfo->pvid_tag_8021q_configured = false; if (commit_to_hardware) { err = vsc73xx_vlan_commit_pvid(vsc, port); if (err) return err; } } Of course, this is not to say that tag_8021q will _currently_ configure ports for an operating mode without PVID. But it's something that the API permits. > + return vsc73xx_update_vlan_table(vsc, port, vid, false); > +}
diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig index 8508b5145bc1..2d10b4d6cfbb 100644 --- a/drivers/net/dsa/Kconfig +++ b/drivers/net/dsa/Kconfig @@ -127,7 +127,7 @@ config NET_DSA_SMSC_LAN9303_MDIO config NET_DSA_VITESSE_VSC73XX tristate - select NET_DSA_TAG_NONE + select NET_DSA_TAG_VSC73XX_8021Q select FIXED_PHY select VITESSE_PHY select GPIOLIB diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c index 71a377a0b917..5134a3344324 100644 --- a/drivers/net/dsa/vitesse-vsc73xx-core.c +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c @@ -597,7 +597,7 @@ static enum dsa_tag_protocol vsc73xx_get_tag_protocol(struct dsa_switch *ds, * cannot access the tag. (See "Internal frame header" section * 3.9.1 in the manual.) */ - return DSA_TAG_PROTO_NONE; + return DSA_TAG_PROTO_VSC73XX_8021Q; } static int vsc73xx_wait_for_vlan_table_cmd(struct vsc73xx *vsc) @@ -687,7 +687,7 @@ vsc73xx_update_vlan_table(struct vsc73xx *vsc, int port, u16 vid, bool set) static int vsc73xx_setup(struct dsa_switch *ds) { struct vsc73xx *vsc = ds->priv; - int i; + int i, ret; dev_info(vsc->dev, "set up the switch\n"); @@ -768,7 +768,18 @@ static int vsc73xx_setup(struct dsa_switch *ds) INIT_LIST_HEAD(&vsc->vlans); - return 0; + rtnl_lock(); + ret = dsa_tag_8021q_register(ds, htons(ETH_P_8021Q)); + rtnl_unlock(); + + return ret; +} + +static void vsc73xx_teardown(struct dsa_switch *ds) +{ + rtnl_lock(); + dsa_tag_8021q_unregister(ds); + rtnl_unlock(); } static void vsc73xx_init_port(struct vsc73xx *vsc, int port) @@ -1525,6 +1536,48 @@ static int vsc73xx_port_vlan_del(struct dsa_switch *ds, int port, return 0; } +static int vsc73xx_tag_8021q_vlan_add(struct dsa_switch *ds, int port, u16 vid, + u16 flags) +{ + bool untagged = flags & BRIDGE_VLAN_INFO_UNTAGGED; + bool pvid = flags & BRIDGE_VLAN_INFO_PVID; + struct vsc73xx_portinfo *portinfo; + struct vsc73xx *vsc = ds->priv; + bool commit_to_hardware; + int ret; + + portinfo = &vsc->portinfo[port]; + + if (untagged) { + portinfo->untagged_tag_8021q_configured = true; + portinfo->untagged_tag_8021q = vid; + } + if (pvid) { + portinfo->pvid_tag_8021q_configured = true; + portinfo->pvid_tag_8021q = vid; + } + + commit_to_hardware = vsc73xx_tag_8021q_active(dsa_to_port(ds, port)); + if (commit_to_hardware) { + ret = vsc73xx_vlan_commit_untagged(vsc, port); + if (ret) + return ret; + + ret = vsc73xx_vlan_commit_pvid(vsc, port); + if (ret) + return ret; + } + + return vsc73xx_update_vlan_table(vsc, port, vid, true); +} + +static int vsc73xx_tag_8021q_vlan_del(struct dsa_switch *ds, int port, u16 vid) +{ + struct vsc73xx *vsc = ds->priv; + + return vsc73xx_update_vlan_table(vsc, port, vid, false); +} + static int vsc73xx_port_setup(struct dsa_switch *ds, int port) { struct vsc73xx_portinfo *portinfo; @@ -1628,6 +1681,7 @@ static const struct phylink_mac_ops vsc73xx_phylink_mac_ops = { static const struct dsa_switch_ops vsc73xx_ds_ops = { .get_tag_protocol = vsc73xx_get_tag_protocol, .setup = vsc73xx_setup, + .teardown = vsc73xx_teardown, .phy_read = vsc73xx_phy_read, .phy_write = vsc73xx_phy_write, .get_strings = vsc73xx_get_strings, @@ -1643,6 +1697,8 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = { .port_vlan_add = vsc73xx_port_vlan_add, .port_vlan_del = vsc73xx_port_vlan_del, .phylink_get_caps = vsc73xx_phylink_get_caps, + .tag_8021q_vlan_add = vsc73xx_tag_8021q_vlan_add, + .tag_8021q_vlan_del = vsc73xx_tag_8021q_vlan_del, }; static int vsc73xx_gpio_get(struct gpio_chip *chip, unsigned int offset)
This patch is a simple implementation of 802.1q tagging in the vsc73xx driver. Currently, devices with DSA_TAG_PROTO_NONE are not functional. The VSC73XX family doesn't provide any tag support for external Ethernet ports. The only option available is VLAN-based tagging, which requires constant hardware VLAN filtering. While the VSC73XX family supports provider bridging, it only supports QinQ without full implementation of 802.1AD. This means it only allows the doubled 0x8100 TPID. In the simple port mode, QinQ is enabled to preserve forwarding of VLAN-tagged frames. Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com> --- v2: - handle raturn values of 'vsc73xx_vlan_commit*' functions v1: - added dsa_tag_8021q_unregister in teardown function - moved dsa_tag_8021q_register after vlan database clean operation --- Before patch series split: https://patchwork.kernel.org/project/netdevbpf/list/?series=841034&state=%2A&archive=both v8: - resend only v7: - adjust tag8021q implementation for vlan filtering implementation changes v6: - resend only v5: - improve commit message v4: - adjust tag8021q implementation for changed untagged vlan storage - minor fixes v3: - Split tagger and tag implementation into separate commits --- drivers/net/dsa/Kconfig | 2 +- drivers/net/dsa/vitesse-vsc73xx-core.c | 62 ++++++++++++++++++++++++-- 2 files changed, 60 insertions(+), 4 deletions(-)