Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 6 Aug 2021 03:17:40 +0300
From:   Vladimir Oltean <olteanv@gmail.com>
To:     DENG Qingfang <dqfext@gmail.com>
Cc:     Sean Wang <sean.wang@mediatek.com>,
        Landen Chao <Landen.Chao@mediatek.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Vivien Didelot <vivien.didelot@gmail.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        netdev@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next] net: dsa: mt7530: drop untagged frames on
 VLAN-aware ports without PVID
Message-ID: <20210806001740.cayorz3vlfrvk75l@skbuf>
References: <20210805172315.362165-1-dqfext@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210805172315.362165-1-dqfext@gmail.com>
Precedence: bulk

On Fri, Aug 06, 2021 at 01:23:14AM +0800, DENG Qingfang wrote:
> @@ -1624,11 +1631,26 @@ mt7530_port_vlan_add(struct dsa_switch *ds, int port,
>  	if (pvid) {
>  		priv->ports[port].pvid = vlan->vid;
>  
> +		/* Accept all frames if PVID is set */
> +		mt7530_rmw(priv, MT7530_PVC_P(port), ACC_FRM_MASK,
> +			   MT7530_VLAN_ACC_ALL);
> +
>  		/* Only configure PVID if VLAN filtering is enabled */
>  		if (dsa_port_is_vlan_filtering(dsa_to_port(ds, port)))
>  			mt7530_rmw(priv, MT7530_PPBV1_P(port),
>  				   G0_PORT_VID_MASK,
>  				   G0_PORT_VID(vlan->vid));
> +	} else if (priv->ports[port].pvid == vlan->vid) {
> +		/* This VLAN is overwritten without PVID, so unset it */
> +		priv->ports[port].pvid = G0_PORT_VID_DEF;
> +
> +		/* Only accept tagged frames if the port is VLAN-aware */
> +		if (dsa_port_is_vlan_filtering(dsa_to_port(ds, port)))
> +			mt7530_rmw(priv, MT7530_PVC_P(port), ACC_FRM_MASK,
> +				   MT7530_VLAN_ACC_TAGGED);
> +
> +		mt7530_rmw(priv, MT7530_PPBV1_P(port), G0_PORT_VID_MASK,
> +			   G0_PORT_VID_DEF);
>  	}
>  
>  	mutex_unlock(&priv->reg_mutex);

Good catch with this condition, sja1105 and ocelot are buggy in this
regard, it seems, probably others too. Need to fix them. Although
honestly I would probably rather spend the time patching the bridge
already to not accept duplicate VLAN entries from user space, just with
different flags, it's just too complex to handle the overwrites everywhere...
Plus, bridge accepting duplicate VLANs means we cannot refcount them on
DSA and CPU ports at the cross-chip level, which in turn means we can
never delete them from those ports.

Anyhow, enough rambling.

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>