[net-next,6/7] net: dsa: don't do devlink port setup early

Commit Message

Jiri Pirko Aug. 25, 2022, 10:33 a.m. UTC

From: Jiri Pirko <[e-mail]>

Commit 3122433eb533 ("net: dsa: Register devlink ports before calling DSA driver setup()")
moved devlink port setup to be done early before driver setup()
was called. That is no longer needed, so move the devlink port
initialization back to dsa_port_setup(), as the first thing done there.

Note there is no longer needed to reinit port as unused if
dsa_port_setup() fails, as it unregisters the devlink port instance on
the error path.

Signed-off-by: Jiri Pirko <[e-mail]>
---
 net/dsa/dsa2.c | 183 ++++++++++++++++++++++---------------------------
 1 file changed, 81 insertions(+), 102 deletions(-)

Comments

Vladimir Oltean Aug. 25, 2022, 10:47 p.m. UTC | #1

On Thu, Aug 25, 2022 at 12:33:59PM +0200, Jiri Pirko wrote:
> Note there is no longer needed to reinit port as unused if
> dsa_port_setup() fails, as it unregisters the devlink port instance on
> the error path.
> @@ -957,8 +941,6 @@ static void dsa_switch_teardown(struct dsa_switch *ds)
>  	dsa_switch_unregister_notifier(ds);
>  
>  	if (ds->devlink) {
> -		dsa_switch_for_each_port(dp, ds)
> -			dsa_port_devlink_teardown(dp);
>  		devlink_free(ds->devlink);
>  		ds->devlink = NULL;
>  	}
> @@ -1010,11 +992,8 @@ static int dsa_tree_setup_ports(struct dsa_switch_tree *dst)
>  	list_for_each_entry(dp, &dst->ports, list) {
>  		if (dsa_port_is_user(dp) || dsa_port_is_unused(dp)) {
>  			err = dsa_port_setup(dp);
> -			if (err) {
> -				err = dsa_port_reinit_as_unused(dp);
> -				if (err)
> -					goto teardown;
> -			}
> +			if (err)
> +				goto teardown;
>  		}
>  	}

Please don't delete this, there is still a need.

