[patch net-next 0/8] devlink: make sure devlink port registers/unregisters only for registered instance

[patch net-next 0/8] devlink: make sure devlink port registers/unregisters only for registered instance

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Currently, the devlink_register() is called as the last thing during
driver init phase. For devlink objects, this is fine as the
notifications of objects creations are withheld to be send only after
devlink instance is registered. Until devlink is registered it is not
visible to userspace.

But if a  netdev is registered before, user gets a notification with
a link to devlink, which is not visible to the user yet.
This is the event order user sees:
 * new netdev event over RT netlink
 * new devlink event over devlink netlink
 * new devlink_port event over devlink netlink

Also, this is not consistent with devlink port split or devlink reload
flows, where user gets notifications in following order:
 * new devlink event over devlink netlink
and then during port split or reload operation:
 * new devlink_port event over devlink netlink
 * new netdev event over RT netlink

In this case, devlink port and related netdev are registered on already
registered devlink instance.

Purpose of this patchset is to fix the drivers init flow so devlink port
gets registered only after devlink instance is registered.

Jiri Pirko (8):
  devlink: call devlink_port_register/unregister() on registered
    instance
  netdevsim: call devl_port_register/unregister() on registered instance
  mlxsw: call devl_port_register/unregister() on registered instance
  nfp: call devl_port_register/unregister() on registered instance
  mlx4: call devl_port_register/unregister() on registered instance
  mlx5: call devl_port_register/unregister() on registered instance
  devlink: assert if devl_port_register/unregister() is called on
    unregistered devlink instance
  devlink: remove port notifications from devlink_register/unregister()

 .../net/ethernet/broadcom/bnxt/bnxt_devlink.c | 29 +++++----
 .../net/ethernet/freescale/dpaa2/dpaa2-eth.c  |  7 ++-
 .../ethernet/fungible/funeth/funeth_main.c    | 17 ++++--
 drivers/net/ethernet/intel/ice/ice_main.c     | 21 ++++---
 .../ethernet/marvell/prestera/prestera_main.c |  6 +-
 drivers/net/ethernet/mellanox/mlx4/main.c     | 60 ++++++++++---------
 drivers/net/ethernet/mellanox/mlx5/core/dev.c | 10 +++-
 .../net/ethernet/mellanox/mlx5/core/main.c    | 17 +++---
 .../mellanox/mlx5/core/sf/dev/driver.c        |  9 +++
 drivers/net/ethernet/mellanox/mlxsw/core.c    | 24 ++++++++
 drivers/net/ethernet/mellanox/mlxsw/core.h    |  2 +
 .../net/ethernet/mellanox/mlxsw/spectrum.c    | 38 +++++++++---
 drivers/net/ethernet/mscc/ocelot_vsc7514.c    | 10 ++--
 .../net/ethernet/netronome/nfp/nfp_net_main.c | 22 +++++--
 .../ethernet/pensando/ionic/ionic_devlink.c   |  6 +-
 drivers/net/ethernet/ti/am65-cpsw-nuss.c      |  7 ++-
 drivers/net/netdevsim/dev.c                   | 31 ++++++++--
 net/core/devlink.c                            |  7 +--
 18 files changed, 218 insertions(+), 105 deletions(-)

-- 
2.37.3

[patch net-next 1/8] devlink: call devlink_port_register/unregister() on registered instance

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Change the drivers that use devlink_port_register/unregister() to call
these functions only in case devlink is registered.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
RFC->v1:
- shortened patch subject
---
 .../net/ethernet/broadcom/bnxt/bnxt_devlink.c | 29 ++++++++++---------
 .../net/ethernet/freescale/dpaa2/dpaa2-eth.c  |  7 +++--
 .../ethernet/fungible/funeth/funeth_main.c    | 17 +++++++----
 drivers/net/ethernet/intel/ice/ice_main.c     | 21 ++++++++------
 .../ethernet/marvell/prestera/prestera_main.c |  6 ++--
 drivers/net/ethernet/mscc/ocelot_vsc7514.c    | 10 +++----
 .../ethernet/pensando/ionic/ionic_devlink.c   |  6 ++--
 drivers/net/ethernet/ti/am65-cpsw-nuss.c      |  7 +++--
 8 files changed, 60 insertions(+), 43 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index 26913dc816d3..c2600ce7313c 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -1285,8 +1285,15 @@ int bnxt_dl_register(struct bnxt *bp)
 	    bp->hwrm_spec_code > 0x10803)
 		bp->eswitch_mode = DEVLINK_ESWITCH_MODE_LEGACY;
 
+	if (BNXT_PF(bp)) {
+		rc = bnxt_dl_params_register(bp);
+		if (rc)
+			goto err_dl_free;
+	}
+
+	devlink_register(dl);
 	if (!BNXT_PF(bp))
-		goto out;
+		return 0;
 
 	attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL;
 	attrs.phys.port_number = bp->pf.port_id;
@@ -1296,20 +1303,16 @@ int bnxt_dl_register(struct bnxt *bp)
 	rc = devlink_port_register(dl, &bp->dl_port, bp->pf.port_id);
 	if (rc) {
 		netdev_err(bp->dev, "devlink_port_register failed\n");
-		goto err_dl_free;
+		goto err_dl_unreg;
 	}
 
-	rc = bnxt_dl_params_register(bp);
-	if (rc)
-		goto err_dl_port_unreg;
-
 	devlink_set_features(dl, DEVLINK_F_RELOAD);
-out:
-	devlink_register(dl);
 	return 0;
 
-err_dl_port_unreg:
-	devlink_port_unregister(&bp->dl_port);
+err_dl_unreg:
+	devlink_unregister(dl);
+	if (BNXT_PF(bp))
+		bnxt_dl_params_unregister(bp);
 err_dl_free:
 	devlink_free(dl);
 	return rc;
@@ -1319,10 +1322,10 @@ void bnxt_dl_unregister(struct bnxt *bp)
 {
 	struct devlink *dl = bp->dl;
 
+	if (BNXT_PF(bp))
+		devlink_port_unregister(&bp->dl_port);
 	devlink_unregister(dl);
-	if (BNXT_PF(bp)) {
+	if (BNXT_PF(bp))
 		bnxt_dl_params_unregister(bp);
-		devlink_port_unregister(&bp->dl_port);
-	}
 	devlink_free(dl);
 }
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
index 0c35abb7d065..4e468c4c20e0 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
@@ -4954,6 +4954,8 @@ static int dpaa2_eth_probe(struct fsl_mc_device *dpni_dev)
 	if (err)
 		goto err_dl_trap_register;
 
+	dpaa2_eth_dl_register(priv);
+
 	err = dpaa2_eth_dl_port_add(priv);
 	if (err)
 		goto err_dl_port_add;
@@ -4968,12 +4970,12 @@ static int dpaa2_eth_probe(struct fsl_mc_device *dpni_dev)
 	dpaa2_dbg_add(priv);
 #endif
 
-	dpaa2_eth_dl_register(priv);
 	dev_info(dev, "Probed interface %s\n", net_dev->name);
 	return 0;
 
 err_netdev_reg:
 	dpaa2_eth_dl_port_del(priv);
+	dpaa2_eth_dl_unregister(priv);
 err_dl_port_add:
 	dpaa2_eth_dl_traps_unregister(priv);
 err_dl_trap_register:
@@ -5026,8 +5028,6 @@ static int dpaa2_eth_remove(struct fsl_mc_device *ls_dev)
 	net_dev = dev_get_drvdata(dev);
 	priv = netdev_priv(net_dev);
 
-	dpaa2_eth_dl_unregister(priv);
-
 #ifdef CONFIG_DEBUG_FS
 	dpaa2_dbg_remove(priv);
 #endif
@@ -5035,6 +5035,7 @@ static int dpaa2_eth_remove(struct fsl_mc_device *ls_dev)
 	unregister_netdev(net_dev);
 
 	dpaa2_eth_dl_port_del(priv);
+	dpaa2_eth_dl_unregister(priv);
 	dpaa2_eth_dl_traps_unregister(priv);
 	dpaa2_eth_dl_free(priv);
 
