[PATCH net 0/2] Fixes for DSA tagging using 802.1Q

[PATCH net 0/2] Fixes for DSA tagging using 802.1Q

[Vladimir Oltean]

During the prototyping for the "Decoupling PHYLINK from struct
net_device" patchset, the CPU port of the sja1105 driver was moved to a
different spot.  This uncovered an issue in the tag_8021q DSA code,
which used to work by mistake - the CPU port was the last hardware port
numerically, and this was masking an ordering issue which is very likely
to be seen in other drivers that make use of 802.1Q tags.

A question was also raised whether the VID numbers bear any meaning, and
the conclusion was that they don't, at least not in an absolute sense.
The second patch defines bit fields inside the DSA 802.1Q VID so that
tcpdump can decode it unambiguously (although the meaning is now clear
even by visual inspection).

Ioana Ciornei (1):
  net: dsa: tag_8021q: Change order of rx_vid setup

Vladimir Oltean (1):
  net: dsa: tag_8021q: Create a stable binary format

 net/dsa/tag_8021q.c | 73 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 59 insertions(+), 14 deletions(-)

-- 
2.17.1

[PATCH net 1/2] net: dsa: tag_8021q: Change order of rx_vid setup

[Vladimir Oltean]

From: Ioana Ciornei <ioana.ciornei@nxp.com>

The 802.1Q tagging performs an unbalanced setup in terms of RX VIDs on
the CPU port. For the ingress path of a 802.1Q switch to work, the RX
VID of a port needs to be seen as tagged egress on the CPU port.

While configuring the other front-panel ports to be part of this VID,
for bridge scenarios, the untagged flag is applied even on the CPU port
in dsa_switch_vlan_add.  This happens because DSA applies the same flags
on the CPU port as on the (bridge-controlled) slave ports, and the
effect in this case is that the CPU port tagged settings get deleted.

Instead of fixing DSA by introducing a way to control VLAN flags on the
CPU port (and hence stop inheriting from the slave ports) - a hard,
perhaps intractable problem - avoid this situation by moving the setup
part of the RX VID on the CPU port after all the other front-panel ports
have been added to the VID.

Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
Fixes: f9bbe4477c30 ("net: dsa: Optional VLAN-based port separation for switches without tagging")
---
 net/dsa/tag_8021q.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/net/dsa/tag_8021q.c b/net/dsa/tag_8021q.c
index 8ae48c7e1e76..4adec6bbfe59 100644
--- a/net/dsa/tag_8021q.c
+++ b/net/dsa/tag_8021q.c
@@ -128,10 +128,7 @@ int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int port, bool enabled)
 		u16 flags;
 
 		if (i == upstream)
-			/* CPU port needs to see this port's RX VID
-			 * as tagged egress.
-			 */
-			flags = 0;
+			continue;
 		else if (i == port)
 			/* The RX VID is pvid on this port */
 			flags = BRIDGE_VLAN_INFO_UNTAGGED |
@@ -150,6 +147,20 @@ int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int port, bool enabled)
 			return err;
 		}
 	}
+
+	/* CPU port needs to see this port's RX VID
+	 * as tagged egress.
+	 */
+	if (enabled)
+		err = dsa_port_vid_add(upstream_dp, rx_vid, 0);
+	else
+		err = dsa_port_vid_del(upstream_dp, rx_vid);
+	if (err) {
+		dev_err(ds->dev, "Failed to apply RX VID %d to port %d: %d\n",
+			rx_vid, port, err);
+		return err;
+	}
+
 	/* Finally apply the TX VID on this port and on the CPU port */
 	if (enabled)
 		err = dsa_port_vid_add(dp, tx_vid, BRIDGE_VLAN_INFO_UNTAGGED);
-- 
2.17.1

[PATCH net 2/2] net: dsa: tag_8021q: Create a stable binary format

[Vladimir Oltean]

Tools like tcpdump need to be able to decode the significance of fake
VLAN headers that DSA uses to separate switch ports.

But currently these have no global significance - they are simply an
ordered list of DSA_MAX_SWITCHES x DSA_MAX_PORTS numbers ending at 4095.

The reason why this is submitted as a fix is that the existing mapping
of VIDs should not enter into a stable kernel, so we can pretend that
only the new format exists. This way tcpdump won't need to try to make
something out of the VLAN tags on 5.2 kernels.

