[patch net-next 0/3] devlink: small port_new/del() cleanup

[patch net-next 0/3] devlink: small port_new/del() cleanup

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

This patchset cleans up couple of leftovers after recent devlink locking
changes. Previously, both port_new/dev() commands were called without
holding instance lock. Currently all devlink commands are called with
instance lock held.

The first patch just removes redundant port notification.
The second one removes couple of outdated comments.
The last patch changes port_dev() to have devlink_port pointer as an arg
instead of port_index, which makes it similar to the rest of port
related ops.

Jiri Pirko (3):
  devlink: remove duplicate port notification
  devlink: remove no longer true locking comment from port_new/del()
  devlink: pass devlink_port pointer to ops->port_del() instead of index

 .../ethernet/mellanox/mlx5/core/sf/devlink.c  | 14 ++---
 .../net/ethernet/mellanox/mlx5/core/sf/sf.h   |  6 +-
 include/net/devlink.h                         | 12 +---
 net/devlink/leftover.c                        | 56 ++-----------------
 4 files changed, 16 insertions(+), 72 deletions(-)

-- 
2.39.2

[patch net-next 1/3] devlink: remove duplicate port notification

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

The notification about created port is send from devl_port_register()
function called from ops->port_new(). No need to send it again here,
so remove the call and the helper function.

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                        | 45 +------------------
 4 files changed, 6 insertions(+), 55 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c b/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c
index 7d955a4d9f14..de15b9c85e1b 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c
@@ -282,8 +282,7 @@ 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,
-		       unsigned int *new_port_index)
+		       struct netlink_ext_ack *extack)
 {
 	struct mlx5_eswitch *esw = dev->priv.eswitch;
 	struct mlx5_sf *sf;
@@ -297,7 +296,6 @@ 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;
-	*new_port_index = sf->port_index;
 	trace_mlx5_sf_add(dev, sf->port_index, sf->controller, sf->hw_fn_id, new_attr->sfnum);
 	return 0;
 
@@ -338,8 +336,7 @@ 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,
-			     unsigned int *new_port_index)
+			     struct netlink_ext_ack *extack)
 {
 	struct mlx5_core_dev *dev = devlink_priv(devlink);
 	struct mlx5_sf_table *table;
@@ -355,7 +352,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, new_port_index);
+	err = mlx5_sf_add(dev, table, new_attr, extack);
 	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 3a480e06ecc0..1f7d8cbd72e8 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/sf/sf.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/sf/sf.h