First of all, dsa_port_setup() for user ports must not fail the probing
of the switch - see commit 86f8b1c01a0a ("net: dsa: Do not make user
port errors fatal").

Also, DSA exposes devlink regions for unused ports too - those have the
{DSA_PORT_TYPE,DEVLINK_PORT_FLAVOUR}_UNUSED flavor.

I also see some weird behavior when I intentionally break the probing of
some ports, but I haven't debugged to see exactly why, and it's likely
I won't have time to debug this week.

Jiri Pirko Aug. 26, 2022, 8:37 a.m. UTC | #2

Fri, Aug 26, 2022 at 12:47:24AM CEST, [e-mail] wrote:
>On Thu, Aug 25, 2022 at 12:33:59PM +0200, Jiri Pirko wrote:
>> Note there is no longer needed to reinit port as unused if
>> dsa_port_setup() fails, as it unregisters the devlink port instance on
>> the error path.
>> @@ -957,8 +941,6 @@ static void dsa_switch_teardown(struct dsa_switch *ds)
>>  	dsa_switch_unregister_notifier(ds);
>>  
>>  	if (ds->devlink) {
>> -		dsa_switch_for_each_port(dp, ds)
>> -			dsa_port_devlink_teardown(dp);
>>  		devlink_free(ds->devlink);
>>  		ds->devlink = NULL;
>>  	}
>> @@ -1010,11 +992,8 @@ static int dsa_tree_setup_ports(struct dsa_switch_tree *dst)
>>  	list_for_each_entry(dp, &dst->ports, list) {
>>  		if (dsa_port_is_user(dp) || dsa_port_is_unused(dp)) {
>>  			err = dsa_port_setup(dp);
>> -			if (err) {
>> -				err = dsa_port_reinit_as_unused(dp);
>> -				if (err)
>> -					goto teardown;
>> -			}
>> +			if (err)
>> +				goto teardown;
>>  		}
>>  	}
>
>Please don't delete this, there is still a need.
>
>First of all, dsa_port_setup() for user ports must not fail the probing
>of the switch - see commit 86f8b1c01a0a ("net: dsa: Do not make user
>port errors fatal").

Got it, will leave the unused port here. I will just use
dsa_port_setup() to init it. Something like:

  	list_for_each_entry(dp, &dst->ports, list) {
  		if (dsa_port_is_user(dp) || dsa_port_is_unused(dp)) {
  			err = dsa_port_setup(dp);
			if (err) {
				dp->type = DSA_PORT_TYPE_UNUSED;
				err = dsa_port_setup(dp);
				if (err)
					goto teardown;
			}
  		}


>
>Also, DSA exposes devlink regions for unused ports too - those have the
>{DSA_PORT_TYPE,DEVLINK_PORT_FLAVOUR}_UNUSED flavor.

Yep.


>
>I also see some weird behavior when I intentionally break the probing of
>some ports, but I haven't debugged to see exactly why, and it's likely
>I won't have time to debug this week.

Nevermind. Will wait until you have time to test it. Thanks!

Patch

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 644aecbe79a0..b53bc1a1aa83 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -447,6 +447,74 @@  static void dsa_tree_teardown_cpu_ports(struct dsa_switch_tree *dst)
 			dp->cpu_dp = NULL;
 }
 
+static int dsa_port_devlink_setup(struct dsa_port *dp)
+{
+	struct devlink_port *dlp = &dp->devlink_port;
+	struct dsa_switch_tree *dst = dp->ds->dst;
+	struct devlink_port_attrs attrs = {};
+	struct devlink *dl = dp->ds->devlink;
+	struct dsa_switch *ds = dp->ds;
+	const unsigned char *id;
+	unsigned char len;
+	int err;
+
+	memset(dlp, 0, sizeof(*dlp));
+	devlink_port_init(dl, dlp);
+
+	if (ds->ops->port_setup) {
+		err = ds->ops->port_setup(ds, dp->index);
+		if (err)
+			return err;
+	}
+
+	id = (const unsigned char *)&dst->index;
+	len = sizeof(dst->index);
+
+	attrs.phys.port_number = dp->index;
+	memcpy(attrs.switch_id.id, id, len);
+	attrs.switch_id.id_len = len;
+
+	switch (dp->type) {
+	case DSA_PORT_TYPE_UNUSED:
+		attrs.flavour = DEVLINK_PORT_FLAVOUR_UNUSED;
+		break;
+	case DSA_PORT_TYPE_CPU:
+		attrs.flavour = DEVLINK_PORT_FLAVOUR_CPU;
+		break;
+	case DSA_PORT_TYPE_DSA:
+		attrs.flavour = DEVLINK_PORT_FLAVOUR_DSA;
+		break;
+	case DSA_PORT_TYPE_USER:
+		attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL;
+		break;
+	}
+
+	devlink_port_attrs_set(dlp, &attrs);
+	err = devlink_port_register(dl, dlp, dp->index);
+	if (err) {
+		if (ds->ops->port_teardown)
+			ds->ops->port_teardown(ds, dp->index);
+		return err;
+	}
+	dp->devlink_port_setup = true;
+
+	return 0;
+}
+
+static void dsa_port_devlink_teardown(struct dsa_port *dp)
+{
+	struct devlink_port *dlp = &dp->devlink_port;
+	struct dsa_switch *ds = dp->ds;
+
+	if (dp->devlink_port_setup) {
+		devlink_port_unregister(dlp);
+		if (ds->ops->port_teardown)
+			ds->ops->port_teardown(ds, dp->index);
+		devlink_port_fini(dlp);
+	}
+	dp->devlink_port_setup = false;
+}
+
 static int dsa_port_setup(struct dsa_port *dp)
 {
 	struct devlink_port *dlp = &dp->devlink_port;
@@ -458,6 +526,10 @@  static int dsa_port_setup(struct dsa_port *dp)
 	if (dp->setup)
 		return 0;
 
+	err = dsa_port_devlink_setup(dp);
+	if (err)
+		return err;
+
 	switch (dp->type) {
 	case DSA_PORT_TYPE_UNUSED:
 		dsa_port_disable(dp);
@@ -512,64 +584,12 @@  static int dsa_port_setup(struct dsa_port *dp)
 		dsa_port_disable(dp);
 	if (err && dsa_port_link_registered)
 		dsa_shared_port_link_unregister_of(dp);
-	if (err)
-		return err;
-
-	dp->setup = true;
-
-	return 0;
-}
-
-static int dsa_port_devlink_setup(struct dsa_port *dp)
-{
-	struct devlink_port *dlp = &dp->devlink_port;
-	struct dsa_switch_tree *dst = dp->ds->dst;
-	struct devlink_port_attrs attrs = {};
-	struct devlink *dl = dp->ds->devlink;
-	struct dsa_switch *ds = dp->ds;
-	const unsigned char *id;
-	unsigned char len;
-	int err;
-
-	memset(dlp, 0, sizeof(*dlp));
-	devlink_port_init(dl, dlp);
-
-	if (ds->ops->port_setup) {
-		err = ds->ops->port_setup(ds, dp->index);
-		if (err)
-			return err;
-	}
-
-	id = (const unsigned char *)&dst->index;
-	len = sizeof(dst->index);
-
-	attrs.phys.port_number = dp->index;
-	memcpy(attrs.switch_id.id, id, len);
-	attrs.switch_id.id_len = len;
-
-	switch (dp->type) {
-	case DSA_PORT_TYPE_UNUSED:
-		attrs.flavour = DEVLINK_PORT_FLAVOUR_UNUSED;
-		break;
-	case DSA_PORT_TYPE_CPU:
-		attrs.flavour = DEVLINK_PORT_FLAVOUR_CPU;
-		break;
-	case DSA_PORT_TYPE_DSA:
-		attrs.flavour = DEVLINK_PORT_FLAVOUR_DSA;
-		break;
-	case DSA_PORT_TYPE_USER:
-		attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL;
-		break;
-	}
-
-	devlink_port_attrs_set(dlp, &attrs);
-	err = devlink_port_register(dl, dlp, dp->index);
 	if (err) {
-		if (ds->ops->port_teardown)
-			ds->ops->port_teardown(ds, dp->index);
+		dsa_port_devlink_teardown(dp);
 		return err;
 	}
-	dp->devlink_port_setup = true;
+
+	dp->setup = true;
 
 	return 0;
 }
@@ -604,31 +624,9 @@  static void dsa_port_teardown(struct dsa_port *dp)
 		break;
 	}
 
-	dp->setup = false;
-}
-
-static void dsa_port_devlink_teardown(struct dsa_port *dp)
-{
-	struct devlink_port *dlp = &dp->devlink_port;
-	struct dsa_switch *ds = dp->ds;
-
-	if (dp->devlink_port_setup) {
-		devlink_port_unregister(dlp);
-		if (ds->ops->port_teardown)
-			ds->ops->port_teardown(ds, dp->index);
-		devlink_port_fini(dlp);
-	}
-	dp->devlink_port_setup = false;
-}
-
-/* Destroy the current devlink port, and create a new one which has the UNUSED
- * flavour.
- */
-static int dsa_port_reinit_as_unused(struct dsa_port *dp)
-{
 	dsa_port_devlink_teardown(dp);
-	dp->type = DSA_PORT_TYPE_UNUSED;
-	return dsa_port_devlink_setup(dp);
+
+	dp->setup = false;
 }
 
 static int dsa_devlink_info_get(struct devlink *dl,
@@ -852,7 +850,6 @@  static int dsa_switch_setup(struct dsa_switch *ds)
 {
 	struct dsa_devlink_priv *dl_priv;
 	struct device_node *dn;
-	struct dsa_port *dp;
 	int err;
 
 	if (ds->setup)
@@ -875,18 +872,9 @@  static int dsa_switch_setup(struct dsa_switch *ds)
 	dl_priv = devlink_priv(ds->devlink);
 	dl_priv->ds = ds;
 
-	/* Setup devlink port instances now, so that the switch
-	 * setup() can register regions etc, against the ports
-	 */
-	dsa_switch_for_each_port(dp, ds) {
-		err = dsa_port_devlink_setup(dp);
-		if (err)
-			goto unregister_devlink_ports;
-	}
-
 	err = dsa_switch_register_notifier(ds);
 	if (err)
-		goto unregister_devlink_ports;
+		goto devlink_free;
 
 	ds->configure_vlan_while_not_filtering = true;
 
@@ -927,9 +915,7 @@  static int dsa_switch_setup(struct dsa_switch *ds)
 		ds->ops->teardown(ds);
 unregister_notifier:
 	dsa_switch_unregister_notifier(ds);
-unregister_devlink_ports:
-	dsa_switch_for_each_port(dp, ds)
-		dsa_port_devlink_teardown(dp);
+devlink_free:
 	devlink_free(ds->devlink);
 	ds->devlink = NULL;
 	return err;
@@ -937,8 +923,6 @@  static int dsa_switch_setup(struct dsa_switch *ds)
 
 static void dsa_switch_teardown(struct dsa_switch *ds)
 {
-	struct dsa_port *dp;
-
 	if (!ds->setup)
 		return;
 
@@ -957,8 +941,6 @@  static void dsa_switch_teardown(struct dsa_switch *ds)
 	dsa_switch_unregister_notifier(ds);
 
 	if (ds->devlink) {
-		dsa_switch_for_each_port(dp, ds)
-			dsa_port_devlink_teardown(dp);
 		devlink_free(ds->devlink);
 		ds->devlink = NULL;
 	}
@@ -1010,11 +992,8 @@  static int dsa_tree_setup_ports(struct dsa_switch_tree *dst)
 	list_for_each_entry(dp, &dst->ports, list) {
 		if (dsa_port_is_user(dp) || dsa_port_is_unused(dp)) {
 			err = dsa_port_setup(dp);
-			if (err) {
-				err = dsa_port_reinit_as_unused(dp);
-				if (err)
-					goto teardown;
-			}
+			if (err)
+				goto teardown;
 		}
 	}