| Message ID | 20230530063829.2493909-1-jiri@resnulli.us (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] devlink: bring port new reply back | expand |
On Tue, 30 May 2023 08:38:29 +0200 Jiri Pirko wrote: > From: Jiri Pirko <jiri@nvidia.com> > > In the offending fixes commit I mistakenly removed the reply message of > the port new command. I was under impression it is a new port > notification, partly due to the "notify" in the name of the helper > function. Bring the code sending reply with new port message back, this > time putting it directly to devlink_nl_cmd_port_new_doit() > > Fixes: c496daeb8630 ("devlink: remove duplicate port notification") > Signed-off-by: Jiri Pirko <jiri@nvidia.com> FWIW it should be fairly trivial to write tests for notifications and replies now that YNL exists and describes devlink..
On Tue, 30 May 2023 09:54:35 -0700 Jakub Kicinski wrote: > On Tue, 30 May 2023 08:38:29 +0200 Jiri Pirko wrote: > > From: Jiri Pirko <jiri@nvidia.com> > > > > In the offending fixes commit I mistakenly removed the reply message of > > the port new command. I was under impression it is a new port > > notification, partly due to the "notify" in the name of the helper > > function. Bring the code sending reply with new port message back, this > > time putting it directly to devlink_nl_cmd_port_new_doit() > > > > Fixes: c496daeb8630 ("devlink: remove duplicate port notification") > > Signed-off-by: Jiri Pirko <jiri@nvidia.com> > > FWIW it should be fairly trivial to write tests for notifications and > replies now that YNL exists and describes devlink.. Actually, I'm not 100% sure notifications work for devlink, with its rtnl-inspired command ID sharing.
Wed, May 31, 2023 at 12:14:44AM CEST, kuba@kernel.org wrote: >On Tue, 30 May 2023 09:54:35 -0700 Jakub Kicinski wrote: >> On Tue, 30 May 2023 08:38:29 +0200 Jiri Pirko wrote: >> > From: Jiri Pirko <jiri@nvidia.com> >> > >> > In the offending fixes commit I mistakenly removed the reply message of >> > the port new command. I was under impression it is a new port >> > notification, partly due to the "notify" in the name of the helper >> > function. Bring the code sending reply with new port message back, this >> > time putting it directly to devlink_nl_cmd_port_new_doit() >> > >> > Fixes: c496daeb8630 ("devlink: remove duplicate port notification") >> > Signed-off-by: Jiri Pirko <jiri@nvidia.com> >> >> FWIW it should be fairly trivial to write tests for notifications and >> replies now that YNL exists and describes devlink.. > >Actually, I'm not 100% sure notifications work for devlink, with its >rtnl-inspired command ID sharing. Could you elaborate more where could be a problem?
On Wed, 31 May 2023 08:36:25 +0200 Jiri Pirko wrote: > >> FWIW it should be fairly trivial to write tests for notifications and > >> replies now that YNL exists and describes devlink.. > > > >Actually, I'm not 100% sure notifications work for devlink, with its > >rtnl-inspired command ID sharing. > > Could you elaborate more where could be a problem? right here https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/tools/net/ynl/lib/ynl.py#n518 ;) If we treat Netlink as more of an RPC than.. state replication(?) mechanism having responses and notifications with the same ID is a bit awkward. I felt like I had to make a recommendation in YNL either to ask users not to enable notifications and issue commands on the same socket, or for family authors to use different IDs. I went with the latter. And made YNL be a bit conservative as to what it will consider to be a notification.
Tue, May 30, 2023 at 08:38:29AM CEST, jiri@resnulli.us wrote: >From: Jiri Pirko <jiri@nvidia.com> > >In the offending fixes commit I mistakenly removed the reply message of >the port new command. I was under impression it is a new port >notification, partly due to the "notify" in the name of the helper >function. Bring the code sending reply with new port message back, this >time putting it directly to devlink_nl_cmd_port_new_doit() > >Fixes: c496daeb8630 ("devlink: remove duplicate port notification") >Signed-off-by: Jiri Pirko <jiri@nvidia.com> Will send v2 rebased on top of the port_ops patchset. Please drop this.
Wed, May 31, 2023 at 08:53:39AM CEST, kuba@kernel.org wrote: >On Wed, 31 May 2023 08:36:25 +0200 Jiri Pirko wrote: >> >> FWIW it should be fairly trivial to write tests for notifications and >> >> replies now that YNL exists and describes devlink.. >> > >> >Actually, I'm not 100% sure notifications work for devlink, with its >> >rtnl-inspired command ID sharing. >> >> Could you elaborate more where could be a problem? > >right here > >https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/tools/net/ynl/lib/ynl.py#n518 > >;) If we treat Netlink as more of an RPC than.. state replication(?) >mechanism having responses and notifications with the same ID is a bit >awkward. I felt like I had to make a recommendation in YNL either to >ask users not to enable notifications and issue commands on the same >socket, or for family authors to use different IDs. I went with the >latter. And made YNL be a bit conservative as to what it will consider >to be a notification. I see. I don't think we can change this devlink kernel behaviour though. Anyway, as the command issuer does not enable notifications, he should be okay.
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c b/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c index c7d4691cb65a..9c02e5ea797c 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c @@ -282,7 +282,8 @@ int mlx5_devlink_sf_port_fn_state_set(struct devlink_port *dl_port, static int mlx5_sf_add(struct mlx5_core_dev *dev, struct mlx5_sf_table *table, const struct devlink_port_new_attrs *new_attr, - struct netlink_ext_ack *extack) + struct netlink_ext_ack *extack, + struct devlink_port **dl_port) { struct mlx5_eswitch *esw = dev->priv.eswitch; struct mlx5_sf *sf; @@ -296,6 +297,7 @@ static int mlx5_sf_add(struct mlx5_core_dev *dev, struct mlx5_sf_table *table, new_attr->controller, new_attr->sfnum); if (err) goto esw_err; + *dl_port = &sf->dl_port; trace_mlx5_sf_add(dev, sf->port_index, sf->controller, sf->hw_fn_id, new_attr->sfnum); return 0; @@ -336,7 +338,8 @@ mlx5_sf_new_check_attr(struct mlx5_core_dev *dev, const struct devlink_port_new_ int mlx5_devlink_sf_port_new(struct devlink *devlink, const struct devlink_port_new_attrs *new_attr, - struct netlink_ext_ack *extack) + struct netlink_ext_ack *extack, + struct devlink_port **dl_port) { struct mlx5_core_dev *dev = devlink_priv(devlink); struct mlx5_sf_table *table; @@ -352,7 +355,7 @@ int mlx5_devlink_sf_port_new(struct devlink *devlink, "Port add is only supported in eswitch switchdev mode or SF ports are disabled."); return -EOPNOTSUPP; } - err = mlx5_sf_add(dev, table, new_attr, extack); + err = mlx5_sf_add(dev, table, new_attr, extack, dl_port); mlx5_sf_table_put(table); return err; } diff --git a/drivers/net/ethernet/mellanox/mlx5/core/sf/sf.h b/drivers/net/ethernet/mellanox/mlx5/core/sf/sf.h index c5430b8dcdf6..860f9ddb7107 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/sf/sf.h +++ b/drivers/net/ethernet/mellanox/mlx5/core/sf/sf.h @@ -20,7 +20,8 @@ void mlx5_sf_table_cleanup(struct mlx5_core_dev *dev); int mlx5_devlink_sf_port_new(struct devlink *devlink, const struct devlink_port_new_attrs *add_attr, - struct netlink_ext_ack *extack); + struct netlink_ext_ack *extack, + struct devlink_port **dl_port); int mlx5_devlink_sf_port_del(struct devlink *devlink, struct devlink_port *dl_port, struct netlink_ext_ack *extack); diff --git a/include/net/devlink.h b/include/net/devlink.h index ec109b39c3ea..2ddb9dad3225 100644 --- a/include/net/devlink.h +++ b/include/net/devlink.h @@ -1500,6 +1500,7 @@ struct devlink_ops { * @devlink: Devlink instance * @attrs: attributes of the new port * @extack: extack for reporting error messages + * @devlink_port: pointer to store new devlink port pointer * * Devlink core will call this device driver function upon user request * to create a new port function of a specified flavor and optional @@ -1512,7 +1513,8 @@ struct devlink_ops { */ int (*port_new)(struct devlink *devlink, const struct devlink_port_new_attrs *attrs, - struct netlink_ext_ack *extack); + struct netlink_ext_ack *extack, + struct devlink_port **devlink_port); /** * port_del() - Delete a port function * @devlink: Devlink instance diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c index 9e801b749194..269aa1a6a13c 100644 --- a/net/devlink/leftover.c +++ b/net/devlink/leftover.c @@ -1360,6 +1360,9 @@ static int devlink_nl_cmd_port_new_doit(struct sk_buff *skb, struct netlink_ext_ack *extack = info->extack; struct devlink_port_new_attrs new_attrs = {}; struct devlink *devlink = info->user_ptr[0]; + struct devlink_port *devlink_port; + struct sk_buff *msg; + int err; if (!devlink->ops->port_new || !devlink->ops->port_del) return -EOPNOTSUPP; @@ -1390,7 +1393,30 @@ static int devlink_nl_cmd_port_new_doit(struct sk_buff *skb, new_attrs.sfnum_valid = true; } - return devlink->ops->port_new(devlink, &new_attrs, extack); + err = devlink->ops->port_new(devlink, &new_attrs, + extack, &devlink_port); + if (err) + return err; + + msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL); + if (!msg) { + err = -ENOMEM; + goto err_out_port_del; + } + err = devlink_nl_port_fill(msg, devlink_port, DEVLINK_CMD_NEW, + info->snd_portid, info->snd_seq, 0, NULL); + if (WARN_ON_ONCE(err)) + goto err_out_msg_free; + err = genlmsg_reply(msg, info); + if (err) + goto err_out_port_del; + return 0; + +err_out_msg_free: + nlmsg_free(msg); +err_out_port_del: + devlink->ops->port_del(devlink, devlink_port, NULL); + return err; } static int devlink_nl_cmd_port_del_doit(struct sk_buff *skb,