[PATCH 0/5] IB/hfi1: Fine-tuning for three function implementations

[PATCH 0/5] IB/hfi1: Fine-tuning for three function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 10 Feb 2017 21:53:21 +0100

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (5):
  Use kcalloc() in hfi1_user_exp_rcv_init()
  Use kcalloc() in hfi1_user_sdma_alloc_queues()
  Adjust another size determination in hfi1_user_sdma_alloc_queues()
  Use memdup_user() rather than duplicating its implementation in hfi1_user_sdma_process_request()
  Improve another size determination in hfi1_user_sdma_process_request()

 drivers/infiniband/hw/hfi1/user_exp_rcv.c |  5 +++--
 drivers/infiniband/hw/hfi1/user_sdma.c    | 35 +++++++++++++++----------------
 2 files changed, 20 insertions(+), 20 deletions(-)

-- 
2.11.1

[PATCH 2/5] IB/hfi1: Use kcalloc() in hfi1_user_sdma_alloc_queues()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 9 Feb 2017 16:06:12 +0100

* Multiplications for the size determination of memory allocations
  indicated that array data structures should be processed.
  Thus reuse the corresponding function "kcalloc".

  This issue was detected by using the Coccinelle software.

* Replace the specification of a data type by a pointer dereference
  to make the corresponding size determination a bit safer according to
  the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/infiniband/hw/hfi1/user_sdma.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/user_sdma.c b/drivers/infiniband/hw/hfi1/user_sdma.c
index 7d22f8ee98ef..15194554a92b 100644
--- a/drivers/infiniband/hw/hfi1/user_sdma.c
+++ b/drivers/infiniband/hw/hfi1/user_sdma.c
@@ -400,13 +400,15 @@ int hfi1_user_sdma_alloc_queues(struct hfi1_ctxtdata *uctxt, struct file *fp)
 	if (!pq)
 		goto pq_nomem;
 
-	memsize = sizeof(*pq->reqs) * hfi1_sdma_comp_ring_size;
-	pq->reqs = kzalloc(memsize, GFP_KERNEL);
+	pq->reqs = kcalloc(hfi1_sdma_comp_ring_size,
+			   sizeof(*pq->reqs),
+			   GFP_KERNEL);
 	if (!pq->reqs)
 		goto pq_reqs_nomem;
 
-	memsize = BITS_TO_LONGS(hfi1_sdma_comp_ring_size) * sizeof(long);
-	pq->req_in_use = kzalloc(memsize, GFP_KERNEL);
+	pq->req_in_use = kcalloc(BITS_TO_LONGS(hfi1_sdma_comp_ring_size),
+				 sizeof(*pq->req_in_use),
+				 GFP_KERNEL);
 	if (!pq->req_in_use)
 		goto pq_reqs_no_in_use;
 
-- 
2.11.1

[PATCH 4/5] IB/hfi1: Use memdup_user() rather than duplicating its implementation in hfi1_user_sdma_

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 10 Feb 2017 21:01:55 +0100

Reuse existing functionality from memdup_user() instead of keeping
duplicate source code.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/infiniband/hw/hfi1/user_sdma.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/user_sdma.c b/drivers/infiniband/hw/hfi1/user_sdma.c
index 991e7f3d8e18..5a73d738f2ba 100644
--- a/drivers/infiniband/hw/hfi1/user_sdma.c
+++ b/drivers/infiniband/hw/hfi1/user_sdma.c
@@ -731,23 +731,18 @@ int hfi1_user_sdma_process_request(struct file *fp, struct iovec *iovec,
 			ret = -EINVAL;
 			goto free_req;
 		}
-		req->tids = kcalloc(ntids, sizeof(*req->tids), GFP_KERNEL);
-		if (!req->tids) {
-			ret = -ENOMEM;
-			goto free_req;
-		}
 		/*
 		 * We have to copy all of the tids because they may vary
 		 * in size and, therefore, the TID count might not be
 		 * equal to the pkt count. However, there is no way to
 		 * tell at this point.
 		 */