diff --git a/drivers/net/ethernet/fungible/funeth/funeth_main.c b/drivers/net/ethernet/fungible/funeth/funeth_main.c
index b4cce30e526a..e335071bf530 100644
--- a/drivers/net/ethernet/fungible/funeth/funeth_main.c
+++ b/drivers/net/ethernet/fungible/funeth/funeth_main.c
@@ -2018,17 +2018,22 @@ static int funeth_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (rc)
 		goto free_devlink;
 
+	fun_devlink_register(devlink);
+
 	rc = fun_get_res_count(fdev, FUN_ADMIN_OP_PORT);
-	if (rc > 0)
-		rc = fun_create_ports(ed, rc);
 	if (rc < 0)
-		goto disable_dev;
+		goto unregister_devlink;
+	if (rc > 0) {
+		rc = fun_create_ports(ed, rc);
+		if (rc < 0)
+			goto unregister_devlink;
+	}
 
 	fun_serv_restart(fdev);
-	fun_devlink_register(devlink);
 	return 0;
 
-disable_dev:
+unregister_devlink:
+	fun_devlink_unregister(devlink);
 	fun_dev_disable(fdev);
 free_devlink:
 	mutex_destroy(&ed->state_mutex);
@@ -2044,7 +2049,6 @@ static void funeth_remove(struct pci_dev *pdev)
 
 	ed = to_fun_ethdev(fdev);
 	devlink = priv_to_devlink(ed);
-	fun_devlink_unregister(devlink);
 
 #ifdef CONFIG_PCI_IOV
 	funeth_sriov_configure(pdev, 0);
@@ -2052,6 +2056,7 @@ static void funeth_remove(struct pci_dev *pdev)
 
 	fun_serv_stop(fdev);
 	fun_destroy_ports(ed);
+	fun_devlink_unregister(devlink);
 	fun_dev_disable(fdev);
 	mutex_destroy(&ed->state_mutex);
 
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 2b23b4714a26..f47d5b87f99b 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -4919,11 +4919,13 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
 	pcie_print_link_status(pf->pdev);
 
 probe_done:
-	err = ice_register_netdev(pf);
+	err = ice_devlink_register_params(pf);
 	if (err)
-		goto err_netdev_reg;
+		goto err_devlink_reg_param;
 
-	err = ice_devlink_register_params(pf);
+	ice_devlink_register(pf);
+
+	err = ice_register_netdev(pf);
 	if (err)
 		goto err_netdev_reg;
 
@@ -4934,7 +4936,7 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
 		if (pf->aux_idx < 0) {
 			dev_err(dev, "Failed to allocate device ID for AUX driver\n");
 			err = -ENOMEM;
-			goto err_devlink_reg_param;
+			goto err_ida_alloc;
 		}
 
 		err = ice_init_rdma(pf);
@@ -4947,15 +4949,16 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
 		dev_warn(dev, "RDMA is not supported on this device\n");
 	}
 
-	ice_devlink_register(pf);
 	return 0;
 
 err_init_aux_unroll:
 	pf->adev = NULL;
 	ida_free(&ice_aux_ida, pf->aux_idx);
-err_devlink_reg_param:
-	ice_devlink_unregister_params(pf);
+err_ida_alloc:
 err_netdev_reg:
+	ice_devlink_unregister(pf);
+	ice_devlink_unregister_params(pf);
+err_devlink_reg_param:
 err_send_version_unroll:
 	ice_vsi_release_all(pf);
 err_alloc_sw_unroll:
@@ -5051,7 +5054,6 @@ static void ice_remove(struct pci_dev *pdev)
 	struct ice_pf *pf = pci_get_drvdata(pdev);
 	int i;
 
-	ice_devlink_unregister(pf);
 	for (i = 0; i < ICE_MAX_RESET_WAIT; i++) {
 		if (!ice_is_reset_in_progress(pf->state))
 			break;
@@ -5071,7 +5073,6 @@ static void ice_remove(struct pci_dev *pdev)
 	ice_unplug_aux_dev(pf);
 	if (pf->aux_idx >= 0)
 		ida_free(&ice_aux_ida, pf->aux_idx);
-	ice_devlink_unregister_params(pf);
 	set_bit(ICE_DOWN, pf->state);
 
 	ice_deinit_lag(pf);
@@ -5083,6 +5084,8 @@ static void ice_remove(struct pci_dev *pdev)
 		ice_remove_arfs(pf);
 	ice_setup_mc_magic_wake(pf);
 	ice_vsi_release_all(pf);
+	ice_devlink_unregister(pf);
+	ice_devlink_unregister_params(pf);
 	mutex_destroy(&(&pf->hw)->fdir_fltr_lock);
 	ice_set_wake(pf);
 	ice_free_irq_msix_misc(pf);
diff --git a/drivers/net/ethernet/marvell/prestera/prestera_main.c b/drivers/net/ethernet/marvell/prestera/prestera_main.c
index 9d504142e51a..815056a130d9 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_main.c
+++ b/drivers/net/ethernet/marvell/prestera/prestera_main.c
@@ -1421,14 +1421,16 @@ static int prestera_switch_init(struct prestera_switch *sw)
 	if (err)
 		goto err_lag_init;
 
+	prestera_devlink_register(sw);
+
 	err = prestera_create_ports(sw);
 	if (err)
 		goto err_ports_create;
 
-	prestera_devlink_register(sw);
 	return 0;
 
 err_ports_create:
+	prestera_devlink_unregister(sw);
 	prestera_lag_fini(sw);
 err_lag_init:
 	prestera_devlink_traps_unregister(sw);
@@ -1455,8 +1457,8 @@ static int prestera_switch_init(struct prestera_switch *sw)
 
 static void prestera_switch_fini(struct prestera_switch *sw)
 {
-	prestera_devlink_unregister(sw);
 	prestera_destroy_ports(sw);
+	prestera_devlink_unregister(sw);
 	prestera_lag_fini(sw);
 	prestera_devlink_traps_unregister(sw);
 	prestera_span_fini(sw);
diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
index b097fd4a4061..69adde1da2a0 100644
--- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
+++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
@@ -562,10 +562,6 @@ static int mscc_ocelot_probe(struct platform_device *pdev)
 	if (err)
 		goto out_put_ports;
 
-	err = mscc_ocelot_init_ports(pdev, ports);
-	if (err)
-		goto out_ocelot_devlink_unregister;
-
 	if (ocelot->fdma)
 		ocelot_fdma_start(ocelot);
 
@@ -589,6 +585,10 @@ static int mscc_ocelot_probe(struct platform_device *pdev)
 	of_node_put(ports);
 	devlink_register(devlink);
 
+	err = mscc_ocelot_init_ports(pdev, ports);
+	if (err)
+		goto out_ocelot_devlink_unregister;
+
 	dev_info(&pdev->dev, "Ocelot switch probed\n");
 
 	return 0;
@@ -611,10 +611,10 @@ static int mscc_ocelot_remove(struct platform_device *pdev)
 
 	if (ocelot->fdma)
 		ocelot_fdma_deinit(ocelot);
+	mscc_ocelot_release_ports(ocelot);
 	devlink_unregister(ocelot->devlink);
 	ocelot_deinit_timestamp(ocelot);
 	ocelot_devlink_sb_unregister(ocelot);
-	mscc_ocelot_release_ports(ocelot);
 	mscc_ocelot_teardown_devlink_ports(ocelot);
 	ocelot_deinit(ocelot);
 	unregister_switchdev_blocking_notifier(&ocelot_switchdev_blocking_nb);
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_devlink.c b/drivers/net/ethernet/pensando/ionic/ionic_devlink.c
index e6ff757895ab..06670343f90b 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_devlink.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_devlink.c
@@ -78,16 +78,18 @@ int ionic_devlink_register(struct ionic *ionic)
 	struct devlink_port_attrs attrs = {};
 	int err;
 
+	devlink_register(dl);
+
 	attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL;
 	devlink_port_attrs_set(&ionic->dl_port, &attrs);
 	err = devlink_port_register(dl, &ionic->dl_port, 0);
 	if (err) {
 		dev_err(ionic->dev, "devlink_port_register failed: %d\n", err);
+		devlink_unregister(dl);
 		return err;
 	}
 
 	SET_NETDEV_DEVLINK_PORT(ionic->lif->netdev, &ionic->dl_port);
-	devlink_register(dl);
 	return 0;
 }
 
@@ -95,6 +97,6 @@ void ionic_devlink_unregister(struct ionic *ionic)
 {
 	struct devlink *dl = priv_to_devlink(ionic);
 
-	devlink_unregister(dl);
 	devlink_port_unregister(&ionic->dl_port);
+	devlink_unregister(dl);
 }
diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index 51c37e99d086..4ac4b7ada890 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -2522,6 +2522,8 @@ static int am65_cpsw_nuss_register_devlink(struct am65_cpsw_common *common)
 		}
 	}
 
