[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <84281d30907c09a2f15d741088833b0ae6597b89.camel@redhat.com>
Date: Fri, 14 Jun 2024 11:42:50 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Pawel Dembicki <paweldembicki@...il.com>, netdev@...r.kernel.org
Cc: Andrew Lunn <andrew@...n.ch>, Florian Fainelli <f.fainelli@...il.com>,
Vladimir Oltean <olteanv@...il.com>, "David S. Miller"
<davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
<kuba@...nel.org>, Claudiu Manoil <claudiu.manoil@....com>, Alexandre
Belloni <alexandre.belloni@...tlin.com>, UNGLinuxDriver@...rochip.com,
Russell King <linux@...linux.org.uk>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 02/12] net: dsa: vsc73xx: Add vlan filtering
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
Powered by blists - more mailing lists