diff mbox series

[net-next] net: dsa: mt7530: drop untagged frames on VLAN-aware ports without PVID

Message ID 20210805172315.362165-1-dqfext@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net-next] net: dsa: mt7530: drop untagged frames on VLAN-aware ports without PVID | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers success CCed 13 of 13 maintainers
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 81 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link

Commit Message

Qingfang Deng Aug. 5, 2021, 5:23 p.m. UTC
The driver currently still accepts untagged frames on VLAN-aware ports
without PVID. Use PVC.ACC_FRM to drop untagged frames in that case.

Signed-off-by: DENG Qingfang <dqfext@gmail.com>
---
 drivers/net/dsa/mt7530.c | 32 ++++++++++++++++++++++++++++++--
 drivers/net/dsa/mt7530.h |  7 +++++++
 2 files changed, 37 insertions(+), 2 deletions(-)

Comments

Vladimir Oltean Aug. 6, 2021, 12:17 a.m. UTC | #1
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>
Qingfang Deng Aug. 6, 2021, 2:34 a.m. UTC | #2
On Fri, Aug 06, 2021 at 03:17:40AM +0300, Vladimir Oltean wrote:
> 
> 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>

Please allow me to send a v2. This sets the CPU port's PVID to 0
on boot, which causes some undefined behaviour..
diff mbox series

Patch

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 385e169080d9..df167f0529bb 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -1257,9 +1257,11 @@  mt7530_port_set_vlan_unaware(struct dsa_switch *ds, int port)
 		mt7530_rmw(priv, MT7530_PCR_P(port), PCR_PORT_VLAN_MASK,
 			   MT7530_PORT_FALLBACK_MODE);
 
-	mt7530_rmw(priv, MT7530_PVC_P(port), VLAN_ATTR_MASK | PVC_EG_TAG_MASK,
+	mt7530_rmw(priv, MT7530_PVC_P(port),
+		   VLAN_ATTR_MASK | PVC_EG_TAG_MASK | ACC_FRM_MASK,
 		   VLAN_ATTR(MT7530_VLAN_TRANSPARENT) |
-		   PVC_EG_TAG(MT7530_VLAN_EG_CONSISTENT));
+		   PVC_EG_TAG(MT7530_VLAN_EG_CONSISTENT) |
+		   MT7530_VLAN_ACC_ALL);
 
 	/* Set PVID to 0 */
 	mt7530_rmw(priv, MT7530_PPBV1_P(port), G0_PORT_VID_MASK,
@@ -1297,6 +1299,11 @@  mt7530_port_set_vlan_aware(struct dsa_switch *ds, int port)
 			   MT7530_PORT_SECURITY_MODE);
 		mt7530_rmw(priv, MT7530_PPBV1_P(port), G0_PORT_VID_MASK,
 			   G0_PORT_VID(priv->ports[port].pvid));
+
+		/* Only accept tagged frames if PVID is not set */
+		if (!priv->ports[port].pvid)
+			mt7530_rmw(priv, MT7530_PVC_P(port), ACC_FRM_MASK,
+				   MT7530_VLAN_ACC_TAGGED);
 	}
 
 	/* Set the port as a user port which is to be able to recognize VID
@@ -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);
@@ -1654,6 +1676,12 @@  mt7530_port_vlan_del(struct dsa_switch *ds, int port,
 	 */
 	if (priv->ports[port].pvid == vlan->vid) {
 		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);
 	}
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 4a91d80f51bb..fe4cd2ac26d0 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -238,6 +238,7 @@  enum mt7530_port_mode {
 #define  PVC_EG_TAG_MASK		PVC_EG_TAG(7)
 #define  VLAN_ATTR(x)			(((x) & 0x3) << 6)
 #define  VLAN_ATTR_MASK			VLAN_ATTR(3)
+#define  ACC_FRM_MASK			GENMASK(1, 0)
 
 enum mt7530_vlan_port_eg_tag {
 	MT7530_VLAN_EG_DISABLED = 0,
@@ -249,6 +250,12 @@  enum mt7530_vlan_port_attr {
 	MT7530_VLAN_TRANSPARENT = 3,
 };
 
+enum mt7530_vlan_port_acc_frm {
+	MT7530_VLAN_ACC_ALL = 0,
+	MT7530_VLAN_ACC_TAGGED = 1,
+	MT7530_VLAN_ACC_UNTAGGED = 2,
+};
+
 #define  STAG_VPID			(((x) & 0xffff) << 16)
 
 /* Register for port port-and-protocol based vlan 1 control */