Message-ID: <CA+h21hpAVz0v6teevSs=EVsGaezOFK=BLa=uqZb7zKO8QA_2fw@mail.gmail.com>
Date:   Fri, 1 May 2020 16:13:16 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        "David S. Miller" <davem@...emloft.net>
Cc:     Jiri Pirko <jiri@...nulli.us>, Ido Schimmel <idosch@...sch.org>,
        Jakub Kicinski <kuba@...nel.org>,
        netdev <netdev@...r.kernel.org>, Li Yang <leoyang.li@....com>,
        Nikolay Aleksandrov <nikolay@...ulusnetworks.com>,
        Roopa Prabhu <roopa@...ulusnetworks.com>
Subject: Re: [PATCH v2 net-next 4/4] net: dsa: sja1105: implement cross-chip
 bridging operations

On Thu, 30 Apr 2020 at 23:25, Vladimir Oltean <olteanv@...il.com> wrote:
>
> From: Vladimir Oltean <vladimir.oltean@....com>
>
> sja1105 uses dsa_8021q for DSA tagging, a format which is VLAN at heart
> and which is compatible with cascading. A complete description of this
> tagging format is in net/dsa/tag_8021q.c, but a quick summary is that
> each external-facing port tags incoming frames with a unique pvid, and
> this special VLAN is transmitted as tagged towards the inside of the
> system, and as untagged towards the exterior. The tag encodes the switch
> id and the source port index.
>
> This means that cross-chip bridging for dsa_8021q only entails adding
> the dsa_8021q pvids of one switch to the RX filter of the other
> switches. Everything else falls naturally into place, as long as the
> bottom-end of ports (the leaves in the tree) is comprised exclusively of
> dsa_8021q-compatible (i.e. sja1105 switches). Otherwise, there would be
> a chance that a front-panel switch transmits a packet tagged with a
> dsa_8021q header, header which it wouldn't be able to remove, and which
> would hence "leak" out.
>
> The only use case I tested (due to lack of board availability) was when
> the sja1105 switches are part of disjoint trees (however, this doesn't
> change the fact that multiple sja1105 switches still need unique switch
> identifiers in such a system). But in principle, even "true" single-tree
> setups (with DSA links) should work just as fine, except for a small
> change which I can't test: dsa_towards_port should be used instead of
> dsa_upstream_port (I made the assumption that the routing port that any
> sja1105 should use towards its neighbours is the CPU port. That might
> not hold true in other setups).
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
> Changes in v2:
> Now replaying the crosschip operations when toggling the VLAN filtering
> state (aka when we need to hide the dsa_8021q VLANs, as the VLAN
> filtering table becomes visible to the user so it needs to be cleared -
> and perhaps restored afterwards).
>
>  drivers/net/dsa/sja1105/sja1105.h      |   9 ++
>  drivers/net/dsa/sja1105/sja1105_main.c | 183 +++++++++++++++++++++++++
>  2 files changed, 192 insertions(+)
>
> diff --git a/drivers/net/dsa/sja1105/sja1105.h b/drivers/net/dsa/sja1105/sja1105.h
> index 2f62942692ec..6b4fad1216bb 100644
> --- a/drivers/net/dsa/sja1105/sja1105.h
> +++ b/drivers/net/dsa/sja1105/sja1105.h
> @@ -127,6 +127,14 @@ struct sja1105_flow_block {
>         bool l2_policer_used[SJA1105_NUM_L2_POLICERS];
>  };
>
> +struct sja1105_crosschip_info {
> +       struct list_head list;
> +       int tree_index;
> +       int sw_index;
> +       int other_port;
> +       struct net_device *br;
> +};
> +
>  struct sja1105_private {
>         struct sja1105_static_config static_config;
>         bool rgmii_rx_delay[SJA1105_NUM_PORTS];
> @@ -135,6 +143,7 @@ struct sja1105_private {
>         struct gpio_desc *reset_gpio;
>         struct spi_device *spidev;
>         struct dsa_switch *ds;
> +       struct list_head crosschip_ports;
>         struct sja1105_flow_block flow_block;
>         struct sja1105_port ports[SJA1105_NUM_PORTS];
>         /* Serializes transmission of management frames so that
> diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
> index 472f4eb20c49..95e00e03d05b 100644
> --- a/drivers/net/dsa/sja1105/sja1105_main.c
> +++ b/drivers/net/dsa/sja1105/sja1105_main.c
> @@ -25,6 +25,8 @@
>  #include "sja1105_sgmii.h"
>  #include "sja1105_tas.h"
>
> +static const struct dsa_switch_ops sja1105_switch_ops;
> +
>  static void sja1105_hw_reset(struct gpio_desc *gpio, unsigned int pulse_len,
>                              unsigned int startup_delay)
>  {
> @@ -1790,6 +1792,176 @@ static int sja1105_vlan_apply(struct sja1105_private *priv, int port, u16 vid,
>         return 0;
>  }
>
> +static int sja1105_crosschip_bridge_member(struct dsa_switch *ds,
> +                                          int tree_index, int sw_index,
> +                                          int other_port,
> +                                          struct net_device *br, bool enabled)

Please don't apply this yet. I need to find a way to make the
crosschip information symmetrical for all switches (at the moment one
switch gets the crosschip_ports list populated and the others don't),
otherwise this doesn't play well with some more features I'm cooking.

> +{
> +       struct dsa_switch *other_ds = dsa_switch_find(tree_index, sw_index);
> +       struct sja1105_private *priv = ds->priv;
> +       struct sja1105_private *other_priv;
> +       u16 rx_vid, other_rx_vid;
> +       int cpu = -1, other_cpu;
> +       int port, rc;
> +
> +       if (other_ds->ops != &sja1105_switch_ops)
> +               return 0;
> +
> +       other_cpu = dsa_upstream_port(other_ds, other_port);
> +       other_priv = other_ds->priv;
> +
> +       /* Make traffic from all local ports enslaved to @br be received
> +        * by @other_ds. This means that our @rx_vid needs to be installed
> +        * on @other_ds's CPU port and user ports. The user ports should be
> +        * egress-untagged so that the can pop the dsa_8021q VLAN. But the
> +        * @other_cpu can be either egress-tagged or untagged: it doesn't
> +        * matter, since it should never egress a frame having our @rx_vid.
> +        */
> +       for (port = 0; port < SJA1105_NUM_PORTS; port++) {
> +               if (!dsa_is_user_port(ds, port))
> +                       continue;
> +               if (dsa_to_port(ds, port)->bridge_dev != br)
> +                       continue;
> +
> +               cpu = dsa_upstream_port(ds, port);
> +
> +               rx_vid = dsa_8021q_rx_vid(ds, port);
> +
> +               /* The VLANs on @other_cpu port should be refcounted, because
> +                * we don't want to remove our @rx_vid just yet when a single
> +                * remote port leaves. But that's tricky to implement, so just
> +                * keep it simple and leave the VLANs installed on the CPU
> +                * ports when leaving. Forwarding will still be denied, as it
> +                * should be, because the @rx_vid has been removed from the
> +                * remote switch's external port.
> +                */
> +               if (enabled) {
> +                       /* @rx_vid of local @ds port @port goes to @other_cpu
> +                        * of @other_ds
> +                        */
> +                       rc = sja1105_vlan_apply(other_priv, other_cpu, rx_vid,
> +                                               enabled, false);
> +                       if (rc < 0)
> +                               return rc;
> +               }
> +
> +               /* @rx_vid of local @ds port @port goes to @other_port of
> +                * @other_ds
> +                */
> +               rc = sja1105_vlan_apply(other_priv, other_port,
> +                                       rx_vid, enabled, true);
> +               if (rc < 0)
> +                       return rc;
> +       }
> +
> +       /* Shorthand way of saying that we had no ports enslaved to @br. Also a
> +        * convenient way of getting a reference to the CPU port via
> +        * dsa_upstream_port(), which needs a valid user port as argument.
> +        */
> +       if (cpu == -1)
> +               return 0;
> +
> +       /* Make traffic from the remote @other_ds switch's @other_port be
> +        * accepted by the local @ds. This means that its @rx_vid needs to be
> +        * installed on our local CPU port as well as local ports that are
> +        * members of @br.
> +        */
> +       other_rx_vid = dsa_8021q_rx_vid(other_ds, other_port);
> +
> +       for (port = 0; port < SJA1105_NUM_PORTS; port++) {
> +               if (!dsa_is_user_port(ds, port))
> +                       continue;
> +               if (dsa_to_port(ds, port)->bridge_dev != br)
> +                       continue;
> +
> +               /* @other_rx_vid of @other_ds port @other_port goes to port
> +                * @port of local @ds.
> +                */
> +               rc = sja1105_vlan_apply(priv, port, other_rx_vid, enabled,
> +                                       true);
> +               if (rc < 0)
> +                       return rc;
> +       }
> +
> +       /* The comment above regarding VLAN refcounting applies here too */
> +       if (!enabled)
> +               return 0;
> +
> +       /* @other_rx_vid of @other_ds port @other_port goes to port @cpu of
> +        * local @ds
> +        */
> +       return sja1105_vlan_apply(priv, cpu, other_rx_vid, enabled, false);
> +}
> +
> +static int sja1105_crosschip_bridge_join(struct dsa_switch *ds,
> +                                        int tree_index, int sw_index,
> +                                        int other_port, struct net_device *br)
> +{
> +       struct sja1105_private *priv = ds->priv;
> +       struct sja1105_crosschip_info *c;
> +       int rc;
> +
> +       if (!br_vlan_enabled(br)) {
> +               rc = sja1105_crosschip_bridge_member(ds, tree_index, sw_index,
> +                                                    other_port, br, true);
> +               if (rc)
> +                       return rc;
> +       }
> +
> +       c = kzalloc(sizeof(*c), GFP_KERNEL);
> +       if (!c)
> +               return -ENOMEM;
> +
> +       c->tree_index = tree_index;
> +       c->sw_index = sw_index;
> +       c->other_port = other_port;
> +       c->br = br;
> +
> +       list_add(&c->list, &priv->crosschip_ports);
> +
> +       return 0;
> +}
> +
> +static void sja1105_crosschip_bridge_leave(struct dsa_switch *ds,
> +                                          int tree_index, int sw_index,
> +                                          int other_port,
> +                                          struct net_device *br)
> +{
> +       struct sja1105_private *priv = ds->priv;
> +       struct sja1105_crosschip_info *c, *n;
> +
> +       if (!br_vlan_enabled(br))
> +               sja1105_crosschip_bridge_member(ds, tree_index, sw_index,
> +                                               other_port, br, false);
> +
> +       list_for_each_entry_safe(c, n, &priv->crosschip_ports, list) {
> +               if (c->tree_index == tree_index && c->sw_index == sw_index &&
> +                   c->other_port == other_port && c->br == br) {
> +                       list_del(&c->list);
> +                       kfree(c);
> +                       break;
> +               }
> +       }
> +}
> +
> +static int sja1105_apply_crosschip_vlans(struct dsa_switch *ds, bool enabled)
> +{
> +       struct sja1105_private *priv = ds->priv;
> +       struct sja1105_crosschip_info *c;
> +       int rc;
> +
> +       list_for_each_entry(c, &priv->crosschip_ports, list) {
> +               rc = sja1105_crosschip_bridge_member(ds, c->tree_index,
> +                                                    c->sw_index,
> +                                                    c->other_port, c->br,
> +                                                    enabled);
> +               if (rc)
> +                       break;
> +       }
> +
> +       return rc;
> +}
> +
>  static int sja1105_setup_8021q_tagging(struct dsa_switch *ds, bool enabled)
>  {
>         int rc, i;
> @@ -1802,6 +1974,13 @@ static int sja1105_setup_8021q_tagging(struct dsa_switch *ds, bool enabled)
>                         return rc;
>                 }
>         }
> +       /* Replay the crosschip operations whenever we need to enable or
> +        * disable DSA tagging.
> +        */
> +       rc = sja1105_apply_crosschip_vlans(ds, enabled);
> +       if (rc)
> +               return rc;
> +
>         dev_info(ds->dev, "%s switch tagging\n",
>                  enabled ? "Enabled" : "Disabled");
>         return 0;
> @@ -2359,6 +2538,8 @@ static const struct dsa_switch_ops sja1105_switch_ops = {
>         .port_policer_del       = sja1105_port_policer_del,
>         .cls_flower_add         = sja1105_cls_flower_add,
>         .cls_flower_del         = sja1105_cls_flower_del,
> +       .crosschip_bridge_join  = sja1105_crosschip_bridge_join,
> +       .crosschip_bridge_leave = sja1105_crosschip_bridge_leave,
>  };
>
>  static int sja1105_check_device_id(struct sja1105_private *priv)
> @@ -2461,6 +2642,8 @@ static int sja1105_probe(struct spi_device *spi)
>         mutex_init(&priv->ptp_data.lock);
>         mutex_init(&priv->mgmt_lock);
>
> +       INIT_LIST_HEAD(&priv->crosschip_ports);
> +
>         sja1105_tas_setup(ds);
>         sja1105_flower_setup(ds);
>
> --
> 2.17.1
>

Thanks,
-Vladimir

Powered by blists - more mailing lists