Date:   Fri,  3 Sep 2021 02:17:38 +0300
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     netdev@...r.kernel.org
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>
Subject: [RFC PATCH net] net: dsa: tear down devlink port regions when tearing down the devlink port on error

Commit 86f8b1c01a0a ("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 fb6ec87f7229 ("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 3700 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 08156ba430b4 ("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. It's pretty horrible, but the other
   methods just don't seem to work.

Fixes: 08156ba430b4 ("net: dsa: Add devlink port regions support to DSA")
Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
---
 drivers/net/dsa/mv88e6xxx/chip.c    |  1 +
 drivers/net/dsa/mv88e6xxx/devlink.c | 16 ++++++++++++++++
 drivers/net/dsa/mv88e6xxx/devlink.h |  1 +
 include/net/dsa.h                   |  9 +++++++++
 net/dsa/dsa.c                       |  6 ++++++
 net/dsa/dsa2.c                      | 22 +++++++++++++++++-----
 6 files changed, 50 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index c45ca2473743..76f580a12bac 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -6173,6 +6173,7 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
 	.crosschip_lag_leave	= mv88e6xxx_crosschip_lag_leave,
 	.port_bridge_tx_fwd_offload = mv88e6xxx_bridge_tx_fwd_offload,
 	.port_bridge_tx_fwd_unoffload = mv88e6xxx_bridge_tx_fwd_unoffload,
+	.port_reinit_as_unused	= mv88e6xxx_port_reinit_as_unused,
 };
 
 static int mv88e6xxx_register_switch(struct mv88e6xxx_chip *chip)
diff --git a/drivers/net/dsa/mv88e6xxx/devlink.c b/drivers/net/dsa/mv88e6xxx/devlink.c
index 0c0f5ea6680c..6c928b6af6d0 100644
--- a/drivers/net/dsa/mv88e6xxx/devlink.c
+++ b/drivers/net/dsa/mv88e6xxx/devlink.c
@@ -792,3 +792,19 @@ int mv88e6xxx_devlink_info_get(struct dsa_switch *ds,
 					      DEVLINK_INFO_VERSION_GENERIC_ASIC_ID,
 					      chip->info->name);
 }
+
+int mv88e6xxx_port_reinit_as_unused(struct dsa_switch *ds, int port)
+{
+	struct dsa_port *dp = dsa_to_port(ds, port);
+	struct mv88e6xxx_chip *chip = ds->priv;
+	int err;
+
+	mv88e6xxx_teardown_devlink_regions_port(chip, port);
+	dsa_port_devlink_teardown(dp);
+	dp->type = DSA_PORT_TYPE_UNUSED;
+	err = dsa_port_devlink_setup(dp);
+	if (err)
+		return err;
+
+	return mv88e6xxx_setup_devlink_regions_port(ds, chip, port);
+}
diff --git a/drivers/net/dsa/mv88e6xxx/devlink.h b/drivers/net/dsa/mv88e6xxx/devlink.h
index 3d72db3dcf95..5a23e115f23f 100644
--- a/drivers/net/dsa/mv88e6xxx/devlink.h
+++ b/drivers/net/dsa/mv88e6xxx/devlink.h
@@ -14,6 +14,7 @@ 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_port_reinit_as_unused(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 f9a17145255a..046dbebbf647 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -846,6 +846,12 @@ struct dsa_switch_ops {
 						   enum devlink_sb_pool_type pool_type,
 						   u32 *p_cur, u32 *p_max);
 
+	/* Hook for drivers to tear down their port devlink regions when a
+	 * port failed to register and its devlink port must be torn down and
+	 * reinitialized by DSA as unused.
+	 */
+	int	(*port_reinit_as_unused)(struct dsa_switch *ds, int port);
+
 	/*
 	 * MTU change functionality. Switches can also adjust their MRU through
 	 * this method. By MTU, one understands the SDU (L2 payload) length.
@@ -961,6 +967,9 @@ static inline int dsa_devlink_port_to_port(struct devlink_port *port)
 	return port->index;
 }
 
+int dsa_port_devlink_setup(struct dsa_port *dp);
+void dsa_port_devlink_teardown(struct dsa_port *dp);
+
 struct dsa_switch_driver {
 	struct list_head	list;
 	const struct dsa_switch_ops *ops;
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 1dc45e40f961..4d9e5fe5bbb7 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -440,6 +440,12 @@ dsa_devlink_port_region_create(struct dsa_switch *ds,
 {
 	struct dsa_port *dp = dsa_to_port(ds, port);
 
+	/* Make sure drivers provide the method for cleaning this up when the
+	 * port might need to be torn down at runtime.
+	 */
+	if (WARN_ON(!ds->ops->port_reinit_as_unused))
+		return NULL;
+
 	return devlink_port_region_create(&dp->devlink_port, ops,
 					  region_max_snapshots,
 					  region_size);
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 1b2b25d7bd02..bc1da54fcf4c 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -488,7 +488,7 @@ static int dsa_port_setup(struct dsa_port *dp)
 	return 0;
 }
 
-static int dsa_port_devlink_setup(struct dsa_port *dp)
+int dsa_port_devlink_setup(struct dsa_port *dp)
 {
 	struct devlink_port *dlp = &dp->devlink_port;
 	struct dsa_switch_tree *dst = dp->ds->dst;
@@ -529,6 +529,7 @@ static int dsa_port_devlink_setup(struct dsa_port *dp)
 
 	return err;
 }
+EXPORT_SYMBOL_GPL(dsa_port_devlink_setup);
 
 static void dsa_port_teardown(struct dsa_port *dp)
 {
@@ -572,7 +573,7 @@ static void dsa_port_teardown(struct dsa_port *dp)
 	dp->setup = false;
 }
 
-static void dsa_port_devlink_teardown(struct dsa_port *dp)
+void dsa_port_devlink_teardown(struct dsa_port *dp)
 {
 	struct devlink_port *dlp = &dp->devlink_port;
 
@@ -580,6 +581,19 @@ static void dsa_port_devlink_teardown(struct dsa_port *dp)
 		devlink_port_unregister(dlp);
 	dp->devlink_port_setup = false;
 }
+EXPORT_SYMBOL_GPL(dsa_port_devlink_teardown);
+
+static int dsa_port_reinit_as_unused(struct dsa_port *dp)
+{
+	struct dsa_switch *ds = dp->ds;
+
+	if (ds->ops->port_reinit_as_unused)
+		return ds->ops->port_reinit_as_unused(ds, dp->index);
+
+	dsa_port_devlink_teardown(dp);
+	dp->type = DSA_PORT_TYPE_UNUSED;
+	return dsa_port_devlink_setup(dp);
+}
 
 static int dsa_devlink_info_get(struct devlink *dl,
 				struct devlink_info_req *req,
@@ -911,9 +925,7 @@ 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;
-- 
2.25.1

Powered by blists - more mailing lists