[PATCH for-rc v1] IB/core, opa_vnic, hfi1, mlx5: Properly free rdma_netdev

[PATCH for-rc v1] IB/core, opa_vnic, hfi1, mlx5: Properly free rdma_netdev

[Doug Ledford]

From: Niranjana Vishwanathapura <niranjana.vishwanathapura-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

IPOIB is calling free_rdma_netdev even though alloc_rdma_netdev has
returned -EOPNOTSUPP.
Move free_rdma_netdev from ib_device structure to rdma_netdev structure
thus ensuring proper cleanup function is called for the rdma net device.

Fix the following trace:

ib0: Failed to modify QP to ERROR state
BUG: unable to handle kernel paging request at 0000000000001d20
IP: hfi1_vnic_free_rn+0x26/0xb0 [hfi1]
Call Trace:
 ipoib_remove_one+0xbe/0x160 [ib_ipoib]
 ib_unregister_device+0xd0/0x170 [ib_core]
 rvt_unregister_device+0x29/0x90 [rdmavt]
 hfi1_unregister_ib_device+0x1a/0x100 [hfi1]
 remove_one+0x4b/0x220 [hfi1]
 pci_device_remove+0x39/0xc0
 device_release_driver_internal+0x141/0x200
 driver_detach+0x3f/0x80
 bus_remove_driver+0x55/0xd0
 driver_unregister+0x2c/0x50
 pci_unregister_driver+0x2a/0xa0
 hfi1_mod_cleanup+0x10/0xf65 [hfi1]
 SyS_delete_module+0x171/0x250
 do_syscall_64+0x67/0x150
 entry_SYSCALL64_slow_path+0x25/0x25

Reviewed-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---

v1 - I fixed this up to resolve Leon's comment.  Leon, please make sure you
     are happy with the change to the mlx5 code.

 drivers/infiniband/hw/hfi1/verbs.c                |  1 -
 drivers/infiniband/hw/hfi1/vnic.h                 |  1 -
 drivers/infiniband/hw/hfi1/vnic_main.c            | 19 ++++++++--------
 drivers/infiniband/hw/mlx5/main.c                 | 27 ++++++++++++++---------
 drivers/infiniband/ulp/ipoib/ipoib_main.c         |  8 +++----
 drivers/infiniband/ulp/opa_vnic/opa_vnic_netdev.c |  8 +++----
 include/rdma/ib_verbs.h                           |  6 +++--
 7 files changed, 39 insertions(+), 31 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/verbs.c b/drivers/infiniband/hw/hfi1/verbs.c
index 90e7b77d68e8..2d19f9bb434d 100644
--- a/drivers/infiniband/hw/hfi1/verbs.c
+++ b/drivers/infiniband/hw/hfi1/verbs.c
@@ -1779,7 +1779,6 @@ int hfi1_register_ib_device(struct hfi1_devdata *dd)
 	ibdev->alloc_hw_stats = alloc_hw_stats;
 	ibdev->get_hw_stats = get_hw_stats;
 	ibdev->alloc_rdma_netdev = hfi1_vnic_alloc_rn;
-	ibdev->free_rdma_netdev = hfi1_vnic_free_rn;
 
 	/* keep process mad in the driver */
 	ibdev->process_mad = hfi1_process_mad;
diff --git a/drivers/infiniband/hw/hfi1/vnic.h b/drivers/infiniband/hw/hfi1/vnic.h
index e2c455299b53..4a621cde4abb 100644
--- a/drivers/infiniband/hw/hfi1/vnic.h
+++ b/drivers/infiniband/hw/hfi1/vnic.h
@@ -176,7 +176,6 @@ struct net_device *hfi1_vnic_alloc_rn(struct ib_device *device,
 				      const char *name,
 				      unsigned char name_assign_type,
 				      void (*setup)(struct net_device *));