Fixes: f9bbe4477c30 ("net: dsa: Optional VLAN-based port separation for switches without tagging")
Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
 net/dsa/tag_8021q.c | 54 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 44 insertions(+), 10 deletions(-)

diff --git a/net/dsa/tag_8021q.c b/net/dsa/tag_8021q.c
index 4adec6bbfe59..4c2c70ce5d54 100644
--- a/net/dsa/tag_8021q.c
+++ b/net/dsa/tag_8021q.c
@@ -11,20 +11,54 @@
 
 #include "dsa_priv.h"
 
-/* Allocating two VLAN tags per port - one for the RX VID and
- * the other for the TX VID - see below
+/* Binary structure of the fake 12-bit VID field (when the TPID is
+ * ETH_P_DSA_8021Q):
+ *
+ * +-----+------+-----------------+------+-----------------------------+-----+
+ * | DIR | RSVD |    SWITCH_ID    | RSVD |             PORT            | MBZ |
+ * +-----+------+-----------------+------+-----------------------------+-----+
+ * 12    11     10                7      6                             1     0
+ *
+ * DIR - VID[11]:
+ *	Direction flag. 0 for RX VLAN, 1 for TX VLAN
+ *
+ * RSVD - VID[10]:
+ *	To be used for further expansion of SWITCH_ID or for other purposes.
+ *
+ * SWITCH_ID - VID[9:7]:
+ *	Index of switch within DSA tree. Must be between 0 and
+ *	DSA_MAX_SWITCHES - 1.
+ *
+ * RSVD - VID[6]:
+ *	To be used for further expansion of PORT or for other purposes.
+ *
+ * PORT - VID[5:1]:
+ *	Index of switch port. Must be between 0 and DSA_MAX_PORTS - 1.
+ *
+ * MBZ - VID[0]:
+ *	Must be zero. This makes the special VIDs of 0, 1 and 4095 to be left
+ *	unused by this coding scheme.
  */
-#define DSA_8021Q_VID_RANGE	(DSA_MAX_SWITCHES * DSA_MAX_PORTS)
-#define DSA_8021Q_VID_BASE	(VLAN_N_VID - 2 * DSA_8021Q_VID_RANGE - 1)
-#define DSA_8021Q_RX_VID_BASE	(DSA_8021Q_VID_BASE)
-#define DSA_8021Q_TX_VID_BASE	(DSA_8021Q_VID_BASE + DSA_8021Q_VID_RANGE)
+
+#define DSA_8021Q_DIR_TX		BIT(11)
+
+#define DSA_8021Q_SWITCH_ID_SHIFT	7
+#define DSA_8021Q_SWITCH_ID_MASK	GENMASK(9, 7)
+#define DSA_8021Q_SWITCH_ID(x)		(((x) << DSA_8021Q_SWITCH_ID_SHIFT) & \
+						 DSA_8021Q_SWITCH_ID_MASK)
+
+#define DSA_8021Q_PORT_MASK		GENMASK(5, 1)
+#define DSA_8021Q_PORT_SHIFT		1
+#define DSA_8021Q_PORT(x)		(((x) << DSA_8021Q_PORT_SHIFT) & \
+						 DSA_8021Q_PORT_MASK)
 
 /* Returns the VID to be inserted into the frame from xmit for switch steering
  * instructions on egress. Encodes switch ID and port ID.
  */
 u16 dsa_8021q_tx_vid(struct dsa_switch *ds, int port)
 {
-	return DSA_8021Q_TX_VID_BASE + (DSA_MAX_PORTS * ds->index) + port;
+	return DSA_8021Q_DIR_TX | DSA_8021Q_SWITCH_ID(ds->index) |
+	       DSA_8021Q_PORT(port);
 }
 EXPORT_SYMBOL_GPL(dsa_8021q_tx_vid);
 