+	devlink_register(common->devlink);
+
 	for (i = 1; i <= common->port_num; i++) {
 		port = am65_common_get_port(common, i);
 		dl_port = &port->devlink_port;
@@ -2542,7 +2544,6 @@ static int am65_cpsw_nuss_register_devlink(struct am65_cpsw_common *common)
 			goto dl_port_unreg;
 		}
 	}
-	devlink_register(common->devlink);
 	return ret;
 
 dl_port_unreg:
@@ -2552,6 +2553,7 @@ static int am65_cpsw_nuss_register_devlink(struct am65_cpsw_common *common)
 
 		devlink_port_unregister(dl_port);
 	}
+	devlink_unregister(common->devlink);
 dl_unreg:
 	devlink_free(common->devlink);
 	return ret;
@@ -2563,14 +2565,13 @@ static void am65_cpsw_unregister_devlink(struct am65_cpsw_common *common)
 	struct am65_cpsw_port *port;
 	int i;
 
-	devlink_unregister(common->devlink);
-
 	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);
 	}
+	devlink_unregister(common->devlink);
 
 	if (!AM65_CPSW_IS_CPSW2G(common) &&
 	    IS_ENABLED(CONFIG_TI_K3_AM65_CPSW_SWITCHDEV))
-- 
2.37.3

Re: [patch net-next 1/8] devlink: call devlink_port_register/unregister() on registered instance

[Shannon Nelson]

On 12/5/22 7:22 AM, Jiri Pirko wrote:
> 
> From: Jiri Pirko <jiri@nvidia.com>
> 
> Change the drivers that use devlink_port_register/unregister() to call
> these functions only in case devlink is registered.
> 
> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
> ---
> RFC->v1:
> - shortened patch subject
> ---
>   .../net/ethernet/broadcom/bnxt/bnxt_devlink.c | 29 ++++++++++---------
>   .../net/ethernet/freescale/dpaa2/dpaa2-eth.c  |  7 +++--
>   .../ethernet/fungible/funeth/funeth_main.c    | 17 +++++++----
>   drivers/net/ethernet/intel/ice/ice_main.c     | 21 ++++++++------
>   .../ethernet/marvell/prestera/prestera_main.c |  6 ++--
>   drivers/net/ethernet/mscc/ocelot_vsc7514.c    | 10 +++----
>   .../ethernet/pensando/ionic/ionic_devlink.c   |  6 ++--
>   drivers/net/ethernet/ti/am65-cpsw-nuss.c      |  7 +++--
>   8 files changed, 60 insertions(+), 43 deletions(-)
> 



> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_devlink.c b/drivers/net/ethernet/pensando/ionic/ionic_devlink.c
> index e6ff757895ab..06670343f90b 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_devlink.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_devlink.c
> @@ -78,16 +78,18 @@ int ionic_devlink_register(struct ionic *ionic)
>          struct devlink_port_attrs attrs = {};
>          int err;
> 
> +       devlink_register(dl);
> +
>          attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL;
>          devlink_port_attrs_set(&ionic->dl_port, &attrs);
>          err = devlink_port_register(dl, &ionic->dl_port, 0);
>          if (err) {
>                  dev_err(ionic->dev, "devlink_port_register failed: %d\n", err);
> +               devlink_unregister(dl);
>                  return err;
>          }
> 
>          SET_NETDEV_DEVLINK_PORT(ionic->lif->netdev, &ionic->dl_port);
> -       devlink_register(dl);
>          return 0;
>   }
> 
> @@ -95,6 +97,6 @@ void ionic_devlink_unregister(struct ionic *ionic)
>   {
>          struct devlink *dl = priv_to_devlink(ionic);
> 
> -       devlink_unregister(dl);
>          devlink_port_unregister(&ionic->dl_port);
> +       devlink_unregister(dl);
>   }

I don't know about the rest of the drivers, but this seems to be the 
exact opposite of what Leon did in this patch over a year ago:
https://lore.kernel.org/netdev/cover.1632565508.git.leonro@nvidia.com/

I haven't kept up on all the discussion about this, but is there no 
longer a worry about registering the devlink object before all the 
related configuration bits are in place?

Does this open any potential issues with userland programs seeing the 
devlink device and trying to access port before they get registered?

sln

Re: [patch net-next 1/8] devlink: call devlink_port_register/unregister() on registered instance

[Jiri Pirko]

Tue, Dec 06, 2022 at 12:55:32AM CET, shnelson@amd.com wrote:
>On 12/5/22 7:22 AM, Jiri Pirko wrote:
>> 
>> From: Jiri Pirko <jiri@nvidia.com>
>> 
>> Change the drivers that use devlink_port_register/unregister() to call
>> these functions only in case devlink is registered.
>> 
>> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>> ---
>> RFC->v1:
>> - shortened patch subject
>> ---
>>   .../net/ethernet/broadcom/bnxt/bnxt_devlink.c | 29 ++++++++++---------
>>   .../net/ethernet/freescale/dpaa2/dpaa2-eth.c  |  7 +++--
>>   .../ethernet/fungible/funeth/funeth_main.c    | 17 +++++++----
>>   drivers/net/ethernet/intel/ice/ice_main.c     | 21 ++++++++------
>>   .../ethernet/marvell/prestera/prestera_main.c |  6 ++--
>>   drivers/net/ethernet/mscc/ocelot_vsc7514.c    | 10 +++----
>>   .../ethernet/pensando/ionic/ionic_devlink.c   |  6 ++--
>>   drivers/net/ethernet/ti/am65-cpsw-nuss.c      |  7 +++--
>>   8 files changed, 60 insertions(+), 43 deletions(-)
>> 
>
>
>
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_devlink.c b/drivers/net/ethernet/pensando/ionic/ionic_devlink.c
>> index e6ff757895ab..06670343f90b 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic_devlink.c
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_devlink.c
>> @@ -78,16 +78,18 @@ int ionic_devlink_register(struct ionic *ionic)
>>          struct devlink_port_attrs attrs = {};
>>          int err;
>> 
>> +       devlink_register(dl);
>> +
>>          attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL;
>>          devlink_port_attrs_set(&ionic->dl_port, &attrs);
>>          err = devlink_port_register(dl, &ionic->dl_port, 0);
>>          if (err) {
>>                  dev_err(ionic->dev, "devlink_port_register failed: %d\n", err);
>> +               devlink_unregister(dl);
>>                  return err;
>>          }
>> 
>>          SET_NETDEV_DEVLINK_PORT(ionic->lif->netdev, &ionic->dl_port);
>> -       devlink_register(dl);
>>          return 0;
>>   }
>> 
>> @@ -95,6 +97,6 @@ void ionic_devlink_unregister(struct ionic *ionic)
>>   {
>>          struct devlink *dl = priv_to_devlink(ionic);
>> 
>> -       devlink_unregister(dl);
>>          devlink_port_unregister(&ionic->dl_port);
>> +       devlink_unregister(dl);
>>   }
>
>I don't know about the rest of the drivers, but this seems to be the exact
>opposite of what Leon did in this patch over a year ago:
>https://lore.kernel.org/netdev/cover.1632565508.git.leonro@nvidia.com/

This patch did move for all objects, even for those where no issue
existed. Ports are such.


>
>I haven't kept up on all the discussion about this, but is there no longer a
>worry about registering the devlink object before all the related
>configuration bits are in place?
>
>Does this open any potential issues with userland programs seeing the devlink
>device and trying to access port before they get registered?

What exactly do you have in mind? Could you please describe it?

>
>sln
>
>

Re: [patch net-next 1/8] devlink: call devlink_port_register/unregister() on registered instance

[Leon Romanovsky]

