| Message ID | 20240401-v6-8-0-net-next-mv88e6xxx-leds-v4-v3-1-221b3fa55f78@lunn.ch (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: Add generic support for netdev LEDs | expand |
Title: "net: dsa:" prefix, no "." On Mon, Apr 01, 2024 at 08:35:46AM -0500, Andrew Lunn wrote: > The drivers call port_setup() is a good place to add the LEDs of a > port to the netdev representing the port. However, when port_setup() > is called in dsa_port_devlink_setup() the netdev does not exist > yet. That only happens in dsa_user_create() which is latter in later > dsa_port_setup(). > > Move the call to port_setup() out of dsa_port_devlink_setup() and to > the end of dsa_port_setup() where the netdev will exist. > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > --- > net/dsa/devlink.c | 17 +---------------- > net/dsa/dsa.c | 3 +++ > 2 files changed, 4 insertions(+), 16 deletions(-) > > diff --git a/net/dsa/devlink.c b/net/dsa/devlink.c > index 431bf52290a1..9c3dc6319269 100644 > --- a/net/dsa/devlink.c > +++ b/net/dsa/devlink.c > @@ -294,20 +294,12 @@ 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); > > @@ -331,14 +323,7 @@ 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; > - } > - > - return 0; > + return devlink_port_register(dl, dlp, dp->index); > } > > void dsa_port_devlink_teardown(struct dsa_port *dp) > diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c > index 09d2f5d4b3dd..6ffee2a7de94 100644 > --- a/net/dsa/dsa.c > +++ b/net/dsa/dsa.c > @@ -520,6 +520,9 @@ static int dsa_port_setup(struct dsa_port *dp) > break; > } > > + if (ds->ops->port_setup) > + err = ds->ops->port_setup(ds, dp->index); > + This overwrites the not-yet-checked "err", masking the dsa_user_create() return code, and breaking the error handling logic below. Not to mention, if ds->ops->port_setup() fails for a user port, we should call dsa_user_destroy(). > if (err && dsa_port_enabled) > dsa_port_disable(dp); > if (err && dsa_port_link_registered) > > -- > 2.43.0 > It would have been good for the API, if we want the netdev to be available for user ports at port_setup() time, for it to be available at port_teardown() time as well. So dsa_port_devlink_teardown() needs changing too. Additionally, for CPU and DSA ports, this change will make ds->ops->port_enable() be visible from the driver API earlier than ds->ops->port_setup(), which isn't exactly intuitive or great or better than before. In fact, I think it's very difficult not to make mistakes changing the code in its current form. These 3 patches I've prepared - which replace this patch - should help (see attached).
diff --git a/net/dsa/devlink.c b/net/dsa/devlink.c index 431bf52290a1..9c3dc6319269 100644 --- a/net/dsa/devlink.c +++ b/net/dsa/devlink.c @@ -294,20 +294,12 @@ 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); @@ -331,14 +323,7 @@ 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; - } - - return 0; + return devlink_port_register(dl, dlp, dp->index); } void dsa_port_devlink_teardown(struct dsa_port *dp) diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c index 09d2f5d4b3dd..6ffee2a7de94 100644 --- a/net/dsa/dsa.c +++ b/net/dsa/dsa.c @@ -520,6 +520,9 @@ static int dsa_port_setup(struct dsa_port *dp) break; } + if (ds->ops->port_setup) + err = ds->ops->port_setup(ds, dp->index); + if (err && dsa_port_enabled) dsa_port_disable(dp); if (err && dsa_port_link_registered)
The drivers call port_setup() is a good place to add the LEDs of a port to the netdev representing the port. However, when port_setup() is called in dsa_port_devlink_setup() the netdev does not exist yet. That only happens in dsa_user_create() which is latter in dsa_port_setup(). Move the call to port_setup() out of dsa_port_devlink_setup() and to the end of dsa_port_setup() where the netdev will exist. Signed-off-by: Andrew Lunn <andrew@lunn.ch> --- net/dsa/devlink.c | 17 +---------------- net/dsa/dsa.c | 3 +++ 2 files changed, 4 insertions(+), 16 deletions(-)