[patch net-next 0/7] devlink: sanitize per-port region creation/destruction

[patch net-next 0/7] devlink: sanitize per-port region creation/destruction

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Currently the only user of per-port devlink regions is DSA. All drivers
that use regions register them before devlink registration. For DSA,
this was not possible as the internals of struct devlink_port needed for
region creation are initialized during port registration.

This introduced a mismatch in creation flow of devlink and devlink port
regions. As you can see, it causes the DSA driver to make the port
init/exit flow a bit cumbersome.

Fix this by introducing port_init/fini() which could be optionally
called by drivers like DSA, to prepare struct devlink_port to be used
for region creation purposes before devlink port register is called.

Force similar limitation as for devlink params and disallow to create
regions after devlink or devlink port are registered. That allows to
simplify the devlink region internal API to not to rely on
devlink->lock.

Tested with dsa_look with out-of-tree patch implementing devlink port
regions there kindly provided by Vladimir Oltean.

Jiri Pirko (7):
  netdevsim: don't re-create dummy region during devlink reload
  net: devlink: introduce port registered assert helper and use it
  net: devlink: introduce a flag to indicate devlink port being
    registered
  net: devlink: add port_init/fini() helpers to allow
    pre-register/post-unregister functions
  net: dsa: move port_setup/teardown to be called outside devlink port
    registered area
  net: dsa: don't do devlink port setup early
  net: devlink: convert region create/destroy() to be forbidden on
    registered devlink/port

 drivers/net/ethernet/mellanox/mlx4/crdump.c |  20 +-
 drivers/net/netdevsim/dev.c                 |  18 +-
 include/net/devlink.h                       |  12 +-
 net/core/devlink.c                          | 154 ++++++++--------
 net/dsa/dsa2.c                              | 191 ++++++++------------
 5 files changed, 178 insertions(+), 217 deletions(-)

-- 
2.37.1

