Received-SPF: pass (google.com: domain of linux-kernel+bounces-93118-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <20e792ad-33ce-43a6-8ed0-8db6e1a25c27@gmail.com>
Date: Tue, 5 Mar 2024 15:51:11 -0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net-next v6 07/16] net: dsa: vsc73xx: Add vlan filtering
Content-Language: en-US
To: Pawel Dembicki <paweldembicki@gmail.com>, netdev@vger.kernel.org
Cc: Linus Walleij <linus.walleij@linaro.org>, Simon Horman
 <horms@kernel.org>, Andrew Lunn <andrew@lunn.ch>,
 Vladimir Oltean <olteanv@gmail.com>, "David S. Miller"
 <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>,
 Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
 Claudiu Manoil <claudiu.manoil@nxp.com>,
 Alexandre Belloni <alexandre.belloni@bootlin.com>,
 UNGLinuxDriver@microchip.com, Russell King <linux@armlinux.org.uk>,
 linux-kernel@vger.kernel.org
References: <20240301221641.159542-1-paweldembicki@gmail.com>
 <20240301221641.159542-8-paweldembicki@gmail.com>
From: Florian Fainelli <f.fainelli@gmail.com>
In-Reply-To: <20240301221641.159542-8-paweldembicki@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 3/1/24 14:16, Pawel Dembicki wrote:
> This patch implements VLAN filtering for the vsc73xx driver.
> 
> After starting VLAN filtering, the switch is reconfigured from QinQ to
> a simple VLAN aware mode. This is required because VSC73XX chips do not
> support inner VLAN tag filtering.
> 
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
> ---

[snip]

> +static size_t
> +vsc73xx_bridge_vlan_num_untagged(struct vsc73xx *vsc, int port, u16 ignored_vid)
> +{
> +	struct vsc73xx_bridge_vlan *vlan;
> +	size_t num_untagged = 0;
> +
> +	list_for_each_entry(vlan, &vsc->vlans, list)
> +		if ((vlan->portmask & BIT(port)) &&
> +		    (vlan->untagged & BIT(port)) &&
> +		    vlan->vid != ignored_vid)
> +			num_untagged++;
> +
> +	return num_untagged;
> +}

You always use both helpers at the same time, so I would suggest 
returning num_tagged and num_untagged by reference to have a single 
linked list lookup.

> +
> +static u16 vsc73xx_find_first_vlan_untagged(struct vsc73xx *vsc, int port)
> +{
> +	struct vsc73xx_bridge_vlan *vlan;
> +
> +	list_for_each_entry(vlan, &vsc->vlans, list)
> +		if ((vlan->portmask & BIT(port)) &&
> +		    (vlan->untagged & BIT(port)))
> +			return vlan->vid;
> +
> +	return VLAN_N_VID;
> +}
> +
> +static int
> +vsc73xx_port_vlan_filtering(struct dsa_switch *ds, int port,
> +			    bool vlan_filtering, struct netlink_ext_ack *extack)
> +{
> +	enum vsc73xx_port_vlan_conf port_vlan_conf = VSC73XX_VLAN_IGNORE;
> +	struct vsc73xx *vsc = ds->priv;
> +	bool store_untagged = false;
> +	bool store_pvid = false;
> +	u16 vid, vlan_untagged;
> +
> +	/* The swap processed below is required because vsc73xx is using
> +	 * tag_8021q. When vlan_filtering is disabled, tag_8021q uses
> +	 * pvid/untagged vlans for port recognition. The values configured for
> +	 * vlans < 3072 are stored in storage table. When vlan_filtering is
> +	 * enabled, we need to restore pvid/untagged from storage and keep
> +	 * values used for tag_8021q.
> +	 */
> +	if (vlan_filtering) {
> +		/* Use VLAN_N_VID to count all vlans */
> +		size_t num_untagged =
> +			vsc73xx_bridge_vlan_num_untagged(vsc, port, VLAN_N_VID);
> +
> +		port_vlan_conf = (num_untagged > 1) ?
> +				 VSC73XX_VLAN_FILTER_UNTAG_ALL :
> +				 VSC73XX_VLAN_FILTER;
> +
> +		vlan_untagged = vsc73xx_find_first_vlan_untagged(vsc, port);
> +		if (vlan_untagged < VLAN_N_VID) {
> +			store_untagged  = vsc73xx_port_get_untagged(vsc, port,
> +								    &vid,
> +								    false);
> +			vsc73xx_vlan_change_untagged(vsc, port, vlan_untagged,
> +						     true, false);
> +			vsc->untagged_storage[port] = store_untagged ?
> +						      vid : VLAN_N_VID;
> +		}
> +	} else {
> +		vsc73xx_vlan_change_untagged(vsc, port,
> +					     vsc->untagged_storage[port],
> +					     vsc->untagged_storage[port] <
> +					     VLAN_N_VID, false);
> +	}
> +
> +	vsc73xx_set_vlan_conf(vsc, port, port_vlan_conf);
> +
> +	store_pvid = vsc73xx_port_get_pvid(vsc, port, &vid, false);
> +	vsc73xx_vlan_change_pvid(vsc, port, vsc->pvid_storage[port],
> +				 vsc->pvid_storage[port] < VLAN_N_VID, false);
> +	vsc->pvid_storage[port] = store_pvid ? vid : VLAN_N_VID;
> +
> +	return 0;
> +}
> +
> +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;
> +	size_t num_tagged, num_untagged;
> +	struct vsc73xx *vsc = ds->priv;
> +	int ret;
> +	u16 vid;
> +
> +	/* 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.
> +	 */
> +	num_tagged = vsc73xx_bridge_vlan_num_tagged(vsc, port, vlan->vid);
> +	num_untagged = vsc73xx_bridge_vlan_num_untagged(vsc, port, vlan->vid);
> +
> +	/* VSC73XX allow only three untagged states: none, one or all */
> +	if ((untagged && num_tagged > 0 && num_untagged > 0) ||
> +	    (!untagged && 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 = BIT(port);
> +		vsc73xx_vlan->untagged = untagged ? BIT(port) : 0;
> +
> +		INIT_LIST_HEAD(&vsc73xx_vlan->list);
> +		list_add_tail(&vsc73xx_vlan->list, &vsc->vlans);
> +	} else {
> +		vsc73xx_vlan->portmask |= BIT(port);
> +
> +		if (untagged)
> +			vsc73xx_vlan->untagged |= BIT(port);
> +		else
> +			vsc73xx_vlan->untagged &= ~BIT(port);

These assignments should be working even when you have a freshly 
allocated VLAN entry, so you can just re-factor this a bit and have a 
common set of assignments applying to an existing or freshly allocated 
VLAN entry?

> +	}
> +
> +	/* CPU port must be always tagged because port separation is based on
> +	 * tag_8021q.
> +	 */
> +	if (port != CPU_PORT) {

Please reduce indentation here.

Have to admit the logic is a bit hard to follow, but that is also 
because of my lack of understanding of the requirements surrounding the 
use of tag_8021q.
-- 
Florian