| 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 |
| 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 |
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>
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 --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 */
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(-)