[patch net-next] devlink: bring port new reply back

[patch net-next] devlink: bring port new reply back

[Jiri Pirko]

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>
---
 .../ethernet/mellanox/mlx5/core/sf/devlink.c  |  9 ++++--
 .../net/ethernet/mellanox/mlx5/core/sf/sf.h   |  3 +-
 include/net/devlink.h                         |  4 ++-
 net/devlink/leftover.c                        | 28 ++++++++++++++++++-
 4 files changed, 38 insertions(+), 6 deletions(-)

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,
-- 
2.39.2

Re: [patch net-next] devlink: bring port new reply back

[Jakub Kicinski]

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..

Re: [patch net-next] devlink: bring port new reply back

[Jakub Kicinski]

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.

Re: [patch net-next] devlink: bring port new reply back

[Jiri Pirko]

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?

Re: [patch net-next] devlink: bring port new reply back

[Jakub Kicinski]

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.

Re: [patch net-next] devlink: bring port new reply back

[Jiri Pirko]

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.

Re: [patch net-next] devlink: bring port new reply back

[Jiri Pirko]

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.