Re: [RFC PATCH net-next] net: dsa: tag_qca: Check for upstream VLAN tag

On 6/5/2021 5:53 PM, Vladimir Oltean wrote:

Hi Matthew,

On Sat, Jun 05, 2021 at 11:39:24PM +0100, Matthew Hagan wrote:

quoted

On 05/06/2021 21:35, Andrew Lunn wrote:

quoted

quoted

The tested case is a Meraki MX65 which features two QCA8337 switches with
their CPU ports attached to a BCM58625 switch ports 4 and 5 respectively.

Hi Matthew

The BCM58625 switch is also running DSA? What does you device tree
look like? I know Florian has used two broadcom switches in cascade
and did not have problems.

    Andrew

Hi Andrew

I did discuss this with Florian, who recommended I submit the changes. Can
confirm the b53 DSA driver is being used. The issue here is that tagging
must occur on all ports. We can't selectively disable for ports 4 and 5
where the QCA switches are attached, thus this patch is required to get
things working.

Setup is like this:
                       sw0p2     sw0p4            sw1p2     sw1p4 
    wan1    wan2  sw0p1  +  sw0p3  +  sw0p5  sw1p1  +  sw1p3  +  sw1p5
     +       +      +    |    +    |    +      +    |    +    |    +
     |       |      |    |    |    |    |      |    |    |    |    |
     |       |    +--+----+----+----+----+-+ +--+----+----+----+----+-+
     |       |    |         QCA8337        | |        QCA8337         |
     |       |    +------------+-----------+ +-----------+------------+
     |       |             sw0 |                     sw1 |
+----+-------+-----------------+-------------------------+------------+
|    0       1    BCM58625     4                         5            |
+----+-------+-----------------+-------------------------+------------+

It is a bit unconventional for the upstream Broadcom switch, which is a
DSA master of its own, to insert a VLAN ID of zero out of the blue,
especially if it operates in standalone mode. Supposedly sw0 and sw1 are
not under a bridge net device, are they?

This is because of the need (or desire) to always tag the CPU port
regardless of the untagged VLAN that one of its downstream port is being
added to. Despite talking with Matthew about this before, I had not
realized that dsa_port_is_cpu() will return true for ports 4 and 5 when
a VLAN is added to one of the two QCA8337 switches because from the
perspective of that switch, those ports have been set as DSA_PORT_TYPE_CPU.

This may also mean that b53_setup() needs fixing as well while it
iterates over the ports of the switch though I am not sure how we could
fix that yet.

If I'm not mistaken, this patch should solve your problem?

How about this:

diff --git a/drivers/net/dsa/b53/b53_common.c

b/drivers/net/dsa/b53/b53_common.c
index 3ca6b394dd5f..6dfcff9018fd 100644

--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c

@@ -1455,6 +1455,22 @@ static int b53_vlan_prepare(struct dsa_switch

*ds, int port,
        return 0;
 }

+static inline bool b53_vlan_can_untag(struct dsa_switch *ds, int port)
+{
+       /* If this switch port is a CPU port */
+       if (dsa_is_cpu_port(ds, port)) {
+               /* We permit untagging to be configured if it is the DSA
+                * master of another switch (cascading).
+                */
+               if (dsa_slave_dev_check(dsa_to_port(ds, port)->master))
+                       return true;
+
+               return false;
+       }
+
+       return true;
+}
+
 int b53_vlan_add(struct dsa_switch *ds, int port,
                 const struct switchdev_obj_port_vlan *vlan,
                 struct netlink_ext_ack *extack)

@@ -1477,7 +1493,7 @@ int b53_vlan_add(struct dsa_switch *ds, int port,
                untagged = true;

        vl->members |= BIT(port);
-       if (untagged && !dsa_is_cpu_port(ds, port))
+       if (untagged && b53_vlan_can_untag(ds, port))
                vl->untag |= BIT(port);
        else
                vl->untag &= ~BIT(port);

@@ -1514,7 +1530,7 @@ int b53_vlan_del(struct dsa_switch *ds, int port,
        if (pvid == vlan->vid)
                pvid = b53_default_pvid(dev);

-       if (untagged && !dsa_is_cpu_port(ds, port))
+       if (untagged && b53_vlan_can_untag(ds, port))
                vl->untag &= ~(BIT(port));

        b53_set_vlan_entry(dev, vlan->vid, vl);

-- 
Florian