Skip to contentSkip to conversation

Remove duplicated devlink registration check

PATCH Applied Leon Romanovsky 2 participants 5 messages Jul 28, 2021

Subsystems (5)
Networking Network drivers Mellanox mlx5 core Mellanox Ethernet driver (mlx5e) +1 more subsystems
lore
Share & tools
lore

Leon Romanovsky

author
#0 lore raw

cover Remove duplicated devlink registration check

From: Leon Romanovsky <[email protected]>

Changelog:
v1:
* Added two new patches that remove registration field from mlx5 and ti drivers.
v0: https://lore.kernel.org/lkml/ed7bbb1e4c51dd58e6035a058e93d16f883b09ce.1627215829.git.leonro@nvidia.com

--------------------------------------------------------------------

Both registered flag and devlink pointer are set at the same time and indicate the same thing - devlink/devlink_port are ready. Instead of checking ->registered use devlink pointer as an indication.

Thanks

Leon Romanovsky (3):
net: ti: am65-cpsw-nuss: fix wrong devlink release order net/mlx5: Don't rely on always true registered field
devlink: Remove duplicated registration check

 .../ethernet/mellanox/mlx5/core/en/devlink.c  | 10 ++----
 .../net/ethernet/mellanox/mlx5/core/en_main.c | 11 ++----
 drivers/net/ethernet/ti/am65-cpsw-nuss.c      | 34 +++++++++----------
 include/net/devlink.h                         |  4 +--
 net/core/devlink.c                            | 19 ++++++-----
 5 files changed, 33 insertions(+), 45 deletions(-)
Signature · 1 line
2.31.1
author
#1 lore raw

PATCH 1/3 net: ti: am65-cpsw-nuss: fix wrong devlink release order

From: Leon Romanovsky <[email protected]>

The commit that introduced devlink support released devlink resources in wrong order, that made an unwind flow to be asymmetrical. In addition, the am65-cpsw-nuss used internal to devlink core field - registered.

In order to fix the unwind flow and remove such access to the registered field, rewrite the code to call devlink_port_unregister only on registered ports.

Fixes 58356eb31d60 ("net: ti: am65-cpsw-nuss: Add devlink support")
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 34 ++++++++++++------------
 1 file changed, 17 insertions(+), 17 deletions(-)
drivers/net/ethernet/ti/am65-cpsw-nuss.c +17 -17
index 718539cdd2f2..61ae01c39140 100644
@@ -2384,21 +2384,6 @@ static const struct devlink_param am65_cpsw_devlink_params[] = {
 			     am65_cpsw_dl_switch_mode_set, NULL),
 };
 
-static void am65_cpsw_unregister_devlink_ports(struct am65_cpsw_common *common)
-{
-	struct devlink_port *dl_port;
-	struct am65_cpsw_port *port;
-	int i;
-
-	for (i = 1; i <= common->port_num; i++) {
-		port = am65_common_get_port(common, i);
-		dl_port = &port->devlink_port;
-
-		if (dl_port->registered)
-			devlink_port_unregister(dl_port);
-	}
-}
-
 static int am65_cpsw_nuss_register_devlink(struct am65_cpsw_common *common)
 {
 	struct devlink_port_attrs attrs = {};

@@ -2460,7 +2445,12 @@ static int am65_cpsw_nuss_register_devlink(struct am65_cpsw_common *common)
 	return ret;
 
 dl_port_unreg:
-	am65_cpsw_unregister_devlink_ports(common);
+	for (i = i - 1; i >= 1; i--) {
+		port = am65_common_get_port(common, i);
+		dl_port = &port->devlink_port;
+
+		devlink_port_unregister(dl_port);
+	}
 dl_unreg:
 	devlink_unregister(common->devlink);
 dl_free:

@@ -2471,6 +2461,17 @@ static int am65_cpsw_nuss_register_devlink(struct am65_cpsw_common *common)
 
 static void am65_cpsw_unregister_devlink(struct am65_cpsw_common *common)
 {
+	struct devlink_port *dl_port;
+	struct am65_cpsw_port *port;
+	int i;
+
+	for (i = 1; i <= common->port_num; i++) {
+		port = am65_common_get_port(common, i);
+		dl_port = &port->devlink_port;
+
+		devlink_port_unregister(dl_port);
+	}
+
 	if (!AM65_CPSW_IS_CPSW2G(common) &&
 	    IS_ENABLED(CONFIG_TI_K3_AM65_CPSW_SWITCHDEV)) {
 		devlink_params_unpublish(common->devlink);

@@ -2478,7 +2479,6 @@ static void am65_cpsw_unregister_devlink(struct am65_cpsw_common *common)
 					  ARRAY_SIZE(am65_cpsw_devlink_params));
 	}
 
-	am65_cpsw_unregister_devlink_ports(common);
 	devlink_unregister(common->devlink);
 	devlink_free(common->devlink);
 }
Signature · 1 line
2.31.1
author
#2 lore raw

PATCH 2/3 net/mlx5: Don't rely on always true registered field

In reply to Leon Romanovsky

From: Leon Romanovsky <[email protected]>

Devlink is an integral part of mlx5 driver and all flows ensure that devlink_*_register() will success. That makes the ->registered check an obsolete.

Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/net/ethernet/mellanox/mlx5/core/en/devlink.c | 10 +++-------
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c    | 11 ++---------
 2 files changed, 5 insertions(+), 16 deletions(-)
drivers/net/ethernet/mellanox/mlx5/core/en/devlink.c +3 -7
index bc33eaada3b9..86e079310ac3 100644
@@ -55,19 +55,15 @@ void mlx5e_devlink_port_unregister(struct mlx5e_priv *priv)
 {
 	struct devlink_port *dl_port = mlx5e_devlink_get_dl_port(priv);
 
-	if (dl_port->registered)
-		devlink_port_unregister(dl_port);
+	devlink_port_unregister(dl_port);
 }
 
 struct devlink_port *mlx5e_get_devlink_port(struct net_device *dev)
 {
 	struct mlx5e_priv *priv = netdev_priv(dev);
-	struct devlink_port *port;
 
 	if (!netif_device_present(dev))
 		return NULL;
-	port = mlx5e_devlink_get_dl_port(priv);
-	if (port->registered)
-		return port;
-	return NULL;
+
+	return mlx5e_devlink_get_dl_port(priv);
 }
drivers/net/ethernet/mellanox/mlx5/core/en_main.c +2 -9
index 308ccace48d0..adcca0b3b7bd 100644
@@ -4933,7 +4933,6 @@ static int mlx5e_nic_init(struct mlx5_core_dev *mdev,
 			  struct net_device *netdev)
 {
 	struct mlx5e_priv *priv = netdev_priv(netdev);
-	struct devlink_port *dl_port;
 	int err;
 
 	mlx5e_build_nic_params(priv, &priv->xsk, netdev->mtu);

@@ -4949,19 +4948,13 @@ static int mlx5e_nic_init(struct mlx5_core_dev *mdev,
 	if (err)
 		mlx5_core_err(mdev, "TLS initialization failed, %d\n", err);
 
-	dl_port = mlx5e_devlink_get_dl_port(priv);
-	if (dl_port->registered)
-		mlx5e_health_create_reporters(priv);
-
+	mlx5e_health_create_reporters(priv);
 	return 0;
 }
 
 static void mlx5e_nic_cleanup(struct mlx5e_priv *priv)
 {
-	struct devlink_port *dl_port = mlx5e_devlink_get_dl_port(priv);
-
-	if (dl_port->registered)
-		mlx5e_health_destroy_reporters(priv);
+	mlx5e_health_destroy_reporters(priv);
 	mlx5e_tls_cleanup(priv);
 	mlx5e_ipsec_cleanup(priv);
 }
Signature · 1 line
2.31.1
author
#3 lore raw

PATCH 3/3 devlink: Remove duplicated registration check

In reply to Leon Romanovsky

From: Leon Romanovsky <[email protected]>

Both registered flag and devlink pointer are set at the same time and indicate the same thing - devlink/devlink_port are ready. Instead of checking ->registered use devlink pointer as an indication.

Signed-off-by Leon Romanovsky <[email protected]>
---
 include/net/devlink.h |  4 +---
 net/core/devlink.c    | 19 ++++++++++---------
 2 files changed, 11 insertions(+), 12 deletions(-)
include/net/devlink.h +1 -3
index 57b738b78073..e48a62320407 100644
@@ -55,8 +55,7 @@ struct devlink {
 			    * port, sb, dpipe, resource, params, region, traps and more.
 			    */
 	u8 reload_failed:1,
-	   reload_enabled:1,
-	   registered:1;
+	   reload_enabled:1;
 	char priv[0] __aligned(NETDEV_ALIGN);
 };
 

@@ -158,7 +157,6 @@ struct devlink_port {
 	struct list_head region_list;
 	struct devlink *devlink;
 	unsigned int index;
-	bool registered;
 	spinlock_t type_lock; /* Protects type and type_dev
 			       * pointer consistency.
 			       */
net/core/devlink.c +10 -9
index 8fdd04f00fd7..b596a971b473 100644
@@ -115,7 +115,7 @@ static void __devlink_net_set(struct devlink *devlink, struct net *net)
 
 void devlink_net_set(struct devlink *devlink, struct net *net)
 {
-	if (WARN_ON(devlink->registered))
+	if (WARN_ON(devlink->dev))
 		return;
 	__devlink_net_set(devlink, net);
 }

@@ -1043,7 +1043,7 @@ static void devlink_port_notify(struct devlink_port *devlink_port,
 	struct sk_buff *msg;
 	int err;
 
-	if (!devlink_port->registered)
+	if (!devlink_port->devlink)
 		return;
 
 	WARN_ON(cmd != DEVLINK_CMD_PORT_NEW && cmd != DEVLINK_CMD_PORT_DEL);

@@ -8817,8 +8817,8 @@ EXPORT_SYMBOL_GPL(devlink_alloc);
  */
 int devlink_register(struct devlink *devlink, struct device *dev)
 {
+	WARN_ON(devlink->dev);
 	devlink->dev = dev;
-	devlink->registered = true;
 	mutex_lock(&devlink_mutex);
 	list_add_tail(&devlink->list, &devlink_list);
 	devlink_notify(devlink, DEVLINK_CMD_NEW);

@@ -8960,9 +8960,10 @@ int devlink_port_register(struct devlink *devlink,
 		mutex_unlock(&devlink->lock);
 		return -EEXIST;
 	}
+
+	WARN_ON(devlink_port->devlink);
 	devlink_port->devlink = devlink;
 	devlink_port->index = port_index;
-	devlink_port->registered = true;
 	spin_lock_init(&devlink_port->type_lock);
 	INIT_LIST_HEAD(&devlink_port->reporter_list);
 	mutex_init(&devlink_port->reporters_lock);

@@ -9001,7 +9002,7 @@ static void __devlink_port_type_set(struct devlink_port *devlink_port,
 				    enum devlink_port_type type,
 				    void *type_dev)
 {
-	if (WARN_ON(!devlink_port->registered))
+	if (WARN_ON(!devlink_port->devlink))
 		return;
 	devlink_port_type_warn_cancel(devlink_port);
 	spin_lock_bh(&devlink_port->type_lock);

@@ -9121,7 +9122,7 @@ void devlink_port_attrs_set(struct devlink_port *devlink_port,
 {
 	int ret;
 
-	if (WARN_ON(devlink_port->registered))
+	if (WARN_ON(devlink_port->devlink))
 		return;
 	devlink_port->attrs = *attrs;
 	ret = __devlink_port_attrs_set(devlink_port, attrs->flavour);

@@ -9145,7 +9146,7 @@ void devlink_port_attrs_pci_pf_set(struct devlink_port *devlink_port, u32 contro
 	struct devlink_port_attrs *attrs = &devlink_port->attrs;
 	int ret;
 
-	if (WARN_ON(devlink_port->registered))
+	if (WARN_ON(devlink_port->devlink))
 		return;
 	ret = __devlink_port_attrs_set(devlink_port,
 				       DEVLINK_PORT_FLAVOUR_PCI_PF);

@@ -9172,7 +9173,7 @@ void devlink_port_attrs_pci_vf_set(struct devlink_port *devlink_port, u32 contro
 	struct devlink_port_attrs *attrs = &devlink_port->attrs;
 	int ret;
 
-	if (WARN_ON(devlink_port->registered))
+	if (WARN_ON(devlink_port->devlink))
 		return;
 	ret = __devlink_port_attrs_set(devlink_port,
 				       DEVLINK_PORT_FLAVOUR_PCI_VF);

@@ -9200,7 +9201,7 @@ void devlink_port_attrs_pci_sf_set(struct devlink_port *devlink_port, u32 contro
 	struct devlink_port_attrs *attrs = &devlink_port->attrs;
 	int ret;
 
-	if (WARN_ON(devlink_port->registered))
+	if (WARN_ON(devlink_port->devlink))
 		return;
 	ret = __devlink_port_attrs_set(devlink_port,
 				       DEVLINK_PORT_FLAVOUR_PCI_SF);
Signature · 1 line
2.31.1
CI Applied
#4 lore raw
In reply to Leon Romanovsky
This series was applied to netdev/net-next.git (refs/heads/master):

Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

Quoted 8 lines · On Wed, 28 Jul 2021 10:33:44 +0300 you wrote:

From: Leon Romanovsky <[email protected]>

Changelog:
v1:
* Added two new patches that remove registration field from mlx5 and ti drivers.
v0: https://lore.kernel.org/lkml/ed7bbb1e4c51dd58e6035a058e93d16f883b09ce.1627215829.git.leonro@nvidia.com

[...]

Here is the summary with links:
- [net-next,v1,1/3] net: ti: am65-cpsw-nuss: fix wrong devlink release order

    https://git.kernel.org/netdev/net-next/c/acf34954efd1
  - [net-next,v1,2/3] net/mlx5: Don't rely on always true registered field
    https://git.kernel.org/netdev/net-next/c/35f6986743d7
  - [net-next,v1,3/3] devlink: Remove duplicated registration check
    https://git.kernel.org/netdev/net-next/c/d7907a2b1a3b

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