[PATCH V2 net] net/mlx4_core: Preserve pci_dev_data after __mlx4_remove_one()

[PATCH V2 net] net/mlx4_core: Preserve pci_dev_data after __mlx4_remove_one()

[Wei Yang]

pci_match_id() just match the static pci_device_id, which may return NULL if
someone binds the driver to a device manually using
/sys/bus/pci/drivers/.../new_id.

This patch wrap up a helper function __mlx4_remove_one() which does the tear
down function but preserve the drv_data. Functions like
mlx4_pci_err_detected() and mlx4_restart_one() will call this one with out
releasing drvdata.

Fixes: 97a5221 "net/mlx4_core: pass pci_device_id.driver_data to __mlx4_init_one during reset".

CC: Bjorn Helgaas <bhelgaas@google.com>
CC: Amir Vadai <amirv@mellanox.com>
CC: Jack Morgenstein <jackm@dev.mellanox.co.il>
CC: Or Gerlitz <ogerlitz@mellanox.com>
Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
Acked-by: Jack Morgenstein <jackm@dev.mellanox.co.il>

---
V0 --> V1:
   1. zero the mlx4_priv at the end of __mlx4_remove_one
   2. add a flag in mlx4_priv to mark it is released or not
V1 --> V2:
   1. rebase it on latest net tree
---
 drivers/net/ethernet/mellanox/mlx4/main.c |  177 ++++++++++++++++-------------
 drivers/net/ethernet/mellanox/mlx4/mlx4.h |    1 +
 2 files changed, 101 insertions(+), 77 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c
index f0ae95f..b6d60e2 100644
--- a/drivers/net/ethernet/mellanox/mlx4/main.c
+++ b/drivers/net/ethernet/mellanox/mlx4/main.c
@@ -2301,13 +2301,8 @@ static int __mlx4_init_one(struct pci_dev *pdev, int pci_dev_data)
 	/* Allow large DMA segments, up to the firmware limit of 1 GB */
 	dma_set_max_seg_size(&pdev->dev, 1024 * 1024 * 1024);
 
-	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
-	if (!priv) {
-		err = -ENOMEM;
-		goto err_release_regions;
-	}
-
-	dev       = &priv->dev;
+	dev       = pci_get_drvdata(pdev);
+	priv      = mlx4_priv(dev);
 	dev->pdev = pdev;
 	INIT_LIST_HEAD(&priv->ctx_list);
 	spin_lock_init(&priv->ctx_lock);
@@ -2535,8 +2530,7 @@ slave_start:
 	mlx4_sense_init(dev);
 	mlx4_start_sense(dev);
 
-	priv->pci_dev_data = pci_dev_data;
-	pci_set_drvdata(pdev, dev);
+	priv->removed = 0;
 
 	return 0;
 
@@ -2604,85 +2598,112 @@ err_disable_pdev:
 
 static int mlx4_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
 {
+	struct mlx4_priv *priv;
+	struct mlx4_dev *dev;
+
 	printk_once(KERN_INFO "%s", mlx4_version);
 
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv) {
+		return -ENOMEM;
+	}
+
+	dev       = &priv->dev;
+	pci_set_drvdata(pdev, dev);
+	priv->pci_dev_data = id->driver_data;
+
 	return __mlx4_init_one(pdev, id->driver_data);
 }
 
