net: dsa: tear down devlink port regions when tearing down the devlink port on error

Commit 93b0f24bf960 ("net: dsa: Do not make user port errors fatal")
decided it was fine to ignore errors on certain ports that fail to
probe, and go on with the ports that do probe fine.

Commit 1a4f6afa2cec ("net: dsa: Fix type was not set for devlink port")
noticed that devlink_port_type_eth_set(dlp, dp->slave); does not get
called, and devlink notices after a timeout of 3600 seconds and prints a
WARN_ON. So it went ahead to unregister the devlink port. And because
there exists an UNUSED port flavour, we actually re-register the devlink
port as UNUSED.

Commit 2bea0956940c ("net: dsa: Add devlink port regions support to
DSA") added devlink port regions, which are set up by the driver and not
by DSA.

When we trigger the devlink port deregistration and reregistration as
unused, devlink now prints another WARN_ON, from here:

devlink_port_unregister:
	WARN_ON(!list_empty(&devlink_port->region_list));

So the port still has regions, which makes sense, because they were set
up by the driver, and the driver doesn't know we're unregistering the
devlink port.

Somebody needs to tear them down, and optionally (actually it would be
nice, to be consistent) set them up again for the new devlink port.

But DSA's layering stays in our way quite badly here.

The options I've considered are:

1. Introduce a function in devlink to just change a port's type and
   flavour. No dice, devlink keeps a lot of state, it really wants the
   port to not be registered when you set its parameters, so changing
   anything can only be done by destroying what we currently have and
   recreating it.

2. Make DSA cache the parameters passed to dsa_devlink_port_region_create,
   and the region returned, keep those in a list, then when the devlink
   port unregister needs to take place, the existing devlink regions are
   destroyed by DSA, and we replay the creation of new regions using the
   cached parameters. Problem: mv88e6xxx keeps the region pointers in
   chip->ports[port].region, and these will remain stale after DSA frees
   them. There are many things DSA can do, but updating mv88e6xxx's
   private pointers is not one of them.

3. Just let the driver do it (i.e. introduce a very specific method
   called ds->ops->port_reinit_as_unused, which unregisters its devlink
   port devlink regions, then the old devlink port, then registers the
   new one, then the devlink port regions for it). While it does work,
   as opposed to the others, it's pretty horrible from an API
   perspective and we can do better.

4. Introduce a new pair of methods, ->port_setup and ->port_teardown,
   which in the case of mv88e6xxx must register and unregister the
   devlink port regions. Call these 2 methods when the port must be
   reinitialized as unused.

Naturally, I went for the 4th approach.

Fixes: 2bea0956940c ("net: dsa: Add devlink port regions support to DSA")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index fb10422d2c33d2720ee24dc0445a5b005ee7ea1a..8ab0be793811e18fd8b8c2455872f81b65619cec 100644 (file)
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3071,7 +3071,7 @@ static void mv88e6xxx_teardown(struct dsa_switch *ds)
 {
        mv88e6xxx_teardown_devlink_params(ds);
        dsa_devlink_resources_unregister(ds);
-       mv88e6xxx_teardown_devlink_regions(ds);
+       mv88e6xxx_teardown_devlink_regions_global(ds);
 }
 
 static int mv88e6xxx_setup(struct dsa_switch *ds)
@@ -3215,7 +3215,7 @@ unlock:
        if (err)
                goto out_resources;
 
-       err = mv88e6xxx_setup_devlink_regions(ds);
+       err = mv88e6xxx_setup_devlink_regions_global(ds);
        if (err)
                goto out_params;
 
@@ -3229,6 +3229,16 @@ out_resources:
        return err;
 }
 