-void hfi1_vnic_free_rn(struct net_device *netdev);
 int hfi1_vnic_send_dma(struct hfi1_devdata *dd, u8 q_idx,
 		       struct hfi1_vnic_vport_info *vinfo,
 		       struct sk_buff *skb, u64 pbc, u8 plen);
diff --git a/drivers/infiniband/hw/hfi1/vnic_main.c b/drivers/infiniband/hw/hfi1/vnic_main.c
index b601c2929f8f..339f0cdd56d6 100644
--- a/drivers/infiniband/hw/hfi1/vnic_main.c
+++ b/drivers/infiniband/hw/hfi1/vnic_main.c
@@ -833,6 +833,15 @@ static const struct net_device_ops hfi1_netdev_ops = {
 	.ndo_get_stats64 = hfi1_vnic_get_stats64,
 };
 
+static void hfi1_vnic_free_rn(struct net_device *netdev)
+{
+	struct hfi1_vnic_vport_info *vinfo = opa_vnic_dev_priv(netdev);
+
+	hfi1_vnic_deinit(vinfo);
+	mutex_destroy(&vinfo->lock);
+	free_netdev(netdev);
+}
+
 struct net_device *hfi1_vnic_alloc_rn(struct ib_device *device,
 				      u8 port_num,
 				      enum rdma_netdev_t type,
@@ -864,6 +873,7 @@ struct net_device *hfi1_vnic_alloc_rn(struct ib_device *device,
 	vinfo->num_tx_q = dd->chip_sdma_engines;
 	vinfo->num_rx_q = HFI1_NUM_VNIC_CTXT;
 	vinfo->netdev = netdev;
+	rn->free_rdma_netdev = hfi1_vnic_free_rn;
 	rn->set_id = hfi1_vnic_set_vesw_id;
 
 	netdev->features = NETIF_F_HIGHDMA | NETIF_F_SG;
@@ -892,12 +902,3 @@ struct net_device *hfi1_vnic_alloc_rn(struct ib_device *device,
 	free_netdev(netdev);
 	return ERR_PTR(rc);
 }
-
-void hfi1_vnic_free_rn(struct net_device *netdev)
-{
-	struct hfi1_vnic_vport_info *vinfo = opa_vnic_dev_priv(netdev);
-
-	hfi1_vnic_deinit(vinfo);
-	mutex_destroy(&vinfo->lock);
-	free_netdev(netdev);
-}
diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index 9ecc089d4529..afa5f6e88e1d 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -3542,6 +3542,11 @@ static int mlx5_ib_get_hw_stats(struct ib_device *ibdev,
 	return num_counters;
 }
 
+static void mlx5_ib_free_rdma_netdev(struct net_device *netdev)
+{
+	return mlx5_rdma_netdev_free(netdev);
+}
+
 static struct net_device*
 mlx5_ib_alloc_rdma_netdev(struct ib_device *hca,
 			  u8 port_num,
@@ -3550,16 +3555,19 @@ mlx5_ib_alloc_rdma_netdev(struct ib_device *hca,
 			  unsigned char name_assign_type,
 			  void (*setup)(struct net_device *))
 {
+	struct net_device *netdev;
+	struct rdma_netdev *rn;
+
 	if (type != RDMA_NETDEV_IPOIB)
 		return ERR_PTR(-EOPNOTSUPP);
 
-	return mlx5_rdma_netdev_alloc(to_mdev(hca)->mdev, hca,
-				      name, setup);
-}
-
-static void mlx5_ib_free_rdma_netdev(struct net_device *netdev)
-{
-	return mlx5_rdma_netdev_free(netdev);
+	netdev = mlx5_rdma_netdev_alloc(to_mdev(hca)->mdev, hca,
+					name, setup);
+	if (likely(!IS_ERR_OR_NULL(netdev))) {
+		rn = netdev_priv(netdev);
+		rn->free_rdma_netdev = mlx5_ib_free_rdma_netdev;
+	}
+	return netdev;
 }
 
 static void *mlx5_ib_add(struct mlx5_core_dev *mdev)
@@ -3692,10 +3700,9 @@ static void *mlx5_ib_add(struct mlx5_core_dev *mdev)
 	dev->ib_dev.check_mr_status	= mlx5_ib_check_mr_status;
 	dev->ib_dev.get_port_immutable  = mlx5_port_immutable;
 	dev->ib_dev.get_dev_fw_str      = get_dev_fw_str;
-	if (MLX5_CAP_GEN(mdev, ipoib_enhanced_offloads)) {
+	if (MLX5_CAP_GEN(mdev, ipoib_enhanced_offloads))
 		dev->ib_dev.alloc_rdma_netdev	= mlx5_ib_alloc_rdma_netdev;
-		dev->ib_dev.free_rdma_netdev	= mlx5_ib_free_rdma_netdev;
-	}
+
 	if (mlx5_core_is_pf(mdev)) {
 		dev->ib_dev.get_vf_config	= mlx5_ib_get_vf_config;
 		dev->ib_dev.set_vf_link_state	= mlx5_ib_set_vf_link_state;
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 1015a63de6ae..9ec0dbea3b6b 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -1893,6 +1893,7 @@ static struct net_device
 	rn->send = ipoib_send;
 	rn->attach_mcast = ipoib_mcast_attach;
 	rn->detach_mcast = ipoib_mcast_detach;
+	rn->free_rdma_netdev = free_netdev;
 	rn->hca = hca;
 
 	dev->netdev_ops = &ipoib_netdev_default_pf;
@@ -2288,6 +2289,8 @@ static void ipoib_remove_one(struct ib_device *device, void *client_data)
 		return;
 
 	list_for_each_entry_safe(priv, tmp, dev_list, list) {
+		struct rdma_netdev *rn = netdev_priv(priv->dev);
+
 		ib_unregister_event_handler(&priv->event_handler);
 		flush_workqueue(ipoib_workqueue);
 
@@ -2304,10 +2307,7 @@ static void ipoib_remove_one(struct ib_device *device, void *client_data)
 		flush_workqueue(priv->wq);
 
 		unregister_netdev(priv->dev);
-		if (device->free_rdma_netdev)
-			device->free_rdma_netdev(priv->dev);
-		else
-			free_netdev(priv->dev);
+		rn->free_rdma_netdev(priv->dev);
 
 		list_for_each_entry_safe(cpriv, tcpriv, &priv->child_intfs, list)
 			kfree(cpriv);
diff --git a/drivers/infiniband/ulp/opa_vnic/opa_vnic_netdev.c b/drivers/infiniband/ulp/opa_vnic/opa_vnic_netdev.c
index 78d9007bc2f6..1a89c6033358 100644
--- a/drivers/infiniband/ulp/opa_vnic/opa_vnic_netdev.c
+++ b/drivers/infiniband/ulp/opa_vnic/opa_vnic_netdev.c
@@ -323,13 +323,13 @@ struct opa_vnic_adapter *opa_vnic_add_netdev(struct ib_device *ibdev,
 	else if (IS_ERR(netdev))
 		return ERR_CAST(netdev);
 
+	rn = netdev_priv(netdev);
 	adapter = kzalloc(sizeof(*adapter), GFP_KERNEL);
 	if (!adapter) {
 		rc = -ENOMEM;
 		goto adapter_err;
 	}
 
-	rn = netdev_priv(netdev);
 	rn->clnt_priv = adapter;
 	rn->hca = ibdev;
 	rn->port_num = port_num;
@@ -366,7 +366,7 @@ struct opa_vnic_adapter *opa_vnic_add_netdev(struct ib_device *ibdev,
 	mutex_destroy(&adapter->mactbl_lock);
 	kfree(adapter);
 adapter_err:
-	ibdev->free_rdma_netdev(netdev);
+	rn->free_rdma_netdev(netdev);
 
 	return ERR_PTR(rc);
 }
@@ -375,7 +375,7 @@ struct opa_vnic_adapter *opa_vnic_add_netdev(struct ib_device *ibdev,
 void opa_vnic_rem_netdev(struct opa_vnic_adapter *adapter)
 {
 	struct net_device *netdev = adapter->netdev;
-	struct ib_device *ibdev = adapter->ibdev;
+	struct rdma_netdev *rn = netdev_priv(netdev);
 
 	v_info("removing\n");
 	unregister_netdev(netdev);
@@ -383,5 +383,5 @@ void opa_vnic_rem_netdev(struct opa_vnic_adapter *adapter)
 	mutex_destroy(&adapter->lock);
 	mutex_destroy(&adapter->mactbl_lock);
 	kfree(adapter);
-	ibdev->free_rdma_netdev(netdev);
+	rn->free_rdma_netdev(netdev);
 }
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index ba8314ec5768..71313d5ca1c8 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -1927,6 +1927,9 @@ struct rdma_netdev {
 	struct ib_device  *hca;
 	u8                 port_num;
 
+	/* cleanup function must be specified */
+	void (*free_rdma_netdev)(struct net_device *netdev);
+
 	/* control functions */
 	void (*set_id)(struct net_device *netdev, int id);
 	/* send packet */
@@ -2194,7 +2197,7 @@ struct ib_device {
 							   struct ib_udata *udata);
 	int                        (*destroy_rwq_ind_table)(struct ib_rwq_ind_table *wq_ind_table);
 	/**
-	 * rdma netdev operations
+	 * rdma netdev operation
 	 *
 	 * Driver implementing alloc_rdma_netdev must return -EOPNOTSUPP if it
 	 * doesn't support the specified rdma netdev type.
@@ -2206,7 +2209,6 @@ struct ib_device {
 					const char *name,
 					unsigned char name_assign_type,
 					void (*setup)(struct net_device *));
-	void (*free_rdma_netdev)(struct net_device *netdev);
 
 	struct module               *owner;
 	struct device                dev;
-- 
2.13.0

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-rc v1] IB/core, opa_vnic, hfi1, mlx5: Properly free rdma_netdev

[Vishwanathapura, Niranjana]

Thanks Doug for updating the patch with the fix.

>@@ -3550,16 +3555,19 @@ mlx5_ib_alloc_rdma_netdev(struct ib_device *hca,
> 			  unsigned char name_assign_type,
> 			  void (*setup)(struct net_device *))
> {
>+	struct net_device *netdev;
>+	struct rdma_netdev *rn;
>+
> 	if (type != RDMA_NETDEV_IPOIB)
> 		return ERR_PTR(-EOPNOTSUPP);
>
>-	return mlx5_rdma_netdev_alloc(to_mdev(hca)->mdev, hca,
>-				      name, setup);
>-}
>-
>-static void mlx5_ib_free_rdma_netdev(struct net_device *netdev)
>-{
>-	return mlx5_rdma_netdev_free(netdev);
>+	netdev = mlx5_rdma_netdev_alloc(to_mdev(hca)->mdev, hca,
>+					name, setup);
>+	if (likely(!IS_ERR_OR_NULL(netdev))) {
>+		rn = netdev_priv(netdev);
>+		rn->free_rdma_netdev = mlx5_ib_free_rdma_netdev;
>+	}

Looks like mlx5_rdma_netdev_alloc() always return NULL in case of error, hence 
null check should have been suffecient. But the above check will also do, I am 
fine with this change.

Niranjana
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-rc v1] IB/core, opa_vnic, hfi1, mlx5: Properly free rdma_netdev

[Doug Ledford]

On Wed, 2017-07-05 at 14:52 -0700, Vishwanathapura, Niranjana wrote:
> Thanks Doug for updating the patch with the fix.
> 
> > @@ -3550,16 +3555,19 @@ mlx5_ib_alloc_rdma_netdev(struct ib_device
> > *hca,
> > 			  unsigned char name_assign_type,
> > 			  void (*setup)(struct net_device *))
> > {
> > +	struct net_device *netdev;
> > +	struct rdma_netdev *rn;
> > +
> > 	if (type != RDMA_NETDEV_IPOIB)
> > 		return ERR_PTR(-EOPNOTSUPP);
> > 
> > -	return mlx5_rdma_netdev_alloc(to_mdev(hca)->mdev, hca,
> > -				      name, setup);
> > -}
> > -
> > -static void mlx5_ib_free_rdma_netdev(struct net_device *netdev)
> > -{
> > -	return mlx5_rdma_netdev_free(netdev);
> > +	netdev = mlx5_rdma_netdev_alloc(to_mdev(hca)->mdev, hca,
> > +					name, setup);
> > +	if (likely(!IS_ERR_OR_NULL(netdev))) {
> > +		rn = netdev_priv(netdev);
> > +		rn->free_rdma_netdev = mlx5_ib_free_rdma_netdev;
> > +	}
> 
> Looks like mlx5_rdma_netdev_alloc() always return NULL in case of
> error, hence 
> null check should have been suffecient. But the above check will also
> do, I am 
> fine with this change.

No, at the very beginning of mlx5_rdma_netdev_alloc() it will return
-EOPNOTSUPP if the card doesn't support IPoIB accelerations.  So we
have to support both NULL and an ERR_PTR (and possibly more options in
the future, you never know, the above is future proof).

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-rc v1] IB/core, opa_vnic, hfi1, mlx5: Properly free rdma_netdev

[Vishwanathapura, Niranjana]

On Wed, Jul 05, 2017 at 07:31:10PM -0400, Doug Ledford wrote:
>On Wed, 2017-07-05 at 14:52 -0700, Vishwanathapura, Niranjana wrote:
>> Thanks Doug for updating the patch with the fix.
>>
>> > @@ -3550,16 +3555,19 @@ mlx5_ib_alloc_rdma_netdev(struct ib_device
>> > *hca,
>> > 			  unsigned char name_assign_type,
>> > 			  void (*setup)(struct net_device *))
>> > {
>> > +	struct net_device *netdev;
>> > +	struct rdma_netdev *rn;
>> > +
>> > 	if (type != RDMA_NETDEV_IPOIB)
>> > 		return ERR_PTR(-EOPNOTSUPP);
>> >
>> > -	return mlx5_rdma_netdev_alloc(to_mdev(hca)->mdev, hca,
>> > -				      name, setup);
>> > -}
>> > -
>> > -static void mlx5_ib_free_rdma_netdev(struct net_device *netdev)
>> > -{
>> > -	return mlx5_rdma_netdev_free(netdev);
>> > +	netdev = mlx5_rdma_netdev_alloc(to_mdev(hca)->mdev, hca,
>> > +					name, setup);
>> > +	if (likely(!IS_ERR_OR_NULL(netdev))) {
>> > +		rn = netdev_priv(netdev);
>> > +		rn->free_rdma_netdev = mlx5_ib_free_rdma_netdev;
>> > +	}
>>
>> Looks like mlx5_rdma_netdev_alloc() always return NULL in case of
>> error, hence
>> null check should have been suffecient. But the above check will also
>> do, I am
>> fine with this change.
>
>No, at the very beginning of mlx5_rdma_netdev_alloc() it will return
>-EOPNOTSUPP if the card doesn't support IPoIB accelerations.  So we
>have to support both NULL and an ERR_PTR (and possibly more options in
>the future, you never know, the above is future proof).
>

I see. Yup, I agree.

Niranjana

>-- 
>Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>    GPG KeyID: B826A3330E572FDD
>    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-rc v1] IB/core, opa_vnic, hfi1, mlx5: Properly free rdma_netdev

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 6480 bytes --]

On Wed, Jul 05, 2017 at 05:17:52PM -0400, Doug Ledford wrote:
> From: Niranjana Vishwanathapura <niranjana.vishwanathapura-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>
> IPOIB is calling free_rdma_netdev even though alloc_rdma_netdev has
> returned -EOPNOTSUPP.
> Move free_rdma_netdev from ib_device structure to rdma_netdev structure
> thus ensuring proper cleanup function is called for the rdma net device.
>
> Fix the following trace:
>
> ib0: Failed to modify QP to ERROR state
> BUG: unable to handle kernel paging request at 0000000000001d20
> IP: hfi1_vnic_free_rn+0x26/0xb0 [hfi1]
> Call Trace:
>  ipoib_remove_one+0xbe/0x160 [ib_ipoib]
>  ib_unregister_device+0xd0/0x170 [ib_core]
>  rvt_unregister_device+0x29/0x90 [rdmavt]
>  hfi1_unregister_ib_device+0x1a/0x100 [hfi1]
>  remove_one+0x4b/0x220 [hfi1]
>  pci_device_remove+0x39/0xc0
>  device_release_driver_internal+0x141/0x200
>  driver_detach+0x3f/0x80
>  bus_remove_driver+0x55/0xd0
>  driver_unregister+0x2c/0x50
>  pci_unregister_driver+0x2a/0xa0
>  hfi1_mod_cleanup+0x10/0xf65 [hfi1]
>  SyS_delete_module+0x171/0x250
>  do_syscall_64+0x67/0x150
>  entry_SYSCALL64_slow_path+0x25/0x25
>
> Reviewed-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>
> v1 - I fixed this up to resolve Leon's comment.  Leon, please make sure you
>      are happy with the change to the mlx5 code.
>
>  drivers/infiniband/hw/hfi1/verbs.c                |  1 -
>  drivers/infiniband/hw/hfi1/vnic.h                 |  1 -
>  drivers/infiniband/hw/hfi1/vnic_main.c            | 19 ++++++++--------
>  drivers/infiniband/hw/mlx5/main.c                 | 27 ++++++++++++++---------
>  drivers/infiniband/ulp/ipoib/ipoib_main.c         |  8 +++----
>  drivers/infiniband/ulp/opa_vnic/opa_vnic_netdev.c |  8 +++----
>  include/rdma/ib_verbs.h                           |  6 +++--
>  7 files changed, 39 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/infiniband/hw/hfi1/verbs.c b/drivers/infiniband/hw/hfi1/verbs.c
> index 90e7b77d68e8..2d19f9bb434d 100644
> --- a/drivers/infiniband/hw/hfi1/verbs.c
> +++ b/drivers/infiniband/hw/hfi1/verbs.c
> @@ -1779,7 +1779,6 @@ int hfi1_register_ib_device(struct hfi1_devdata *dd)
>  	ibdev->alloc_hw_stats = alloc_hw_stats;
>  	ibdev->get_hw_stats = get_hw_stats;
>  	ibdev->alloc_rdma_netdev = hfi1_vnic_alloc_rn;
> -	ibdev->free_rdma_netdev = hfi1_vnic_free_rn;
>
>  	/* keep process mad in the driver */
>  	ibdev->process_mad = hfi1_process_mad;
> diff --git a/drivers/infiniband/hw/hfi1/vnic.h b/drivers/infiniband/hw/hfi1/vnic.h
> index e2c455299b53..4a621cde4abb 100644
> --- a/drivers/infiniband/hw/hfi1/vnic.h
> +++ b/drivers/infiniband/hw/hfi1/vnic.h
> @@ -176,7 +176,6 @@ struct net_device *hfi1_vnic_alloc_rn(struct ib_device *device,
>  				      const char *name,
>  				      unsigned char name_assign_type,
>  				      void (*setup)(struct net_device *));
> -void hfi1_vnic_free_rn(struct net_device *netdev);
>  int hfi1_vnic_send_dma(struct hfi1_devdata *dd, u8 q_idx,
>  		       struct hfi1_vnic_vport_info *vinfo,
>  		       struct sk_buff *skb, u64 pbc, u8 plen);
> diff --git a/drivers/infiniband/hw/hfi1/vnic_main.c b/drivers/infiniband/hw/hfi1/vnic_main.c
> index b601c2929f8f..339f0cdd56d6 100644
> --- a/drivers/infiniband/hw/hfi1/vnic_main.c
> +++ b/drivers/infiniband/hw/hfi1/vnic_main.c
> @@ -833,6 +833,15 @@ static const struct net_device_ops hfi1_netdev_ops = {
>  	.ndo_get_stats64 = hfi1_vnic_get_stats64,
>  };
>
> +static void hfi1_vnic_free_rn(struct net_device *netdev)
> +{
> +	struct hfi1_vnic_vport_info *vinfo = opa_vnic_dev_priv(netdev);
> +
> +	hfi1_vnic_deinit(vinfo);
> +	mutex_destroy(&vinfo->lock);
> +	free_netdev(netdev);
> +}
> +
>  struct net_device *hfi1_vnic_alloc_rn(struct ib_device *device,
>  				      u8 port_num,
>  				      enum rdma_netdev_t type,
> @@ -864,6 +873,7 @@ struct net_device *hfi1_vnic_alloc_rn(struct ib_device *device,
>  	vinfo->num_tx_q = dd->chip_sdma_engines;
>  	vinfo->num_rx_q = HFI1_NUM_VNIC_CTXT;
>  	vinfo->netdev = netdev;
> +	rn->free_rdma_netdev = hfi1_vnic_free_rn;
>  	rn->set_id = hfi1_vnic_set_vesw_id;
>
>  	netdev->features = NETIF_F_HIGHDMA | NETIF_F_SG;
> @@ -892,12 +902,3 @@ struct net_device *hfi1_vnic_alloc_rn(struct ib_device *device,
>  	free_netdev(netdev);
>  	return ERR_PTR(rc);
>  }
> -
> -void hfi1_vnic_free_rn(struct net_device *netdev)
> -{
> -	struct hfi1_vnic_vport_info *vinfo = opa_vnic_dev_priv(netdev);
> -
> -	hfi1_vnic_deinit(vinfo);
> -	mutex_destroy(&vinfo->lock);
> -	free_netdev(netdev);
> -}
> diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
> index 9ecc089d4529..afa5f6e88e1d 100644
> --- a/drivers/infiniband/hw/mlx5/main.c
> +++ b/drivers/infiniband/hw/mlx5/main.c
> @@ -3542,6 +3542,11 @@ static int mlx5_ib_get_hw_stats(struct ib_device *ibdev,
>  	return num_counters;
>  }
>
> +static void mlx5_ib_free_rdma_netdev(struct net_device *netdev)
> +{
> +	return mlx5_rdma_netdev_free(netdev);
> +}
> +
>  static struct net_device*
>  mlx5_ib_alloc_rdma_netdev(struct ib_device *hca,
>  			  u8 port_num,
> @@ -3550,16 +3555,19 @@ mlx5_ib_alloc_rdma_netdev(struct ib_device *hca,
>  			  unsigned char name_assign_type,
>  			  void (*setup)(struct net_device *))
>  {
> +	struct net_device *netdev;
> +	struct rdma_netdev *rn;
> +
>  	if (type != RDMA_NETDEV_IPOIB)
>  		return ERR_PTR(-EOPNOTSUPP);
>
> -	return mlx5_rdma_netdev_alloc(to_mdev(hca)->mdev, hca,
> -				      name, setup);
> -}
> -
> -static void mlx5_ib_free_rdma_netdev(struct net_device *netdev)
> -{
> -	return mlx5_rdma_netdev_free(netdev);
> +	netdev = mlx5_rdma_netdev_alloc(to_mdev(hca)->mdev, hca,
> +					name, setup);
> +	if (likely(!IS_ERR_OR_NULL(netdev))) {
> +		rn = netdev_priv(netdev);
> +		rn->free_rdma_netdev = mlx5_ib_free_rdma_netdev;
> +	}
> +	return netdev;
>  }


Thanks Doug, it looks good enough for the fix.

In general, the "likely" is not needed here (we are not in data path)
and our preference is to avoid "if(!error) { do something }" constructions
in favor of "if(error) { return ...}" (fail as early as you can).

Reviewed-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH for-rc v1] IB/core, opa_vnic, hfi1, mlx5: Properly free rdma_netdev

[Doug Ledford]

On 7/6/2017 12:23 AM, Leon Romanovsky wrote:
> On Wed, Jul 05, 2017 at 05:17:52PM -0400, Doug Ledford wrote:
> > From: Niranjana Vishwanathapura <niranjana.vishwanathapura-ral2JQCrhuE@public.gmane.org
> > m>
> > 
> > -static void mlx5_ib_free_rdma_netdev(struct net_device *netdev)
> > -{
> > -	return mlx5_rdma_netdev_free(netdev);
> > +	netdev = mlx5_rdma_netdev_alloc(to_mdev(hca)->mdev, hca,
> > +					name, setup);
> > +	if (likely(!IS_ERR_OR_NULL(netdev))) {
> > +		rn = netdev_priv(netdev);
> > +		rn->free_rdma_netdev = mlx5_ib_free_rdma_netdev;
> > +	}
> > +	return netdev;
> >  }
> 
> 
> Thanks Doug, it looks good enough for the fix.
> 
> In general, the "likely" is not needed here (we are not in data path)

It doesn't hurt though...

> and our preference is to avoid "if(!error) { do something }"
> constructions
> in favor of "if(error) { return ...}" (fail as early as you can).

Normally I would agree with you on that point.  But when you aren't
returning an error code but instead are returning the same thing you
return in the non error case, and when there are so few things to be
done in the non error case, I think this sort of construct becomes more
appealing (mainly because it will more closely match the assembler that
GCC puts out when compiling this code and I think that has value for
those times when you need to debug the object code, but that's just my
personal opinion).

> Reviewed-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-rc v1] IB/core, opa_vnic, hfi1, mlx5: Properly free rdma_netdev

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1867 bytes --]

On Thu, Jul 06, 2017 at 09:40:04AM -0400, Doug Ledford wrote:
> On 7/6/2017 12:23 AM, Leon Romanovsky wrote:
> > On Wed, Jul 05, 2017 at 05:17:52PM -0400, Doug Ledford wrote:
> > > From: Niranjana Vishwanathapura <niranjana.vishwanathapura-ral2JQCrhuE@public.gmane.org
> > > m>
> > >
> > > -static void mlx5_ib_free_rdma_netdev(struct net_device *netdev)
> > > -{
> > > -	return mlx5_rdma_netdev_free(netdev);
> > > +	netdev = mlx5_rdma_netdev_alloc(to_mdev(hca)->mdev, hca,
> > > +					name, setup);
> > > +	if (likely(!IS_ERR_OR_NULL(netdev))) {
> > > +		rn = netdev_priv(netdev);
> > > +		rn->free_rdma_netdev = mlx5_ib_free_rdma_netdev;
> > > +	}
> > > +	return netdev;
> > >  }
> >
> >
> > Thanks Doug, it looks good enough for the fix.
> >
> > In general, the "likely" is not needed here (we are not in data path)
>
> It doesn't hurt though...
>
> > and our preference is to avoid "if(!error) { do something }"
> > constructions
> > in favor of "if(error) { return ...}" (fail as early as you can).
>
> Normally I would agree with you on that point.  But when you aren't
> returning an error code but instead are returning the same thing you
> return in the non error case, and when there are so few things to be
> done in the non error case, I think this sort of construct becomes more
> appealing (mainly because it will more closely match the assembler that
> GCC puts out when compiling this code and I think that has value for
> those times when you need to debug the object code, but that's just my
> personal opinion).
>
> > Reviewed-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >

Any options are good for me, it fixes the crash :)

Thanks

>
>
> --
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>     GPG KeyID: B826A3330E572FDD
>     Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]