| Message ID | 20200926210632.3888886-4-andrew@lunn.ch |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | mv88e6xxx: Add per port devlink regions | expand |
On Sat, Sep 26, 2020 at 11:06:28PM +0200, Andrew Lunn wrote: > DSA drivers want to create regions on devlink ports as well as the > devlink device instance, in order to export registers and other tables > per port. To keep all this code together in the drivers, have the > devlink ports registered early, so the setup() method can setup both > device and port devlink regions. > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > --- > include/net/dsa.h | 1 + > net/dsa/dsa2.c | 133 ++++++++++++++++++++++++++++------------------ > 2 files changed, 83 insertions(+), 51 deletions(-) > > diff --git a/include/net/dsa.h b/include/net/dsa.h > index d16057c5987a..9aa44dc8ecdb 100644 > --- a/include/net/dsa.h > +++ b/include/net/dsa.h > @@ -208,6 +208,7 @@ struct dsa_port { > u8 stp_state; > struct net_device *bridge_dev; > struct devlink_port devlink_port; > + bool devlink_port_setup; > struct phylink *pl; > struct phylink_config pl_config; > > diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c > index 2c149fb36928..90cc70bd7c22 100644 > --- a/net/dsa/dsa2.c > +++ b/net/dsa/dsa2.c > @@ -251,47 +251,19 @@ static void dsa_tree_teardown_default_cpu(struct dsa_switch_tree *dst) > > static int dsa_port_setup(struct dsa_port *dp) > { > - struct dsa_switch *ds = dp->ds; > - struct dsa_switch_tree *dst = ds->dst; > - const unsigned char *id = (const unsigned char *)&dst->index; > - const unsigned char len = sizeof(dst->index); > struct devlink_port *dlp = &dp->devlink_port; > bool dsa_port_link_registered = false; > - bool devlink_port_registered = false; > - struct devlink_port_attrs attrs = {}; > - struct devlink *dl = ds->devlink; > bool dsa_port_enabled = false; > int err = 0; > > - attrs.phys.port_number = dp->index; > - memcpy(attrs.switch_id.id, id, len); > - attrs.switch_id.id_len = len; > - > if (dp->setup) > return 0; > > switch (dp->type) { > case DSA_PORT_TYPE_UNUSED: > - memset(dlp, 0, sizeof(*dlp)); > - attrs.flavour = DEVLINK_PORT_FLAVOUR_UNUSED; > - devlink_port_attrs_set(dlp, &attrs); > - err = devlink_port_register(dl, dlp, dp->index); > - if (err) > - break; > - > - devlink_port_registered = true; > - > dsa_port_disable(dp); > break; > case DSA_PORT_TYPE_CPU: > - memset(dlp, 0, sizeof(*dlp)); > - attrs.flavour = DEVLINK_PORT_FLAVOUR_CPU; > - devlink_port_attrs_set(dlp, &attrs); > - err = devlink_port_register(dl, dlp, dp->index); > - if (err) > - break; > - devlink_port_registered = true; > - > err = dsa_port_link_register_of(dp); > if (err) > break; > @@ -304,14 +276,6 @@ static int dsa_port_setup(struct dsa_port *dp) > > break; > case DSA_PORT_TYPE_DSA: > - memset(dlp, 0, sizeof(*dlp)); > - attrs.flavour = DEVLINK_PORT_FLAVOUR_DSA; > - devlink_port_attrs_set(dlp, &attrs); > - err = devlink_port_register(dl, dlp, dp->index); > - if (err) > - break; > - devlink_port_registered = true; > - > err = dsa_port_link_register_of(dp); > if (err) > break; > @@ -324,14 +288,6 @@ static int dsa_port_setup(struct dsa_port *dp) > > break; > case DSA_PORT_TYPE_USER: > - memset(dlp, 0, sizeof(*dlp)); > - attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL; > - devlink_port_attrs_set(dlp, &attrs); > - err = devlink_port_register(dl, dlp, dp->index); > - if (err) > - break; > - devlink_port_registered = true; > - > dp->mac = of_get_mac_address(dp->dn); > err = dsa_slave_create(dp); > if (err) > @@ -345,8 +301,6 @@ static int dsa_port_setup(struct dsa_port *dp) > dsa_port_disable(dp); > if (err && dsa_port_link_registered) > dsa_port_link_unregister_of(dp); > - if (err && devlink_port_registered) > - devlink_port_unregister(dlp); > if (err) > return err; > > @@ -355,30 +309,77 @@ static int dsa_port_setup(struct dsa_port *dp) > return 0; > } > > -static void dsa_port_teardown(struct dsa_port *dp) > +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; > + const unsigned char *id; > + unsigned char len; > + int 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; > + > + if (dp->setup) > + return 0; > I wonder what this is protecting against? I ran on a multi-switch tree without these 2 lines and I didn't get anything like multiple registration or things like that. What is the call path that would call dsa_port_devlink_setup twice? > + switch (dp->type) { > + case DSA_PORT_TYPE_UNUSED: > + memset(dlp, 0, sizeof(*dlp)); > + attrs.flavour = DEVLINK_PORT_FLAVOUR_UNUSED; > + devlink_port_attrs_set(dlp, &attrs); > + err = devlink_port_register(dl, dlp, dp->index); These 2 lines are common everywhere. Could you move them out of the switch-case statement? > + break; > + case DSA_PORT_TYPE_CPU: > + memset(dlp, 0, sizeof(*dlp)); > + attrs.flavour = DEVLINK_PORT_FLAVOUR_CPU; > + devlink_port_attrs_set(dlp, &attrs); > + err = devlink_port_register(dl, dlp, dp->index); > + break; > + case DSA_PORT_TYPE_DSA: > + memset(dlp, 0, sizeof(*dlp)); > + attrs.flavour = DEVLINK_PORT_FLAVOUR_DSA; > + devlink_port_attrs_set(dlp, &attrs); > + err = devlink_port_register(dl, dlp, dp->index); > + break; > + case DSA_PORT_TYPE_USER: > + memset(dlp, 0, sizeof(*dlp)); > + attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL; > + devlink_port_attrs_set(dlp, &attrs); > + err = devlink_port_register(dl, dlp, dp->index); > + break; > + } > + > + if (!err) > + dp->devlink_port_setup = true; > + > + return err; > +} > + > +static void dsa_port_teardown(struct dsa_port *dp) > +{ > if (!dp->setup) > return; > > switch (dp->type) { > case DSA_PORT_TYPE_UNUSED: > - devlink_port_unregister(dlp); > break; > case DSA_PORT_TYPE_CPU: > dsa_port_disable(dp); > dsa_tag_driver_put(dp->tag_ops); > - devlink_port_unregister(dlp); > dsa_port_link_unregister_of(dp); > break; > case DSA_PORT_TYPE_DSA: > dsa_port_disable(dp); > - devlink_port_unregister(dlp); > dsa_port_link_unregister_of(dp); > break; > case DSA_PORT_TYPE_USER: > - devlink_port_unregister(dlp); > if (dp->slave) { > dsa_slave_destroy(dp->slave); > dp->slave = NULL; > @@ -389,6 +390,15 @@ static void dsa_port_teardown(struct dsa_port *dp) > dp->setup = false; > } > > +static void dsa_port_devlink_teardown(struct dsa_port *dp) > +{ > + struct devlink_port *dlp = &dp->devlink_port; > + > + if (dp->devlink_port_setup) > + devlink_port_unregister(dlp); > + dp->devlink_port_setup = false; > +} > + > static int dsa_devlink_info_get(struct devlink *dl, > struct devlink_info_req *req, > struct netlink_ext_ack *extack) > @@ -408,6 +418,7 @@ static const struct devlink_ops dsa_devlink_ops = { > static int dsa_switch_setup(struct dsa_switch *ds) > { > struct dsa_devlink_priv *dl_priv; > + struct dsa_port *dp; > int err; > > if (ds->setup) > @@ -433,6 +444,17 @@ static int dsa_switch_setup(struct dsa_switch *ds) > if (err) > goto free_devlink; > > + /* Setup devlink port instances now, so that the switch > + * setup() can register regions etc, against the ports > + */ > + list_for_each_entry(dp, &ds->dst->ports, list) { > + if (dp->ds == ds) { > + err = dsa_port_devlink_setup(dp); > + if (err) > + goto unregister_devlink_ports; > + } > + } > + > err = dsa_switch_register_notifier(ds); > if (err) > goto unregister_devlink; Need to use goto unregister_devlink_ports here. > @@ -463,6 +485,10 @@ static int dsa_switch_setup(struct dsa_switch *ds) > > unregister_notifier: > dsa_switch_unregister_notifier(ds); > +unregister_devlink_ports: > + list_for_each_entry(dp, &ds->dst->ports, list) > + if (dp->ds == ds) > + dsa_port_devlink_teardown(dp); > unregister_devlink: > devlink_unregister(ds->devlink); > free_devlink: > @@ -474,6 +500,8 @@ 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; > > @@ -486,6 +514,9 @@ static void dsa_switch_teardown(struct dsa_switch *ds) > ds->ops->teardown(ds); > > if (ds->devlink) { > + list_for_each_entry(dp, &ds->dst->ports, list) > + if (dp->ds == ds) > + dsa_port_devlink_teardown(dp); > devlink_unregister(ds->devlink); > devlink_free(ds->devlink); > ds->devlink = NULL; > -- > 2.28.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; > > + const unsigned char *id; > > + unsigned char len; > > + int 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; > > + > > + if (dp->setup) > > + return 0; > > > > I wonder what this is protecting against? I ran on a multi-switch tree > without these 2 lines and I didn't get anything like multiple > registration or things like that. What is the call path that would call > dsa_port_devlink_setup twice? I made a duplicate copy of dsa_port_setup() and trimmed out what was not needed to give the new dsa_port_setup() and dsa_port_devlink_setup(). I did not trim enough... > > > + switch (dp->type) { > > + case DSA_PORT_TYPE_UNUSED: > > + memset(dlp, 0, sizeof(*dlp)); > > + attrs.flavour = DEVLINK_PORT_FLAVOUR_UNUSED; > > > + devlink_port_attrs_set(dlp, &attrs); > > + err = devlink_port_register(dl, dlp, dp->index); > > These 2 lines are common everywhere. Could you move them out of the > switch-case statement? Yes, that makes sense. Too much blind copy/paste without actually reviewing the code afterwards. Andrew
diff --git a/include/net/dsa.h b/include/net/dsa.h index d16057c5987a..9aa44dc8ecdb 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -208,6 +208,7 @@ struct dsa_port { u8 stp_state; struct net_device *bridge_dev; struct devlink_port devlink_port; + bool devlink_port_setup; struct phylink *pl; struct phylink_config pl_config; diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c index 2c149fb36928..90cc70bd7c22 100644 --- a/net/dsa/dsa2.c +++ b/net/dsa/dsa2.c @@ -251,47 +251,19 @@ static void dsa_tree_teardown_default_cpu(struct dsa_switch_tree *dst) static int dsa_port_setup(struct dsa_port *dp) { - struct dsa_switch *ds = dp->ds; - struct dsa_switch_tree *dst = ds->dst; - const unsigned char *id = (const unsigned char *)&dst->index; - const unsigned char len = sizeof(dst->index); struct devlink_port *dlp = &dp->devlink_port; bool dsa_port_link_registered = false; - bool devlink_port_registered = false; - struct devlink_port_attrs attrs = {}; - struct devlink *dl = ds->devlink; bool dsa_port_enabled = false; int err = 0; - attrs.phys.port_number = dp->index; - memcpy(attrs.switch_id.id, id, len); - attrs.switch_id.id_len = len; - if (dp->setup) return 0; switch (dp->type) { case DSA_PORT_TYPE_UNUSED: - memset(dlp, 0, sizeof(*dlp)); - attrs.flavour = DEVLINK_PORT_FLAVOUR_UNUSED; - devlink_port_attrs_set(dlp, &attrs); - err = devlink_port_register(dl, dlp, dp->index); - if (err) - break; - - devlink_port_registered = true; - dsa_port_disable(dp); break; case DSA_PORT_TYPE_CPU: - memset(dlp, 0, sizeof(*dlp)); - attrs.flavour = DEVLINK_PORT_FLAVOUR_CPU; - devlink_port_attrs_set(dlp, &attrs); - err = devlink_port_register(dl, dlp, dp->index); - if (err) - break; - devlink_port_registered = true; - err = dsa_port_link_register_of(dp); if (err) break; @@ -304,14 +276,6 @@ static int dsa_port_setup(struct dsa_port *dp) break; case DSA_PORT_TYPE_DSA: - memset(dlp, 0, sizeof(*dlp)); - attrs.flavour = DEVLINK_PORT_FLAVOUR_DSA; - devlink_port_attrs_set(dlp, &attrs); - err = devlink_port_register(dl, dlp, dp->index); - if (err) - break; - devlink_port_registered = true; - err = dsa_port_link_register_of(dp); if (err) break; @@ -324,14 +288,6 @@ static int dsa_port_setup(struct dsa_port *dp) break; case DSA_PORT_TYPE_USER: - memset(dlp, 0, sizeof(*dlp)); - attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL; - devlink_port_attrs_set(dlp, &attrs); - err = devlink_port_register(dl, dlp, dp->index); - if (err) - break; - devlink_port_registered = true; - dp->mac = of_get_mac_address(dp->dn); err = dsa_slave_create(dp); if (err) @@ -345,8 +301,6 @@ static int dsa_port_setup(struct dsa_port *dp) dsa_port_disable(dp); if (err && dsa_port_link_registered) dsa_port_link_unregister_of(dp); - if (err && devlink_port_registered) - devlink_port_unregister(dlp); if (err) return err; @@ -355,30 +309,77 @@ static int dsa_port_setup(struct dsa_port *dp) return 0; } -static void dsa_port_teardown(struct dsa_port *dp) +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; + const unsigned char *id; + unsigned char len; + int 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; + + if (dp->setup) + return 0; + switch (dp->type) { + case DSA_PORT_TYPE_UNUSED: + memset(dlp, 0, sizeof(*dlp)); + attrs.flavour = DEVLINK_PORT_FLAVOUR_UNUSED; + devlink_port_attrs_set(dlp, &attrs); + err = devlink_port_register(dl, dlp, dp->index); + break; + case DSA_PORT_TYPE_CPU: + memset(dlp, 0, sizeof(*dlp)); + attrs.flavour = DEVLINK_PORT_FLAVOUR_CPU; + devlink_port_attrs_set(dlp, &attrs); + err = devlink_port_register(dl, dlp, dp->index); + break; + case DSA_PORT_TYPE_DSA: + memset(dlp, 0, sizeof(*dlp)); + attrs.flavour = DEVLINK_PORT_FLAVOUR_DSA; + devlink_port_attrs_set(dlp, &attrs); + err = devlink_port_register(dl, dlp, dp->index); + break; + case DSA_PORT_TYPE_USER: + memset(dlp, 0, sizeof(*dlp)); + attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL; + devlink_port_attrs_set(dlp, &attrs); + err = devlink_port_register(dl, dlp, dp->index); + break; + } + + if (!err) + dp->devlink_port_setup = true; + + return err; +} + +static void dsa_port_teardown(struct dsa_port *dp) +{ if (!dp->setup) return; switch (dp->type) { case DSA_PORT_TYPE_UNUSED: - devlink_port_unregister(dlp); break; case DSA_PORT_TYPE_CPU: dsa_port_disable(dp); dsa_tag_driver_put(dp->tag_ops); - devlink_port_unregister(dlp); dsa_port_link_unregister_of(dp); break; case DSA_PORT_TYPE_DSA: dsa_port_disable(dp); - devlink_port_unregister(dlp); dsa_port_link_unregister_of(dp); break; case DSA_PORT_TYPE_USER: - devlink_port_unregister(dlp); if (dp->slave) { dsa_slave_destroy(dp->slave); dp->slave = NULL; @@ -389,6 +390,15 @@ static void dsa_port_teardown(struct dsa_port *dp) dp->setup = false; } +static void dsa_port_devlink_teardown(struct dsa_port *dp) +{ + struct devlink_port *dlp = &dp->devlink_port; + + if (dp->devlink_port_setup) + devlink_port_unregister(dlp); + dp->devlink_port_setup = false; +} + static int dsa_devlink_info_get(struct devlink *dl, struct devlink_info_req *req, struct netlink_ext_ack *extack) @@ -408,6 +418,7 @@ static const struct devlink_ops dsa_devlink_ops = { static int dsa_switch_setup(struct dsa_switch *ds) { struct dsa_devlink_priv *dl_priv; + struct dsa_port *dp; int err; if (ds->setup) @@ -433,6 +444,17 @@ static int dsa_switch_setup(struct dsa_switch *ds) if (err) goto free_devlink; + /* Setup devlink port instances now, so that the switch + * setup() can register regions etc, against the ports + */ + list_for_each_entry(dp, &ds->dst->ports, list) { + if (dp->ds == ds) { + err = dsa_port_devlink_setup(dp); + if (err) + goto unregister_devlink_ports; + } + } + err = dsa_switch_register_notifier(ds); if (err) goto unregister_devlink; @@ -463,6 +485,10 @@ static int dsa_switch_setup(struct dsa_switch *ds) unregister_notifier: dsa_switch_unregister_notifier(ds); +unregister_devlink_ports: + list_for_each_entry(dp, &ds->dst->ports, list) + if (dp->ds == ds) + dsa_port_devlink_teardown(dp); unregister_devlink: devlink_unregister(ds->devlink); free_devlink: @@ -474,6 +500,8 @@ 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; @@ -486,6 +514,9 @@ static void dsa_switch_teardown(struct dsa_switch *ds) ds->ops->teardown(ds); if (ds->devlink) { + list_for_each_entry(dp, &ds->dst->ports, list) + if (dp->ds == ds) + dsa_port_devlink_teardown(dp); devlink_unregister(ds->devlink); devlink_free(ds->devlink); ds->devlink = NULL;
DSA drivers want to create regions on devlink ports as well as the devlink device instance, in order to export registers and other tables per port. To keep all this code together in the drivers, have the devlink ports registered early, so the setup() method can setup both device and port devlink regions. Signed-off-by: Andrew Lunn <andrew@lunn.ch> --- include/net/dsa.h | 1 + net/dsa/dsa2.c | 133 ++++++++++++++++++++++++++++------------------ 2 files changed, 83 insertions(+), 51 deletions(-)