From 3fcb698d9c773d1e0ce5eb4374273457eb4c8338 Mon Sep 17 00:00:00 2001 From: Jiri Pirko Date: Thu, 29 Sep 2022 09:28:56 +0200 Subject: [PATCH 1/7] net: devlink: introduce port registered assert helper and use it 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 Signed-off-by: Jakub Kicinski --- 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 7776dc82f88d..f5bfbdb0301e 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) { @@ -9869,7 +9874,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); @@ -9952,8 +9958,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; @@ -10072,8 +10078,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) @@ -10096,8 +10102,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) @@ -10123,8 +10129,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) @@ -10151,8 +10157,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) @@ -10267,8 +10273,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); From 081adcfe930e4b01a55eaa329b2e453a442f35a9 Mon Sep 17 00:00:00 2001 From: Jiri Pirko Date: Thu, 29 Sep 2022 09:28:57 +0200 Subject: [PATCH 2/7] net: devlink: introduce a flag to indicate devlink port being registered 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 Signed-off-by: Jakub Kicinski --- 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 264aa98e6da6..bcacd8dab297 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 f5bfbdb0301e..17529e6b2bbf 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) @@ -9876,6 +9876,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); @@ -9934,6 +9935,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); From ae3bbc04d4bfef5d0332cd4edda3ac8f714cea23 Mon Sep 17 00:00:00 2001 From: Jiri Pirko Date: Thu, 29 Sep 2022 09:28:58 +0200 Subject: [PATCH 3/7] net: devlink: add port_init/fini() helpers to allow pre-register/post-unregister functions Lifetime of some of the devlink objects, like regions, is currently forced to be different for devlink instance and devlink port instance (per-port regions). The reason is that for devlink ports, the internal structures initialization happens only after devlink_port_register() is called. To resolve this inconsistency, 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 Signed-off-by: Jakub Kicinski --- 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 bcacd8dab297..ba6b8b094943 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 */ @@ -1563,6 +1564,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 17529e6b2bbf..89baa7c0938b 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) @@ -9852,6 +9854,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 * @@ -9876,14 +9916,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); @@ -9933,7 +9972,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; } @@ -11347,6 +11385,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); From d82acd85cc41a8e5d5e0e4c2a3f4b645def29723 Mon Sep 17 00:00:00 2001 From: Jiri Pirko Date: Thu, 29 Sep 2022 09:28:59 +0200 Subject: [PATCH 4/7] net: dsa: move port_setup/teardown to be called outside devlink port registered area 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 Signed-off-by: Jakub Kicinski --- 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 7024e2120de1..6f555b1bb483 100644 --- a/net/dsa/dsa2.c +++ b/net/dsa/dsa2.c @@ -472,12 +472,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); @@ -532,11 +526,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; @@ -549,17 +540,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: @@ -578,24 +578,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) { @@ -625,40 +624,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, From cf5ca4ddc37a693b17fdb653cb84b920b1185d71 Mon Sep 17 00:00:00 2001 From: Vladimir Oltean Date: Thu, 29 Sep 2022 09:29:00 +0200 Subject: [PATCH 5/7] net: dsa: don't leave dangling pointers in dp->pl when failing There is a desire to simplify the dsa_port registration path with devlink, and this involves reworking a bit how user ports which fail to connect to their PHY (because it's missing) get reinitialized as UNUSED devlink ports. The desire is for the change to look something like this; basically dsa_port_setup() has failed, we just change dp->type and call dsa_port_setup() again. -/* 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) +static int dsa_port_setup_as_unused(struct dsa_port *dp) { - dsa_port_devlink_teardown(dp); dp->type = DSA_PORT_TYPE_UNUSED; - return dsa_port_devlink_setup(dp); + return dsa_port_setup(dp); } For an UNUSED port, dsa_port_setup() mostly only calls dsa_port_devlink_setup() anyway, so we could get away with calling just that. But if we call the full blown dsa_port_setup(dp) (which will be needed to properly set dp->setup = true), the callee will have the tendency to go through this code block too, and call dsa_port_disable(dp): switch (dp->type) { case DSA_PORT_TYPE_UNUSED: dsa_port_disable(dp); break; That is not very good, because dsa_port_disable() has this hidden inside of it: if (dp->pl) phylink_stop(dp->pl); Fact is, we are not prepared to handle a call to dsa_port_disable() with a struct dsa_port that came from a previous (and failed) call to dsa_port_setup(). We do not clean up dp->pl, and this will make the second call to dsa_port_setup() call phylink_stop() on a dangling dp->pl pointer. Solve this by creating an API for phylink destruction which is symmetric to the phylink creation, and never leave dp->pl set to anything except NULL or a valid phylink structure. Signed-off-by: Vladimir Oltean Signed-off-by: Jiri Pirko Signed-off-by: Jakub Kicinski --- net/dsa/dsa_priv.h | 1 + net/dsa/port.c | 22 +++++++++++++++------- net/dsa/slave.c | 6 +++--- 3 files changed, 19 insertions(+), 10 deletions(-) diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h index 129e4a649c7e..6e65c7ffd6f3 100644 --- a/net/dsa/dsa_priv.h +++ b/net/dsa/dsa_priv.h @@ -294,6 +294,7 @@ int dsa_port_mrp_add_ring_role(const struct dsa_port *dp, int dsa_port_mrp_del_ring_role(const struct dsa_port *dp, const struct switchdev_obj_ring_role_mrp *mrp); int dsa_port_phylink_create(struct dsa_port *dp); +void dsa_port_phylink_destroy(struct dsa_port *dp); int dsa_shared_port_link_register_of(struct dsa_port *dp); void dsa_shared_port_link_unregister_of(struct dsa_port *dp); int dsa_port_hsr_join(struct dsa_port *dp, struct net_device *hsr); diff --git a/net/dsa/port.c b/net/dsa/port.c index e6289a1db0a0..e4a0513816bb 100644 --- a/net/dsa/port.c +++ b/net/dsa/port.c @@ -1661,6 +1661,7 @@ int dsa_port_phylink_create(struct dsa_port *dp) { struct dsa_switch *ds = dp->ds; phy_interface_t mode; + struct phylink *pl; int err; err = of_get_phy_mode(dp->dn, &mode); @@ -1677,16 +1678,24 @@ int dsa_port_phylink_create(struct dsa_port *dp) if (ds->ops->phylink_get_caps) ds->ops->phylink_get_caps(ds, dp->index, &dp->pl_config); - dp->pl = phylink_create(&dp->pl_config, of_fwnode_handle(dp->dn), - mode, &dsa_port_phylink_mac_ops); - if (IS_ERR(dp->pl)) { + pl = phylink_create(&dp->pl_config, of_fwnode_handle(dp->dn), + mode, &dsa_port_phylink_mac_ops); + if (IS_ERR(pl)) { pr_err("error creating PHYLINK: %ld\n", PTR_ERR(dp->pl)); - return PTR_ERR(dp->pl); + return PTR_ERR(pl); } + dp->pl = pl; + return 0; } +void dsa_port_phylink_destroy(struct dsa_port *dp) +{ + phylink_destroy(dp->pl); + dp->pl = NULL; +} + static int dsa_shared_port_setup_phy_of(struct dsa_port *dp, bool enable) { struct dsa_switch *ds = dp->ds; @@ -1781,7 +1790,7 @@ static int dsa_shared_port_phylink_register(struct dsa_port *dp) return 0; err_phy_connect: - phylink_destroy(dp->pl); + dsa_port_phylink_destroy(dp); return err; } @@ -1983,8 +1992,7 @@ void dsa_shared_port_link_unregister_of(struct dsa_port *dp) rtnl_lock(); phylink_disconnect_phy(dp->pl); rtnl_unlock(); - phylink_destroy(dp->pl); - dp->pl = NULL; + dsa_port_phylink_destroy(dp); return; } diff --git a/net/dsa/slave.c b/net/dsa/slave.c index aa47ddc19fdf..1a59918d3b30 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -2304,7 +2304,7 @@ static int dsa_slave_phy_setup(struct net_device *slave_dev) if (ret) { netdev_err(slave_dev, "failed to connect to PHY: %pe\n", ERR_PTR(ret)); - phylink_destroy(dp->pl); + dsa_port_phylink_destroy(dp); } return ret; @@ -2476,7 +2476,7 @@ int dsa_slave_create(struct dsa_port *port) rtnl_lock(); phylink_disconnect_phy(p->dp->pl); rtnl_unlock(); - phylink_destroy(p->dp->pl); + dsa_port_phylink_destroy(p->dp); out_gcells: gro_cells_destroy(&p->gcells); out_free: @@ -2499,7 +2499,7 @@ void dsa_slave_destroy(struct net_device *slave_dev) phylink_disconnect_phy(dp->pl); rtnl_unlock(); - phylink_destroy(dp->pl); + dsa_port_phylink_destroy(dp); gro_cells_destroy(&p->gcells); free_percpu(slave_dev->tstats); free_netdev(slave_dev); From c698a5fbf7fd9f5bb909d09626319b59d55db36b Mon Sep 17 00:00:00 2001 From: Jiri Pirko Date: Thu, 29 Sep 2022 09:29:01 +0200 Subject: [PATCH 6/7] net: dsa: don't do devlink port setup early 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 Signed-off-by: Jakub Kicinski --- net/dsa/dsa2.c | 176 +++++++++++++++++++++++-------------------------- 1 file changed, 82 insertions(+), 94 deletions(-) diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c index 6f555b1bb483..747c0364fb0f 100644 --- a/net/dsa/dsa2.c +++ b/net/dsa/dsa2.c @@ -461,6 +461,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; @@ -472,6 +540,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); @@ -526,68 +598,16 @@ 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 (err) { + dsa_port_devlink_teardown(dp); 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); - return err; - } - dp->devlink_port_setup = true; - - return 0; -} - static void dsa_port_teardown(struct dsa_port *dp) { struct devlink_port *dlp = &dp->devlink_port; @@ -618,31 +638,15 @@ static void dsa_port_teardown(struct dsa_port *dp) break; } + dsa_port_devlink_teardown(dp); + dp->setup = false; } -static void dsa_port_devlink_teardown(struct dsa_port *dp) +static int dsa_port_setup_as_unused(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); + return dsa_port_setup(dp); } static int dsa_devlink_info_get(struct devlink *dl, @@ -866,7 +870,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) @@ -889,18 +892,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; @@ -941,9 +935,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; @@ -951,8 +943,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; @@ -971,8 +961,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; } @@ -1025,7 +1013,7 @@ static int dsa_tree_setup_ports(struct dsa_switch_tree *dst) 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); + err = dsa_port_setup_as_unused(dp); if (err) goto teardown; } From 61e4a51621587c939672d6a9354f6d0aa3d4e131 Mon Sep 17 00:00:00 2001 From: Vladimir Oltean Date: Thu, 29 Sep 2022 09:29:02 +0200 Subject: [PATCH 7/7] net: dsa: remove bool devlink_port_setup Since dsa_port_devlink_setup() and dsa_port_devlink_teardown() are already called from code paths which only execute once per port (due to the existing bool dp->setup), keeping another dp->devlink_port_setup is redundant, because we can already manage to balance the calls properly (and not call teardown when setup was never called, or call setup twice, or things like that). Signed-off-by: Vladimir Oltean Signed-off-by: Jiri Pirko Signed-off-by: Jakub Kicinski --- include/net/dsa.h | 2 -- net/dsa/dsa2.c | 14 ++++++-------- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/include/net/dsa.h b/include/net/dsa.h index d777eac5694f..ee369670e20e 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -294,8 +294,6 @@ struct dsa_port { u8 lag_tx_enabled:1; - u8 devlink_port_setup:1; - /* Master state bits, valid only on CPU ports */ u8 master_admin_up:1; u8 master_oper_up:1; diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c index 747c0364fb0f..af0e2c0394ac 100644 --- a/net/dsa/dsa2.c +++ b/net/dsa/dsa2.c @@ -510,7 +510,6 @@ static int dsa_port_devlink_setup(struct dsa_port *dp) ds->ops->port_teardown(ds, dp->index); return err; } - dp->devlink_port_setup = true; return 0; } @@ -520,13 +519,12 @@ 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; + devlink_port_unregister(dlp); + + if (ds->ops->port_teardown) + ds->ops->port_teardown(ds, dp->index); + + devlink_port_fini(dlp); } static int dsa_port_setup(struct dsa_port *dp)