[PATCH net 1/1] ice: move devlink port creation/deletion

[PATCH net 1/1] ice: move devlink port creation/deletion

[Tony Nguyen]

From: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>

Commit a286ba738714 ("ice: reorder PF/representor devlink
port register/unregister flows") moved the code to create
and destroy the devlink PF port. This was fine, but created
a corner case issue in the case of ice_register_netdev()
failing. In that case, the driver would end up calling
ice_devlink_destroy_pf_port() twice.

Additionally, it makes no sense to tie creation of the devlink
PF port to the creation of the netdev so separate out the
code to create/destroy the devlink PF port from the netdev
code. This makes it a cleaner interface.

Fixes: a286ba738714 ("ice: reorder PF/representor devlink port register/unregister flows")
Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
Tested-by: Gurucharan G <gurucharanx.g@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
There will be a merge conflict when rebasing with next-next.

Resolution:
static void ice_remove(struct pci_dev *
  		ice_remove_arfs(pf);
  	ice_setup_mc_magic_wake(pf);
  	ice_vsi_release_all(pf);
 -	mutex_destroy(&(&pf->hw)->fdir_fltr_lock);
 +	mutex_destroy(&hw->fdir_fltr_lock);
+ 	ice_devlink_destroy_pf_port(pf);
  	ice_set_wake(pf);
  	ice_free_irq_msix_misc(pf);
  	ice_for_each_vsi(pf, i) {

 drivers/net/ethernet/intel/ice/ice_lib.c  |  3 ---
 drivers/net/ethernet/intel/ice/ice_main.c | 25 +++++++++++++++--------
 2 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index 94aa834cd9a6..a596e07b3ce9 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -3235,9 +3235,6 @@ int ice_vsi_release(struct ice_vsi *vsi)
 		}
 	}
 
-	if (vsi->type == ICE_VSI_PF)
-		ice_devlink_destroy_pf_port(pf);
-
 	if (vsi->type == ICE_VSI_VF &&
 	    vsi->agg_node && vsi->agg_node->valid)
 		vsi->agg_node->num_vsis--;
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index a9a7f8b52140..237ede2cffb0 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -4590,7 +4590,7 @@ static void ice_print_wake_reason(struct ice_pf *pf)
 }
 
 /**
- * ice_register_netdev - register netdev and devlink port
+ * ice_register_netdev - register netdev
  * @pf: pointer to the PF struct
  */
 static int ice_register_netdev(struct ice_pf *pf)
@@ -4602,11 +4602,6 @@ static int ice_register_netdev(struct ice_pf *pf)
 	if (!vsi || !vsi->netdev)
 		return -EIO;
 
-	err = ice_devlink_create_pf_port(pf);
-	if (err)
-		goto err_devlink_create;
-
-	SET_NETDEV_DEVLINK_PORT(vsi->netdev, &pf->devlink_port);
 	err = register_netdev(vsi->netdev);
 	if (err)
 		goto err_register_netdev;
@@ -4617,8 +4612,6 @@ static int ice_register_netdev(struct ice_pf *pf)
 
 	return 0;
 err_register_netdev:
-	ice_devlink_destroy_pf_port(pf);
-err_devlink_create:
 	free_netdev(vsi->netdev);
 	vsi->netdev = NULL;
 	clear_bit(ICE_VSI_NETDEV_ALLOCD, vsi->state);
@@ -4636,6 +4629,7 @@ static int
 ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
 {
 	struct device *dev = &pdev->dev;
+	struct ice_vsi *vsi;
 	struct ice_pf *pf;
 	struct ice_hw *hw;
 	int i, err;
@@ -4918,6 +4912,18 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
 	pcie_print_link_status(pf->pdev);
 
 probe_done:
+	err = ice_devlink_create_pf_port(pf);
+	if (err)
+		goto err_create_pf_port;
+
+	vsi = ice_get_main_vsi(pf);
+	if (!vsi || !vsi->netdev) {
+		err = -EINVAL;
+		goto err_netdev_reg;
+	}
+
+	SET_NETDEV_DEVLINK_PORT(vsi->netdev, &pf->devlink_port);
+
 	err = ice_register_netdev(pf);
 	if (err)
 		goto err_netdev_reg;
@@ -4955,6 +4961,8 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
 err_devlink_reg_param:
 	ice_devlink_unregister_params(pf);
 err_netdev_reg:
+	ice_devlink_destroy_pf_port(pf);
+err_create_pf_port:
 err_send_version_unroll:
 	ice_vsi_release_all(pf);
 err_alloc_sw_unroll:
@@ -5083,6 +5091,7 @@ static void ice_remove(struct pci_dev *pdev)
 	ice_setup_mc_magic_wake(pf);
 	ice_vsi_release_all(pf);
 	mutex_destroy(&(&pf->hw)->fdir_fltr_lock);
+	ice_devlink_destroy_pf_port(pf);
 	ice_set_wake(pf);
 	ice_free_irq_msix_misc(pf);
 	ice_for_each_vsi(pf, i) {
-- 
2.38.1

Re: [PATCH net 1/1] ice: move devlink port creation/deletion

[Leon Romanovsky]

On Mon, Jan 23, 2023 at 04:57:14PM -0800, Tony Nguyen wrote:
> From: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
> 
> Commit a286ba738714 ("ice: reorder PF/representor devlink
> port register/unregister flows") moved the code to create
> and destroy the devlink PF port. This was fine, but created
> a corner case issue in the case of ice_register_netdev()
> failing. In that case, the driver would end up calling
> ice_devlink_destroy_pf_port() twice.
> 
> Additionally, it makes no sense to tie creation of the devlink
> PF port to the creation of the netdev so separate out the
> code to create/destroy the devlink PF port from the netdev
> code. This makes it a cleaner interface.
> 
> Fixes: a286ba738714 ("ice: reorder PF/representor devlink port register/unregister flows")
> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
> Tested-by: Gurucharan G <gurucharanx.g@intel.com> (A Contingent worker at Intel)
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
> There will be a merge conflict when rebasing with next-next.
> 
> Resolution:
> static void ice_remove(struct pci_dev *
>   		ice_remove_arfs(pf);
>   	ice_setup_mc_magic_wake(pf);
>   	ice_vsi_release_all(pf);
>  -	mutex_destroy(&(&pf->hw)->fdir_fltr_lock);
>  +	mutex_destroy(&hw->fdir_fltr_lock);
> + 	ice_devlink_destroy_pf_port(pf);
>   	ice_set_wake(pf);
>   	ice_free_irq_msix_misc(pf);
>   	ice_for_each_vsi(pf, i) {
> 
>  drivers/net/ethernet/intel/ice/ice_lib.c  |  3 ---
>  drivers/net/ethernet/intel/ice/ice_main.c | 25 +++++++++++++++--------
>  2 files changed, 17 insertions(+), 11 deletions(-)

If you seek cleaner interface, you will be better separate
ice_vsi_release_all() to many small building blocks, so unwind will be
symmetrical to probe.

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

Re: [PATCH net 1/1] ice: move devlink port creation/deletion

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 23 Jan 2023 16:57:14 -0800 you wrote:
> From: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
> 
> Commit a286ba738714 ("ice: reorder PF/representor devlink
> port register/unregister flows") moved the code to create
> and destroy the devlink PF port. This was fine, but created
> a corner case issue in the case of ice_register_netdev()
> failing. In that case, the driver would end up calling
> ice_devlink_destroy_pf_port() twice.
> 
> [...]

Here is the summary with links:
  - [net,1/1] ice: move devlink port creation/deletion
    https://git.kernel.org/netdev/net/c/418e53401e47

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