-		ret = copy_from_user(req->tids, iovec[idx].iov_base,
-				     ntids * sizeof(*req->tids));
-		if (ret) {
+		req->tids = memdup_user(iovec[idx].iov_base,
+					sizeof(*req->tids) * ntids);
+		if (IS_ERR(req->tids)) {
+			ret = PTR_ERR(req->tids);
 			SDMA_DBG(req, "Failed to copy %d TIDs (%d)",
 				 ntids, ret);
-			ret = -EFAULT;
 			goto free_req;
 		}
 		req->n_tids = ntids;
@@ -1606,7 +1601,8 @@ static void user_sdma_free_request(struct user_sdma_request *req, bool unpin)
 				atomic_dec(&node->refcount);
 		}
 	}
-	kfree(req->tids);
+	if (!IS_ERR(req->tids))
+		kfree(req->tids);
 	clear_bit(req->info.comp_idx, req->pq->req_in_use);
 }
 
-- 
2.11.1

Re: [PATCH 4/5] IB/hfi1: Use memdup_user() rather than duplicating its implementation in hfi1_user_s

[Dennis Dalessandro]

On 02/10/2017 04:04 PM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 10 Feb 2017 21:01:55 +0100
>
> Reuse existing functionality from memdup_user() instead of keeping
> duplicate source code.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Thanks for the patch, but this one is already taken care of along with 
other similar uses of kmalloc/copy:

http://marc.info/?l=linux-rdma&m\x148656088729538&w=2

Will review the rest of the patch series soon.

-Denny

Re: [PATCH 27/27] IB/hfi1: Code reuse with memdup_copy

[SF Markus Elfring]

> Thanks for the patch, but this one is already taken care of along with other similar uses of kmalloc/copy:
> http://marc.info/?l=linux-rdma&m\x148656088729538&w=2

Thanks for your information.

The shown source code is reasonable in the update step “[PATCH 27/27] IB/hfi1:
Code reuse with memdup_copy”.
https://patchwork.kernel.org/patch/9562565/
https://lkml.kernel.org/r/<20170208132830.16442.93943.stgit@scvm10.sc.intel.com>

I find the commit subject and message partly inappropriate.
How do you think about to mention other function names there?

Regards,
Markus

Re: [PATCH 4/5] IB/hfi1: Use memdup_user() rather than duplicating its implementation in hfi1_user_s

[Dan Carpenter]

On Sat, Feb 11, 2017 at 10:32:59AM -0500, Dennis Dalessandro wrote:
> On 02/10/2017 04:04 PM, SF Markus Elfring wrote:
> >From: Markus Elfring <elfring@users.sourceforge.net>
> >Date: Fri, 10 Feb 2017 21:01:55 +0100
> >
> >Reuse existing functionality from memdup_user() instead of keeping
> >duplicate source code.
> >
> >Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> 
> Thanks for the patch, but this one is already taken care of along
> with other similar uses of kmalloc/copy:
> 
> http://marc.info/?l=linux-rdma&m\x148656088729538&w=2
> 

Michael's patch doesn't change user_sdma_free_request() so it introduces
a kfreeing an error pointer bug.

regards,
dan carpenter

Re: IB/hfi1: Use memdup_user() rather than duplicating its implementation in hfi1_user_sdma_process_

[SF Markus Elfring]

>> Thanks for the patch, but this one is already taken care of along
>> with other similar uses of kmalloc/copy:
>>
>> http://marc.info/?l=linux-rdma&m\x148656088729538&w=2
>>
> 
> Michael's patch doesn't change user_sdma_free_request() so it introduces
> a kfreeing an error pointer bug.

Did you notice that another local variable “tmp” was introduced in the update step
“[PATCH 27/27] IB/hfi1: Code reuse with memdup_copy” so that
the mentioned function will usually get a null pointer after a failure there?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: IB/hfi1: Use memdup_user() rather than duplicating its implementation in hfi1_user_sdma_process_

[Dan Carpenter]