+static int mv88e6xxx_port_setup(struct dsa_switch *ds, int port)
+{
+       return mv88e6xxx_setup_devlink_regions_port(ds, port);
+}
+
+static void mv88e6xxx_port_teardown(struct dsa_switch *ds, int port)
+{
+       mv88e6xxx_teardown_devlink_regions_port(ds, port);
+}
+
 /* prod_id for switch families which do not have a PHY model number */
 static const u16 family_prod_id_table[] = {
        [MV88E6XXX_FAMILY_6341] = MV88E6XXX_PORT_SWITCH_ID_PROD_6341,
@@ -6116,6 +6126,8 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
        .change_tag_protocol    = mv88e6xxx_change_tag_protocol,
        .setup                  = mv88e6xxx_setup,
        .teardown               = mv88e6xxx_teardown,
+       .port_setup             = mv88e6xxx_port_setup,
+       .port_teardown          = mv88e6xxx_port_teardown,
        .phylink_validate       = mv88e6xxx_validate,
        .phylink_mac_link_state = mv88e6xxx_serdes_pcs_get_state,
        .phylink_mac_config     = mv88e6xxx_mac_config,

diff --git a/drivers/net/dsa/mv88e6xxx/devlink.c b/drivers/net/dsa/mv88e6xxx/devlink.c
index 0c0f5ea6680c3ccd33c4f7d1bd7a22a1185ea647..381068395c63ba7cffad32c4cffbf85b5154dec3 100644 (file)
--- a/drivers/net/dsa/mv88e6xxx/devlink.c
+++ b/drivers/net/dsa/mv88e6xxx/devlink.c
@@ -647,26 +647,25 @@ static struct mv88e6xxx_region mv88e6xxx_regions[] = {
        },
 };
 
-static void
-mv88e6xxx_teardown_devlink_regions_global(struct mv88e6xxx_chip *chip)
+void mv88e6xxx_teardown_devlink_regions_global(struct dsa_switch *ds)
 {
+       struct mv88e6xxx_chip *chip = ds->priv;
        int i;
 
        for (i = 0; i < ARRAY_SIZE(mv88e6xxx_regions); i++)
                dsa_devlink_region_destroy(chip->regions[i]);
 }
 
-static void
-mv88e6xxx_teardown_devlink_regions_port(struct mv88e6xxx_chip *chip,
-                                       int port)
+void mv88e6xxx_teardown_devlink_regions_port(struct dsa_switch *ds, int port)
 {
+       struct mv88e6xxx_chip *chip = ds->priv;
+
        dsa_devlink_region_destroy(chip->ports[port].region);
 }
 