@@ -20,8 +20,7 @@ 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,
-			     unsigned int *new_port_index);
+			     struct netlink_ext_ack *extack);
 int mlx5_devlink_sf_port_del(struct devlink *devlink, unsigned int port_index,
 			     struct netlink_ext_ack *extack);
 int mlx5_devlink_sf_port_fn_state_get(struct devlink_port *dl_port,
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 6a942e70e451..ccea6e079777 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1500,7 +1500,6 @@ struct devlink_ops {
 	 * @devlink: Devlink instance
 	 * @attrs: attributes of the new port
 	 * @extack: extack for reporting error messages
-	 * @new_port_index: index of the new port
 	 *
 	 * Devlink core will call this device driver function upon user request
 	 * to create a new port function of a specified flavor and optional
@@ -1515,8 +1514,7 @@ struct devlink_ops {
 	 */
 	int (*port_new)(struct devlink *devlink,
 			const struct devlink_port_new_attrs *attrs,
-			struct netlink_ext_ack *extack,
-			unsigned int *new_port_index);
+			struct netlink_ext_ack *extack);
 	/**
 	 * port_del() - Delete a port function
 	 * @devlink: Devlink instance
diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c
index cd0254968076..cb60e42b2761 100644
--- a/net/devlink/leftover.c
+++ b/net/devlink/leftover.c
@@ -1354,45 +1354,12 @@ static int devlink_nl_cmd_port_unsplit_doit(struct sk_buff *skb,
 	return devlink->ops->port_unsplit(devlink, devlink_port, info->extack);
 }
 
-static int devlink_port_new_notify(struct devlink *devlink,
-				   unsigned int port_index,
-				   struct genl_info *info)
-{
-	struct devlink_port *devlink_port;
-	struct sk_buff *msg;
-	int err;
-
-	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
-	if (!msg)
-		return -ENOMEM;
-
-	lockdep_assert_held(&devlink->lock);
-	devlink_port = devlink_port_get_by_index(devlink, port_index);
-	if (!devlink_port) {
-		err = -ENODEV;
-		goto out;
-	}
-
-	err = devlink_nl_port_fill(msg, devlink_port, DEVLINK_CMD_NEW,
-				   info->snd_portid, info->snd_seq, 0, NULL);
-	if (err)
-		goto out;
-
-	return genlmsg_reply(msg, info);
-
-out:
-	nlmsg_free(msg);
-	return err;
-}
-
 static int devlink_nl_cmd_port_new_doit(struct sk_buff *skb,
 					struct genl_info *info)
 {
 	struct netlink_ext_ack *extack = info->extack;
 	struct devlink_port_new_attrs new_attrs = {};
 	struct devlink *devlink = info->user_ptr[0];
-	unsigned int new_port_index;
-	int err;
 
 	if (!devlink->ops->port_new || !devlink->ops->port_del)
 		return -EOPNOTSUPP;
@@ -1423,17 +1390,7 @@ static int devlink_nl_cmd_port_new_doit(struct sk_buff *skb,
 		new_attrs.sfnum_valid = true;
 	}
 
-	err = devlink->ops->port_new(devlink, &new_attrs, extack,
-				     &new_port_index);
-	if (err)
-		return err;
-
-	err = devlink_port_new_notify(devlink, new_port_index, info);
-	if (err && err != -ENODEV) {
-		/* Fail to send the response; destroy newly created port. */
-		devlink->ops->port_del(devlink, new_port_index, extack);
-	}
-	return err;
+	return devlink->ops->port_new(devlink, &new_attrs, extack);
 }
 
 static int devlink_nl_cmd_port_del_doit(struct sk_buff *skb,
-- 
2.39.2

[patch net-next 2/3] devlink: remove no longer true locking comment from port_new/del()

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

All commands are called holding instance lock. Remove the outdated
comment that says otherwise.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 include/net/devlink.h | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index ccea6e079777..24a48f3d4c35 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1506,8 +1506,6 @@ struct devlink_ops {
 	 * attributes
 	 *
 	 * Notes:
-	 *	- Called without devlink instance lock being held. Drivers must
-	 *	  implement own means of synchronization
 	 *	- On success, drivers must register a port with devlink core
 	 *
 	 * Return: 0 on success, negative value otherwise.
@@ -1525,8 +1523,6 @@ struct devlink_ops {
 	 * to delete a previously created port function
 	 *
 	 * Notes:
-	 *	- Called without devlink instance lock being held. Drivers must
-	 *	  implement own means of synchronization
 	 *	- On success, drivers must unregister the corresponding devlink
 	 *	  port
 	 *
-- 
2.39.2

[patch net-next 3/3] devlink: pass devlink_port pointer to ops->port_del() instead of index

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Historically there was a reason why port_dev() along with for example
port_split() did get port_index instead of the devlink_port pointer.
With the locking changes that were done which ensured devlink instance
mutex is hold for every command, the port ops could get devlink_port
pointer directly. Change the forgotten port_dev() op to be as others
and pass devlink_port pointer instead of port_index.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c |  5 +++--
 drivers/net/ethernet/mellanox/mlx5/core/sf/sf.h      |  3 ++-
 include/net/devlink.h                                |  4 ++--
 net/devlink/leftover.c                               | 11 +++--------
 4 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c b/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c
index de15b9c85e1b..c7d4691cb65a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/sf/devlink.c
@@ -376,7 +376,8 @@ static void mlx5_sf_dealloc(struct mlx5_sf_table *table, struct mlx5_sf *sf)
 	}
 }
 
-int mlx5_devlink_sf_port_del(struct devlink *devlink, unsigned int port_index,
+int mlx5_devlink_sf_port_del(struct devlink *devlink,
+			     struct devlink_port *dl_port,
 			     struct netlink_ext_ack *extack)
 {
 	struct mlx5_core_dev *dev = devlink_priv(devlink);
@@ -391,7 +392,7 @@ int mlx5_devlink_sf_port_del(struct devlink *devlink, unsigned int port_index,
 				   "Port del is only supported in eswitch switchdev mode or SF ports are disabled.");
 		return -EOPNOTSUPP;
 	}
-	sf = mlx5_sf_lookup_by_index(table, port_index);
+	sf = mlx5_sf_lookup_by_index(table, dl_port->index);
 	if (!sf) {
 		err = -ENODEV;
 		goto sf_err;
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/sf/sf.h b/drivers/net/ethernet/mellanox/mlx5/core/sf/sf.h
index 1f7d8cbd72e8..c5430b8dcdf6 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/sf/sf.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/sf/sf.h
@@ -21,7 +21,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);
-int mlx5_devlink_sf_port_del(struct devlink *devlink, unsigned int port_index,
+int mlx5_devlink_sf_port_del(struct devlink *devlink,
+			     struct devlink_port *dl_port,
 			     struct netlink_ext_ack *extack);
 int mlx5_devlink_sf_port_fn_state_get(struct devlink_port *dl_port,
 				      enum devlink_port_fn_state *state,
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 24a48f3d4c35..1bd56c8d6f3c 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1516,7 +1516,7 @@ struct devlink_ops {
 	/**
 	 * port_del() - Delete a port function
 	 * @devlink: Devlink instance
-	 * @port_index: port function index to delete
+	 * @port: The devlink port
 	 * @extack: extack for reporting error messages
 	 *
 	 * Devlink core will call this device driver function upon user request
@@ -1528,7 +1528,7 @@ struct devlink_ops {
 	 *
 	 * Return: 0 on success, negative value otherwise.
 	 */
-	int (*port_del)(struct devlink *devlink, unsigned int port_index,
+	int (*port_del)(struct devlink *devlink, struct devlink_port *port,
 			struct netlink_ext_ack *extack);
 	/**
 	 * port_fn_state_get() - Get the state of a port function
diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c
index cb60e42b2761..0410137a4a31 100644
--- a/net/devlink/leftover.c
+++ b/net/devlink/leftover.c
@@ -1396,20 +1396,14 @@ static int devlink_nl_cmd_port_new_doit(struct sk_buff *skb,
 static int devlink_nl_cmd_port_del_doit(struct sk_buff *skb,
 					struct genl_info *info)
 {
+	struct devlink_port *devlink_port = info->user_ptr[1];
 	struct netlink_ext_ack *extack = info->extack;
 	struct devlink *devlink = info->user_ptr[0];
-	unsigned int port_index;
 
 	if (!devlink->ops->port_del)
 		return -EOPNOTSUPP;
 
-	if (GENL_REQ_ATTR_CHECK(info, DEVLINK_ATTR_PORT_INDEX)) {
-		NL_SET_ERR_MSG(extack, "Port index is not specified");
-		return -EINVAL;
-	}
-	port_index = nla_get_u32(info->attrs[DEVLINK_ATTR_PORT_INDEX]);
-
-	return devlink->ops->port_del(devlink, port_index, extack);
+	return devlink->ops->port_del(devlink, devlink_port, extack);
 }
 
 static int
@@ -6341,6 +6335,7 @@ const struct genl_small_ops devlink_nl_ops[56] = {
 		.cmd = DEVLINK_CMD_PORT_DEL,
 		.doit = devlink_nl_cmd_port_del_doit,
 		.flags = GENL_ADMIN_PERM,
+		.internal_flags = DEVLINK_NL_FLAG_NEED_PORT,
 	},
 	{
 		.cmd = DEVLINK_CMD_LINECARD_GET,
-- 
2.39.2

Re: [patch net-next 1/3] devlink: remove duplicate port notification

[Simon Horman]

On Tue, May 23, 2023 at 02:37:59PM +0200, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> The notification about created port is send from devl_port_register()
> function called from ops->port_new(). No need to send it again here,
> so remove the call and the helper function.
> 
> Signed-off-by: Jiri Pirko <jiri@nvidia.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [patch net-next 2/3] devlink: remove no longer true locking comment from port_new/del()

[Simon Horman]

On Tue, May 23, 2023 at 02:38:00PM +0200, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> All commands are called holding instance lock. Remove the outdated
> comment that says otherwise.
> 
> Signed-off-by: Jiri Pirko <jiri@nvidia.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [patch net-next 3/3] devlink: pass devlink_port pointer to ops->port_del() instead of index

[Simon Horman]

On Tue, May 23, 2023 at 02:38:01PM +0200, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> Historically there was a reason why port_dev() along with for example
> port_split() did get port_index instead of the devlink_port pointer.
> With the locking changes that were done which ensured devlink instance
> mutex is hold for every command, the port ops could get devlink_port
> pointer directly. Change the forgotten port_dev() op to be as others
> and pass devlink_port pointer instead of port_index.
> 
> Signed-off-by: Jiri Pirko <jiri@nvidia.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [patch net-next 0/3] devlink: small port_new/del() cleanup

[Jakub Kicinski]

On Tue, 23 May 2023 14:37:58 +0200 Jiri Pirko wrote:
> This patchset cleans up couple of leftovers after recent devlink locking
> changes. Previously, both port_new/dev() commands were called without
> holding instance lock. Currently all devlink commands are called with
> instance lock held.
> 
> The first patch just removes redundant port notification.
> The second one removes couple of outdated comments.
> The last patch changes port_dev() to have devlink_port pointer as an arg
> instead of port_index, which makes it similar to the rest of port
> related ops.

Reviewed-by: Jakub Kicinski <kuba@kernel.org>

Re: [patch net-next 0/3] devlink: small port_new/del() cleanup

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

On Tue, 23 May 2023 14:37:58 +0200 you wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> This patchset cleans up couple of leftovers after recent devlink locking
> changes. Previously, both port_new/dev() commands were called without
> holding instance lock. Currently all devlink commands are called with
> instance lock held.
> 
> [...]

Here is the summary with links:
  - [net-next,1/3] devlink: remove duplicate port notification
    https://git.kernel.org/netdev/net-next/c/c496daeb8630
  - [net-next,2/3] devlink: remove no longer true locking comment from port_new/del()
    https://git.kernel.org/netdev/net-next/c/1bb1b5789850
  - [net-next,3/3] devlink: pass devlink_port pointer to ops->port_del() instead of index
    https://git.kernel.org/netdev/net-next/c/9277649c66fe

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html