On Tue, Dec 06, 2022 at 08:41:36AM +0100, Jiri Pirko wrote:
> Tue, Dec 06, 2022 at 12:55:32AM CET, shnelson@amd.com wrote:
> >On 12/5/22 7:22 AM, Jiri Pirko wrote:
> >> 
> >> From: Jiri Pirko <jiri@nvidia.com>
> >> 
> >> Change the drivers that use devlink_port_register/unregister() to call
> >> these functions only in case devlink is registered.
> >> 
> >> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
> >> ---
> >> RFC->v1:
> >> - shortened patch subject
> >> ---
> >>   .../net/ethernet/broadcom/bnxt/bnxt_devlink.c | 29 ++++++++++---------
> >>   .../net/ethernet/freescale/dpaa2/dpaa2-eth.c  |  7 +++--
> >>   .../ethernet/fungible/funeth/funeth_main.c    | 17 +++++++----
> >>   drivers/net/ethernet/intel/ice/ice_main.c     | 21 ++++++++------
> >>   .../ethernet/marvell/prestera/prestera_main.c |  6 ++--
> >>   drivers/net/ethernet/mscc/ocelot_vsc7514.c    | 10 +++----
> >>   .../ethernet/pensando/ionic/ionic_devlink.c   |  6 ++--
> >>   drivers/net/ethernet/ti/am65-cpsw-nuss.c      |  7 +++--
> >>   8 files changed, 60 insertions(+), 43 deletions(-)
> >> 

<...>

> >I don't know about the rest of the drivers, but this seems to be the exact
> >opposite of what Leon did in this patch over a year ago:
> >https://lore.kernel.org/netdev/cover.1632565508.git.leonro@nvidia.com/
> 
> This patch did move for all objects, even for those where no issue
> existed. Ports are such.

Yeah, I was focused to move devlink_register() without any checks if
other devlink_* calls can be after it. The reason for that was desire
to make sure that we have devl_lock() on all netlink devlink calls.

Now, all commands are locked and we don't have commands which unlock
devlink internally and it is safe to move some devlink_* calls to be
after devlink_register() as long as they take devl_lock() and don't
mess with static devlink object data.

Thanks

Re: [patch net-next 1/8] devlink: call devlink_port_register/unregister() on registered instance

[Shannon Nelson]

On 12/5/22 11:41 PM, Jiri Pirko wrote:
> Tue, Dec 06, 2022 at 12:55:32AM CET, shnelson@amd.com wrote:
>> On 12/5/22 7:22 AM, Jiri Pirko wrote:
>>>
>>> From: Jiri Pirko <jiri@nvidia.com>
>>>
>>> Change the drivers that use devlink_port_register/unregister() to call
>>> these functions only in case devlink is registered.


>>
>> I haven't kept up on all the discussion about this, but is there no longer a
>> worry about registering the devlink object before all the related
>> configuration bits are in place?
>>
>> Does this open any potential issues with userland programs seeing the devlink
>> device and trying to access port before they get registered?
> 
> What exactly do you have in mind? Could you please describe it?

It looks like this could be setting up a race condition that some 
userland udev automation might hit if it notices the device, looks for 
the port, but doesn't see the port yet.  Leon's patch turned this around 
so that the ports would show up at the same time as the device.

sln

Re: [patch net-next 1/8] devlink: call devlink_port_register/unregister() on registered instance

[Jiri Pirko]

Tue, Dec 06, 2022 at 06:35:32PM CET, shnelson@amd.com wrote:
>On 12/5/22 11:41 PM, Jiri Pirko wrote:
>> Tue, Dec 06, 2022 at 12:55:32AM CET, shnelson@amd.com wrote:
>> > On 12/5/22 7:22 AM, Jiri Pirko wrote:
>> > > 
>> > > From: Jiri Pirko <jiri@nvidia.com>
>> > > 
>> > > Change the drivers that use devlink_port_register/unregister() to call
>> > > these functions only in case devlink is registered.
>
>
>> > 
>> > I haven't kept up on all the discussion about this, but is there no longer a
>> > worry about registering the devlink object before all the related
>> > configuration bits are in place?
>> > 
>> > Does this open any potential issues with userland programs seeing the devlink
>> > device and trying to access port before they get registered?
>> 
>> What exactly do you have in mind? Could you please describe it?
>
>It looks like this could be setting up a race condition that some userland
>udev automation might hit if it notices the device, looks for the port, but
>doesn't see the port yet.  Leon's patch turned this around so that the ports
>would show up at the same time as the device.

Any userland automation should not rely on that. Ports may come and go
with the current code as well, see port split/unsplit, linecard
provision unprovision.

Re: [patch net-next 1/8] devlink: call devlink_port_register/unregister() on registered instance

[Shannon Nelson]

On 12/7/22 5:27 AM, Jiri Pirko wrote:
> Tue, Dec 06, 2022 at 06:35:32PM CET, shnelson@amd.com wrote:
>> On 12/5/22 11:41 PM, Jiri Pirko wrote:
>>> Tue, Dec 06, 2022 at 12:55:32AM CET, shnelson@amd.com wrote:
>>>> On 12/5/22 7:22 AM, Jiri Pirko wrote:
>>>>>
>>>>> From: Jiri Pirko <jiri@nvidia.com>
>>>>>
>>>>> Change the drivers that use devlink_port_register/unregister() to call
>>>>> these functions only in case devlink is registered.
>>
>>
>>>>
>>>> I haven't kept up on all the discussion about this, but is there no longer a
>>>> worry about registering the devlink object before all the related
>>>> configuration bits are in place?
>>>>
>>>> Does this open any potential issues with userland programs seeing the devlink
>>>> device and trying to access port before they get registered?
>>>
>>> What exactly do you have in mind? Could you please describe it?
>>
>> It looks like this could be setting up a race condition that some userland
>> udev automation might hit if it notices the device, looks for the port, but
>> doesn't see the port yet.  Leon's patch turned this around so that the ports
>> would show up at the same time as the device.
> 
> Any userland automation should not rely on that. Ports may come and go
> with the current code as well, see port split/unsplit, linecard
> provision unprovision.

As long as this is a clear expectation, I'm fine with this.
sln

[patch net-next 2/8] netdevsim: call devl_port_register/unregister() on registered instance

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Move the code so devl_port_register/unregister() are called only
then devlink is registered.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
RFC->v1:
- shortened patch subject
---
 drivers/net/netdevsim/dev.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index b962fc8e1397..8ed235ac986f 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -949,6 +949,7 @@ static void nsim_dev_traps_exit(struct devlink *devlink)
 
 static int nsim_dev_reload_create(struct nsim_dev *nsim_dev,
 				  struct netlink_ext_ack *extack);
+static void nsim_dev_reload_ports_destroy(struct nsim_dev *nsim_dev);
 static void nsim_dev_reload_destroy(struct nsim_dev *nsim_dev);
 
 static int nsim_dev_reload_down(struct devlink *devlink, bool netns_change,
@@ -965,6 +966,7 @@ static int nsim_dev_reload_down(struct devlink *devlink, bool netns_change,
 		return -EOPNOTSUPP;
 	}
 
+	nsim_dev_reload_ports_destroy(nsim_dev);
 	nsim_dev_reload_destroy(nsim_dev);
 	return 0;
 }
@@ -1600,17 +1602,22 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
 	if (err)
 		goto err_psample_exit;
 
-	err = nsim_dev_port_add_all(nsim_dev, nsim_bus_dev->port_count);
-	if (err)
-		goto err_hwstats_exit;
-
 	nsim_dev->esw_mode = DEVLINK_ESWITCH_MODE_LEGACY;
 	devlink_set_features(devlink, DEVLINK_F_RELOAD);
 	devl_unlock(devlink);
 	devlink_register(devlink);
+
+	devl_lock(devlink);
+	err = nsim_dev_port_add_all(nsim_dev, nsim_bus_dev->port_count);
+	devl_unlock(devlink);
+	if (err)
+		goto err_devlink_unregister;
+
 	return 0;
 
-err_hwstats_exit:
+err_devlink_unregister:
+	devlink_unregister(devlink);
+	devl_lock(devlink);
 	nsim_dev_hwstats_exit(nsim_dev);
 err_psample_exit:
 	nsim_dev_psample_exit(nsim_dev);
@@ -1640,6 +1647,15 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
 	return err;
 }
 