-static void mlx4_remove_one(struct pci_dev *pdev)
+static void __mlx4_remove_one(struct pci_dev *pdev)
 {
 	struct mlx4_dev  *dev  = pci_get_drvdata(pdev);
 	struct mlx4_priv *priv = mlx4_priv(dev);
+	int               pci_dev_data;
 	int p;
 
-	if (dev) {
-		/* in SRIOV it is not allowed to unload the pf's
-		 * driver while there are alive vf's */
-		if (mlx4_is_master(dev)) {
-			if (mlx4_how_many_lives_vf(dev))
-				printk(KERN_ERR "Removing PF when there are assigned VF's !!!\n");
-		}
-		mlx4_stop_sense(dev);
-		mlx4_unregister_device(dev);
+	if (priv->removed)
+		return;
 
-		for (p = 1; p <= dev->caps.num_ports; p++) {
-			mlx4_cleanup_port_info(&priv->port[p]);
-			mlx4_CLOSE_PORT(dev, p);
-		}
+	pci_dev_data = priv->pci_dev_data;
 
-		if (mlx4_is_master(dev))
-			mlx4_free_resource_tracker(dev,
-						   RES_TR_FREE_SLAVES_ONLY);
-
-		mlx4_cleanup_counters_table(dev);
-		mlx4_cleanup_qp_table(dev);
-		mlx4_cleanup_srq_table(dev);
-		mlx4_cleanup_cq_table(dev);
-		mlx4_cmd_use_polling(dev);
-		mlx4_cleanup_eq_table(dev);
-		mlx4_cleanup_mcg_table(dev);
-		mlx4_cleanup_mr_table(dev);
-		mlx4_cleanup_xrcd_table(dev);
-		mlx4_cleanup_pd_table(dev);
+	/* in SRIOV it is not allowed to unload the pf's
+	 * driver while there are alive vf's */
+	if (mlx4_is_master(dev)) {
+		if (mlx4_how_many_lives_vf(dev))
+			printk(KERN_ERR "Removing PF when there are assigned VF's !!!\n");
+	}
+	mlx4_stop_sense(dev);
+	mlx4_unregister_device(dev);
 
-		if (mlx4_is_master(dev))
-			mlx4_free_resource_tracker(dev,
-						   RES_TR_FREE_STRUCTS_ONLY);
-
-		iounmap(priv->kar);
-		mlx4_uar_free(dev, &priv->driver_uar);
-		mlx4_cleanup_uar_table(dev);
-		if (!mlx4_is_slave(dev))
-			mlx4_clear_steering(dev);
-		mlx4_free_eq_table(dev);
-		if (mlx4_is_master(dev))
-			mlx4_multi_func_cleanup(dev);
-		mlx4_close_hca(dev);
-		if (mlx4_is_slave(dev))
-			mlx4_multi_func_cleanup(dev);
-		mlx4_cmd_cleanup(dev);
-
-		if (dev->flags & MLX4_FLAG_MSI_X)
-			pci_disable_msix(pdev);
-		if (dev->flags & MLX4_FLAG_SRIOV) {
-			mlx4_warn(dev, "Disabling SR-IOV\n");
-			pci_disable_sriov(pdev);
-		}
+	for (p = 1; p <= dev->caps.num_ports; p++) {
+		mlx4_cleanup_port_info(&priv->port[p]);
+		mlx4_CLOSE_PORT(dev, p);
+	}
+
+	if (mlx4_is_master(dev))
+		mlx4_free_resource_tracker(dev,
+				   RES_TR_FREE_SLAVES_ONLY);
+
+	mlx4_cleanup_counters_table(dev);
+	mlx4_cleanup_qp_table(dev);
+	mlx4_cleanup_srq_table(dev);
+	mlx4_cleanup_cq_table(dev);
+	mlx4_cmd_use_polling(dev);
+	mlx4_cleanup_eq_table(dev);
+	mlx4_cleanup_mcg_table(dev);
+	mlx4_cleanup_mr_table(dev);
+	mlx4_cleanup_xrcd_table(dev);
+	mlx4_cleanup_pd_table(dev);
 
-		if (!mlx4_is_slave(dev))
-			mlx4_free_ownership(dev);
+	if (mlx4_is_master(dev))
+		mlx4_free_resource_tracker(dev,
+				   RES_TR_FREE_STRUCTS_ONLY);
 
-		kfree(dev->caps.qp0_tunnel);
-		kfree(dev->caps.qp0_proxy);
-		kfree(dev->caps.qp1_tunnel);
-		kfree(dev->caps.qp1_proxy);
-		kfree(dev->dev_vfs);
+	iounmap(priv->kar);
+	mlx4_uar_free(dev, &priv->driver_uar);
+	mlx4_cleanup_uar_table(dev);
+	if (!mlx4_is_slave(dev))
+		mlx4_clear_steering(dev);
+	mlx4_free_eq_table(dev);
+	if (mlx4_is_master(dev))
+		mlx4_multi_func_cleanup(dev);
+	mlx4_close_hca(dev);
+	if (mlx4_is_slave(dev))
+		mlx4_multi_func_cleanup(dev);
+	mlx4_cmd_cleanup(dev);
 
-		kfree(priv);
-		pci_release_regions(pdev);
-		pci_disable_device(pdev);
-		pci_set_drvdata(pdev, NULL);
+	if (dev->flags & MLX4_FLAG_MSI_X)
+		pci_disable_msix(pdev);
+	if (dev->flags & MLX4_FLAG_SRIOV) {
+		mlx4_warn(dev, "Disabling SR-IOV\n");
+		pci_disable_sriov(pdev);
 	}
+
+	if (!mlx4_is_slave(dev))
+		mlx4_free_ownership(dev);
+
+	kfree(dev->caps.qp0_tunnel);
+	kfree(dev->caps.qp0_proxy);
+	kfree(dev->caps.qp1_tunnel);
+	kfree(dev->caps.qp1_proxy);
+	kfree(dev->dev_vfs);
+
+	pci_release_regions(pdev);
+	pci_disable_device(pdev);
+	memset(priv, 0, sizeof(*priv));
+	priv->pci_dev_data = pci_dev_data;
+	priv->removed = 1;
+}
+
+static void mlx4_remove_one(struct pci_dev *pdev)
+{
+	struct mlx4_dev  *dev  = pci_get_drvdata(pdev);
+	struct mlx4_priv *priv = mlx4_priv(dev);
+
+	__mlx4_remove_one(pdev);
+	kfree(priv);
+	pci_set_drvdata(pdev, NULL);
 }
 
 int mlx4_restart_one(struct pci_dev *pdev)
@@ -2692,7 +2713,7 @@ int mlx4_restart_one(struct pci_dev *pdev)
 	int		  pci_dev_data;
 
 	pci_dev_data = priv->pci_dev_data;
-	mlx4_remove_one(pdev);
+	__mlx4_remove_one(pdev);
 	return __mlx4_init_one(pdev, pci_dev_data);
 }
 
