Re: [PATCH net-next 02/12] net: dsa: vsc73xx: Add vlan filtering

[Paolo Abeni <pabeni@redhat.com>]

Hi,

On Tue, 2024-06-11 at 21:49 +0200, Pawel Dembicki wrote:
> +static int vsc73xx_port_vlan_add(struct dsa_switch *ds, int port,
> +				 const struct switchdev_obj_port_vlan *vlan,
> +				 struct netlink_ext_ack *extack)
> +{
> +	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
> +	bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
> +	struct dsa_port *dp = dsa_to_port(ds, port);
> +	struct vsc73xx_bridge_vlan *vsc73xx_vlan;
> +	struct vsc73xx_vlan_summary summary;
> +	struct vsc73xx_portinfo *portinfo;
> +	struct vsc73xx *vsc = ds->priv;
> +	bool commit_to_hardware;
> +	int ret = 0;
> +
> +	/* Be sure to deny alterations to the configuration done by tag_8021q.
> +	 */
> +	if (vid_is_dsa_8021q(vlan->vid)) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Range 3072-4095 reserved for dsa_8021q operation");
> +		return -EBUSY;
> +	}
> +
> +	/* The processed vlan->vid is excluded from the search because the VLAN
> +	 * can be re-added with a different set of flags, so it's easiest to
> +	 * ignore its old flags from the VLAN database software copy.
> +	 */
> +	vsc73xx_bridge_vlan_summary(vsc, port, &summary, vlan->vid);
> +
> +	/* VSC73XX allow only three untagged states: none, one or all */
> +	if ((untagged && summary.num_tagged > 0 && summary.num_untagged > 0) ||
> +	    (!untagged && summary.num_untagged > 1)) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Port can have only none, one or all untagged vlan");
> +		return -EBUSY;
> +	}
> +
> +	vsc73xx_vlan = vsc73xx_bridge_vlan_find(vsc, vlan->vid);
> +
> +	if (!vsc73xx_vlan) {
> +		vsc73xx_vlan = kzalloc(sizeof(*vsc73xx_vlan), GFP_KERNEL);
> +		if (!vsc73xx_vlan)
> +			return -ENOMEM;
> +
> +		vsc73xx_vlan->vid = vlan->vid;
> +		vsc73xx_vlan->portmask = 0;
> +		vsc73xx_vlan->untagged = 0;
> +
> +		INIT_LIST_HEAD(&vsc73xx_vlan->list);

INIT_LIST_HEAD() is not needed, as the entry will be unconditionally
added in the statement below.

> +		list_add_tail(&vsc73xx_vlan->list, &vsc->vlans);
> +	}
> +
> +	/* CPU port must be always tagged because port separation is based on
> +	 * tag_8021q.
> +	 */
> +	if (port == CPU_PORT)
> +		goto update_vlan_table;
> +
> +	vsc73xx_vlan->portmask |= BIT(port);
> +
> +	if (untagged)
> +		vsc73xx_vlan->untagged |= BIT(port);
> +	else
> +		vsc73xx_vlan->untagged &= ~BIT(port);
> +
> +	portinfo = &vsc->portinfo[port];
> +
> +	if (pvid) {
> +		portinfo->pvid_vlan_filtering_configured = true;
> +		portinfo->pvid_vlan_filtering = vlan->vid;
> +	} else if (portinfo->pvid_vlan_filtering_configured &&
> +		   portinfo->pvid_vlan_filtering == vlan->vid) {
> +		portinfo->pvid_vlan_filtering_configured = false;
> +	}
> +
> +	commit_to_hardware = !vsc73xx_tag_8021q_active(dp);
> +	if (commit_to_hardware) {
> +		vsc73xx_vlan_commit_pvid(vsc, port);
> +		vsc73xx_vlan_commit_untagged(vsc, port);
> +		vsc73xx_vlan_commit_conf(vsc, port);
> +	}
> +
> +update_vlan_table:
> +	ret = vsc73xx_update_vlan_table(vsc, port, vlan->vid, true);
> +	if (!ret)
> +		return 0;
> +
> +	list_del(&vsc73xx_vlan->list);
> +	kfree(vsc73xx_vlan);

This does not look correct. I guess you should clear bit(port) in
vsc73xx_vlan->portmask and dispose the entry only when portmask becomes
0. You probably can factor out a common helper to be used here and in
vsc73xx_port_vlan_del(). 

Thanks,

Paolo