Re: [PATCH net-next v2 08/12] net: dsa: vsc73xx: Implement the tag_8021q VLAN operations

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);
+}