On Mon, Feb 13, 2017 at 12:12:51PM +0100, SF Markus Elfring wrote:
> >> Thanks for the patch, but this one is already taken care of along
> >> with other similar uses of kmalloc/copy:
> >>
> >> http://marc.info/?l=linux-rdma&m\x148656088729538&w=2
> >>
> > 
> > Michael's patch doesn't change user_sdma_free_request() so it introduces
> > a kfreeing an error pointer bug.
> 
> Did you notice that another local variable “tmp” was introduced in the update step
> “[PATCH 27/27] IB/hfi1: Code reuse with memdup_copy” so that
> the mentioned function will usually get a null pointer after a failure there?
> 

Ah right.  Thanks.  I missed that.

regards,
dan carpenter

[PATCH 1/5] IB/hfi1: Use kcalloc() in hfi1_user_exp_rcv_init()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 9 Feb 2017 15:30:53 +0100

* A multiplication for the size determination of a memory allocation
  indicated that an array data structure should be processed.
  Thus reuse the corresponding function "kcalloc".

  This issue was detected by using the Coccinelle software.

* Replace the specification of a data type by a pointer dereference
  to make the corresponding size determination a bit safer according to
  the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/infiniband/hw/hfi1/user_exp_rcv.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/user_exp_rcv.c b/drivers/infiniband/hw/hfi1/user_exp_rcv.c