[patch net-next 1/7] netdevsim: don't re-create dummy region during devlink reload

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Follow the pattern of other drivers and do not create/destroy region for
registered devlink instance.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 drivers/net/netdevsim/dev.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index e88f783c297e..cd3debc9921a 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -1460,13 +1460,9 @@ static int nsim_dev_reload_create(struct nsim_dev *nsim_dev,
 
 	nsim_devlink_param_load_driverinit_values(devlink);
 
-	err = nsim_dev_dummy_region_init(nsim_dev, devlink);
-	if (err)
-		return err;
-
 	err = nsim_dev_traps_init(devlink);
 	if (err)
-		goto err_dummy_region_exit;
+		return err;
 
 	nsim_dev->fib_data = nsim_fib_create(devlink, extack);
 	if (IS_ERR(nsim_dev->fib_data)) {
@@ -1507,8 +1503,6 @@ static int nsim_dev_reload_create(struct nsim_dev *nsim_dev,
 	nsim_fib_destroy(devlink, nsim_dev->fib_data);
 err_traps_exit:
 	nsim_dev_traps_exit(devlink);
-err_dummy_region_exit:
-	nsim_dev_dummy_region_exit(nsim_dev);
 	return err;
 }
 
@@ -1648,7 +1642,6 @@ static void nsim_dev_reload_destroy(struct nsim_dev *nsim_dev)
 	nsim_dev_health_exit(nsim_dev);
 	nsim_fib_destroy(devlink, nsim_dev->fib_data);
 	nsim_dev_traps_exit(devlink);
-	nsim_dev_dummy_region_exit(nsim_dev);
 }
 
 void nsim_drv_remove(struct nsim_bus_dev *nsim_bus_dev)
@@ -1662,6 +1655,7 @@ void nsim_drv_remove(struct nsim_bus_dev *nsim_bus_dev)
 
 	nsim_bpf_dev_exit(nsim_dev);
 	nsim_dev_debugfs_exit(nsim_dev);
+	nsim_dev_dummy_region_exit(nsim_dev);
 	devlink_params_unregister(devlink, nsim_devlink_params,
 				  ARRAY_SIZE(nsim_devlink_params));
 	devl_resources_unregister(devlink);
-- 
2.37.1

[patch net-next 2/7] net: devlink: introduce port registered assert helper and use it

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Instead of checking devlink_port->devlink pointer for not being NULL
which indicates that devlink port is registered, put this check to new
pair of helpers similar to what we have for devlink and use them in
other functions.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 net/core/devlink.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 6854f574e3ae..2737dad89f51 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -371,6 +371,11 @@ static struct devlink *devlink_get_from_attrs(struct net *net,
 	return ERR_PTR(-ENODEV);
 }
 
+#define ASSERT_DEVLINK_PORT_REGISTERED(devlink_port)				\
+	WARN_ON_ONCE(!(devlink_port)->devlink)
+#define ASSERT_DEVLINK_PORT_NOT_REGISTERED(devlink_port)			\
+	WARN_ON_ONCE((devlink_port)->devlink)
+
 static struct devlink_port *devlink_port_get_by_index(struct devlink *devlink,
 						      unsigned int port_index)
 {
@@ -9771,7 +9776,8 @@ int devl_port_register(struct devlink *devlink,
 	if (devlink_port_index_exists(devlink, port_index))
 		return -EEXIST;
 
-	WARN_ON(devlink_port->devlink);
+	ASSERT_DEVLINK_PORT_NOT_REGISTERED(devlink_port);
+
 	devlink_port->devlink = devlink;
 	devlink_port->index = port_index;
 	spin_lock_init(&devlink_port->type_lock);
@@ -9854,8 +9860,8 @@ static void __devlink_port_type_set(struct devlink_port *devlink_port,
 				    enum devlink_port_type type,
 				    void *type_dev)
 {
-	if (WARN_ON(!devlink_port->devlink))
-		return;
+	ASSERT_DEVLINK_PORT_REGISTERED(devlink_port);
+
 	devlink_port_type_warn_cancel(devlink_port);
 	spin_lock_bh(&devlink_port->type_lock);
 	devlink_port->type = type;
@@ -9974,8 +9980,8 @@ void devlink_port_attrs_set(struct devlink_port *devlink_port,
 {
 	int ret;
 
-	if (WARN_ON(devlink_port->devlink))
-		return;
+	ASSERT_DEVLINK_PORT_NOT_REGISTERED(devlink_port);
+
 	devlink_port->attrs = *attrs;
 	ret = __devlink_port_attrs_set(devlink_port, attrs->flavour);
 	if (ret)
@@ -9998,8 +10004,8 @@ void devlink_port_attrs_pci_pf_set(struct devlink_port *devlink_port, u32 contro
 	struct devlink_port_attrs *attrs = &devlink_port->attrs;
 	int ret;
 
-	if (WARN_ON(devlink_port->devlink))
-		return;
+	ASSERT_DEVLINK_PORT_NOT_REGISTERED(devlink_port);
+
 	ret = __devlink_port_attrs_set(devlink_port,
 				       DEVLINK_PORT_FLAVOUR_PCI_PF);
 	if (ret)
@@ -10025,8 +10031,8 @@ void devlink_port_attrs_pci_vf_set(struct devlink_port *devlink_port, u32 contro
 	struct devlink_port_attrs *attrs = &devlink_port->attrs;
 	int ret;
 
-	if (WARN_ON(devlink_port->devlink))
-		return;
+	ASSERT_DEVLINK_PORT_NOT_REGISTERED(devlink_port);
+
 	ret = __devlink_port_attrs_set(devlink_port,
 				       DEVLINK_PORT_FLAVOUR_PCI_VF);
 	if (ret)
@@ -10053,8 +10059,8 @@ void devlink_port_attrs_pci_sf_set(struct devlink_port *devlink_port, u32 contro
 	struct devlink_port_attrs *attrs = &devlink_port->attrs;
 	int ret;
 
-	if (WARN_ON(devlink_port->devlink))
-		return;
+	ASSERT_DEVLINK_PORT_NOT_REGISTERED(devlink_port);
+
 	ret = __devlink_port_attrs_set(devlink_port,
 				       DEVLINK_PORT_FLAVOUR_PCI_SF);
 	if (ret)
@@ -10169,8 +10175,8 @@ EXPORT_SYMBOL_GPL(devl_rate_nodes_destroy);
 void devlink_port_linecard_set(struct devlink_port *devlink_port,
 			       struct devlink_linecard *linecard)
 {
-	if (WARN_ON(devlink_port->devlink))
-		return;
+	ASSERT_DEVLINK_PORT_NOT_REGISTERED(devlink_port);
+
 	devlink_port->linecard = linecard;
 }
 EXPORT_SYMBOL_GPL(devlink_port_linecard_set);
-- 
2.37.1

[patch net-next 3/7] net: devlink: introduce a flag to indicate devlink port being registered

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Instead of relying on devlink pointer not being initialized, introduce
an extra flag to indicate if devlink port is registered. This is needed
as later on devlink pointer is going to be initialized even in case
devlink port is not registered yet.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 include/net/devlink.h | 3 ++-
 net/core/devlink.c    | 6 ++++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index 0b45d44a3348..7b41ebbaf379 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -129,7 +129,8 @@ struct devlink_port {
 	void *type_dev;
 	struct devlink_port_attrs attrs;
 	u8 attrs_set:1,
-	   switch_port:1;
+	   switch_port:1,
+	   registered:1;
 	struct delayed_work type_warn_dw;
 	struct list_head reporter_list;
 	struct mutex reporters_lock; /* Protects reporter_list */
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 2737dad89f51..af13f95384d9 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -372,9 +372,9 @@ static struct devlink *devlink_get_from_attrs(struct net *net,
 }
 
 #define ASSERT_DEVLINK_PORT_REGISTERED(devlink_port)				\
-	WARN_ON_ONCE(!(devlink_port)->devlink)
+	WARN_ON_ONCE(!(devlink_port)->registered)
 #define ASSERT_DEVLINK_PORT_NOT_REGISTERED(devlink_port)			\
-	WARN_ON_ONCE((devlink_port)->devlink)
+	WARN_ON_ONCE((devlink_port)->registered)
 
 static struct devlink_port *devlink_port_get_by_index(struct devlink *devlink,
 						      unsigned int port_index)
@@ -9778,6 +9778,7 @@ int devl_port_register(struct devlink *devlink,
 
 	ASSERT_DEVLINK_PORT_NOT_REGISTERED(devlink_port);
 
+	devlink_port->registered = true;
 	devlink_port->devlink = devlink;
 	devlink_port->index = port_index;
 	spin_lock_init(&devlink_port->type_lock);
@@ -9836,6 +9837,7 @@ void devl_port_unregister(struct devlink_port *devlink_port)
 	WARN_ON(!list_empty(&devlink_port->reporter_list));
 	WARN_ON(!list_empty(&devlink_port->region_list));
 	mutex_destroy(&devlink_port->reporters_lock);
+	devlink_port->registered = false;
 }
 EXPORT_SYMBOL_GPL(devl_port_unregister);
 
-- 
2.37.1

[patch net-next 4/7] net: devlink: add port_init/fini() helpers to allow pre-register/post-unregister functions

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

To be consistent with devlink regions, devlink port regions are going to
be forbidden to be created once devlink port is registered. Prepare for
this and introduce new set of helpers to allow driver to initialize
devlink pointer and region list before devlink_register() is called
That allows port regions to be created before devlink port registration
and destroyed after devlink port unregistration.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 include/net/devlink.h |  6 +++++-
 net/core/devlink.c    | 46 ++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 48 insertions(+), 4 deletions(-)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index 7b41ebbaf379..bc7c423891c2 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -130,7 +130,8 @@ struct devlink_port {
 	struct devlink_port_attrs attrs;
 	u8 attrs_set:1,
 	   switch_port:1,
-	   registered:1;
+	   registered:1,
+	   initialized:1;
 	struct delayed_work type_warn_dw;
 	struct list_head reporter_list;
 	struct mutex reporters_lock; /* Protects reporter_list */
@@ -1564,6 +1565,9 @@ void devlink_set_features(struct devlink *devlink, u64 features);
 void devlink_register(struct devlink *devlink);
 void devlink_unregister(struct devlink *devlink);
 void devlink_free(struct devlink *devlink);
+void devlink_port_init(struct devlink *devlink,
+		       struct devlink_port *devlink_port);
+void devlink_port_fini(struct devlink_port *devlink_port);
 int devl_port_register(struct devlink *devlink,
 		       struct devlink_port *devlink_port,
 		       unsigned int port_index);
diff --git a/net/core/devlink.c b/net/core/devlink.c
index af13f95384d9..a9b31a05d611 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -375,6 +375,8 @@ static struct devlink *devlink_get_from_attrs(struct net *net,
 	WARN_ON_ONCE(!(devlink_port)->registered)
 #define ASSERT_DEVLINK_PORT_NOT_REGISTERED(devlink_port)			\
 	WARN_ON_ONCE((devlink_port)->registered)
+#define ASSERT_DEVLINK_PORT_INITIALIZED(devlink_port)				\
+	WARN_ON_ONCE(!(devlink_port)->initialized)
 
 static struct devlink_port *devlink_port_get_by_index(struct devlink *devlink,
 						      unsigned int port_index)
@@ -9754,6 +9756,44 @@ static void devlink_port_type_warn_cancel(struct devlink_port *devlink_port)
 	cancel_delayed_work_sync(&devlink_port->type_warn_dw);
 }
 
+/**
+ * devlink_port_init() - Init devlink port
+ *
+ * @devlink: devlink
+ * @devlink_port: devlink port
+ *
+ * Initialize essencial stuff that is needed for functions
+ * that may be called before devlink port registration.
+ * Call to this function is optional and not needed
+ * in case the driver does not use such functions.
+ */
+void devlink_port_init(struct devlink *devlink,
+		       struct devlink_port *devlink_port)
+{
+	if (devlink_port->initialized)
+		return;
+	devlink_port->devlink = devlink;
+	INIT_LIST_HEAD(&devlink_port->region_list);
+	devlink_port->initialized = true;
+}
+EXPORT_SYMBOL_GPL(devlink_port_init);
+
+/**
+ * devlink_port_fini() - Deinitialize devlink port
+ *
+ * @devlink_port: devlink port
+ *
+ * Deinitialize essencial stuff that is in use for functions
+ * that may be called after devlink port unregistration.
+ * Call to this function is optional and not needed
+ * in case the driver does not use such functions.
+ */
+void devlink_port_fini(struct devlink_port *devlink_port)
+{
+	WARN_ON(!list_empty(&devlink_port->region_list));
+}
+EXPORT_SYMBOL_GPL(devlink_port_fini);
+
 /**
  * devl_port_register() - Register devlink port
  *
@@ -9778,14 +9818,13 @@ int devl_port_register(struct devlink *devlink,
 
 	ASSERT_DEVLINK_PORT_NOT_REGISTERED(devlink_port);
 
+	devlink_port_init(devlink, devlink_port);
 	devlink_port->registered = true;
-	devlink_port->devlink = devlink;
 	devlink_port->index = port_index;
 	spin_lock_init(&devlink_port->type_lock);
 	INIT_LIST_HEAD(&devlink_port->reporter_list);
 	mutex_init(&devlink_port->reporters_lock);
 	list_add_tail(&devlink_port->list, &devlink->port_list);
-	INIT_LIST_HEAD(&devlink_port->region_list);
 
 	INIT_DELAYED_WORK(&devlink_port->type_warn_dw, &devlink_port_type_warn);
 	devlink_port_type_warn_schedule(devlink_port);
@@ -9835,7 +9874,6 @@ void devl_port_unregister(struct devlink_port *devlink_port)
 	devlink_port_notify(devlink_port, DEVLINK_CMD_PORT_DEL);
 	list_del(&devlink_port->list);
 	WARN_ON(!list_empty(&devlink_port->reporter_list));
-	WARN_ON(!list_empty(&devlink_port->region_list));
 	mutex_destroy(&devlink_port->reporters_lock);
 	devlink_port->registered = false;
 }
@@ -11249,6 +11287,8 @@ devlink_port_region_create(struct devlink_port *port,
 	struct devlink_region *region;
 	int err = 0;
 
+	ASSERT_DEVLINK_PORT_INITIALIZED(port);
+
 	if (WARN_ON(!ops) || WARN_ON(!ops->destructor))
 		return ERR_PTR(-EINVAL);
 
-- 
2.37.1

[patch net-next 5/7] net: dsa: move port_setup/teardown to be called outside devlink port registered area

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Move port_setup() op to be called before devlink_port_register() and
port_teardown() after devlink_port_unregister().

Note it makes sense to move this alongside the rest of the devlink port
code, the reinit() function also gets much nicer, as clearly the fact that
port_setup()->devlink_port_region_create() was called in dsa_port_setup
did not fit the flow.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 net/dsa/dsa2.c | 68 +++++++++++++++++++-------------------------------
 1 file changed, 26 insertions(+), 42 deletions(-)

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index ed56c7a554b8..644aecbe79a0 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -458,12 +458,6 @@ static int dsa_port_setup(struct dsa_port *dp)
 	if (dp->setup)
 		return 0;
 
-	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);
@@ -518,11 +512,8 @@ 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) {
-		if (ds->ops->port_teardown)
-			ds->ops->port_teardown(ds, dp->index);
+	if (err)
 		return err;
-	}
 
 	dp->setup = true;
 
@@ -535,17 +526,26 @@ static int dsa_port_devlink_setup(struct dsa_port *dp)
 	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;
-	memset(dlp, 0, sizeof(*dlp));
 
 	switch (dp->type) {
 	case DSA_PORT_TYPE_UNUSED:
@@ -564,24 +564,23 @@ static int dsa_port_devlink_setup(struct dsa_port *dp)
 
 	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;
 
-	if (!err)
-		dp->devlink_port_setup = true;
-
-	return err;
+	return 0;
 }
 
 static void dsa_port_teardown(struct dsa_port *dp)
 {
 	struct devlink_port *dlp = &dp->devlink_port;
-	struct dsa_switch *ds = dp->ds;
 
 	if (!dp->setup)
 		return;
 
-	if (ds->ops->port_teardown)
-		ds->ops->port_teardown(ds, dp->index);
-
 	devlink_port_type_clear(dlp);
 
 	switch (dp->type) {
@@ -611,40 +610,25 @@ static void dsa_port_teardown(struct dsa_port *dp)
 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)
+	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. 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.
+ * flavour.
  */
 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;
+	return dsa_port_devlink_setup(dp);
 }
 
 static int dsa_devlink_info_get(struct devlink *dl,
-- 
2.37.1

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

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

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 <jiri@nvidia.com>
---
 net/dsa/dsa2.c | 183 ++++++++++++++++++++++---------------------------
 1 file changed, 81 insertions(+), 102 deletions(-)

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;
 		}
 	}
 
-- 
2.37.1

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

[Vladimir Oltean]

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.

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

[Jiri Pirko]

Fri, Aug 26, 2022 at 12:47:24AM CEST, olteanv@gmail.com 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 net-next 7/7] net: devlink: convert region create/destroy() to be forbidden on registered devlink/port

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

No need to create or destroy region when devlink or devlink ports are
registered. Limit the possibility to call the region create/destroy()
only for non-registered devlink or devlink port. Benefit from that and
avoid need to take devl_lock.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx4/crdump.c | 20 +++---
 drivers/net/netdevsim/dev.c                 |  8 +--
 include/net/devlink.h                       |  5 --
 net/core/devlink.c                          | 74 +++++----------------
 4 files changed, 29 insertions(+), 78 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/crdump.c b/drivers/net/ethernet/mellanox/mlx4/crdump.c
index 82a07a31cde7..ac5468b77488 100644
--- a/drivers/net/ethernet/mellanox/mlx4/crdump.c
+++ b/drivers/net/ethernet/mellanox/mlx4/crdump.c
@@ -226,10 +226,10 @@ int mlx4_crdump_init(struct mlx4_dev *dev)
 
 	/* Create cr-space region */
 	crdump->region_crspace =
-		devl_region_create(devlink,
-				   &region_cr_space_ops,
-				   MAX_NUM_OF_DUMPS_TO_STORE,
-				   pci_resource_len(pdev, 0));
+		devlink_region_create(devlink,
+				      &region_cr_space_ops,
+				      MAX_NUM_OF_DUMPS_TO_STORE,
+				      pci_resource_len(pdev, 0));
 	if (IS_ERR(crdump->region_crspace))
 		mlx4_warn(dev, "crdump: create devlink region %s err %ld\n",
 			  region_cr_space_str,
@@ -237,10 +237,10 @@ int mlx4_crdump_init(struct mlx4_dev *dev)
 
 	/* Create fw-health region */
 	crdump->region_fw_health =
-		devl_region_create(devlink,
-				   &region_fw_health_ops,
-				   MAX_NUM_OF_DUMPS_TO_STORE,
-				   HEALTH_BUFFER_SIZE);
+		devlink_region_create(devlink,
+				      &region_fw_health_ops,
+				      MAX_NUM_OF_DUMPS_TO_STORE,
+				      HEALTH_BUFFER_SIZE);
 	if (IS_ERR(crdump->region_fw_health))
 		mlx4_warn(dev, "crdump: create devlink region %s err %ld\n",
 			  region_fw_health_str,
@@ -253,6 +253,6 @@ void mlx4_crdump_end(struct mlx4_dev *dev)
 {
 	struct mlx4_fw_crdump *crdump = &dev->persist->crdump;
 
-	devl_region_destroy(crdump->region_fw_health);
-	devl_region_destroy(crdump->region_crspace);
+	devlink_region_destroy(crdump->region_fw_health);
+	devlink_region_destroy(crdump->region_crspace);
 }
diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index cd3debc9921a..a5c69888dfa6 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -557,15 +557,15 @@ static int nsim_dev_dummy_region_init(struct nsim_dev *nsim_dev,
 				      struct devlink *devlink)
 {
 	nsim_dev->dummy_region =
-		devl_region_create(devlink, &dummy_region_ops,
-				   NSIM_DEV_DUMMY_REGION_SNAPSHOT_MAX,
-				   NSIM_DEV_DUMMY_REGION_SIZE);
+		devlink_region_create(devlink, &dummy_region_ops,
+				      NSIM_DEV_DUMMY_REGION_SNAPSHOT_MAX,
+				      NSIM_DEV_DUMMY_REGION_SIZE);
 	return PTR_ERR_OR_ZERO(nsim_dev->dummy_region);
 }
 
 static void nsim_dev_dummy_region_exit(struct nsim_dev *nsim_dev)
 {
-	devl_region_destroy(nsim_dev->dummy_region);
+	devlink_region_destroy(nsim_dev->dummy_region);
 }
 
 static int
diff --git a/include/net/devlink.h b/include/net/devlink.h
index bc7c423891c2..e8e7eb386acc 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1692,10 +1692,6 @@ int devlink_param_driverinit_value_get(struct devlink *devlink, u32 param_id,
 int devlink_param_driverinit_value_set(struct devlink *devlink, u32 param_id,
 				       union devlink_param_value init_val);
 void devlink_param_value_changed(struct devlink *devlink, u32 param_id);
-struct devlink_region *devl_region_create(struct devlink *devlink,
-					  const struct devlink_region_ops *ops,
-					  u32 region_max_snapshots,
-					  u64 region_size);
 struct devlink_region *
 devlink_region_create(struct devlink *devlink,
 		      const struct devlink_region_ops *ops,
@@ -1704,7 +1700,6 @@ struct devlink_region *
 devlink_port_region_create(struct devlink_port *port,
 			   const struct devlink_port_region_ops *ops,
 			   u32 region_max_snapshots, u64 region_size);
-void devl_region_destroy(struct devlink_region *region);
 void devlink_region_destroy(struct devlink_region *region);
 void devlink_port_region_destroy(struct devlink_region *region);
 
diff --git a/net/core/devlink.c b/net/core/devlink.c
index a9b31a05d611..988476f44900 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -5765,8 +5765,7 @@ static void devlink_nl_region_notify(struct devlink_region *region,
 	struct sk_buff *msg;
 
 	WARN_ON(cmd != DEVLINK_CMD_REGION_NEW && cmd != DEVLINK_CMD_REGION_DEL);
-	if (!xa_get_mark(&devlinks, devlink->index, DEVLINK_REGISTERED))
-		return;
+	ASSERT_DEVLINK_REGISTERED(devlink);
 
 	msg = devlink_nl_region_notify_build(region, snapshot, cmd, 0, 0);
 	if (IS_ERR(msg))
@@ -11204,21 +11203,22 @@ void devlink_param_value_changed(struct devlink *devlink, u32 param_id)
 EXPORT_SYMBOL_GPL(devlink_param_value_changed);
 
 /**
- * devl_region_create - create a new address region
+ * devlink_region_create - create a new address region
  *
  * @devlink: devlink
  * @ops: region operations and name
  * @region_max_snapshots: Maximum supported number of snapshots for region
  * @region_size: size of region
  */
-struct devlink_region *devl_region_create(struct devlink *devlink,
-					  const struct devlink_region_ops *ops,
-					  u32 region_max_snapshots,
-					  u64 region_size)
+struct devlink_region *
+devlink_region_create(struct devlink *devlink,
+		      const struct devlink_region_ops *ops,
+		      u32 region_max_snapshots,
+		      u64 region_size)
 {
 	struct devlink_region *region;
 
-	devl_assert_locked(devlink);
+	ASSERT_DEVLINK_NOT_REGISTERED(devlink);
 
 	if (WARN_ON(!ops) || WARN_ON(!ops->destructor))
 		return ERR_PTR(-EINVAL);
@@ -11237,35 +11237,9 @@ struct devlink_region *devl_region_create(struct devlink *devlink,
 	INIT_LIST_HEAD(&region->snapshot_list);
 	mutex_init(&region->snapshot_lock);
 	list_add_tail(&region->list, &devlink->region_list);
-	devlink_nl_region_notify(region, NULL, DEVLINK_CMD_REGION_NEW);
 
 	return region;
 }
-EXPORT_SYMBOL_GPL(devl_region_create);
-
-/**
- *	devlink_region_create - create a new address region
- *
- *	@devlink: devlink
- *	@ops: region operations and name
- *	@region_max_snapshots: Maximum supported number of snapshots for region
- *	@region_size: size of region
- *
- *	Context: Takes and release devlink->lock <mutex>.
- */
-struct devlink_region *
-devlink_region_create(struct devlink *devlink,
-		      const struct devlink_region_ops *ops,
-		      u32 region_max_snapshots, u64 region_size)
-{
-	struct devlink_region *region;
-
-	devl_lock(devlink);
-	region = devl_region_create(devlink, ops, region_max_snapshots,
-				    region_size);
-	devl_unlock(devlink);
-	return region;
-}
 EXPORT_SYMBOL_GPL(devlink_region_create);
 
 /**
@@ -11275,8 +11249,6 @@ EXPORT_SYMBOL_GPL(devlink_region_create);
  *	@ops: region operations and name
  *	@region_max_snapshots: Maximum supported number of snapshots for region
  *	@region_size: size of region
- *
- *	Context: Takes and release devlink->lock <mutex>.
  */
 struct devlink_region *
 devlink_port_region_create(struct devlink_port *port,
@@ -11288,6 +11260,7 @@ devlink_port_region_create(struct devlink_port *port,
 	int err = 0;
 
 	ASSERT_DEVLINK_PORT_INITIALIZED(port);
+	ASSERT_DEVLINK_PORT_NOT_REGISTERED(port);
 
 	if (WARN_ON(!ops) || WARN_ON(!ops->destructor))
 		return ERR_PTR(-EINVAL);
@@ -11313,7 +11286,6 @@ devlink_port_region_create(struct devlink_port *port,
 	INIT_LIST_HEAD(&region->snapshot_list);
 	mutex_init(&region->snapshot_lock);
 	list_add_tail(&region->list, &port->region_list);
-	devlink_nl_region_notify(region, NULL, DEVLINK_CMD_REGION_NEW);
 
 	devl_unlock(devlink);
 	return region;
@@ -11325,16 +11297,18 @@ devlink_port_region_create(struct devlink_port *port,
 EXPORT_SYMBOL_GPL(devlink_port_region_create);
 
 /**
- * devl_region_destroy - destroy address region
+ * devlink_region_destroy - destroy address region
  *
  * @region: devlink region to destroy
  */
-void devl_region_destroy(struct devlink_region *region)
+void devlink_region_destroy(struct devlink_region *region)
 {
-	struct devlink *devlink = region->devlink;
 	struct devlink_snapshot *snapshot, *ts;
 
-	devl_assert_locked(devlink);
+	if (region->port)
+		ASSERT_DEVLINK_PORT_NOT_REGISTERED(region->port);
+	else
+		ASSERT_DEVLINK_NOT_REGISTERED(region->devlink);
 
 	/* Free all snapshots of region */
 	list_for_each_entry_safe(snapshot, ts, &region->snapshot_list, list)
@@ -11343,26 +11317,8 @@ void devl_region_destroy(struct devlink_region *region)
 	list_del(&region->list);
 	mutex_destroy(&region->snapshot_lock);
 
-	devlink_nl_region_notify(region, NULL, DEVLINK_CMD_REGION_DEL);
 	kfree(region);
 }
-EXPORT_SYMBOL_GPL(devl_region_destroy);
-
-/**
- *	devlink_region_destroy - destroy address region
- *
- *	@region: devlink region to destroy
- *
- *	Context: Takes and release devlink->lock <mutex>.
- */
-void devlink_region_destroy(struct devlink_region *region)
-{
-	struct devlink *devlink = region->devlink;
-
-	devl_lock(devlink);
-	devl_region_destroy(region);
-	devl_unlock(devlink);
-}
 EXPORT_SYMBOL_GPL(devlink_region_destroy);
 
 /**
-- 
2.37.1

Re: [patch net-next 0/7] devlink: sanitize per-port region creation/destruction

[Jakub Kicinski]

On Thu, 25 Aug 2022 12:33:53 +0200 Jiri Pirko wrote:
> Currently the only user of per-port devlink regions is DSA. All drivers
> that use regions register them before devlink registration. For DSA,
> this was not possible as the internals of struct devlink_port needed for
> region creation are initialized during port registration.
> 
> This introduced a mismatch in creation flow of devlink and devlink port
> regions. As you can see, it causes the DSA driver to make the port
> init/exit flow a bit cumbersome.
> 
> Fix this by introducing port_init/fini() which could be optionally
> called by drivers like DSA, to prepare struct devlink_port to be used
> for region creation purposes before devlink port register is called.
> 
> Force similar limitation as for devlink params and disallow to create
> regions after devlink or devlink port are registered. That allows to
> simplify the devlink region internal API to not to rely on
> devlink->lock.

The point of exposing the devlink lock was to avoid forcing drivers 
to order object registration in a specific way. I don't like.

Please provide some solid motivation here 'cause if it's you like vs 
I like...

Also the patches don't apply.

Re: [patch net-next 0/7] devlink: sanitize per-port region creation/destruction

[Jiri Pirko]

Fri, Aug 26, 2022 at 12:01:32AM CEST, kuba@kernel.org wrote:
>On Thu, 25 Aug 2022 12:33:53 +0200 Jiri Pirko wrote:
>> Currently the only user of per-port devlink regions is DSA. All drivers
>> that use regions register them before devlink registration. For DSA,
>> this was not possible as the internals of struct devlink_port needed for
>> region creation are initialized during port registration.
>> 
>> This introduced a mismatch in creation flow of devlink and devlink port
>> regions. As you can see, it causes the DSA driver to make the port
>> init/exit flow a bit cumbersome.
>> 
>> Fix this by introducing port_init/fini() which could be optionally
>> called by drivers like DSA, to prepare struct devlink_port to be used
>> for region creation purposes before devlink port register is called.
>> 
>> Force similar limitation as for devlink params and disallow to create
>> regions after devlink or devlink port are registered. That allows to
>> simplify the devlink region internal API to not to rely on
>> devlink->lock.
>
>The point of exposing the devlink lock was to avoid forcing drivers 
>to order object registration in a specific way. I don't like.

Well for params, we are also forcing them in a specific way. The
regions, with the DSA exception which is not voluntary, don't need to be
created/destroyed during devlink/port being registered.

I try to bring some order to the a bit messy devlink world. The
intention is to make everyone's live happier :)

>
>Please provide some solid motivation here 'cause if it's you like vs 
>I like...
>
>Also the patches don't apply.

Re: [patch net-next 0/7] devlink: sanitize per-port region creation/destruction

[Jakub Kicinski]

On Fri, 26 Aug 2022 10:21:25 +0200 Jiri Pirko wrote:
>> The point of exposing the devlink lock was to avoid forcing drivers 
>> to order object registration in a specific way. I don't like.  
> 
> Well for params, we are also forcing them in a specific way. The
> regions, with the DSA exception which is not voluntary, don't need to be
> created/destroyed during devlink/port being registered.
> 
> I try to bring some order to the a bit messy devlink world. The
> intention is to make everyone's live happier :)

The way I remember it - we had to keep the ordering on resources for
mlx4 because of complicated locking/async nature of events, and since
it's a driver for a part which is much EoL we won't go back now and do
major surgery, that's fine.

But that shouldn't mean that the recommended way of using resources is
"hook them up before register". The overall devlink locking ordering
should converge towards the "hold devl_lock() around registration of
your components, or whenever the device goes out of consistent state".

Re: [patch net-next 0/7] devlink: sanitize per-port region creation/destruction

[Jiri Pirko]

Sat, Aug 27, 2022 at 02:22:43AM CEST, kuba@kernel.org wrote:
>On Fri, 26 Aug 2022 10:21:25 +0200 Jiri Pirko wrote:
>>> The point of exposing the devlink lock was to avoid forcing drivers 
>>> to order object registration in a specific way. I don't like.  
>> 
>> Well for params, we are also forcing them in a specific way. The
>> regions, with the DSA exception which is not voluntary, don't need to be
>> created/destroyed during devlink/port being registered.
>> 
>> I try to bring some order to the a bit messy devlink world. The
>> intention is to make everyone's live happier :)
>
>The way I remember it - we had to keep the ordering on resources for
>mlx4 because of complicated locking/async nature of events, and since
>it's a driver for a part which is much EoL we won't go back now and do
>major surgery, that's fine.
>
>But that shouldn't mean that the recommended way of using resources is
>"hook them up before register". The overall devlink locking ordering
>should converge towards the "hold devl_lock() around registration of
>your components, or whenever the device goes out of consistent state".

As you wish.