Received-SPF: pass (google.com: domain of linux-kernel+bounces-133310-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Fri, 5 Apr 2024 19:07:12 +0300
From: Vladimir Oltean <olteanv@gmail.com>
To: Pawel Dembicki <paweldembicki@gmail.com>
Cc: netdev@vger.kernel.org, Linus Walleij <linus.walleij@linaro.org>,
	Simon Horman <horms@kernel.org>, Andrew Lunn <andrew@lunn.ch>,
	Florian Fainelli <f.fainelli@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
Subject: Re: [PATCH net-next v8 07/16] net: dsa: vsc73xx: Add vlan filtering
Message-ID: <20240405160712.pfgt7lnutsqvxfob@skbuf>
References: <20240403103734.3033398-1-paweldembicki@gmail.com>
 <20240403103734.3033398-8-paweldembicki@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240403103734.3033398-8-paweldembicki@gmail.com>

On Wed, Apr 03, 2024 at 12:37:23PM +0200, Pawel Dembicki wrote:
> +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;
> +	u16 vid_pvid = 0, vid_untagged = 0;
> +	struct vsc73xx_portinfo *portinfo;
> +	struct vsc73xx *vsc = ds->priv;
> +	bool set_untagged = false;
> +	bool set_pvid = false;
> +
> +	portinfo = &vsc->portinfo[port];
> +
> +	/* 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 and pvid/untagged states are stored in portinfo structure.
> +	 * When vlan_filtering is enabled, we need to restore pvid/untagged from
> +	 * portinfo structure. Analogic routine is processed when vlan_filtering

Analogous

> +	 * is disabled, but values used for tag_8021q are restored.
> +	 */
> +	if (vlan_filtering) {
> +		struct vsc73xx_vlan_summary summary;
> +
> +		/* Use VLAN_N_VID to count all vlans */
> +		vsc73xx_bridge_vlan_summary(vsc, port, &summary, VLAN_N_VID);
> +
> +		port_vlan_conf = (summary.num_untagged > 1) ?
> +				 VSC73XX_VLAN_FILTER_UNTAG_ALL :
> +				 VSC73XX_VLAN_FILTER;
> +
> +		if (summary.num_untagged == 1) {
> +			vid_untagged = vsc73xx_find_first_vlan_untagged(vsc,
> +									port);
> +			set_untagged = true;
> +		}

Is there really any functional difference between the above, vs this in
port_vlan_add():

	port_vlan_conf = VSC73XX_VLAN_FILTER;

	if (summary.num_tagged == 0 && untagged)
		port_vlan_conf = VSC73XX_VLAN_FILTER_UNTAG_ALL;
	vsc73xx_set_vlan_conf(vsc, port, port_vlan_conf);

	if (port_vlan_conf == VSC73XX_VLAN_FILTER_UNTAG_ALL)
		goto update_vlan_table;

vs this in port_vlan_del():

		enum vsc73xx_port_vlan_conf port_vlan_conf =
							VSC73XX_VLAN_FILTER;

		if (summary.num_tagged == 0)
			port_vlan_conf = VSC73XX_VLAN_FILTER_UNTAG_ALL;
		vsc73xx_set_vlan_conf(vsc, port, port_vlan_conf);

		if (summary.num_untagged <= 1) {
			vid = vsc73xx_find_first_vlan_untagged(vsc, port);
			vsc73xx_vlan_change_untagged_hw(vsc, port, vid,
							summary.num_untagged);
		}

?

If not, I have to agree with Florian that it's dizzying to follow the
vsc73xx_set_vlan_conf() calls, all with slightly different input.
Even worse now than before, I think.

I see you've changed the storage variable names, so maybe you are open
to some more refactoring, to make the code more maintainable.

I would suggest calling a single vsc73xx_update_vlan_conf() from all
places, which figures out what to do by itself, based on the current
(updated) state. All ifs and buts are on the inside, not on the outside.

Also, the operate_on_storage variable is not a great name now. Because
you always operate on storage. It means "operate _just_ on storage", aka
"don't commit to hardware".

I wanted to avoid requesting you to make more changes to the
implementation, but you started it, and now it's a confusing mix of
concepts from old and new patch sets.

I would like to also see the "operate_on_storage" logic rewritten and
embedded into those functions which must update shared resources (port
PVID, port untagged VID, etc).

Basically, in the end I would like the driver to be written in the style
of every other similar function: ocelot_port_manage_port_tag(),
sja1105_commit_pvid(), mv88e6xxx_commit_pvid() etc. No arguments, except
the port on which to run. Figure out what to do from the current state,
and make sure it is called from all places that change that state.
It also serves very well as self-documenting code, because "how register
X should be set" is centralized into a single function.

Sorry for making this request so late in the development process. I hope
it's not discouraging.