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

[Vladimir Oltean <olteanv@gmail.com>]

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