index 64d26525435a..8ae0d26a34c6 100644
--- a/drivers/infiniband/hw/hfi1/user_exp_rcv.c
+++ b/drivers/infiniband/hw/hfi1/user_exp_rcv.c
@@ -199,8 +199,9 @@ int hfi1_user_exp_rcv_init(struct file *fp)
 
 	if (!HFI1_CAP_UGET_MASK(uctxt->flags, TID_UNMAP)) {
 		fd->invalid_tid_idx = 0;
-		fd->invalid_tids = kzalloc(uctxt->expected_count *
-					   sizeof(u32), GFP_KERNEL);
+		fd->invalid_tids = kcalloc(uctxt->expected_count,
+					   sizeof(*fd->invalid_tids),
+					   GFP_KERNEL);
 		if (!fd->invalid_tids) {
 			ret = -ENOMEM;
 			goto done;
-- 
2.11.1

[PATCH 3/5] IB/hfi1: Adjust another size determination in hfi1_user_sdma_alloc_queues()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 10 Feb 2017 08:50:45 +0100

* Pass a product for a call of the function "vmalloc_user" without storing
  it in an intermediate variable.

* Delete the local variable "memsize" which became unnecessary with
  this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/infiniband/hw/hfi1/user_sdma.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/user_sdma.c b/drivers/infiniband/hw/hfi1/user_sdma.c
index 15194554a92b..991e7f3d8e18 100644
--- a/drivers/infiniband/hw/hfi1/user_sdma.c
+++ b/drivers/infiniband/hw/hfi1/user_sdma.c
@@ -375,7 +375,6 @@ int hfi1_user_sdma_alloc_queues(struct hfi1_ctxtdata *uctxt, struct file *fp)
 {
 	struct hfi1_filedata *fd;
 	int ret = 0;
-	unsigned memsize;
 	char buf[64];
 	struct hfi1_devdata *dd;
 	struct hfi1_user_sdma_comp_q *cq;
@@ -443,8 +442,8 @@ int hfi1_user_sdma_alloc_queues(struct hfi1_ctxtdata *uctxt, struct file *fp)
 	if (!cq)
 		goto cq_nomem;
 
-	memsize = PAGE_ALIGN(sizeof(*cq->comps) * hfi1_sdma_comp_ring_size);
-	cq->comps = vmalloc_user(memsize);
+	cq->comps = vmalloc_user(PAGE_ALIGN(sizeof(*cq->comps)
+				 * hfi1_sdma_comp_ring_size));
 	if (!cq->comps)
 		goto cq_comps_nomem;
 
-- 
2.11.1

Re: [PATCH 3/5] IB/hfi1: Adjust another size determination in hfi1_user_sdma_alloc_queues()

[Johannes Thumshirn]

On 02/10/2017 10:03 PM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 10 Feb 2017 08:50:45 +0100
> 
> * Pass a product for a call of the function "vmalloc_user" without storing
>   it in an intermediate variable.
> 
> * Delete the local variable "memsize" which became unnecessary with
>   this refactoring.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/infiniband/hw/hfi1/user_sdma.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/hfi1/user_sdma.c b/drivers/infiniband/hw/hfi1/user_sdma.c
> index 15194554a92b..991e7f3d8e18 100644
> --- a/drivers/infiniband/hw/hfi1/user_sdma.c
> +++ b/drivers/infiniband/hw/hfi1/user_sdma.c
> @@ -375,7 +375,6 @@ int hfi1_user_sdma_alloc_queues(struct hfi1_ctxtdata *uctxt, struct file *fp)
>  {
>  	struct hfi1_filedata *fd;
>  	int ret = 0;
> -	unsigned memsize;
>  	char buf[64];
>  	struct hfi1_devdata *dd;
>  	struct hfi1_user_sdma_comp_q *cq;
> @@ -443,8 +442,8 @@ int hfi1_user_sdma_alloc_queues(struct hfi1_ctxtdata *uctxt, struct file *fp)
>  	if (!cq)
>  		goto cq_nomem;
>  
> -	memsize = PAGE_ALIGN(sizeof(*cq->comps) * hfi1_sdma_comp_ring_size);
> -	cq->comps = vmalloc_user(memsize);
> +	cq->comps = vmalloc_user(PAGE_ALIGN(sizeof(*cq->comps)
> +				 * hfi1_sdma_comp_ring_size));
>  	if (!cq->comps)
>  		goto cq_comps_nomem;
>  
> 

IMHO this makes readability worse. What's the intention behind this
patch? Is there any difference in binary size or something in the likes?
I doubt so as the compiler should take care of this anyways.

I'm just a casual reader of the RDMA list not an active reviewer, but if
you did this in e.g. SCSI I'd NACK it. Code has to be readable for
humans and that means the less that's done in one line of code the
better. Let the compiler do the optimizations and get rid of local
variables.

Just my 2c.

Byte,
	Johannes

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 3/5] IB/hfi1: Adjust another size determination in hfi1_user_sdma_alloc_queues()

[SF Markus Elfring]

>> @@ -443,8 +442,8 @@ int hfi1_user_sdma_alloc_queues(struct hfi1_ctxtdata *uctxt, struct file *fp)
>>  	if (!cq)
>>  		goto cq_nomem;
>>  
>> -	memsize = PAGE_ALIGN(sizeof(*cq->comps) * hfi1_sdma_comp_ring_size);
>> -	cq->comps = vmalloc_user(memsize);
>> +	cq->comps = vmalloc_user(PAGE_ALIGN(sizeof(*cq->comps)
>> +				 * hfi1_sdma_comp_ring_size));
>>  	if (!cq->comps)
>>  		goto cq_comps_nomem;
>>  
>>
> 
> IMHO this makes readability worse.

How often does it really make sense to keep such a product in this local variable?


> What's the intention behind this patch?

I suggested just another simple omission of an extra variable.

Regards,
Markus

Re: [PATCH 3/5] IB/hfi1: Adjust another size determination in hfi1_user_sdma_alloc_queues()

[Johannes Thumshirn]

On 02/13/2017 10:32 AM, SF Markus Elfring wrote:
>>> @@ -443,8 +442,8 @@ int hfi1_user_sdma_alloc_queues(struct hfi1_ctxtdata *uctxt, struct file *fp)
>>>  	if (!cq)
>>>  		goto cq_nomem;
>>>  
>>> -	memsize = PAGE_ALIGN(sizeof(*cq->comps) * hfi1_sdma_comp_ring_size);
>>> -	cq->comps = vmalloc_user(memsize);
>>> +	cq->comps = vmalloc_user(PAGE_ALIGN(sizeof(*cq->comps)
>>> +				 * hfi1_sdma_comp_ring_size));
>>>  	if (!cq->comps)
>>>  		goto cq_comps_nomem;
>>>  
>>>
>>
>> IMHO this makes readability worse.
> 
> How often does it really make sense to keep such a product in this local variable?

It depends. Lets take it the other way round. If I this in a review I'd
suggest the submitter to create a local variable for the multiplication
to get rid of the line break. It's avoidable. And again, the compiler
will optimize it away.

Apart from the fact that you haven't tested your patch at all:

jthumshirn@linux-x5ow:linux (test)$ git am ~/\[PATCH\ 3_5\]\ IB_hfi1\:\
Adjust\ another\ size\ determination\ in\
hfi1_user_sdma_alloc_queues\(\).eml
Applying: IB/hfi1: Adjust another size determination in
hfi1_user_sdma_alloc_queues()
jthumshirn@linux-x5ow:linux (test)$ make
drivers/infiniband/hw/hfi1/user_sdma.o  CHK
include/config/kernel.release
  CHK     include/generated/uapi/linux/version.h
  CHK     include/generated/utsrelease.h
  CHK     include/generated/bounds.h
  CHK     include/generated/timeconst.h
  CHK     include/generated/asm-offsets.h
  CALL    scripts/checksyscalls.sh
  CC      drivers/infiniband/hw/hfi1/user_sdma.o
drivers/infiniband/hw/hfi1/user_sdma.c: In function
‘hfi1_user_sdma_alloc_queues’:
drivers/infiniband/hw/hfi1/user_sdma.c:402:2: error: ‘memsize’
undeclared (first use in this function)
  memsize = sizeof(*pq->reqs) * hfi1_sdma_comp_ring_size;
  ^
drivers/infiniband/hw/hfi1/user_sdma.c:402:2: note: each undeclared
identifier is reported only once for each function it appears in
scripts/Makefile.build:294: recipe for target
'drivers/infiniband/hw/hfi1/user_sdma.o' failed
make[1]: *** [drivers/infiniband/hw/hfi1/user_sdma.o] Error 1
Makefile:1640: recipe for target
'drivers/infiniband/hw/hfi1/user_sdma.o' failed
make: *** [drivers/infiniband/hw/hfi1/user_sdma.o] Error 2

With this fixed:

jthumshirn@linux-x5ow:linux (test)$ size
drivers/infiniband/hw/hfi1/user_sdma.o.*
   text	   data	    bss	    dec	    hex	filename
  15393	    212	    292	  15897	   3e19
drivers/infiniband/hw/hfi1/user_sdma.o.old
  15393	    212	    292	  15897	   3e19
drivers/infiniband/hw/hfi1/user_sdma.o.patched


Glancing over the diff of the objdump of these two object files you'll
notice that all your patch is doing is moving the code around in the
.text section of the binary.

So to sum up: there is no evident improvement in the resulting binary
and you introduce a stylistic glitch (the new line break in a function
call).

But all of what I've provided above would have been your job as patch
submitter. You have to reason why your patch is good, it's not our job
to reason why it's bad.

Byte,
	Johannes

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: IB/hfi1: Adjust another size determination in hfi1_user_sdma_alloc_queues()

[SF Markus Elfring]

>> How often does it really make sense to keep such a product in this local variable?
> 
> It depends. Lets take it the other way round. If I this in a review I'd
> suggest the submitter to create a local variable for the multiplication
> to get rid of the line break. It's avoidable.

I imagine that there are further possibilities to improve the involved
programming for various arrays.


> And again, the compiler will optimize it away.
> 
> Apart from the fact that you haven't tested your patch at all:

This is true in principle as I could compile the source code adjustment at least.


> jthumshirn@linux-x5ow:linux (test)$ git am ~/\[PATCH\ 3_5\]\ IB_hfi1\:\
> Adjust\ another\ size\ determination\ in\
> hfi1_user_sdma_alloc_queues\(\).eml
> Applying: IB/hfi1: Adjust another size determination in
> hfi1_user_sdma_alloc_queues()
> jthumshirn@linux-x5ow:linux (test)$ make
> drivers/infiniband/hw/hfi1/user_sdma.o  CHK
> include/config/kernel.release
>   CHK     include/generated/uapi/linux/version.h
>   CHK     include/generated/utsrelease.h
>   CHK     include/generated/bounds.h
>   CHK     include/generated/timeconst.h
>   CHK     include/generated/asm-offsets.h
>   CALL    scripts/checksyscalls.sh
>   CC      drivers/infiniband/hw/hfi1/user_sdma.o
> drivers/infiniband/hw/hfi1/user_sdma.c: In function
> ‘hfi1_user_sdma_alloc_queues’:
> drivers/infiniband/hw/hfi1/user_sdma.c:402:2: error: ‘memsize’
> undeclared (first use in this function)
>   memsize = sizeof(*pq->reqs) * hfi1_sdma_comp_ring_size;
>   ^

How do you think about to apply also the previous update step like “[PATCH 2/5] IB/hfi1:
Use kcalloc() in hfi1_user_sdma_alloc_queues()”?


> So to sum up: there is no evident improvement in the resulting binary

There might not be a remarkable difference with the default software build parameters.


> and you introduce a stylistic glitch (the new line break in a function call).

There are different opinions about this implementation detail, aren't there?

Regards,
Markus

Re: IB/hfi1: Adjust another size determination in hfi1_user_sdma_alloc_queues()

[Johannes Thumshirn]

On 02/13/2017 11:37 AM, SF Markus Elfring wrote:
>> and you introduce a stylistic glitch (the new line break in a function call).
> 
> There are different opinions about this implementation detail, aren't there?

As I said, I'm just a casual reader of the RDMA list and I expressed my
opinion that this change is counter productive. It's up to the RDMA
maintainers to decide whether they want to take the change or not.

But as someone who's dayjob is to work with this code (or better
backport patches to stable trees and fix customer issues) I prefer
keeping it as it was.

So enough bikeshedding for today.
-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 5/5] IB/hfi1: Improve another size determination in hfi1_user_sdma_process_request()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 10 Feb 2017 21:45:38 +0100

Replace the specification of a data structure by a reference to
the desired member as the parameter for the operator "sizeof" to make
the corresponding size determination a bit safer according to
the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/infiniband/hw/hfi1/user_sdma.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/hfi1/user_sdma.c b/drivers/infiniband/hw/hfi1/user_sdma.c
index 5a73d738f2ba..da131a530b88 100644
--- a/drivers/infiniband/hw/hfi1/user_sdma.c
+++ b/drivers/infiniband/hw/hfi1/user_sdma.c
@@ -704,7 +704,9 @@ int hfi1_user_sdma_process_request(struct file *fp, struct iovec *iovec,
 	/* Save all the IO vector structures */
 	for (i = 0; i < req->data_iovs; i++) {
 		INIT_LIST_HEAD(&req->iovs[i].list);
-		memcpy(&req->iovs[i].iov, iovec + idx++, sizeof(struct iovec));
+		memcpy(&req->iovs[i].iov,
+		       iovec + idx++,
+		       sizeof(req->iovs[i].iov));
 		ret = pin_vector_pages(req, &req->iovs[i]);
 		if (ret) {
 			req->status = ret;
-- 
2.11.1

Re: [PATCH 0/5] IB/hfi1: Fine-tuning for three function implementations

[Doug Ledford]

On Fri, 2017-02-10 at 22:00 +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 10 Feb 2017 21:53:21 +0100
> 
> A few update suggestions were taken into account
> from static source code analysis.
> 
> Markus Elfring (5):
>   Use kcalloc() in hfi1_user_exp_rcv_init()
>   Use kcalloc() in hfi1_user_sdma_alloc_queues()
>   Adjust another size determination in hfi1_user_sdma_alloc_queues()
>   Use memdup_user() rather than duplicating its implementation in
> hfi1_user_sdma_process_request()
>   Improve another size determination in
> hfi1_user_sdma_process_request()
> 
>  drivers/infiniband/hw/hfi1/user_exp_rcv.c |  5 +++--
>  drivers/infiniband/hw/hfi1/user_sdma.c    | 35 +++++++++++++++----
> ------------
>  2 files changed, 20 insertions(+), 20 deletions(-)

I took 4 of the 5, leaving out the one that was addressed separately by
an Intel patch.

-- 
Doug Ledford <dledford@redhat.com>
    GPG KeyID: B826A3330E572FDD
   
Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD