[patch net-next 0/3] devlink: fix reload notifications and remove features

[patch net-next 0/3] devlink: fix reload notifications and remove features

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

First two patches adjust notifications during devlink reload.
The last patch removes no longer needed devlink features.

Jiri Pirko (3):
  devlink: move devlink reload notifications back in between _down() and
    _up() calls
  devlink: send objects notifications during devlink reload
  devlink: remove devlink features

 .../net/ethernet/broadcom/bnxt/bnxt_devlink.c |  1 -
 .../hisilicon/hns3/hns3pf/hclge_devlink.c     |  1 -
 .../hisilicon/hns3/hns3vf/hclgevf_devlink.c   |  1 -
 drivers/net/ethernet/intel/ice/ice_devlink.c  |  1 -
 drivers/net/ethernet/mellanox/mlx4/main.c     |  1 -
 .../net/ethernet/mellanox/mlx5/core/devlink.c |  9 ++--
 drivers/net/ethernet/mellanox/mlxsw/core.c    |  1 -
 drivers/net/netdevsim/dev.c                   |  1 -
 include/net/devlink.h                         |  2 +-
 net/devlink/core.c                            | 19 --------
 net/devlink/devl_internal.h                   |  1 -
 net/devlink/leftover.c                        | 46 +++++--------------
 12 files changed, 17 insertions(+), 67 deletions(-)

-- 
2.39.0

[patch net-next 1/3] devlink: move devlink reload notifications back in between _down() and _up() calls

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

This effectively reverts commit 05a7f4a8dff1 ("devlink: Break parameter
notification sequence to be before/after unload/load driver").

Cited commit resolved a problem in mlx5 params implementation,
when param notification code accessed memory previously freed
during reload.

Now, when the params can be registered and unregistered when devlink
instance is registered, mlx5 code unregisters the problematic param
during devlink reload. The fix is therefore no longer needed.

Current behavior is a it problematic, as it sends DEL notifications even
in potential case when reload_down() call fails which might confuse
userspace notifications listener.

So move the reload notifications back where they were originally in
between reload_down() and reload_up() calls.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 net/devlink/leftover.c | 37 ++++++++++++++++---------------------
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c
index bd4c5d2dd612..24e20861a28b 100644
--- a/net/devlink/leftover.c
+++ b/net/devlink/leftover.c
@@ -4235,12 +4235,11 @@ static void devlink_param_notify(struct devlink *devlink,
 				 struct devlink_param_item *param_item,
 				 enum devlink_command cmd);
 
-static void devlink_ns_change_notify(struct devlink *devlink,
-				     struct net *dest_net, struct net *curr_net,
-				     bool new)
+static void devlink_reload_netns_change(struct devlink *devlink,
+					struct net *curr_net,
+					struct net *dest_net)
 {
 	struct devlink_param_item *param_item;
-	enum devlink_command cmd;
 
 	/* Userspace needs to be notified about devlink objects
 	 * removed from original and entering new network namespace.
@@ -4248,18 +4247,19 @@ static void devlink_ns_change_notify(struct devlink *devlink,
 	 * reload process so the notifications are generated separatelly.
 	 */
 
-	if (!dest_net || net_eq(dest_net, curr_net))
-		return;
+	list_for_each_entry(param_item, &devlink->param_list, list)
+		devlink_param_notify(devlink, 0, param_item,
+				     DEVLINK_CMD_PARAM_DEL);
+	devlink_notify(devlink, DEVLINK_CMD_DEL);
 
-	if (new)
-		devlink_notify(devlink, DEVLINK_CMD_NEW);
+	move_netdevice_notifier_net(curr_net, dest_net,
+				    &devlink->netdevice_nb);
+	write_pnet(&devlink->_net, dest_net);
 
-	cmd = new ? DEVLINK_CMD_PARAM_NEW : DEVLINK_CMD_PARAM_DEL;
+	devlink_notify(devlink, DEVLINK_CMD_NEW);
 	list_for_each_entry(param_item, &devlink->param_list, list)
-		devlink_param_notify(devlink, 0, param_item, cmd);
-
-	if (!new)
-		devlink_notify(devlink, DEVLINK_CMD_DEL);
+		devlink_param_notify(devlink, 0, param_item,
+				     DEVLINK_CMD_PARAM_NEW);
 }
 
 static void devlink_reload_failed_set(struct devlink *devlink,
@@ -4341,24 +4341,19 @@ int devlink_reload(struct devlink *devlink, struct net *dest_net,
 	memcpy(remote_reload_stats, devlink->stats.remote_reload_stats,
 	       sizeof(remote_reload_stats));
 
-	curr_net = devlink_net(devlink);
-	devlink_ns_change_notify(devlink, dest_net, curr_net, false);
 	err = devlink->ops->reload_down(devlink, !!dest_net, action, limit, extack);
 	if (err)
 		return err;
 
-	if (dest_net && !net_eq(dest_net, curr_net)) {
-		move_netdevice_notifier_net(curr_net, dest_net,
-					    &devlink->netdevice_nb);
-		write_pnet(&devlink->_net, dest_net);
-	}
+	curr_net = devlink_net(devlink);
+	if (dest_net && !net_eq(dest_net, curr_net))
+		devlink_reload_netns_change(devlink, curr_net, dest_net);
 
 	err = devlink->ops->reload_up(devlink, action, limit, actions_performed, extack);
 	devlink_reload_failed_set(devlink, !!err);
 	if (err)
 		return err;
 
-	devlink_ns_change_notify(devlink, dest_net, curr_net, true);
 	WARN_ON(!(*actions_performed & BIT(action)));
 	/* Catch driver on updating the remote action within devlink reload */
 	WARN_ON(memcmp(remote_reload_stats, devlink->stats.remote_reload_stats,
-- 
2.39.0

[patch net-next 2/3] devlink: send objects notifications during devlink reload

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Currently, the notifications are only sent for params. People who
introduced other objects forgot to add the reload notifications here.

To make sure all notifications happen according to existing comment,
benefit from existence of devlink_notify_register/unregister() helpers
and use them in reload code.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 net/devlink/leftover.c | 20 ++------------------
 1 file changed, 2 insertions(+), 18 deletions(-)

diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c
index 24e20861a28b..4f78ef5a46af 100644
--- a/net/devlink/leftover.c
+++ b/net/devlink/leftover.c
@@ -4230,36 +4230,20 @@ static struct net *devlink_netns_get(struct sk_buff *skb,
 	return net;
 }
 
-static void devlink_param_notify(struct devlink *devlink,
-				 unsigned int port_index,
-				 struct devlink_param_item *param_item,
-				 enum devlink_command cmd);
-
 static void devlink_reload_netns_change(struct devlink *devlink,
 					struct net *curr_net,
 					struct net *dest_net)
 {
-	struct devlink_param_item *param_item;
-
 	/* Userspace needs to be notified about devlink objects
 	 * removed from original and entering new network namespace.
 	 * The rest of the devlink objects are re-created during
 	 * reload process so the notifications are generated separatelly.
 	 */
-
-	list_for_each_entry(param_item, &devlink->param_list, list)
-		devlink_param_notify(devlink, 0, param_item,
-				     DEVLINK_CMD_PARAM_DEL);
-	devlink_notify(devlink, DEVLINK_CMD_DEL);
-
+	devlink_notify_unregister(devlink);
 	move_netdevice_notifier_net(curr_net, dest_net,
 				    &devlink->netdevice_nb);
 	write_pnet(&devlink->_net, dest_net);
-
-	devlink_notify(devlink, DEVLINK_CMD_NEW);
-	list_for_each_entry(param_item, &devlink->param_list, list)
-		devlink_param_notify(devlink, 0, param_item,
-				     DEVLINK_CMD_PARAM_NEW);
+	devlink_notify_register(devlink);
 }
 
 static void devlink_reload_failed_set(struct devlink *devlink,
-- 
2.39.0

[patch net-next 3/3] devlink: remove devlink features

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Devlink features were introduced to disallow devlink reload calls of
userspace before the devlink was fully initialized. The reason for this
workaround was the fact that devlink reload was originally called
without devlink instance lock held.

However, with recent changes that converted devlink reload to be
performed under devlink instance lock, this is redundant so remove
devlink features entirely.

Note that mlx5 used this to enable devlink reload conditionally only
when device didn't act as multi port slave. Move the multi port check
into mlx5_devlink_reload_down() callback alongside with the other
checks preventing the device from reload in certain states.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 .../net/ethernet/broadcom/bnxt/bnxt_devlink.c |  1 -
 .../hisilicon/hns3/hns3pf/hclge_devlink.c     |  1 -
 .../hisilicon/hns3/hns3vf/hclgevf_devlink.c   |  1 -
 drivers/net/ethernet/intel/ice/ice_devlink.c  |  1 -
 drivers/net/ethernet/mellanox/mlx4/main.c     |  1 -
 .../net/ethernet/mellanox/mlx5/core/devlink.c |  9 +++++----
 drivers/net/ethernet/mellanox/mlxsw/core.c    |  1 -
 drivers/net/netdevsim/dev.c                   |  1 -
 include/net/devlink.h                         |  2 +-
 net/devlink/core.c                            | 19 -------------------
 net/devlink/devl_internal.h                   |  1 -
 net/devlink/leftover.c                        |  3 ---
 12 files changed, 6 insertions(+), 35 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index 26913dc816d3..8b3e7697390f 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -1303,7 +1303,6 @@ int bnxt_dl_register(struct bnxt *bp)
 	if (rc)
 		goto err_dl_port_unreg;
 
-	devlink_set_features(dl, DEVLINK_F_RELOAD);
 out:
 	devlink_register(dl);
 	return 0;
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_devlink.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_devlink.c
index 3d3b69605423..9a939c0b217f 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_devlink.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_devlink.c
@@ -114,7 +114,6 @@ int hclge_devlink_init(struct hclge_dev *hdev)
 	priv->hdev = hdev;
 	hdev->devlink = devlink;
 
-	devlink_set_features(devlink, DEVLINK_F_RELOAD);
 	devlink_register(devlink);
 	return 0;
 }
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_devlink.c b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_devlink.c
index a6c3c5e8f0ab..1b535142c65a 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_devlink.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_devlink.c
@@ -116,7 +116,6 @@ int hclgevf_devlink_init(struct hclgevf_dev *hdev)
 	priv->hdev = hdev;
 	hdev->devlink = devlink;
 
-	devlink_set_features(devlink, DEVLINK_F_RELOAD);
 	devlink_register(devlink);
 	return 0;
 }
diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
index ce753d23aba9..88497363fc4c 100644
--- a/drivers/net/ethernet/intel/ice/ice_devlink.c
+++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
@@ -1376,7 +1376,6 @@ void ice_devlink_register(struct ice_pf *pf)
 {
 	struct devlink *devlink = priv_to_devlink(pf);
 
-	devlink_set_features(devlink, DEVLINK_F_RELOAD);
 	devlink_register(devlink);
 }
 
diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c
index 6152f77dcfd8..277738c50c56 100644
--- a/drivers/net/ethernet/mellanox/mlx4/main.c
+++ b/drivers/net/ethernet/mellanox/mlx4/main.c
@@ -4031,7 +4031,6 @@ static int mlx4_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
 		goto err_params_unregister;
 
 	pci_save_state(pdev);
-	devlink_set_features(devlink, DEVLINK_F_RELOAD);
 	devl_unlock(devlink);
 	devlink_register(devlink);
 	return 0;
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
index 95a69544a685..63fb7912b032 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
@@ -156,6 +156,11 @@ static int mlx5_devlink_reload_down(struct devlink *devlink, bool netns_change,
 		return -EOPNOTSUPP;
 	}
 
+	if (mlx5_core_is_mp_slave(dev)) {
+		NL_SET_ERR_MSG_MOD(extack, "reload is unsupported for multi port slave");
+		return -EOPNOTSUPP;
+	}
+
 	if (pci_num_vf(pdev)) {
 		NL_SET_ERR_MSG_MOD(extack, "reload while VFs are present is unfavorable");
 	}
@@ -744,7 +749,6 @@ void mlx5_devlink_traps_unregister(struct devlink *devlink)
 
 int mlx5_devlink_params_register(struct devlink *devlink)
 {
-	struct mlx5_core_dev *dev = devlink_priv(devlink);
 	int err;
 
 	err = devl_params_register(devlink, mlx5_devlink_params,
@@ -762,9 +766,6 @@ int mlx5_devlink_params_register(struct devlink *devlink)
 	if (err)
 		goto max_uc_list_err;
 
-	if (!mlx5_core_is_mp_slave(dev))
-		devlink_set_features(devlink, DEVLINK_F_RELOAD);
-
 	return 0;
 
 max_uc_list_err:
diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c
index f8623e8388c8..42422a106433 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/core.c
@@ -2285,7 +2285,6 @@ __mlxsw_core_bus_device_register(const struct mlxsw_bus_info *mlxsw_bus_info,
 	}
 
 	if (!reload) {
-		devlink_set_features(devlink, DEVLINK_F_RELOAD);
 		devl_unlock(devlink);
 		devlink_register(devlink);
 	}
diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index f88095b0f836..6045bece2654 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -1609,7 +1609,6 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
 		goto err_hwstats_exit;
 
 	nsim_dev->esw_mode = DEVLINK_ESWITCH_MODE_LEGACY;
-	devlink_set_features(devlink, DEVLINK_F_RELOAD);
 	devl_unlock(devlink);
 	return 0;
 
diff --git a/include/net/devlink.h b/include/net/devlink.h
index ab654cf552b8..2e85a5970a32 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1645,7 +1645,7 @@ static inline struct devlink *devlink_alloc(const struct devlink_ops *ops,
 {
 	return devlink_alloc_ns(ops, priv_size, &init_net, dev);
 }
-void devlink_set_features(struct devlink *devlink, u64 features);
+
 int devl_register(struct devlink *devlink);
 void devl_unregister(struct devlink *devlink);
 void devlink_register(struct devlink *devlink);
diff --git a/net/devlink/core.c b/net/devlink/core.c
index 6c0e2fc57e45..aeffd1b8206d 100644
--- a/net/devlink/core.c
+++ b/net/devlink/core.c
@@ -125,23 +125,6 @@ struct devlink *devlinks_xa_find_get(struct net *net, unsigned long *indexp)
 	goto retry;
 }
 
-/**
- *	devlink_set_features - Set devlink supported features
- *
- *	@devlink: devlink
- *	@features: devlink support features
- *
- *	This interface allows us to set reload ops separatelly from
- *	the devlink_alloc.
- */
-void devlink_set_features(struct devlink *devlink, u64 features)
-{
-	WARN_ON(features & DEVLINK_F_RELOAD &&
-		!devlink_reload_supported(devlink->ops));
-	devlink->features = features;
-}
-EXPORT_SYMBOL_GPL(devlink_set_features);
-
 /**
  * devl_register - Register devlink instance
  * @devlink: devlink
@@ -303,7 +286,6 @@ static void __net_exit devlink_pernet_pre_exit(struct net *net)
 	 * all devlink instances from this namespace into init_net.
 	 */
 	devlinks_xa_for_each_registered_get(net, index, devlink) {
-		WARN_ON(!(devlink->features & DEVLINK_F_RELOAD));
 		devl_lock(devlink);
 		err = 0;
 		if (devl_is_registered(devlink))
@@ -313,7 +295,6 @@ static void __net_exit devlink_pernet_pre_exit(struct net *net)
 					     &actions_performed, NULL);
 		devl_unlock(devlink);
 		devlink_put(devlink);
-
 		if (err && err != -EOPNOTSUPP)
 			pr_warn("Failed to reload devlink instance into init_net\n");
 	}
diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
index d0d889038138..ba161de4120e 100644
--- a/net/devlink/devl_internal.h
+++ b/net/devlink/devl_internal.h
@@ -38,7 +38,6 @@ struct devlink {
 	struct list_head trap_policer_list;
 	struct list_head linecard_list;
 	const struct devlink_ops *ops;
-	u64 features;
 	struct xarray snapshot_ids;
 	struct devlink_dev_stats stats;
 	struct device *dev;
diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c
index 4f78ef5a46af..92210587d349 100644
--- a/net/devlink/leftover.c
+++ b/net/devlink/leftover.c
@@ -4387,9 +4387,6 @@ static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info)
 	u32 actions_performed;
 	int err;
 
-	if (!(devlink->features & DEVLINK_F_RELOAD))
-		return -EOPNOTSUPP;
-
 	err = devlink_resources_validate(devlink, NULL, info);
 	if (err) {
 		NL_SET_ERR_MSG_MOD(info->extack, "resources size validation failed");
-- 
2.39.0

Re: [patch net-next 1/3] devlink: move devlink reload notifications back in between _down() and _up() calls

[Jacob Keller]

On 1/27/2023 7:50 AM, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> This effectively reverts commit 05a7f4a8dff1 ("devlink: Break parameter
> notification sequence to be before/after unload/load driver").
> 
> Cited commit resolved a problem in mlx5 params implementation,
> when param notification code accessed memory previously freed
> during reload.
> 
> Now, when the params can be registered and unregistered when devlink
> instance is registered, mlx5 code unregisters the problematic param
> during devlink reload. The fix is therefore no longer needed.
> 
> Current behavior is a it problematic, as it sends DEL notifications even
> in potential case when reload_down() call fails which might confuse
> userspace notifications listener.
> 
> So move the reload notifications back where they were originally in
> between reload_down() and reload_up() calls.
> 
> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
> ---

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

>  net/devlink/leftover.c | 37 ++++++++++++++++---------------------
>  1 file changed, 16 insertions(+), 21 deletions(-)
> 
> diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c
> index bd4c5d2dd612..24e20861a28b 100644
> --- a/net/devlink/leftover.c
> +++ b/net/devlink/leftover.c
> @@ -4235,12 +4235,11 @@ static void devlink_param_notify(struct devlink *devlink,
>  				 struct devlink_param_item *param_item,
>  				 enum devlink_command cmd);
>  
> -static void devlink_ns_change_notify(struct devlink *devlink,
> -				     struct net *dest_net, struct net *curr_net,
> -				     bool new)
> +static void devlink_reload_netns_change(struct devlink *devlink,
> +					struct net *curr_net,
> +					struct net *dest_net)
>  {
>  	struct devlink_param_item *param_item;
> -	enum devlink_command cmd;
>  
>  	/* Userspace needs to be notified about devlink objects
>  	 * removed from original and entering new network namespace.
> @@ -4248,18 +4247,19 @@ static void devlink_ns_change_notify(struct devlink *devlink,
>  	 * reload process so the notifications are generated separatelly.
>  	 */
>  
> -	if (!dest_net || net_eq(dest_net, curr_net))
> -		return;
> +	list_for_each_entry(param_item, &devlink->param_list, list)
> +		devlink_param_notify(devlink, 0, param_item,
> +				     DEVLINK_CMD_PARAM_DEL);
> +	devlink_notify(devlink, DEVLINK_CMD_DEL);
>  
> -	if (new)
> -		devlink_notify(devlink, DEVLINK_CMD_NEW);
> +	move_netdevice_notifier_net(curr_net, dest_net,
> +				    &devlink->netdevice_nb);
> +	write_pnet(&devlink->_net, dest_net);
>  
> -	cmd = new ? DEVLINK_CMD_PARAM_NEW : DEVLINK_CMD_PARAM_DEL;
> +	devlink_notify(devlink, DEVLINK_CMD_NEW);
>  	list_for_each_entry(param_item, &devlink->param_list, list)
> -		devlink_param_notify(devlink, 0, param_item, cmd);
> -
> -	if (!new)
> -		devlink_notify(devlink, DEVLINK_CMD_DEL);
> +		devlink_param_notify(devlink, 0, param_item,
> +				     DEVLINK_CMD_PARAM_NEW);
>  }
>  
>  static void devlink_reload_failed_set(struct devlink *devlink,
> @@ -4341,24 +4341,19 @@ int devlink_reload(struct devlink *devlink, struct net *dest_net,
>  	memcpy(remote_reload_stats, devlink->stats.remote_reload_stats,
>  	       sizeof(remote_reload_stats));
>  
> -	curr_net = devlink_net(devlink);
> -	devlink_ns_change_notify(devlink, dest_net, curr_net, false);
>  	err = devlink->ops->reload_down(devlink, !!dest_net, action, limit, extack);
>  	if (err)
>  		return err;
>  
> -	if (dest_net && !net_eq(dest_net, curr_net)) {
> -		move_netdevice_notifier_net(curr_net, dest_net,
> -					    &devlink->netdevice_nb);
> -		write_pnet(&devlink->_net, dest_net);
> -	}
> +	curr_net = devlink_net(devlink);
> +	if (dest_net && !net_eq(dest_net, curr_net))
> +		devlink_reload_netns_change(devlink, curr_net, dest_net);
>  
>  	err = devlink->ops->reload_up(devlink, action, limit, actions_performed, extack);
>  	devlink_reload_failed_set(devlink, !!err);
>  	if (err)
>  		return err;
>  
> -	devlink_ns_change_notify(devlink, dest_net, curr_net, true);
>  	WARN_ON(!(*actions_performed & BIT(action)));
>  	/* Catch driver on updating the remote action within devlink reload */
>  	WARN_ON(memcmp(remote_reload_stats, devlink->stats.remote_reload_stats,

Re: [patch net-next 2/3] devlink: send objects notifications during devlink reload

[Jacob Keller]

On 1/27/2023 7:50 AM, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> Currently, the notifications are only sent for params. People who
> introduced other objects forgot to add the reload notifications here.
> 
> To make sure all notifications happen according to existing comment,
> benefit from existence of devlink_notify_register/unregister() helpers
> and use them in reload code.
> 

Nice cleanup. Should be harder to forget notifications in the future!


Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
> ---
>  net/devlink/leftover.c | 20 ++------------------
>  1 file changed, 2 insertions(+), 18 deletions(-)
> 
> diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c
> index 24e20861a28b..4f78ef5a46af 100644
> --- a/net/devlink/leftover.c
> +++ b/net/devlink/leftover.c
> @@ -4230,36 +4230,20 @@ static struct net *devlink_netns_get(struct sk_buff *skb,
>  	return net;
>  }
>  
> -static void devlink_param_notify(struct devlink *devlink,
> -				 unsigned int port_index,
> -				 struct devlink_param_item *param_item,
> -				 enum devlink_command cmd);
> -
>  static void devlink_reload_netns_change(struct devlink *devlink,
>  					struct net *curr_net,
>  					struct net *dest_net)
>  {
> -	struct devlink_param_item *param_item;
> -
>  	/* Userspace needs to be notified about devlink objects
>  	 * removed from original and entering new network namespace.
>  	 * The rest of the devlink objects are re-created during
>  	 * reload process so the notifications are generated separatelly.
>  	 */
> -
> -	list_for_each_entry(param_item, &devlink->param_list, list)
> -		devlink_param_notify(devlink, 0, param_item,
> -				     DEVLINK_CMD_PARAM_DEL);
> -	devlink_notify(devlink, DEVLINK_CMD_DEL);
> -
> +	devlink_notify_unregister(devlink);
>  	move_netdevice_notifier_net(curr_net, dest_net,
>  				    &devlink->netdevice_nb);
>  	write_pnet(&devlink->_net, dest_net);
> -
> -	devlink_notify(devlink, DEVLINK_CMD_NEW);
> -	list_for_each_entry(param_item, &devlink->param_list, list)
> -		devlink_param_notify(devlink, 0, param_item,
> -				     DEVLINK_CMD_PARAM_NEW);
> +	devlink_notify_register(devlink);
>  }
>  
>  static void devlink_reload_failed_set(struct devlink *devlink,

Re: [patch net-next 3/3] devlink: remove devlink features

[Jacob Keller]

On 1/27/2023 7:50 AM, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> Devlink features were introduced to disallow devlink reload calls of
> userspace before the devlink was fully initialized. The reason for this
> workaround was the fact that devlink reload was originally called
> without devlink instance lock held.
> 
> However, with recent changes that converted devlink reload to be
> performed under devlink instance lock, this is redundant so remove
> devlink features entirely.
> 
> Note that mlx5 used this to enable devlink reload conditionally only
> when device didn't act as multi port slave. Move the multi port check
> into mlx5_devlink_reload_down() callback alongside with the other
> checks preventing the device from reload in certain states.
> 

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
> ---
>  .../net/ethernet/broadcom/bnxt/bnxt_devlink.c |  1 -
>  .../hisilicon/hns3/hns3pf/hclge_devlink.c     |  1 -
>  .../hisilicon/hns3/hns3vf/hclgevf_devlink.c   |  1 -
>  drivers/net/ethernet/intel/ice/ice_devlink.c  |  1 -
>  drivers/net/ethernet/mellanox/mlx4/main.c     |  1 -
>  .../net/ethernet/mellanox/mlx5/core/devlink.c |  9 +++++----
>  drivers/net/ethernet/mellanox/mlxsw/core.c    |  1 -
>  drivers/net/netdevsim/dev.c                   |  1 -
>  include/net/devlink.h                         |  2 +-
>  net/devlink/core.c                            | 19 -------------------
>  net/devlink/devl_internal.h                   |  1 -
>  net/devlink/leftover.c                        |  3 ---
>  12 files changed, 6 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> index 26913dc816d3..8b3e7697390f 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
> @@ -1303,7 +1303,6 @@ int bnxt_dl_register(struct bnxt *bp)
>  	if (rc)
>  		goto err_dl_port_unreg;
>  
> -	devlink_set_features(dl, DEVLINK_F_RELOAD);
>  out:
>  	devlink_register(dl);
>  	return 0;
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_devlink.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_devlink.c
> index 3d3b69605423..9a939c0b217f 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_devlink.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_devlink.c
> @@ -114,7 +114,6 @@ int hclge_devlink_init(struct hclge_dev *hdev)
>  	priv->hdev = hdev;
>  	hdev->devlink = devlink;
>  
> -	devlink_set_features(devlink, DEVLINK_F_RELOAD);
>  	devlink_register(devlink);
>  	return 0;
>  }
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_devlink.c b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_devlink.c
> index a6c3c5e8f0ab..1b535142c65a 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_devlink.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_devlink.c
> @@ -116,7 +116,6 @@ int hclgevf_devlink_init(struct hclgevf_dev *hdev)
>  	priv->hdev = hdev;
>  	hdev->devlink = devlink;
>  
> -	devlink_set_features(devlink, DEVLINK_F_RELOAD);
>  	devlink_register(devlink);
>  	return 0;
>  }
> diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
> index ce753d23aba9..88497363fc4c 100644
> --- a/drivers/net/ethernet/intel/ice/ice_devlink.c
> +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
> @@ -1376,7 +1376,6 @@ void ice_devlink_register(struct ice_pf *pf)
>  {
>  	struct devlink *devlink = priv_to_devlink(pf);
>  
> -	devlink_set_features(devlink, DEVLINK_F_RELOAD);
>  	devlink_register(devlink);
>  }
>  
> diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c
> index 6152f77dcfd8..277738c50c56 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/main.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/main.c
> @@ -4031,7 +4031,6 @@ static int mlx4_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
>  		goto err_params_unregister;
>  
>  	pci_save_state(pdev);
> -	devlink_set_features(devlink, DEVLINK_F_RELOAD);
>  	devl_unlock(devlink);
>  	devlink_register(devlink);
>  	return 0;
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
> index 95a69544a685..63fb7912b032 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
> @@ -156,6 +156,11 @@ static int mlx5_devlink_reload_down(struct devlink *devlink, bool netns_change,
>  		return -EOPNOTSUPP;
>  	}
>  
> +	if (mlx5_core_is_mp_slave(dev)) {
> +		NL_SET_ERR_MSG_MOD(extack, "reload is unsupported for multi port slave");
> +		return -EOPNOTSUPP;
> +	}
> +
>  	if (pci_num_vf(pdev)) {
>  		NL_SET_ERR_MSG_MOD(extack, "reload while VFs are present is unfavorable");
>  	}
> @@ -744,7 +749,6 @@ void mlx5_devlink_traps_unregister(struct devlink *devlink)
>  
>  int mlx5_devlink_params_register(struct devlink *devlink)
>  {
> -	struct mlx5_core_dev *dev = devlink_priv(devlink);
>  	int err;
>  
>  	err = devl_params_register(devlink, mlx5_devlink_params,
> @@ -762,9 +766,6 @@ int mlx5_devlink_params_register(struct devlink *devlink)
>  	if (err)
>  		goto max_uc_list_err;
>  
> -	if (!mlx5_core_is_mp_slave(dev))
> -		devlink_set_features(devlink, DEVLINK_F_RELOAD);
> -
>  	return 0;
>  
>  max_uc_list_err:
> diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c
> index f8623e8388c8..42422a106433 100644
> --- a/drivers/net/ethernet/mellanox/mlxsw/core.c
> +++ b/drivers/net/ethernet/mellanox/mlxsw/core.c
> @@ -2285,7 +2285,6 @@ __mlxsw_core_bus_device_register(const struct mlxsw_bus_info *mlxsw_bus_info,
>  	}
>  
>  	if (!reload) {
> -		devlink_set_features(devlink, DEVLINK_F_RELOAD);
>  		devl_unlock(devlink);
>  		devlink_register(devlink);
>  	}
> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
> index f88095b0f836..6045bece2654 100644
> --- a/drivers/net/netdevsim/dev.c
> +++ b/drivers/net/netdevsim/dev.c
> @@ -1609,7 +1609,6 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
>  		goto err_hwstats_exit;
>  
>  	nsim_dev->esw_mode = DEVLINK_ESWITCH_MODE_LEGACY;
> -	devlink_set_features(devlink, DEVLINK_F_RELOAD);
>  	devl_unlock(devlink);
>  	return 0;
>  
> diff --git a/include/net/devlink.h b/include/net/devlink.h
> index ab654cf552b8..2e85a5970a32 100644
> --- a/include/net/devlink.h
> +++ b/include/net/devlink.h
> @@ -1645,7 +1645,7 @@ static inline struct devlink *devlink_alloc(const struct devlink_ops *ops,
>  {
>  	return devlink_alloc_ns(ops, priv_size, &init_net, dev);
>  }
> -void devlink_set_features(struct devlink *devlink, u64 features);
> +
>  int devl_register(struct devlink *devlink);
>  void devl_unregister(struct devlink *devlink);
>  void devlink_register(struct devlink *devlink);
> diff --git a/net/devlink/core.c b/net/devlink/core.c
> index 6c0e2fc57e45..aeffd1b8206d 100644
> --- a/net/devlink/core.c
> +++ b/net/devlink/core.c
> @@ -125,23 +125,6 @@ struct devlink *devlinks_xa_find_get(struct net *net, unsigned long *indexp)
>  	goto retry;
>  }
>  
> -/**
> - *	devlink_set_features - Set devlink supported features
> - *
> - *	@devlink: devlink
> - *	@features: devlink support features
> - *
> - *	This interface allows us to set reload ops separatelly from
> - *	the devlink_alloc.
> - */
> -void devlink_set_features(struct devlink *devlink, u64 features)
> -{
> -	WARN_ON(features & DEVLINK_F_RELOAD &&
> -		!devlink_reload_supported(devlink->ops));
> -	devlink->features = features;
> -}
> -EXPORT_SYMBOL_GPL(devlink_set_features);
> -
>  /**
>   * devl_register - Register devlink instance
>   * @devlink: devlink
> @@ -303,7 +286,6 @@ static void __net_exit devlink_pernet_pre_exit(struct net *net)
>  	 * all devlink instances from this namespace into init_net.
>  	 */
>  	devlinks_xa_for_each_registered_get(net, index, devlink) {
> -		WARN_ON(!(devlink->features & DEVLINK_F_RELOAD));
>  		devl_lock(devlink);
>  		err = 0;
>  		if (devl_is_registered(devlink))
> @@ -313,7 +295,6 @@ static void __net_exit devlink_pernet_pre_exit(struct net *net)
>  					     &actions_performed, NULL);
>  		devl_unlock(devlink);
>  		devlink_put(devlink);
> -
>  		if (err && err != -EOPNOTSUPP)
>  			pr_warn("Failed to reload devlink instance into init_net\n");
>  	}
> diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h
> index d0d889038138..ba161de4120e 100644
> --- a/net/devlink/devl_internal.h
> +++ b/net/devlink/devl_internal.h
> @@ -38,7 +38,6 @@ struct devlink {
>  	struct list_head trap_policer_list;
>  	struct list_head linecard_list;
>  	const struct devlink_ops *ops;
> -	u64 features;
>  	struct xarray snapshot_ids;
>  	struct devlink_dev_stats stats;
>  	struct device *dev;
> diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c
> index 4f78ef5a46af..92210587d349 100644
> --- a/net/devlink/leftover.c
> +++ b/net/devlink/leftover.c
> @@ -4387,9 +4387,6 @@ static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info)
>  	u32 actions_performed;
>  	int err;
>  
> -	if (!(devlink->features & DEVLINK_F_RELOAD))
> -		return -EOPNOTSUPP;
> -
>  	err = devlink_resources_validate(devlink, NULL, info);
>  	if (err) {
>  		NL_SET_ERR_MSG_MOD(info->extack, "resources size validation failed");

Re: [patch net-next 0/3] devlink: fix reload notifications and remove features

[Jakub Kicinski]

On Fri, 27 Jan 2023 16:50:39 +0100 Jiri Pirko wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> First two patches adjust notifications during devlink reload.
> The last patch removes no longer needed devlink features.

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

Thanks!

Re: [patch net-next 0/3] devlink: fix reload notifications and remove features

[patchwork-bot+netdevbpf]

Hello:

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

On Fri, 27 Jan 2023 16:50:39 +0100 you wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> First two patches adjust notifications during devlink reload.
> The last patch removes no longer needed devlink features.
> 
> Jiri Pirko (3):
>   devlink: move devlink reload notifications back in between _down() and
>     _up() calls
>   devlink: send objects notifications during devlink reload
>   devlink: remove devlink features
> 
> [...]

Here is the summary with links:
  - [net-next,1/3] devlink: move devlink reload notifications back in between _down() and _up() calls
    https://git.kernel.org/netdev/net-next/c/7d7e9169a3ec
  - [net-next,2/3] devlink: send objects notifications during devlink reload
    https://git.kernel.org/netdev/net-next/c/a131315a47bb
  - [net-next,3/3] devlink: remove devlink features
    https://git.kernel.org/netdev/net-next/c/fb8421a94c56

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

Re: [patch net-next 0/3] devlink: fix reload notifications and remove features

[Jiri Pirko]

Mon, Jan 30, 2023 at 09:50:16AM CET, patchwork-bot+netdevbpf@kernel.org wrote:
>Hello:
>
>This series was applied to netdev/net-next.git (master)
>by David S. Miller <davem@davemloft.net>:
>
>On Fri, 27 Jan 2023 16:50:39 +0100 you wrote:
>> From: Jiri Pirko <jiri@nvidia.com>
>> 
>> First two patches adjust notifications during devlink reload.
>> The last patch removes no longer needed devlink features.
>> 
>> Jiri Pirko (3):
>>   devlink: move devlink reload notifications back in between _down() and
>>     _up() calls
>>   devlink: send objects notifications during devlink reload
>>   devlink: remove devlink features
>> 
>> [...]
>
>Here is the summary with links:
>  - [net-next,1/3] devlink: move devlink reload notifications back in between _down() and _up() calls
>    https://git.kernel.org/netdev/net-next/c/7d7e9169a3ec
>  - [net-next,2/3] devlink: send objects notifications during devlink reload
>    https://git.kernel.org/netdev/net-next/c/a131315a47bb
>  - [net-next,3/3] devlink: remove devlink features
>    https://git.kernel.org/netdev/net-next/c/fb8421a94c56

Hi.
The merge commit text is wrong:
    Merge branch 'devlink-next'

    Jakub Kicinski says:

    ====================
    devlink: fix reload notifications and remove features

    First two patches adjust notifications during devlink reload.
    The last patch removes no longer needed devlink features.

"Jakub Kicinsky says" should be "Jiri Pirko says". I thought this is
done by a bot? Is the bot buggy?



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

Re: [patch net-next 0/3] devlink: fix reload notifications and remove features

[Jakub Kicinski]

On Mon, 30 Jan 2023 11:50:08 +0100 Jiri Pirko wrote:
> The merge commit text is wrong:
>     Merge branch 'devlink-next'
> 
>     Jakub Kicinski says:
> 
>     ====================
>     devlink: fix reload notifications and remove features
> 
>     First two patches adjust notifications during devlink reload.
>     The last patch removes no longer needed devlink features.
> 
> "Jakub Kicinsky says" should be "Jiri Pirko says". I thought this is
> done by a bot? Is the bot buggy?

Hah :D 
It's done locally, not by the bot. Our (Daniel Borkmann's) script
fetches the author from patchwork, but I think DaveM has his own
automation.