[PATCH 0/3] IB/hfi1, rdmavt: Additional bug fixes for 4.6 RC

[PATCH 0/3] IB/hfi1, rdmavt: Additional bug fixes for 4.6 RC

[Dennis Dalessandro]

Here are three more fixes that should probably end up going in the next RC for
4.6. These apply on top of the previously submitted  RC patch set:

http://marc.info/?l=linux-rdma&m=146048314707180&w=4

These fixes address a memory leak, locking problem, and a panic.

They can also be seen in my GitHub repo.
https://github.com/ddalessa/kernel/tree/for-4.6

---

Jubin John (1):
      IB/rdmavt,hfi1,qib: Fix memory leak

Mike Marciniszyn (1):
      IB/hfi1: Fix missing lock/unlock in verbs drain callback

Mitko Haralanov (1):
      IB/hfi1: Don't attempt to free resources if initialization failed


 drivers/infiniband/hw/qib/qib_init.c     |    4 +-
 drivers/infiniband/sw/rdmavt/vt.c        |   13 +++++++
 drivers/staging/rdma/hfi1/file_ops.c     |   60 ++++++++++++++----------------
 drivers/staging/rdma/hfi1/init.c         |    4 +-
 drivers/staging/rdma/hfi1/qp.c           |    2 +
 drivers/staging/rdma/hfi1/user_exp_rcv.c |    2 +
 include/rdma/rdma_vt.h                   |    1 +
 7 files changed, 49 insertions(+), 37 deletions(-)

-- 
-Denny
--
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

[PATCH 1/3] IB/rdmavt,hfi1,qib: Fix memory leak

[Dennis Dalessandro]

From: Jubin John <jubin.john-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

rdi->ports has memory allocated in rvt_alloc_device(), but does not get
freed because the hfi1 and qib drivers drivers call ib_dealloc_device()
directly instead of going through rdmavt. Add a rvt_dealloc_device()
that frees rdi->ports and then calls ib_dealloc_device(). Switch hfi1
and qib drivers to calling rvt_dealloc_device() instead of
ib_dealloc_device() directly.

Reviewed-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Reviewed-by: Brian Welty <brian.welty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Jubin John <jubin.john-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/infiniband/hw/qib/qib_init.c |    4 ++--
 drivers/infiniband/sw/rdmavt/vt.c    |   13 +++++++++++++
 drivers/staging/rdma/hfi1/init.c     |    4 ++--
 include/rdma/rdma_vt.h               |    1 +
 4 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/hw/qib/qib_init.c b/drivers/infiniband/hw/qib/qib_init.c
index 3f062f0..f253111 100644
--- a/drivers/infiniband/hw/qib/qib_init.c
+++ b/drivers/infiniband/hw/qib/qib_init.c
@@ -1090,7 +1090,7 @@ void qib_free_devdata(struct qib_devdata *dd)
 	qib_dbg_ibdev_exit(&dd->verbs_dev);
 #endif
 	free_percpu(dd->int_counter);
-	ib_dealloc_device(&dd->verbs_dev.rdi.ibdev);
+	rvt_dealloc_device(&dd->verbs_dev.rdi);
 }
 
 u64 qib_int_counter(struct qib_devdata *dd)
@@ -1183,7 +1183,7 @@ struct qib_devdata *qib_alloc_devdata(struct pci_dev *pdev, size_t extra)
 bail:
 	if (!list_empty(&dd->list))
 		list_del_init(&dd->list);
-	ib_dealloc_device(&dd->verbs_dev.rdi.ibdev);
+	rvt_dealloc_device(&dd->verbs_dev.rdi);
 	return ERR_PTR(ret);
 }
 
diff --git a/drivers/infiniband/sw/rdmavt/vt.c b/drivers/infiniband/sw/rdmavt/vt.c
index 6caf527..e1cc2cc 100644
--- a/drivers/infiniband/sw/rdmavt/vt.c
+++ b/drivers/infiniband/sw/rdmavt/vt.c
@@ -106,6 +106,19 @@ struct rvt_dev_info *rvt_alloc_device(size_t size, int nports)
 }
 EXPORT_SYMBOL(rvt_alloc_device);
 