@@ -33,21 +67,21 @@ EXPORT_SYMBOL_GPL(dsa_8021q_tx_vid);
  */
 u16 dsa_8021q_rx_vid(struct dsa_switch *ds, int port)
 {
-	return DSA_8021Q_RX_VID_BASE + (DSA_MAX_PORTS * ds->index) + port;
+	return DSA_8021Q_SWITCH_ID(ds->index) | DSA_8021Q_PORT(port);
 }
 EXPORT_SYMBOL_GPL(dsa_8021q_rx_vid);
 
 /* Returns the decoded switch ID from the RX VID. */
 int dsa_8021q_rx_switch_id(u16 vid)
 {
-	return ((vid - DSA_8021Q_RX_VID_BASE) / DSA_MAX_PORTS);
+	return (vid & DSA_8021Q_SWITCH_ID_MASK) >> DSA_8021Q_SWITCH_ID_SHIFT;
 }
 EXPORT_SYMBOL_GPL(dsa_8021q_rx_switch_id);
 
 /* Returns the decoded port ID from the RX VID. */
 int dsa_8021q_rx_source_port(u16 vid)
 {
-	return ((vid - DSA_8021Q_RX_VID_BASE) % DSA_MAX_PORTS);
+	return (vid & DSA_8021Q_PORT_MASK) >> DSA_8021Q_PORT_SHIFT;
 }
 EXPORT_SYMBOL_GPL(dsa_8021q_rx_source_port);
 
-- 
2.17.1

Re: [PATCH net 1/2] net: dsa: tag_8021q: Change order of rx_vid setup

[Florian Fainelli]

On 5/28/19 3:50 PM, Vladimir Oltean wrote:
> From: Ioana Ciornei <ioana.ciornei@nxp.com>
> 
> The 802.1Q tagging performs an unbalanced setup in terms of RX VIDs on
> the CPU port. For the ingress path of a 802.1Q switch to work, the RX
> VID of a port needs to be seen as tagged egress on the CPU port.
> 
> While configuring the other front-panel ports to be part of this VID,
> for bridge scenarios, the untagged flag is applied even on the CPU port
> in dsa_switch_vlan_add.  This happens because DSA applies the same flags
> on the CPU port as on the (bridge-controlled) slave ports, and the
> effect in this case is that the CPU port tagged settings get deleted.
> 
> Instead of fixing DSA by introducing a way to control VLAN flags on the
> CPU port (and hence stop inheriting from the slave ports) - a hard,
> perhaps intractable problem - avoid this situation by moving the setup
> part of the RX VID on the CPU port after all the other front-panel ports
> have been added to the VID.
> 
> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> Fixes: f9bbe4477c30 ("net: dsa: Optional VLAN-based port separation for switches without tagging")

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net 2/2] net: dsa: tag_8021q: Create a stable binary format

[Florian Fainelli]

On 5/28/2019 3:50 PM, Vladimir Oltean wrote:
> Tools like tcpdump need to be able to decode the significance of fake
> VLAN headers that DSA uses to separate switch ports.
> 
> But currently these have no global significance - they are simply an
> ordered list of DSA_MAX_SWITCHES x DSA_MAX_PORTS numbers ending at 4095.
> 
> The reason why this is submitted as a fix is that the existing mapping
> of VIDs should not enter into a stable kernel, so we can pretend that
> only the new format exists. This way tcpdump won't need to try to make
> something out of the VLAN tags on 5.2 kernels.
> 
> Fixes: f9bbe4477c30 ("net: dsa: Optional VLAN-based port separation for switches without tagging")
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

This looks a lot nicer actually, and kudos for documenting the format.
-- 
Florian

Re: [PATCH net 2/2] net: dsa: tag_8021q: Create a stable binary format

[Vladimir Oltean]

On Wed, 29 May 2019 at 04:08, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
>
> On 5/28/2019 3:50 PM, Vladimir Oltean wrote:
> > Tools like tcpdump need to be able to decode the significance of fake
> > VLAN headers that DSA uses to separate switch ports.
> >
> > But currently these have no global significance - they are simply an
> > ordered list of DSA_MAX_SWITCHES x DSA_MAX_PORTS numbers ending at 4095.
> >
> > The reason why this is submitted as a fix is that the existing mapping
> > of VIDs should not enter into a stable kernel, so we can pretend that
> > only the new format exists. This way tcpdump won't need to try to make
> > something out of the VLAN tags on 5.2 kernels.
> >
> > Fixes: f9bbe4477c30 ("net: dsa: Optional VLAN-based port separation for switches without tagging")
> > Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
>
> This looks a lot nicer actually, and kudos for documenting the format.
> --
> Florian

Please don't merge this. The MBZ bit doesn't actually prevent the VID
from taking the reserved value of 0. I don't know what I was thinking.
I'll send out a v2 soon.

Thanks,
-Vladimir