Re: [PATCH net-next 3/6] net: dsa: Delete the VID from the upstream port as well

[Vivien Didelot <vivien.didelot@gmail.com>]

Vladimir,

On Tue, 20 Aug 2019 02:59:59 +0300, Vladimir Oltean <olteanv@gmail.com> wrote:
> Commit b2f81d304cee ("net: dsa: add CPU and DSA ports as VLAN members")
> is littering a lot. After deleting a VLAN added on a DSA port, it still
> remains installed in the hardware filter of the upstream port. Fix this.

Littering a lot, really?

FYI we are not removing the target VLAN from the hardware yet because it would
be too expensive to cache data in DSA core in order to know if the VID is not
used by any other slave port of the fabric anymore, and thus safe to remove.

Keeping the VID programmed for DSA and CPU ports is simpler for the moment,
as an hardware VLAN with only these ports as members is unlikely to harm.

> 
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> ---
>  net/dsa/switch.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/net/dsa/switch.c b/net/dsa/switch.c
> index 09d9286b27cc..84ab2336131e 100644
> --- a/net/dsa/switch.c
> +++ b/net/dsa/switch.c
> @@ -295,11 +295,20 @@ static int dsa_switch_vlan_del(struct dsa_switch *ds,
>  			       struct dsa_notifier_vlan_info *info)
>  {
>  	const struct switchdev_obj_port_vlan *vlan = info->vlan;
> +	int port;
>  
>  	if (!ds->ops->port_vlan_del)
>  		return -EOPNOTSUPP;
>  
> +	/* Build a mask of VLAN members */
> +	bitmap_zero(ds->bitmap, ds->num_ports);
>  	if (ds->index == info->sw_index)
> +		set_bit(info->port, ds->bitmap);
> +	for (port = 0; port < ds->num_ports; port++)
> +		if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
> +			set_bit(port, ds->bitmap);
> +
> +	for_each_set_bit(port, ds->bitmap, ds->num_ports)
>  		return ds->ops->port_vlan_del(ds, info->port, vlan);

You return right away from the loop? You use info->port instead of port?
	
>  
>  	return 0;

Even if you patch wasn't badly broken, "bridge vlan del" targeting a single
switch port would also remove the VLAN from the CPU port and thus breaking
offloaded 802.1q. It would also remove it from the DSA ports interconnecting
multiple switches, thus breaking the 802.1q conduit for the whole fabric.

So you're not fixing anything here, but you're breaking single-chip and
cross-chip hardware VLAN. Seriously wtf is this patch?

NAK!


	Vivien