+/**
+ * rvt_dealloc_device - deallocate rdi
+ * @rdi: structure to free
+ *
+ * Free a structure allocated with rvt_alloc_device()
+ */
+void rvt_dealloc_device(struct rvt_dev_info *rdi)
+{
+	kfree(rdi->ports);
+	ib_dealloc_device(&rdi->ibdev);
+}
+EXPORT_SYMBOL(rvt_dealloc_device);
+
 static int rvt_query_device(struct ib_device *ibdev,
 			    struct ib_device_attr *props,
 			    struct ib_udata *uhw)
diff --git a/drivers/staging/rdma/hfi1/init.c b/drivers/staging/rdma/hfi1/init.c
index cfcdc16..00edd50 100644
--- a/drivers/staging/rdma/hfi1/init.c
+++ b/drivers/staging/rdma/hfi1/init.c
@@ -1007,7 +1007,7 @@ void hfi1_free_devdata(struct hfi1_devdata *dd)
 	free_percpu(dd->rcv_limit);
 	hfi1_dev_affinity_free(dd);
 	free_percpu(dd->send_schedule);
-	ib_dealloc_device(&dd->verbs_dev.rdi.ibdev);
+	rvt_dealloc_device(&dd->verbs_dev.rdi);
 }
 
 /*
@@ -1110,7 +1110,7 @@ struct hfi1_devdata *hfi1_alloc_devdata(struct pci_dev *pdev, size_t extra)
 bail:
 	if (!list_empty(&dd->list))
 		list_del_init(&dd->list);
-	ib_dealloc_device(&dd->verbs_dev.rdi.ibdev);
+	rvt_dealloc_device(&dd->verbs_dev.rdi);
 	return ERR_PTR(ret);
 }
 
diff --git a/include/rdma/rdma_vt.h b/include/rdma/rdma_vt.h
index a869655..d57ceee 100644
--- a/include/rdma/rdma_vt.h
+++ b/include/rdma/rdma_vt.h
@@ -467,6 +467,7 @@ static inline struct rvt_qp *rvt_lookup_qpn(struct rvt_dev_info *rdi,
 }
 
 struct rvt_dev_info *rvt_alloc_device(size_t size, int nports);
+void rvt_dealloc_device(struct rvt_dev_info *rdi);
 int rvt_register_device(struct rvt_dev_info *rvd);
 void rvt_unregister_device(struct rvt_dev_info *rvd);
 int rvt_check_ah(struct ib_device *ibdev, struct ib_ah_attr *ah_attr);

--
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

[PATCH 2/3] IB/hfi1: Fix missing lock/unlock in verbs drain callback

[Dennis Dalessandro]

From: Mike Marciniszyn <mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

The iowait_sdma_drained() callback lacked locking to
protect the qp s_flags field.

This causes the s_flags to be out of sync
on multiple CPUs, potentially corrupting the s_flags.

Fixes: a545f5308b6c ("staging/rdma/hfi: fix CQ completion order issue")
Reviewed-by: Sebastian Sanchez <sebastian.sanchez-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Mike Marciniszyn <mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/staging/rdma/hfi1/qp.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/rdma/hfi1/qp.c b/drivers/staging/rdma/hfi1/qp.c
index 29a5ad2..dc9119e 100644
--- a/drivers/staging/rdma/hfi1/qp.c
+++ b/drivers/staging/rdma/hfi1/qp.c
@@ -519,10 +519,12 @@ static void iowait_sdma_drained(struct iowait *wait)
 	 * do the flush work until that QP's
 	 * sdma work has finished.
 	 */
+	spin_lock(&qp->s_lock);
 	if (qp->s_flags & RVT_S_WAIT_DMA) {
 		qp->s_flags &= ~RVT_S_WAIT_DMA;
 		hfi1_schedule_send(qp);
 	}