-static int mv88e6xxx_setup_devlink_regions_port(struct dsa_switch *ds,
-                                               struct mv88e6xxx_chip *chip,
-                                               int port)
+int mv88e6xxx_setup_devlink_regions_port(struct dsa_switch *ds, int port)
 {
+       struct mv88e6xxx_chip *chip = ds->priv;
        struct devlink_region *region;
 
        region = dsa_devlink_port_region_create(ds,
@@ -681,40 +680,10 @@ static int mv88e6xxx_setup_devlink_regions_port(struct dsa_switch *ds,
        return 0;
 }
 
-static void
-mv88e6xxx_teardown_devlink_regions_ports(struct mv88e6xxx_chip *chip)
-{
-       int port;
-
-       for (port = 0; port < mv88e6xxx_num_ports(chip); port++)
-               mv88e6xxx_teardown_devlink_regions_port(chip, port);
-}
-
-static int mv88e6xxx_setup_devlink_regions_ports(struct dsa_switch *ds,
-                                                struct mv88e6xxx_chip *chip)
-{
-       int port;
-       int err;
-
-       for (port = 0; port < mv88e6xxx_num_ports(chip); port++) {
-               err = mv88e6xxx_setup_devlink_regions_port(ds, chip, port);
-               if (err)
-                       goto out;
-       }
-
-       return 0;
-
-out:
-       while (port-- > 0)
-               mv88e6xxx_teardown_devlink_regions_port(chip, port);
-
-       return err;
-}
-
-static int mv88e6xxx_setup_devlink_regions_global(struct dsa_switch *ds,
-                                                 struct mv88e6xxx_chip *chip)
+int mv88e6xxx_setup_devlink_regions_global(struct dsa_switch *ds)
 {
        bool (*cond)(struct mv88e6xxx_chip *chip);
+       struct mv88e6xxx_chip *chip = ds->priv;
        struct devlink_region_ops *ops;
        struct devlink_region *region;
        u64 size;
@@ -753,30 +722,6 @@ out:
        return PTR_ERR(region);
 }
 
-int mv88e6xxx_setup_devlink_regions(struct dsa_switch *ds)
-{
-       struct mv88e6xxx_chip *chip = ds->priv;
-       int err;
-
-       err = mv88e6xxx_setup_devlink_regions_global(ds, chip);
-       if (err)
-               return err;
-
-       err = mv88e6xxx_setup_devlink_regions_ports(ds, chip);
-       if (err)
-               mv88e6xxx_teardown_devlink_regions_global(chip);
-
-       return err;
-}
-
-void mv88e6xxx_teardown_devlink_regions(struct dsa_switch *ds)
-{
-       struct mv88e6xxx_chip *chip = ds->priv;
-
-       mv88e6xxx_teardown_devlink_regions_ports(chip);
-       mv88e6xxx_teardown_devlink_regions_global(chip);
-}
-
 int mv88e6xxx_devlink_info_get(struct dsa_switch *ds,
                               struct devlink_info_req *req,
                               struct netlink_ext_ack *extack)

diff --git a/drivers/net/dsa/mv88e6xxx/devlink.h b/drivers/net/dsa/mv88e6xxx/devlink.h
index 3d72db3dcf950ea327cc793b360ae8195ff89ebc..65ce6a6858b9faa9f7b3d9e4964c518ea8591c32 100644 (file)
--- a/drivers/net/dsa/mv88e6xxx/devlink.h
+++ b/drivers/net/dsa/mv88e6xxx/devlink.h
@@ -12,8 +12,10 @@ int mv88e6xxx_devlink_param_get(struct dsa_switch *ds, u32 id,
                                struct devlink_param_gset_ctx *ctx);
 int mv88e6xxx_devlink_param_set(struct dsa_switch *ds, u32 id,
                                struct devlink_param_gset_ctx *ctx);
-int mv88e6xxx_setup_devlink_regions(struct dsa_switch *ds);
-void mv88e6xxx_teardown_devlink_regions(struct dsa_switch *ds);
+int mv88e6xxx_setup_devlink_regions_global(struct dsa_switch *ds);
+void mv88e6xxx_teardown_devlink_regions_global(struct dsa_switch *ds);
+int mv88e6xxx_setup_devlink_regions_port(struct dsa_switch *ds, int port);
+void mv88e6xxx_teardown_devlink_regions_port(struct dsa_switch *ds, int port);
 
 int mv88e6xxx_devlink_info_get(struct dsa_switch *ds,
                               struct devlink_info_req *req,

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 6e29c0e080f65dc2283383f453b523f03bea694e..d784e76113b8d4593f07b29f6cf4fe6465eae39b 100644 (file)
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -585,8 +585,16 @@ struct dsa_switch_ops {
        int     (*change_tag_protocol)(struct dsa_switch *ds, int port,
                                       enum dsa_tag_protocol proto);
 
+       /* Optional switch-wide initialization and destruction methods */
        int     (*setup)(struct dsa_switch *ds);
        void    (*teardown)(struct dsa_switch *ds);
+
+       /* Per-port initialization and destruction methods. Mandatory if the
+        * driver registers devlink port regions, optional otherwise.
+        */
+       int     (*port_setup)(struct dsa_switch *ds, int port);
+       void    (*port_teardown)(struct dsa_switch *ds, int port);
+
        u32     (*get_phy_flags)(struct dsa_switch *ds, int port);
 
        /*

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index fa88e58705f02a89b6e543abc1b7479f5b7a0089..f14897d9b31d9ba1242efc4451b991edfb5e15cf 100644 (file)
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -429,6 +429,7 @@ static int dsa_port_setup(struct dsa_port *dp)
 {
        struct devlink_port *dlp = &dp->devlink_port;
        bool dsa_port_link_registered = false;
+       struct dsa_switch *ds = dp->ds;
        bool dsa_port_enabled = false;
        int err = 0;
 
@@ -438,6 +439,12 @@ static int dsa_port_setup(struct dsa_port *dp)
        INIT_LIST_HEAD(&dp->fdbs);
        INIT_LIST_HEAD(&dp->mdbs);
 
+       if (ds->ops->port_setup) {
+               err = ds->ops->port_setup(ds, dp->index);
+               if (err)
+                       return err;
+       }
+
        switch (dp->type) {
        case DSA_PORT_TYPE_UNUSED:
                dsa_port_disable(dp);
@@ -480,8 +487,11 @@ static int dsa_port_setup(struct dsa_port *dp)
                dsa_port_disable(dp);
        if (err && dsa_port_link_registered)
                dsa_port_link_unregister_of(dp);
-       if (err)
+       if (err) {
+               if (ds->ops->port_teardown)
+                       ds->ops->port_teardown(ds, dp->index);
                return err;
+       }
 
        dp->setup = true;
 
@@ -533,11 +543,15 @@ static int dsa_port_devlink_setup(struct dsa_port *dp)
 static void dsa_port_teardown(struct dsa_port *dp)
 {
        struct devlink_port *dlp = &dp->devlink_port;
+       struct dsa_switch *ds = dp->ds;
        struct dsa_mac_addr *a, *tmp;
 
        if (!dp->setup)
                return;
 
+       if (ds->ops->port_teardown)
+               ds->ops->port_teardown(ds, dp->index);
+
        devlink_port_type_clear(dlp);
 
        switch (dp->type) {
@@ -581,6 +595,36 @@ static void dsa_port_devlink_teardown(struct dsa_port *dp)
        dp->devlink_port_setup = false;
 }
 
+/* Destroy the current devlink port, and create a new one which has the UNUSED
+ * flavour. At this point, any call to ds->ops->port_setup has been already
+ * balanced out by a call to ds->ops->port_teardown, so we know that any
+ * devlink port regions the driver had are now unregistered. We then call its
+ * ds->ops->port_setup again, in order for the driver to re-create them on the
+ * new devlink port.
+ */
+static int dsa_port_reinit_as_unused(struct dsa_port *dp)
+{
+       struct dsa_switch *ds = dp->ds;
+       int err;
+
+       dsa_port_devlink_teardown(dp);
+       dp->type = DSA_PORT_TYPE_UNUSED;
+       err = dsa_port_devlink_setup(dp);
+       if (err)
+               return err;
+
+       if (ds->ops->port_setup) {
+               /* On error, leave the devlink port registered,
+                * dsa_switch_teardown will clean it up later.
+                */
+               err = ds->ops->port_setup(ds, dp->index);
+               if (err)
+                       return err;
+       }
+
+       return 0;
+}
+
 static int dsa_devlink_info_get(struct devlink *dl,
                                struct devlink_info_req *req,
                                struct netlink_ext_ack *extack)
@@ -938,12 +982,9 @@ static int dsa_tree_setup_switches(struct dsa_switch_tree *dst)
        list_for_each_entry(dp, &dst->ports, list) {
                err = dsa_port_setup(dp);
                if (err) {
-                       dsa_port_devlink_teardown(dp);
-                       dp->type = DSA_PORT_TYPE_UNUSED;
-                       err = dsa_port_devlink_setup(dp);
+                       err = dsa_port_reinit_as_unused(dp);
                        if (err)
                                goto teardown;
-                       continue;
                }
        }