+static void nsim_dev_reload_ports_destroy(struct nsim_dev *nsim_dev)
+{
+	struct devlink *devlink = priv_to_devlink(nsim_dev);
+
+	if (devlink_is_reload_failed(devlink))
+		return;
+	nsim_dev_port_del_all(nsim_dev);
+}
+
 static void nsim_dev_reload_destroy(struct nsim_dev *nsim_dev)
 {
 	struct devlink *devlink = priv_to_devlink(nsim_dev);
@@ -1654,7 +1670,6 @@ static void nsim_dev_reload_destroy(struct nsim_dev *nsim_dev)
 			nsim_esw_legacy_enable(nsim_dev, NULL);
 	}
 
-	nsim_dev_port_del_all(nsim_dev);
 	nsim_dev_hwstats_exit(nsim_dev);
 	nsim_dev_psample_exit(nsim_dev);
 	nsim_dev_health_exit(nsim_dev);
@@ -1668,6 +1683,10 @@ void nsim_drv_remove(struct nsim_bus_dev *nsim_bus_dev)
 	struct nsim_dev *nsim_dev = dev_get_drvdata(&nsim_bus_dev->dev);
 	struct devlink *devlink = priv_to_devlink(nsim_dev);
 
+	devl_lock(devlink);
+	nsim_dev_reload_ports_destroy(nsim_dev);
+	devl_unlock(devlink);
+
 	devlink_unregister(devlink);
 	devl_lock(devlink);
 	nsim_dev_reload_destroy(nsim_dev);
-- 
2.37.3

[patch net-next 3/8] mlxsw: call devl_port_register/unregister() on registered instance

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Move the code so devl_port_register/unregister() are called only
then devlink is registered.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
RFC->v1:
- shortened patch subject
---
 drivers/net/ethernet/mellanox/mlxsw/core.c    | 24 ++++++++++++
 drivers/net/ethernet/mellanox/mlxsw/core.h    |  2 +
 .../net/ethernet/mellanox/mlxsw/spectrum.c    | 38 ++++++++++++++-----
 3 files changed, 55 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c
index a0a06e2eff82..9908fb0f2d8a 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/core.c
@@ -2215,8 +2215,24 @@ __mlxsw_core_bus_device_register(const struct mlxsw_bus_info *mlxsw_bus_info,
 		devl_unlock(devlink);
 		devlink_register(devlink);
 	}
+
+	if (mlxsw_driver->ports_init) {
+		if (!reload)
+			devl_lock(devlink);
+		err = mlxsw_driver->ports_init(mlxsw_core);
+		if (!reload)
+			devl_unlock(devlink);
+		if (err)
+			goto err_driver_ports_init;
+	}
+
 	return 0;
 
+err_driver_ports_init:
+	if (!reload) {
+		devlink_unregister(devlink);
+		devl_lock(devlink);
+	}
 err_driver_init:
 	mlxsw_env_fini(mlxsw_core->env);
 err_env_init:
@@ -2284,6 +2300,14 @@ void mlxsw_core_bus_device_unregister(struct mlxsw_core *mlxsw_core,
 {
 	struct devlink *devlink = priv_to_devlink(mlxsw_core);
 
+	if (mlxsw_core->driver->ports_fini) {
+		if (!reload)
+			devl_lock(devlink);
+		mlxsw_core->driver->ports_fini(mlxsw_core);
+		if (!reload)
+			devl_unlock(devlink);
+	}
+
 	if (!reload) {
 		devlink_unregister(devlink);
 		devl_lock(devlink);
diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.h b/drivers/net/ethernet/mellanox/mlxsw/core.h
index e0a6fcbbcb19..a41ca92318e8 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/core.h
@@ -348,6 +348,8 @@ struct mlxsw_driver {
 		    const struct mlxsw_bus_info *mlxsw_bus_info,
 		    struct netlink_ext_ack *extack);
 	void (*fini)(struct mlxsw_core *mlxsw_core);
+	int (*ports_init)(struct mlxsw_core *mlxsw_core);
+	void (*ports_fini)(struct mlxsw_core *mlxsw_core);
 	int (*port_split)(struct mlxsw_core *mlxsw_core, u16 local_port,
 			  unsigned int count, struct netlink_ext_ack *extack);
 	int (*port_unsplit)(struct mlxsw_core *mlxsw_core, u16 local_port,
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index f5b2d965d476..b216c7dd8419 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -3251,16 +3251,8 @@ static int mlxsw_sp_init(struct mlxsw_core *mlxsw_core,
 		goto err_sample_trigger_init;
 	}
 
-	err = mlxsw_sp_ports_create(mlxsw_sp);
-	if (err) {
-		dev_err(mlxsw_sp->bus_info->dev, "Failed to create ports\n");
-		goto err_ports_create;
-	}
-
 	return 0;
 
-err_ports_create:
-	rhashtable_destroy(&mlxsw_sp->sample_trigger_ht);
 err_sample_trigger_init:
 	mlxsw_sp_port_module_info_fini(mlxsw_sp);
 err_port_module_info_init:
@@ -3445,11 +3437,24 @@ static int mlxsw_sp4_init(struct mlxsw_core *mlxsw_core,
 	return mlxsw_sp_init(mlxsw_core, mlxsw_bus_info, extack);
 }
 
+static int mlxsw_sp_ports_init(struct mlxsw_core *mlxsw_core)
+{
+	struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
+	int err;
+
+	err = mlxsw_sp_ports_create(mlxsw_sp);
+	if (err) {
+		dev_err(mlxsw_sp->bus_info->dev, "Failed to create ports\n");
+		return err;
+	}
+
+	return 0;
+}
+
 static void mlxsw_sp_fini(struct mlxsw_core *mlxsw_core)
 {
 	struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
 
-	mlxsw_sp_ports_remove(mlxsw_sp);
 	rhashtable_destroy(&mlxsw_sp->sample_trigger_ht);
 	mlxsw_sp_port_module_info_fini(mlxsw_sp);
 	mlxsw_sp_dpipe_fini(mlxsw_sp);
@@ -3478,6 +3483,13 @@ static void mlxsw_sp_fini(struct mlxsw_core *mlxsw_core)
 	mlxsw_sp_parsing_fini(mlxsw_sp);
 }
 
+static void mlxsw_sp_ports_fini(struct mlxsw_core *mlxsw_core)
+{
+	struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
+
+	mlxsw_sp_ports_remove(mlxsw_sp);
+}
+
 static const struct mlxsw_config_profile mlxsw_sp1_config_profile = {
 	.used_flood_mode                = 1,
 	.flood_mode                     = MLXSW_CMD_MBOX_CONFIG_PROFILE_FLOOD_MODE_CONTROLLED,
@@ -3934,6 +3946,8 @@ static struct mlxsw_driver mlxsw_sp1_driver = {
 	.fw_filename			= MLXSW_SP1_FW_FILENAME,
 	.init				= mlxsw_sp1_init,
 	.fini				= mlxsw_sp_fini,
+	.ports_init			= mlxsw_sp_ports_init,
+	.ports_fini			= mlxsw_sp_ports_fini,
 	.port_split			= mlxsw_sp_port_split,
 	.port_unsplit			= mlxsw_sp_port_unsplit,
 	.sb_pool_get			= mlxsw_sp_sb_pool_get,
@@ -3971,6 +3985,8 @@ static struct mlxsw_driver mlxsw_sp2_driver = {
 	.fw_filename			= MLXSW_SP2_FW_FILENAME,
 	.init				= mlxsw_sp2_init,
 	.fini				= mlxsw_sp_fini,
+	.ports_init			= mlxsw_sp_ports_init,
+	.ports_fini			= mlxsw_sp_ports_fini,
 	.port_split			= mlxsw_sp_port_split,
 	.port_unsplit			= mlxsw_sp_port_unsplit,
 	.ports_remove_selected		= mlxsw_sp_ports_remove_selected,
@@ -4010,6 +4026,8 @@ static struct mlxsw_driver mlxsw_sp3_driver = {
 	.fw_filename			= MLXSW_SP3_FW_FILENAME,
 	.init				= mlxsw_sp3_init,
 	.fini				= mlxsw_sp_fini,
+	.ports_init			= mlxsw_sp_ports_init,
+	.ports_fini			= mlxsw_sp_ports_fini,
 	.port_split			= mlxsw_sp_port_split,
 	.port_unsplit			= mlxsw_sp_port_unsplit,
 	.ports_remove_selected		= mlxsw_sp_ports_remove_selected,
@@ -4047,6 +4065,8 @@ static struct mlxsw_driver mlxsw_sp4_driver = {
 	.priv_size			= sizeof(struct mlxsw_sp),
 	.init				= mlxsw_sp4_init,
 	.fini				= mlxsw_sp_fini,
+	.ports_init			= mlxsw_sp_ports_init,
+	.ports_fini			= mlxsw_sp_ports_fini,
 	.port_split			= mlxsw_sp_port_split,
 	.port_unsplit			= mlxsw_sp_port_unsplit,
 	.ports_remove_selected		= mlxsw_sp_ports_remove_selected,
-- 
2.37.3

[patch net-next 4/8] nfp: call devl_port_register/unregister() on registered instance

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Move the code so devl_port_register/unregister() are called only
then devlink is registered.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
RFC->v1:
- shortened patch subject
---
 .../net/ethernet/netronome/nfp/nfp_net_main.c | 22 ++++++++++++++-----
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_main.c b/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
index abfe788d558f..9b4c48defd5c 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
@@ -752,7 +752,7 @@ int nfp_net_pci_probe(struct nfp_pf *pf)
 
 	err = nfp_shared_buf_register(pf);
 	if (err)
-		goto err_devlink_unreg;
+		goto err_clean_app;
 
 	err = nfp_devlink_params_register(pf);
 	if (err)
@@ -769,23 +769,29 @@ int nfp_net_pci_probe(struct nfp_pf *pf)
 	err = nfp_net_pf_alloc_irqs(pf);
 	if (err)
 		goto err_free_vnics;
+	devl_unlock(devlink);
 
+	devlink_register(devlink);
+
+	devl_lock(devlink);
 	err = nfp_net_pf_app_start(pf);
 	if (err)
-		goto err_free_irqs;
+		goto err_devlink_unreg;
 
 	err = nfp_net_pf_init_vnics(pf);
 	if (err)
 		goto err_stop_app;
 
 	devl_unlock(devlink);
-	devlink_register(devlink);
 
 	return 0;
 
 err_stop_app:
 	nfp_net_pf_app_stop(pf);
-err_free_irqs:
+	devl_unlock(devlink);
+err_devlink_unreg:
+	devlink_unregister(devlink);
+	devl_lock(devlink);
 	nfp_net_pf_free_irqs(pf);
 err_free_vnics:
 	nfp_net_pf_free_vnics(pf);
@@ -795,7 +801,7 @@ int nfp_net_pci_probe(struct nfp_pf *pf)
 	nfp_devlink_params_unregister(pf);
 err_shared_buf_unreg:
 	nfp_shared_buf_unregister(pf);
-err_devlink_unreg:
+err_clean_app:
 	cancel_work_sync(&pf->port_refresh_work);
 	nfp_net_pf_app_clean(pf);
 err_unmap:
@@ -808,7 +814,6 @@ void nfp_net_pci_remove(struct nfp_pf *pf)
 	struct devlink *devlink = priv_to_devlink(pf);
 	struct nfp_net *nn, *next;
 
-	devlink_unregister(priv_to_devlink(pf));
 	devl_lock(devlink);
 	list_for_each_entry_safe(nn, next, &pf->vnics, vnic_list) {
 		if (!nfp_net_is_data_vnic(nn))
@@ -818,6 +823,11 @@ void nfp_net_pci_remove(struct nfp_pf *pf)
 	}
 
 	nfp_net_pf_app_stop(pf);
+	devl_unlock(devlink);
+
+	devlink_unregister(priv_to_devlink(pf));
+
+	devl_lock(devlink);
 	/* stop app first, to avoid double free of ctrl vNIC's ddir */
 	nfp_net_debugfs_dir_clean(&pf->ddir);
 
-- 
2.37.3

[patch net-next 5/8] mlx4: call devl_port_register/unregister() on registered instance

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Move the code so devl_port_register/unregister() are called only
then devlink is registered.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
RFC->v1:
- shortened patch subject
---
 drivers/net/ethernet/mellanox/mlx4/main.c | 60 +++++++++++++----------
 1 file changed, 33 insertions(+), 27 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c
index 3ae246391549..14f1c76a50eb 100644
--- a/drivers/net/ethernet/mellanox/mlx4/main.c
+++ b/drivers/net/ethernet/mellanox/mlx4/main.c
@@ -3730,14 +3730,13 @@ static int mlx4_load_one(struct pci_dev *pdev, int pci_dev_data,
 }
 
 static int __mlx4_init_one(struct pci_dev *pdev, int pci_dev_data,
-			   struct mlx4_priv *priv)
+			   unsigned int *total_vfs,
+			   int *nvfs, struct mlx4_priv *priv)
 {
 	int err;
-	int nvfs[MLX4_MAX_PORTS + 1] = {0, 0, 0};
 	int prb_vf[MLX4_MAX_PORTS + 1] = {0, 0, 0};
 	const int param_map[MLX4_MAX_PORTS + 1][MLX4_MAX_PORTS + 1] = {
 		{2, 0, 0}, {0, 1, 2}, {0, 1, 2} };
-	unsigned total_vfs = 0;
 	unsigned int i;
 
 	pr_info(DRV_NAME ": Initializing %s\n", pci_name(pdev));
@@ -3752,8 +3751,8 @@ static int __mlx4_init_one(struct pci_dev *pdev, int pci_dev_data,
 	 * per port, we must limit the number of VFs to 63 (since their are
 	 * 128 MACs)
 	 */
-	for (i = 0; i < ARRAY_SIZE(nvfs) && i < num_vfs_argc;
-	     total_vfs += nvfs[param_map[num_vfs_argc - 1][i]], i++) {
+	for (i = 0; i <= MLX4_MAX_PORTS && i < num_vfs_argc;
+	     *total_vfs += nvfs[param_map[num_vfs_argc - 1][i]], i++) {
 		nvfs[param_map[num_vfs_argc - 1][i]] = num_vfs[i];
 		if (nvfs[i] < 0) {
 			dev_err(&pdev->dev, "num_vfs module parameter cannot be negative\n");
@@ -3770,10 +3769,10 @@ static int __mlx4_init_one(struct pci_dev *pdev, int pci_dev_data,
 			goto err_disable_pdev;
 		}
 	}
-	if (total_vfs > MLX4_MAX_NUM_VF) {
+	if (*total_vfs > MLX4_MAX_NUM_VF) {
 		dev_err(&pdev->dev,
 			"Requested more VF's (%d) than allowed by hw (%d)\n",
-			total_vfs, MLX4_MAX_NUM_VF);
+			*total_vfs, MLX4_MAX_NUM_VF);
 		err = -EINVAL;
 		goto err_disable_pdev;
 	}
@@ -3828,14 +3827,14 @@ static int __mlx4_init_one(struct pci_dev *pdev, int pci_dev_data,
 		/* When acting as pf, we normally skip vfs unless explicitly
 		 * requested to probe them.
 		 */
-		if (total_vfs) {
+		if (*total_vfs) {
 			unsigned vfs_offset = 0;
 
-			for (i = 0; i < ARRAY_SIZE(nvfs) &&
+			for (i = 0; i <= MLX4_MAX_PORTS &&
 			     vfs_offset + nvfs[i] < extended_func_num(pdev);
 			     vfs_offset += nvfs[i], i++)
 				;
-			if (i == ARRAY_SIZE(nvfs)) {
+			if (i == MLX4_MAX_PORTS + 1) {
 				err = -ENODEV;
 				goto err_release_regions;
 			}
@@ -3857,15 +3856,8 @@ static int __mlx4_init_one(struct pci_dev *pdev, int pci_dev_data,
 	if (err)
 		goto err_crdump;
 
-	err = mlx4_load_one(pdev, pci_dev_data, total_vfs, nvfs, priv, 0);
-	if (err)
-		goto err_catas;
-
 	return 0;
 
-err_catas:
-	mlx4_catas_end(&priv->dev);
-
 err_crdump:
 	mlx4_crdump_end(&priv->dev);
 
@@ -3994,6 +3986,8 @@ static const struct devlink_ops mlx4_devlink_ops = {
 
 static int mlx4_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
 {
+	int nvfs[MLX4_MAX_PORTS + 1] = {0, 0, 0};
+	unsigned int total_vfs = 0;
 	struct devlink *devlink;
 	struct mlx4_priv *priv;
 	struct mlx4_dev *dev;
@@ -4024,9 +4018,9 @@ static int mlx4_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
 	ret = devlink_params_register(devlink, mlx4_devlink_params,
 				      ARRAY_SIZE(mlx4_devlink_params));
 	if (ret)
-		goto err_devlink_unregister;
+		goto err_persist_free;
 	mlx4_devlink_set_params_init_values(devlink);
-	ret =  __mlx4_init_one(pdev, id->driver_data, priv);
+	ret =  __mlx4_init_one(pdev, id->driver_data, &total_vfs, nvfs, priv);
 	if (ret)
 		goto err_params_unregister;
 
@@ -4034,12 +4028,21 @@ static int mlx4_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
 	devlink_set_features(devlink, DEVLINK_F_RELOAD);
 	devl_unlock(devlink);
 	devlink_register(devlink);
+	devl_lock(devlink);
+	ret = mlx4_load_one(pdev, priv->pci_dev_data, total_vfs, nvfs, priv, 0);
+	devl_unlock(devlink);
+	if (ret)
+		goto err_devlink_unregister;
+
 	return 0;
 
+err_devlink_unregister:
+	devlink_unregister(devlink);
+	devl_lock(devlink);
 err_params_unregister:
 	devlink_params_unregister(devlink, mlx4_devlink_params,
 				  ARRAY_SIZE(mlx4_devlink_params));
-err_devlink_unregister:
+err_persist_free:
 	kfree(dev->persist);
 err_devlink_free:
 	devl_unlock(devlink);
@@ -4146,6 +4149,16 @@ static void mlx4_remove_one(struct pci_dev *pdev)
 	struct devlink *devlink = priv_to_devlink(priv);
 	int active_vfs = 0;
 
+	/* device marked to be under deletion running now without the lock
+	 * letting other tasks to be terminated
+	 */
+	devl_lock(devlink);
+	if (persist->interface_state & MLX4_INTERFACE_STATE_UP)
+		mlx4_unload_one(pdev);
+	else
+		mlx4_info(dev, "%s: interface is down\n", __func__);
+	devl_unlock(devlink);
+
 	devlink_unregister(devlink);
 
 	devl_lock(devlink);
@@ -4165,13 +4178,6 @@ static void mlx4_remove_one(struct pci_dev *pdev)
 		}
 	}
 
-	/* device marked to be under deletion running now without the lock
-	 * letting other tasks to be terminated
-	 */
-	if (persist->interface_state & MLX4_INTERFACE_STATE_UP)
-		mlx4_unload_one(pdev);
-	else
-		mlx4_info(dev, "%s: interface is down\n", __func__);
 	mlx4_catas_end(dev);
 	mlx4_crdump_end(dev);
 	if (dev->flags & MLX4_FLAG_SRIOV && !active_vfs) {
-- 
2.37.3

[patch net-next 6/8] mlx5: call devl_port_register/unregister() on registered instance

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Move the code so devl_port_register/unregister() are called only
then devlink is registered.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
RFC->v1:
- shortened patch subject
---
 drivers/net/ethernet/mellanox/mlx5/core/dev.c   | 10 ++++++++--
 drivers/net/ethernet/mellanox/mlx5/core/main.c  | 17 ++++++++++-------
 .../ethernet/mellanox/mlx5/core/sf/dev/driver.c |  9 +++++++++
 3 files changed, 27 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/dev.c b/drivers/net/ethernet/mellanox/mlx5/core/dev.c
index 0571e40c6ee5..dd3801198898 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/dev.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/dev.c
@@ -444,11 +444,14 @@ int mlx5_register_device(struct mlx5_core_dev *dev)
 {
 	int ret;
 
-	devl_assert_locked(priv_to_devlink(dev));
+	devl_lock(priv_to_devlink(dev));
+	mutex_lock(&dev->intf_state_mutex);
 	mutex_lock(&mlx5_intf_mutex);
 	dev->priv.flags &= ~MLX5_PRIV_FLAGS_DISABLE_ALL_ADEV;
 	ret = mlx5_rescan_drivers_locked(dev);
 	mutex_unlock(&mlx5_intf_mutex);
+	mutex_unlock(&dev->intf_state_mutex);
+	devl_unlock(priv_to_devlink(dev));
 	if (ret)
 		mlx5_unregister_device(dev);
 
@@ -457,11 +460,14 @@ int mlx5_register_device(struct mlx5_core_dev *dev)
 
 void mlx5_unregister_device(struct mlx5_core_dev *dev)
 {
-	devl_assert_locked(priv_to_devlink(dev));
+	devl_lock(priv_to_devlink(dev));
+	mutex_lock(&dev->intf_state_mutex);
 	mutex_lock(&mlx5_intf_mutex);
 	dev->priv.flags = MLX5_PRIV_FLAGS_DISABLE_ALL_ADEV;
 	mlx5_rescan_drivers_locked(dev);
 	mutex_unlock(&mlx5_intf_mutex);
+	mutex_unlock(&dev->intf_state_mutex);
+	devl_unlock(priv_to_devlink(dev));
 }
 
 static int add_drivers(struct mlx5_core_dev *dev)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
index 7f5db13e3550..f6f37289b49d 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
@@ -1392,16 +1392,10 @@ int mlx5_init_one(struct mlx5_core_dev *dev)
 	if (err)
 		goto err_devlink_reg;
 
-	err = mlx5_register_device(dev);
-	if (err)
-		goto err_register;
-
 	mutex_unlock(&dev->intf_state_mutex);
 	devl_unlock(devlink);
 	return 0;
 
-err_register:
-	mlx5_devlink_unregister(priv_to_devlink(dev));
 err_devlink_reg:
 	clear_bit(MLX5_INTERFACE_STATE_UP, &dev->intf_state);
 	mlx5_unload(dev);
@@ -1423,7 +1417,6 @@ void mlx5_uninit_one(struct mlx5_core_dev *dev)
 	devl_lock(devlink);
 	mutex_lock(&dev->intf_state_mutex);
 
-	mlx5_unregister_device(dev);
 	mlx5_devlink_unregister(priv_to_devlink(dev));
 
 	if (!test_bit(MLX5_INTERFACE_STATE_UP, &dev->intf_state)) {
@@ -1747,8 +1740,17 @@ static int probe_one(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	pci_save_state(pdev);
 	devlink_register(devlink);
+	err = mlx5_register_device(dev);
+	if (err) {
+		mlx5_core_err(dev, "mlx5_register_device failed with error code %d\n",
+			      err);
+		goto err_register_device;
+	}
+
 	return 0;
 
+err_register_device:
+	devlink_unregister(devlink);
 err_init_one:
 	mlx5_pci_close(dev);
 pci_init_err:
@@ -1771,6 +1773,7 @@ static void remove_one(struct pci_dev *pdev)
 	 */
 	mlx5_drain_fw_reset(dev);
 	set_bit(MLX5_BREAK_FW_WAIT, &dev->intf_state);
+	mlx5_unregister_device(dev);
 	devlink_unregister(devlink);
 	mlx5_sriov_disable(pdev);
 	mlx5_crdump_disable(dev);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/sf/dev/driver.c b/drivers/net/ethernet/mellanox/mlx5/core/sf/dev/driver.c
index 7b4783ce213e..90fcb30f7481 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/sf/dev/driver.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/sf/dev/driver.c
@@ -46,9 +46,17 @@ static int mlx5_sf_dev_probe(struct auxiliary_device *adev, const struct auxilia
 		mlx5_core_warn(mdev, "mlx5_init_one err=%d\n", err);
 		goto init_one_err;
 	}
+
 	devlink_register(devlink);
+
+	err = mlx5_register_device(mdev);
+	if (err)
+		goto register_device_err;
+
 	return 0;
 
+register_device_err:
+	devlink_unregister(devlink);
 init_one_err:
 	iounmap(mdev->iseg);
 remap_err:
@@ -63,6 +71,7 @@ static void mlx5_sf_dev_remove(struct auxiliary_device *adev)
 	struct mlx5_sf_dev *sf_dev = container_of(adev, struct mlx5_sf_dev, adev);
 	struct devlink *devlink = priv_to_devlink(sf_dev->mdev);
 
+	mlx5_unregister_device(sf_dev->mdev);
 	devlink_unregister(devlink);
 	mlx5_uninit_one(sf_dev->mdev);
 	iounmap(sf_dev->mdev->iseg);
-- 
2.37.3

[patch net-next 7/8] devlink: assert if devl_port_register/unregister() is called on unregistered devlink instance

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Now when all drivers do call devl_port_register/unregister() within the
time frame during which the devlink is registered, put and assertion to
the functions to check that and avoid going back.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
RFC->v1:
- rebased
---
 net/core/devlink.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 907df7124157..9b9775bc10b3 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -10084,6 +10084,7 @@ int devl_port_register(struct devlink *devlink,
 {
 	int err;
 
+	ASSERT_DEVLINK_REGISTERED(devlink);
 	devl_assert_locked(devlink);
 
 	ASSERT_DEVLINK_PORT_NOT_REGISTERED(devlink_port);
@@ -10142,6 +10143,7 @@ EXPORT_SYMBOL_GPL(devlink_port_register);
  */
 void devl_port_unregister(struct devlink_port *devlink_port)
 {
+	ASSERT_DEVLINK_REGISTERED(devlink_port->devlink);
 	lockdep_assert_held(&devlink_port->devlink->lock);
 	WARN_ON(devlink_port->type != DEVLINK_PORT_TYPE_NOTSET);
 
-- 
2.37.3

[patch net-next 8/8] devlink: remove port notifications from devlink_register/unregister()

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Now when all drivers do call devl_port_register/unregister() within the
time frame during which the devlink is registered, don't walk
through empty list for port notifications in
devlink_register/unregister().

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
RFC->v1:
- new patch
---
 net/core/devlink.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 9b9775bc10b3..1293069896c9 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -9859,9 +9859,6 @@ static void devlink_notify_register(struct devlink *devlink)
 	list_for_each_entry(linecard, &devlink->linecard_list, list)
 		devlink_linecard_notify(linecard, DEVLINK_CMD_LINECARD_NEW);
 
-	xa_for_each(&devlink->ports, port_index, devlink_port)
-		devlink_port_notify(devlink_port, DEVLINK_CMD_PORT_NEW);
-
 	list_for_each_entry(policer_item, &devlink->trap_policer_list, list)
 		devlink_trap_policer_notify(devlink, policer_item,
 					    DEVLINK_CMD_TRAP_POLICER_NEW);
@@ -9916,8 +9913,6 @@ static void devlink_notify_unregister(struct devlink *devlink)
 		devlink_trap_policer_notify(devlink, policer_item,
 					    DEVLINK_CMD_TRAP_POLICER_DEL);
 
-	xa_for_each(&devlink->ports, port_index, devlink_port)
-		devlink_port_notify(devlink_port, DEVLINK_CMD_PORT_DEL);
 	devlink_notify(devlink, DEVLINK_CMD_DEL);
 }
 
-- 
2.37.3

Re: [patch net-next 0/8] devlink: make sure devlink port registers/unregisters only for registered instance

[Jakub Kicinski]

On Mon,  5 Dec 2022 16:22:49 +0100 Jiri Pirko wrote:
> Currently, the devlink_register() is called as the last thing during
> driver init phase. For devlink objects, this is fine as the
> notifications of objects creations are withheld to be send only after
> devlink instance is registered. Until devlink is registered it is not
> visible to userspace.
> 
> But if a  netdev is registered before, user gets a notification with
> a link to devlink, which is not visible to the user yet.
> This is the event order user sees:
>  * new netdev event over RT netlink
>  * new devlink event over devlink netlink
>  * new devlink_port event over devlink netlink
> 
> Also, this is not consistent with devlink port split or devlink reload
> flows, where user gets notifications in following order:
>  * new devlink event over devlink netlink
> and then during port split or reload operation:
>  * new devlink_port event over devlink netlink
>  * new netdev event over RT netlink
> 
> In this case, devlink port and related netdev are registered on already
> registered devlink instance.
> 
> Purpose of this patchset is to fix the drivers init flow so devlink port
> gets registered only after devlink instance is registered.

I didn't reply because I don't have much to add beyond what 
I've already said too many times. I prefer to move to my
initial full refcounting / full locking design. I haven't posted 
any patches because I figured it's too low priority and too risky
to be doing right before the merge window.

I agree that reordering is a good idea but not as a fix, and hopefully
without conditional locking in the drivers:

+		if (!reload)
+			devl_lock(devlink);
+		err = mlxsw_driver->ports_init(mlxsw_core);
+		if (!reload)
+			devl_unlock(devlink);

Re: [patch net-next 0/8] devlink: make sure devlink port registers/unregisters only for registered instance

[Jiri Pirko]

Tue, Dec 06, 2022 at 02:08:26AM CET, kuba@kernel.org wrote:
>On Mon,  5 Dec 2022 16:22:49 +0100 Jiri Pirko wrote:
>> Currently, the devlink_register() is called as the last thing during
>> driver init phase. For devlink objects, this is fine as the
>> notifications of objects creations are withheld to be send only after
>> devlink instance is registered. Until devlink is registered it is not
>> visible to userspace.
>> 
>> But if a  netdev is registered before, user gets a notification with
>> a link to devlink, which is not visible to the user yet.
>> This is the event order user sees:
>>  * new netdev event over RT netlink
>>  * new devlink event over devlink netlink
>>  * new devlink_port event over devlink netlink
>> 
>> Also, this is not consistent with devlink port split or devlink reload
>> flows, where user gets notifications in following order:
>>  * new devlink event over devlink netlink
>> and then during port split or reload operation:
>>  * new devlink_port event over devlink netlink
>>  * new netdev event over RT netlink
>> 
>> In this case, devlink port and related netdev are registered on already
>> registered devlink instance.
>> 
>> Purpose of this patchset is to fix the drivers init flow so devlink port
>> gets registered only after devlink instance is registered.
>
>I didn't reply because I don't have much to add beyond what 
>I've already said too many times. I prefer to move to my
>initial full refcounting / full locking design. I haven't posted 
>any patches because I figured it's too low priority and too risky
>to be doing right before the merge window.

I'm missing how what you describe is relevant to this patchset and to
the issue it is trying to solve :/ 


>
>I agree that reordering is a good idea but not as a fix, and hopefully

I don't see other way to fix the netdev/devlink events ordering problem
I described above. Do you?



>without conditional locking in the drivers:
>
>+		if (!reload)
>+			devl_lock(devlink);
>+		err = mlxsw_driver->ports_init(mlxsw_core);
>+		if (!reload)
>+			devl_unlock(devlink);

Re: [patch net-next 0/8] devlink: make sure devlink port registers/unregisters only for registered instance

[Jakub Kicinski]

On Tue, 6 Dec 2022 08:44:16 +0100 Jiri Pirko wrote:
>> I didn't reply because I don't have much to add beyond what 
>> I've already said too many times. I prefer to move to my
>> initial full refcounting / full locking design. I haven't posted 
>> any patches because I figured it's too low priority and too risky
>> to be doing right before the merge window.  
> 
> I'm missing how what you describe is relevant to this patchset and to
> the issue it is trying to solve :/ 
> 
>> I agree that reordering is a good idea but not as a fix, and hopefully  
> 
> I don't see other way to fix the netdev/devlink events ordering problem
> I described above. Do you?

Just hold off with your patches until I post mine. Which as I said will
be during/after the merge window. I've been explaining this for a year now.

Re: [patch net-next 0/8] devlink: make sure devlink port registers/unregisters only for registered instance

[Jiri Pirko]

Tue, Dec 06, 2022 at 09:12:26PM CET, kuba@kernel.org wrote:
>On Tue, 6 Dec 2022 08:44:16 +0100 Jiri Pirko wrote:
>>> I didn't reply because I don't have much to add beyond what 
>>> I've already said too many times. I prefer to move to my
>>> initial full refcounting / full locking design. I haven't posted 
>>> any patches because I figured it's too low priority and too risky
>>> to be doing right before the merge window.  
>> 
>> I'm missing how what you describe is relevant to this patchset and to
>> the issue it is trying to solve :/ 
>> 
>>> I agree that reordering is a good idea but not as a fix, and hopefully  
>> 
>> I don't see other way to fix the netdev/devlink events ordering problem
>> I described above. Do you?
>
>Just hold off with your patches until I post mine. Which as I said will
>be during/after the merge window. I've been explaining this for a year now.

Sure, no problem.