+	spin_unlock(&qp->s_lock);
 }
 
 /**

--
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

[PATCH 3/3] IB/hfi1: Don't attempt to free resources if initialization failed

[Dennis Dalessandro]

From: Mitko Haralanov <mitko.haralanov-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Attempting to free resources which have not been allocated and
initialized properly led to the following kernel backtrace:

    BUG: unable to handle kernel NULL pointer dereference at           (null)
    IP: [<ffffffffa09658fe>] unlock_exp_tids.isra.8+0x2e/0x120 [hfi1]
    PGD 852a43067 PUD 85d4a6067 PMD 0
    Oops: 0000 [#1] SMP
    CPU: 0 PID: 2831 Comm: osu_bw Tainted: G          IO 3.12.18-wfr+ #1
    task: ffff88085b15b540 ti: ffff8808588fe000 task.ti: ffff8808588fe000
    RIP: 0010:[<ffffffffa09658fe>]  [<ffffffffa09658fe>] unlock_exp_tids.isra.8+0x2e/0x120 [hfi1]
    RSP: 0018:ffff8808588ffde0  EFLAGS: 00010282
    RAX: 0000000000000000 RBX: ffff880858a31800 RCX: 0000000000000000
    RDX: ffff88085d971bc0 RSI: ffff880858a318f8 RDI: ffff880858a318c0
    RBP: ffff8808588ffe20 R08: 0000000000000000 R09: 0000000000000000
    R10: ffff88087ffd6f40 R11: 0000000001100348 R12: ffff880852900000
    R13: ffff880858a318c0 R14: 0000000000000000 R15: ffff88085d971be8
    FS:  00007f4674e83740(0000) GS:ffff88087f400000(0000) knlGS:0000000000000000
    CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
    CR2: 0000000000000000 CR3: 000000085c377000 CR4: 00000000001407f0
    Stack:
     ffffffffa0941a71 ffff880858a318f8 ffff88085d971bc0 ffff880858a31800
     ffff880852900000 ffff880858a31800 00000000003ffff7 ffff88085d971bc0
     ffff8808588ffe60 ffffffffa09663fc ffff8808588ffe60 ffff880858a31800
    Call Trace:
     [<ffffffffa0941a71>] ? find_mmu_handler+0x51/0x70 [hfi1]
     [<ffffffffa09663fc>] hfi1_user_exp_rcv_free+0x6c/0x120 [hfi1]
     [<ffffffffa0932809>] hfi1_file_close+0x1a9/0x340 [hfi1]
     [<ffffffff8116c189>] __fput+0xe9/0x270
     [<ffffffff8116c35e>] ____fput+0xe/0x10
     [<ffffffff81065707>] task_work_run+0xa7/0xe0
     [<ffffffff81002969>] do_notify_resume+0x59/0x80
     [<ffffffff814ffc1a>] int_signal+0x12/0x17

This commit re-arranges the context initialization code in a way that
would allow for context event flags to be used to determine whether
the context has been successfully initialized.

In turn, this can be used to skip the resource de-allocation if they
were never allocated in the first place.

Fixes: 3abb33ac6521 ("staging/hfi1: Add TID cache receive init and free funcs")
Reviewed-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Mitko Haralanov <mitko.haralanov-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/staging/rdma/hfi1/file_ops.c     |   60 ++++++++++++++----------------
 drivers/staging/rdma/hfi1/user_exp_rcv.c |    2 +
 2 files changed, 29 insertions(+), 33 deletions(-)

diff --git a/drivers/staging/rdma/hfi1/file_ops.c b/drivers/staging/rdma/hfi1/file_ops.c
index 8396dc5..ec6c226 100644
--- a/drivers/staging/rdma/hfi1/file_ops.c
+++ b/drivers/staging/rdma/hfi1/file_ops.c
@@ -791,15 +791,16 @@ static int hfi1_file_close(struct inode *inode, struct file *fp)
 	spin_unlock_irqrestore(&dd->uctxt_lock, flags);
 
 	dd->rcd[uctxt->ctxt] = NULL;
+
+	hfi1_user_exp_rcv_free(fdata);
+	hfi1_clear_ctxt_pkey(dd, uctxt->ctxt);
+
 	uctxt->rcvwait_to = 0;
 	uctxt->piowait_to = 0;
 	uctxt->rcvnowait = 0;
 	uctxt->pionowait = 0;
 	uctxt->event_flags = 0;
 
-	hfi1_user_exp_rcv_free(fdata);
-	hfi1_clear_ctxt_pkey(dd, uctxt->ctxt);
-
 	hfi1_stats.sps_ctxts--;
 	if (++dd->freectxts == dd->num_user_contexts)
 		aspm_enable_all(dd);
@@ -1127,27 +1128,13 @@ bail:
 
 static int user_init(struct file *fp)
 {
-	int ret;
 	unsigned int rcvctrl_ops = 0;
 	struct hfi1_filedata *fd = fp->private_data;
 	struct hfi1_ctxtdata *uctxt = fd->uctxt;
 
 	/* make sure that the context has already been setup */
-	if (!test_bit(HFI1_CTXT_SETUP_DONE, &uctxt->event_flags)) {
-		ret = -EFAULT;
-		goto done;
-	}
-
-	/*
-	 * Subctxts don't need to initialize anything since master
-	 * has done it.
-	 */
-	if (fd->subctxt) {
-		ret = wait_event_interruptible(uctxt->wait, !test_bit(
-					       HFI1_CTXT_MASTER_UNINIT,
-					       &uctxt->event_flags));
-		goto expected;
-	}
+	if (!test_bit(HFI1_CTXT_SETUP_DONE, &uctxt->event_flags))
+		return -EFAULT;
 
 	/* initialize poll variables... */
 	uctxt->urgent = 0;
@@ -1202,19 +1189,7 @@ static int user_init(struct file *fp)
 		wake_up(&uctxt->wait);
 	}
 
-expected:
-	/*
-	 * Expected receive has to be setup for all processes (including
-	 * shared contexts). However, it has to be done after the master
-	 * context has been fully configured as it depends on the
-	 * eager/expected split of the RcvArray entries.
-	 * Setting it up here ensures that the subcontexts will be waiting
-	 * (due to the above wait_event_interruptible() until the master
-	 * is setup.
-	 */
-	ret = hfi1_user_exp_rcv_init(fp);
-done:
-	return ret;
+	return 0;
 }
 
 static int get_ctxt_info(struct file *fp, void __user *ubase, __u32 len)
@@ -1261,7 +1236,7 @@ static int setup_ctxt(struct file *fp)
 	int ret = 0;
 
 	/*
-	 * Context should be set up only once (including allocation and
+	 * Context should be set up only once, including allocation and
 	 * programming of eager buffers. This is done if context sharing
 	 * is not requested or by the master process.
 	 */
@@ -1282,10 +1257,29 @@ static int setup_ctxt(struct file *fp)
 			if (ret)
 				goto done;
 		}
+	} else {
+		ret = wait_event_interruptible(uctxt->wait, !test_bit(
+					       HFI1_CTXT_MASTER_UNINIT,
+					       &uctxt->event_flags));
+		if (ret)
+			goto done;
 	}
+
 	ret = hfi1_user_sdma_alloc_queues(uctxt, fp);
 	if (ret)
 		goto done;
+	/*
+	 * Expected receive has to be setup for all processes (including
+	 * shared contexts). However, it has to be done after the master
+	 * context has been fully configured as it depends on the
+	 * eager/expected split of the RcvArray entries.
+	 * Setting it up here ensures that the subcontexts will be waiting
+	 * (due to the above wait_event_interruptible() until the master
+	 * is setup.
+	 */
+	ret = hfi1_user_exp_rcv_init(fp);
+	if (ret)
+		goto done;
 
 	set_bit(HFI1_CTXT_SETUP_DONE, &uctxt->event_flags);
 done:
diff --git a/drivers/staging/rdma/hfi1/user_exp_rcv.c b/drivers/staging/rdma/hfi1/user_exp_rcv.c
index 7507b62..1b640a3 100644
--- a/drivers/staging/rdma/hfi1/user_exp_rcv.c
+++ b/drivers/staging/rdma/hfi1/user_exp_rcv.c
@@ -255,6 +255,8 @@ int hfi1_user_exp_rcv_free(struct hfi1_filedata *fd)
 	struct hfi1_ctxtdata *uctxt = fd->uctxt;
 	struct tid_group *grp, *gptr;
 