@@ -2747,7 +2768,7 @@ MODULE_DEVICE_TABLE(pci, mlx4_pci_table);
 static pci_ers_result_t mlx4_pci_err_detected(struct pci_dev *pdev,
 					      pci_channel_state_t state)
 {
-	mlx4_remove_one(pdev);
+	__mlx4_remove_one(pdev);
 
 	return state == pci_channel_io_perm_failure ?
 		PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_NEED_RESET;
@@ -2755,11 +2776,13 @@ static pci_ers_result_t mlx4_pci_err_detected(struct pci_dev *pdev,
 
 static pci_ers_result_t mlx4_pci_slot_reset(struct pci_dev *pdev)
 {
-	const struct pci_device_id *id;
-	int ret;
+	struct mlx4_dev	 *dev  = pci_get_drvdata(pdev);
+	struct mlx4_priv *priv = mlx4_priv(dev);
+	int		  pci_dev_data;
+	int               ret;
 
-	id = pci_match_id(mlx4_pci_table, pdev);
-	ret = __mlx4_init_one(pdev, id->driver_data);
+	pci_dev_data = priv->pci_dev_data;
+	ret = __mlx4_init_one(pdev, pci_dev_data);
 
 	return ret ? PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_RECOVERED;
 }
diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4.h b/drivers/net/ethernet/mellanox/mlx4/mlx4.h
index cf8be41..f9c4651 100644
--- a/drivers/net/ethernet/mellanox/mlx4/mlx4.h
+++ b/drivers/net/ethernet/mellanox/mlx4/mlx4.h
@@ -800,6 +800,7 @@ struct mlx4_priv {
 	spinlock_t		ctx_lock;
 
 	int			pci_dev_data;
+	int                     removed;
 
 	struct list_head        pgdir_list;
 	struct mutex            pgdir_mutex;
-- 
1.7.9.5

Re: [PATCH V2 net] net/mlx4_core: Preserve pci_dev_data after __mlx4_remove_one()

[Sergei Shtylyov]

Hello.

On 13-04-2014 7:22, Wei Yang wrote:

> pci_match_id() just match the static pci_device_id, which may return NULL if
> someone binds the driver to a device manually using
> /sys/bus/pci/drivers/.../new_id.

> This patch wrap up a helper function __mlx4_remove_one() which does the tear
> down function but preserve the drv_data. Functions like
> mlx4_pci_err_detected() and mlx4_restart_one() will call this one with out
> releasing drvdata.

> Fixes: 97a5221 "net/mlx4_core: pass pci_device_id.driver_data to __mlx4_init_one during reset".

> CC: Bjorn Helgaas <bhelgaas@google.com>
> CC: Amir Vadai <amirv@mellanox.com>
> CC: Jack Morgenstein <jackm@dev.mellanox.co.il>
> CC: Or Gerlitz <ogerlitz@mellanox.com>
> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
> Acked-by: Jack Morgenstein <jackm@dev.mellanox.co.il>

[...]

> diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c
> index f0ae95f..b6d60e2 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/main.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/main.c
[...]
> @@ -2604,85 +2598,112 @@ err_disable_pdev:
>
>   static int mlx4_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
>   {
> +	struct mlx4_priv *priv;
> +	struct mlx4_dev *dev;
> +
>   	printk_once(KERN_INFO "%s", mlx4_version);
>
> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +	if (!priv) {
> +		return -ENOMEM;
> +	}

    {} not needed here.

[...]
> +	/* in SRIOV it is not allowed to unload the pf's
> +	 * driver while there are alive vf's */
> +	if (mlx4_is_master(dev)) {
> +		if (mlx4_how_many_lives_vf(dev))

    Could be folded into single *if*.

> +			printk(KERN_ERR "Removing PF when there are assigned VF's !!!\n");
> +	}
> +	mlx4_stop_sense(dev);
> +	mlx4_unregister_device(dev);
[...]
> +	for (p = 1; p <= dev->caps.num_ports; p++) {
> +		mlx4_cleanup_port_info(&priv->port[p]);
> +		mlx4_CLOSE_PORT(dev, p);
> +	}
> +
> +	if (mlx4_is_master(dev))
> +		mlx4_free_resource_tracker(dev,
> +				   RES_TR_FREE_SLAVES_ONLY);

    Please re-align this line so that it starts right under 'dev'.

[...]
>
> -		if (!mlx4_is_slave(dev))
> -			mlx4_free_ownership(dev);
> +	if (mlx4_is_master(dev))
> +		mlx4_free_resource_tracker(dev,
> +				   RES_TR_FREE_STRUCTS_ONLY);

    Likewise.

[...]
> @@ -2755,11 +2776,13 @@ static pci_ers_result_t mlx4_pci_err_detected(struct pci_dev *pdev,
>
>   static pci_ers_result_t mlx4_pci_slot_reset(struct pci_dev *pdev)
>   {
> -	const struct pci_device_id *id;
> -	int ret;
> +	struct mlx4_dev	 *dev  = pci_get_drvdata(pdev);
> +	struct mlx4_priv *priv = mlx4_priv(dev);
> +	int		  pci_dev_data;

    Doesn't seem that this variable is necessary.

> +	int               ret;
>
> -	id = pci_match_id(mlx4_pci_table, pdev);
> -	ret = __mlx4_init_one(pdev, id->driver_data);
> +	pci_dev_data = priv->pci_dev_data;
> +	ret = __mlx4_init_one(pdev, pci_dev_data);
>
>   	return ret ? PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_RECOVERED;
>   }
[...]

WBR, Sergei

Re: [PATCH V2 net] net/mlx4_core: Preserve pci_dev_data after __mlx4_remove_one()

[Wei Yang]

On Sun, Apr 13, 2014 at 03:06:10PM +0400, Sergei Shtylyov wrote:
>Hello.

Thanks for the comments.

Changed accordingly and will send another version.

>
>On 13-04-2014 7:22, Wei Yang wrote:
>
>>pci_match_id() just match the static pci_device_id, which may return NULL if
>>someone binds the driver to a device manually using
>>/sys/bus/pci/drivers/.../new_id.
>
>>This patch wrap up a helper function __mlx4_remove_one() which does the tear
>>down function but preserve the drv_data. Functions like
>>mlx4_pci_err_detected() and mlx4_restart_one() will call this one with out
>>releasing drvdata.
>
>>Fixes: 97a5221 "net/mlx4_core: pass pci_device_id.driver_data to __mlx4_init_one during reset".
>
>>CC: Bjorn Helgaas <bhelgaas@google.com>
>>CC: Amir Vadai <amirv@mellanox.com>
>>CC: Jack Morgenstein <jackm@dev.mellanox.co.il>
>>CC: Or Gerlitz <ogerlitz@mellanox.com>
>>Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
>>Acked-by: Jack Morgenstein <jackm@dev.mellanox.co.il>
>
>[...]
>
>>diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c
>>index f0ae95f..b6d60e2 100644
>>--- a/drivers/net/ethernet/mellanox/mlx4/main.c
>>+++ b/drivers/net/ethernet/mellanox/mlx4/main.c
>[...]
>>@@ -2604,85 +2598,112 @@ err_disable_pdev:
>>
>>  static int mlx4_init_one(struct pci_dev *pdev, const struct pci_device_id *id)
>>  {
>>+	struct mlx4_priv *priv;
>>+	struct mlx4_dev *dev;
>>+
>>  	printk_once(KERN_INFO "%s", mlx4_version);
>>
>>+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>>+	if (!priv) {
>>+		return -ENOMEM;
>>+	}
>
>   {} not needed here.
>
>[...]
>>+	/* in SRIOV it is not allowed to unload the pf's
>>+	 * driver while there are alive vf's */
>>+	if (mlx4_is_master(dev)) {
>>+		if (mlx4_how_many_lives_vf(dev))
>
>   Could be folded into single *if*.
>
>>+			printk(KERN_ERR "Removing PF when there are assigned VF's !!!\n");
>>+	}
>>+	mlx4_stop_sense(dev);
>>+	mlx4_unregister_device(dev);
>[...]
>>+	for (p = 1; p <= dev->caps.num_ports; p++) {
>>+		mlx4_cleanup_port_info(&priv->port[p]);
>>+		mlx4_CLOSE_PORT(dev, p);
>>+	}
>>+
>>+	if (mlx4_is_master(dev))
>>+		mlx4_free_resource_tracker(dev,
>>+				   RES_TR_FREE_SLAVES_ONLY);
>
>   Please re-align this line so that it starts right under 'dev'.
>
>[...]
>>
>>-		if (!mlx4_is_slave(dev))
>>-			mlx4_free_ownership(dev);
>>+	if (mlx4_is_master(dev))
>>+		mlx4_free_resource_tracker(dev,
>>+				   RES_TR_FREE_STRUCTS_ONLY);
>
>   Likewise.
>
>[...]
>>@@ -2755,11 +2776,13 @@ static pci_ers_result_t mlx4_pci_err_detected(struct pci_dev *pdev,
>>
>>  static pci_ers_result_t mlx4_pci_slot_reset(struct pci_dev *pdev)
>>  {
>>-	const struct pci_device_id *id;
>>-	int ret;
>>+	struct mlx4_dev	 *dev  = pci_get_drvdata(pdev);
>>+	struct mlx4_priv *priv = mlx4_priv(dev);
>>+	int		  pci_dev_data;
>
>   Doesn't seem that this variable is necessary.
>
>>+	int               ret;
>>
>>-	id = pci_match_id(mlx4_pci_table, pdev);
>>-	ret = __mlx4_init_one(pdev, id->driver_data);
>>+	pci_dev_data = priv->pci_dev_data;
>>+	ret = __mlx4_init_one(pdev, pci_dev_data);
>>
>>  	return ret ? PCI_ERS_RESULT_DISCONNECT : PCI_ERS_RESULT_RECOVERED;
>>  }
>[...]
>
>WBR, Sergei

-- 
Richard Yang
Help you, Help me