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

From: Paolo Abeni
Date: Fri Jun 14 2024 - 05:43:16 EST


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