Re: [PATCH net-next v6 07/16] net: dsa: vsc73xx: Add vlan filtering

[Vladimir Oltean]

On Tue, Mar 05, 2024 at 03:51:11PM -0800, Florian Fainelli wrote:
> 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@xxxxxxxxx>
> > ---
> 
> [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.

vsc73xx_port_vlan_filtering() calls vsc73xx_bridge_vlan_num_untagged()
but not vsc73xx_bridge_vlan_num_tagged(). Doing as you suggest, while
keeping the code otherwise the same, would imply adding a dummy
num_tagged variable in vlan_filtering().

Though I agree it is generally confusing, because "port_vlan_conf" is
assigned based on inconsistent conditions in vsc73xx_port_vlan_filtering()
vs vsc73xx_port_vlan_add() vs vsc73xx_port_vlan_del(). Namely the number
of tagged VLANs is taken into consideration only on vlan_add(), and of
remaining tagged VLANs on vlan_del(), respectively.

Maybe something like this could help?

struct vsc73xx_vlan_summary {
	size_t num_tagged;
	size_t num_untagged;
};

static void vsc73xx_bridge_vlan_summary(struct vsc73xx *vsc, int port,
					struct vsc73xx_vlan_summary *summary,
					u16 ignored_vid)
{
	size_t num_tagged = 0, num_untagged = 0;
	struct vsc73xx_bridge_vlan *vlan;

	list_for_each_entry(vlan, &vsc->vlans, list) {
		if (!(vlan->portmask & BIT(port)) || vlan->vid == ignored_vid)
			continue;

		if (vlan->untagged & BIT(port))
			num_untagged++;
		else
			num_tagged++;
	}

	summary->num_untagged = num_untagged;
	summary->num_tagged = num_tagged;
}

. and use what you need from the summary.

> > +	}
> > +
> > +	/* 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.

It's not only that. The code is also a bit hard on the brain for me.

An alternative coding pattern would be to observe that certain hardware
registers (the egress-untagged VLAN, the PVID) depend on a constellation
of N input variables (the bridge VLAN filtering state, the tag_8021q
active state, the bridge VLAN table). So, to make the code easier to
follow and to ensure correctness, in theory a central function could be
written, which embeds the same invariant logic of determining what to
program the registers with, depending on the N inputs. This invariant
function is called from every place that modifies any of the N inputs.

What Paweł did here was to have slightly different code paths for
modifying the hardware registers, each code path adjusted slightly on
the state change transition of individual inputs.

This was a design choice on which I commented very early on, stating
that it's unusual but that I can go along with it. It is probably very
ingrained with the choice of the untagged_storage[] and pvid_storage[]
arrays, the logic of swapping the storage with the hardware at VLAN
filtering state change, and thus very hard to change at this stage of
development.