+	if (!test_bit(HFI1_CTXT_SETUP_DONE, &uctxt->event_flags))
+		return 0;
 	/*
 	 * The notifier would have been removed when the process'es mm
 	 * was freed.

--
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 1/3] IB/rdmavt,hfi1,qib: Fix memory leak

[Leon Romanovsky]

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

On Wed, Apr 20, 2016 at 06:05:24AM -0700, Dennis Dalessandro wrote:
> From: Jubin John <jubin.john-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> rdi->ports has memory allocated in rvt_alloc_device(), but does not get
> freed because the hfi1 and qib drivers drivers call ib_dealloc_device()
> directly instead of going through rdmavt. Add a rvt_dealloc_device()
> that frees rdi->ports and then calls ib_dealloc_device(). Switch hfi1
> and qib drivers to calling rvt_dealloc_device() instead of
> ib_dealloc_device() directly.
> 
> Reviewed-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Dennis,
You posted Fixes line in two other patches, please do in this one two.
It is needed for proper tracking.

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/3] IB/hfi1: Fix missing lock/unlock in verbs drain callback

[Leon Romanovsky]

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

On Wed, Apr 20, 2016 at 06:05:30AM -0700, Dennis Dalessandro wrote:
> From: Mike Marciniszyn <mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> The iowait_sdma_drained() callback lacked locking to
> protect the qp s_flags field.
> 
> This causes the s_flags to be out of sync
> on multiple CPUs, potentially corrupting the s_flags.
> 
> Fixes: a545f5308b6c ("staging/rdma/hfi: fix CQ completion order issue")
> Reviewed-by: Sebastian Sanchez <sebastian.sanchez-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Mike Marciniszyn <mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/3] IB/hfi1: Don't attempt to free resources if initialization failed

[Leon Romanovsky]

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

On Wed, Apr 20, 2016 at 06:05:36AM -0700, Dennis Dalessandro wrote:
> From: Mitko Haralanov <mitko.haralanov-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> Attempting to free resources which have not been allocated and
> initialized properly led to the following kernel backtrace:
> 
>     BUG: unable to handle kernel NULL pointer dereference at           (null)
>     IP: [<ffffffffa09658fe>] unlock_exp_tids.isra.8+0x2e/0x120 [hfi1]
>     PGD 852a43067 PUD 85d4a6067 PMD 0
>     Oops: 0000 [#1] SMP
>     CPU: 0 PID: 2831 Comm: osu_bw Tainted: G          IO 3.12.18-wfr+ #1
>     task: ffff88085b15b540 ti: ffff8808588fe000 task.ti: ffff8808588fe000
>     RIP: 0010:[<ffffffffa09658fe>]  [<ffffffffa09658fe>] unlock_exp_tids.isra.8+0x2e/0x120 [hfi1]
>     RSP: 0018:ffff8808588ffde0  EFLAGS: 00010282
>     RAX: 0000000000000000 RBX: ffff880858a31800 RCX: 0000000000000000
>     RDX: ffff88085d971bc0 RSI: ffff880858a318f8 RDI: ffff880858a318c0
>     RBP: ffff8808588ffe20 R08: 0000000000000000 R09: 0000000000000000
>     R10: ffff88087ffd6f40 R11: 0000000001100348 R12: ffff880852900000
>     R13: ffff880858a318c0 R14: 0000000000000000 R15: ffff88085d971be8
>     FS:  00007f4674e83740(0000) GS:ffff88087f400000(0000) knlGS:0000000000000000
>     CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>     CR2: 0000000000000000 CR3: 000000085c377000 CR4: 00000000001407f0
>     Stack:
>      ffffffffa0941a71 ffff880858a318f8 ffff88085d971bc0 ffff880858a31800
>      ffff880852900000 ffff880858a31800 00000000003ffff7 ffff88085d971bc0
>      ffff8808588ffe60 ffffffffa09663fc ffff8808588ffe60 ffff880858a31800
>     Call Trace:
>      [<ffffffffa0941a71>] ? find_mmu_handler+0x51/0x70 [hfi1]
>      [<ffffffffa09663fc>] hfi1_user_exp_rcv_free+0x6c/0x120 [hfi1]
>      [<ffffffffa0932809>] hfi1_file_close+0x1a9/0x340 [hfi1]
>      [<ffffffff8116c189>] __fput+0xe9/0x270
>      [<ffffffff8116c35e>] ____fput+0xe/0x10
>      [<ffffffff81065707>] task_work_run+0xa7/0xe0
>      [<ffffffff81002969>] do_notify_resume+0x59/0x80
>      [<ffffffff814ffc1a>] int_signal+0x12/0x17
> 
> This commit re-arranges the context initialization code in a way that
> would allow for context event flags to be used to determine whether
> the context has been successfully initialized.
> 
> In turn, this can be used to skip the resource de-allocation if they
> were never allocated in the first place.
> 
> Fixes: 3abb33ac6521 ("staging/hfi1: Add TID cache receive init and free funcs")
> Reviewed-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Mitko Haralanov <mitko.haralanov-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Reviewed-by: Leon Romanovsky <leonro-VPRAkNaXOzUU04JRNCRQjg@public.gmane.org

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/3] IB/rdmavt,hfi1,qib: Fix memory leak

[Dennis Dalessandro]

On Thu, Apr 21, 2016 at 12:38:27PM +0300, Leon Romanovsky wrote:
>On Wed, Apr 20, 2016 at 06:05:24AM -0700, Dennis Dalessandro wrote:
>> From: Jubin John <jubin.john-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> 
>> rdi->ports has memory allocated in rvt_alloc_device(), but does not get
>> freed because the hfi1 and qib drivers drivers call ib_dealloc_device()
>> directly instead of going through rdmavt. Add a rvt_dealloc_device()
>> that frees rdi->ports and then calls ib_dealloc_device(). Switch hfi1
>> and qib drivers to calling rvt_dealloc_device() instead of
>> ib_dealloc_device() directly.
>> 
>> Reviewed-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>
>Dennis,
>You posted Fixes line in two other patches, please do in this one two.
>It is needed for proper tracking.
>
>Except this,
>Reviewed-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Hi Leon, I don't think that a fixes line is warranted in this case. There 
isn't a single commit that causes this issue. It is a missed code path being 
moved to rdmavt.

-Denny
--
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 1/3] IB/rdmavt,hfi1,qib: Fix memory leak

[Leon Romanovsky]

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

On Thu, Apr 21, 2016 at 08:22:17AM -0400, Dennis Dalessandro wrote:
> On Thu, Apr 21, 2016 at 12:38:27PM +0300, Leon Romanovsky wrote:
> >On Wed, Apr 20, 2016 at 06:05:24AM -0700, Dennis Dalessandro wrote:
> >>From: Jubin John <jubin.john-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >>
> >>rdi->ports has memory allocated in rvt_alloc_device(), but does not get
> >>freed because the hfi1 and qib drivers drivers call ib_dealloc_device()
> >>directly instead of going through rdmavt. Add a rvt_dealloc_device()
> >>that frees rdi->ports and then calls ib_dealloc_device(). Switch hfi1
> >>and qib drivers to calling rvt_dealloc_device() instead of
> >>ib_dealloc_device() directly.
> >>
> >>Reviewed-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >
> >Dennis,
> >You posted Fixes line in two other patches, please do in this one two.
> >It is needed for proper tracking.
> >
> >Except this,
> >Reviewed-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> Hi Leon, I don't think that a fixes line is warranted in this case. There
> isn't a single commit that causes this issue. It is a missed code path being
> moved to rdmavt.

So, it fixed the initial commit.
You can easily send an appropriate Fixes line as a response to this
email without need to resubmit the patches.

> 
> -Denny

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/3] IB/rdmavt,hfi1,qib: Fix memory leak

[Dennis Dalessandro]

On Thu, Apr 21, 2016 at 03:50:42PM +0300, Leon Romanovsky wrote:
>On Thu, Apr 21, 2016 at 08:22:17AM -0400, Dennis Dalessandro wrote:
>> On Thu, Apr 21, 2016 at 12:38:27PM +0300, Leon Romanovsky wrote:
>> >On Wed, Apr 20, 2016 at 06:05:24AM -0700, Dennis Dalessandro wrote:
>> >>From: Jubin John <jubin.john-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> >>
>> >>rdi->ports has memory allocated in rvt_alloc_device(), but does not get
>> >>freed because the hfi1 and qib drivers drivers call ib_dealloc_device()
>> >>directly instead of going through rdmavt. Add a rvt_dealloc_device()
>> >>that frees rdi->ports and then calls ib_dealloc_device(). Switch hfi1
>> >>and qib drivers to calling rvt_dealloc_device() instead of
>> >>ib_dealloc_device() directly.
>> >>
>> >>Reviewed-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> >
>> >Dennis,
>> >You posted Fixes line in two other patches, please do in this one two.
>> >It is needed for proper tracking.
>> >
>> >Except this,
>> >Reviewed-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>> 
>> Hi Leon, I don't think that a fixes line is warranted in this case. There
>> isn't a single commit that causes this issue. It is a missed code path being
>> moved to rdmavt.
>
>So, it fixed the initial commit.
>You can easily send an appropriate Fixes line as a response to this
>email without need to resubmit the patches.
>

We have the commit which added the alloc_device() to rdmavt.

Fixes: ff6acd69518e ("IB/rdmavt: Add device structure allocation")

However the dealloc_device would probably have been a follow on patch.  So 
that fixes line is probably not really appropriate. 

Then we have the patches which used the alloc_device in qib and hfi1 and you 
could make the case the dealloc_device should have went in those commits.

So we have these two:
Fixes: 5df1673f1de2 ("IB/qib: Use rdmavt device allocation function")
Fixes: 7af6d00654a1 ("staging/rdma/hfi1: Use rdmavt device allocation 
function")

-Denny
--
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