[PATCH 1/3] staging/rdma/hfi1: consolidate kmalloc_array+memset into kcalloc

[PATCH 1/3] staging/rdma/hfi1: consolidate kmalloc_array+memset into kcalloc

[Nicholas Mc Guire]

rather than using kmalloc_array + memset it seems cleaner to simply use
kcalloc which will deliver memory set to zero.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---

Patch was compile tested with: x86_64_defconfig
CONFIG_INFINIBAND=m, CONFIG_STAGING=y, CONFIG_STAGING_RDMA=m

Patch is against linux-next (localversion-next is -next-20151214)

 drivers/staging/rdma/hfi1/chip.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/rdma/hfi1/chip.c b/drivers/staging/rdma/hfi1/chip.c
index dc69159..31eec8a 100644
--- a/drivers/staging/rdma/hfi1/chip.c
+++ b/drivers/staging/rdma/hfi1/chip.c
@@ -10128,8 +10128,7 @@ static void init_qos(struct hfi1_devdata *dd, u32 first_ctxt)
 		goto bail;
 	if (num_vls * qpns_per_vl > dd->chip_rcv_contexts)
 		goto bail;
-	rsmmap = kmalloc_array(NUM_MAP_REGS, sizeof(u64), GFP_KERNEL);
-	memset(rsmmap, rxcontext, NUM_MAP_REGS * sizeof(u64));
+	rsmmap = kcalloc(NUM_MAP_REGS, sizeof(u64), GFP_KERNEL);
 	/* init the local copy of the table */
 	for (i = 0, ctxt = first_ctxt; i < num_vls; i++) {
 		unsigned tctxt;
-- 
1.7.10.4

[PATCH 2/3] staging/rdma/hfi1: check return value of kcalloc

[Nicholas Mc Guire]

Add a null check after the kcalloc call as proposed by
Mike Marciniszyn <mike.marciniszyn@intel.com>.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---

Patch was compile tested with: x86_64_defconfig
CONFIG_INFINIBAND=m, CONFIG_STAGING=y, CONFIG_STAGING_RDMA=m

Patch is against linux-next (localversion-next is -next-20151214)

 drivers/staging/rdma/hfi1/chip.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/rdma/hfi1/chip.c b/drivers/staging/rdma/hfi1/chip.c
index 31eec8a..52d2bd7 100644
--- a/drivers/staging/rdma/hfi1/chip.c
+++ b/drivers/staging/rdma/hfi1/chip.c
@@ -10129,6 +10129,9 @@ static void init_qos(struct hfi1_devdata *dd, u32 first_ctxt)
 	if (num_vls * qpns_per_vl > dd->chip_rcv_contexts)
 		goto bail;
 	rsmmap = kcalloc(NUM_MAP_REGS, sizeof(u64), GFP_KERNEL);
+	if (!rsmmap)
+		goto bail;
+
 	/* init the local copy of the table */
 	for (i = 0, ctxt = first_ctxt; i < num_vls; i++) {
 		unsigned tctxt;
-- 
1.7.10.4

RE: [PATCH 2/3] staging/rdma/hfi1: check return value of kcalloc

[Marciniszyn, Mike]

> @@ -10129,6 +10129,9 @@ static void init_qos(struct hfi1_devdata *dd,
> u32 first_ctxt)
>  	if (num_vls * qpns_per_vl > dd->chip_rcv_contexts)
>  		goto bail;
>  	rsmmap = kcalloc(NUM_MAP_REGS, sizeof(u64), GFP_KERNEL);
> +	if (!rsmmap)
> +		goto bail;
> +

I checked out a linux-next remote at the next-20151214 tag.

The allocation method is clearly kmalloc_array() not kcalloc().

Where are you seeing the kcalloc()?

While it is tempting to allocate and zero, there is a chip rev specific difference.

Mike
--
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 2/3] staging/rdma/hfi1: check return value of kcalloc

[Marciniszyn, Mike]

> @@ -10129,6 +10129,9 @@ static void init_qos(struct hfi1_devdata *dd,
> u32 first_ctxt)
>  	if (num_vls * qpns_per_vl > dd->chip_rcv_contexts)
>  		goto bail;
>  	rsmmap = kcalloc(NUM_MAP_REGS, sizeof(u64), GFP_KERNEL);
> +	if (!rsmmap)
> +		goto bail;
> +

I checked out a linux-next remote at the next-20151214 tag.

The allocation method is clearly kmalloc_array() not kcalloc().

Where are you seeing the kcalloc()?

While it is tempting to allocate and zero, there is a chip rev specific difference.

Mike

Re: [PATCH 2/3] staging/rdma/hfi1: check return value of kcalloc

[Nicholas Mc Guire]

On Mon, Dec 14, 2015 at 03:21:24PM +0000, Marciniszyn, Mike wrote:
> > @@ -10129,6 +10129,9 @@ static void init_qos(struct hfi1_devdata *dd,
> > u32 first_ctxt)
> >  	if (num_vls * qpns_per_vl > dd->chip_rcv_contexts)
> >  		goto bail;
> >  	rsmmap = kcalloc(NUM_MAP_REGS, sizeof(u64), GFP_KERNEL);
> > +	if (!rsmmap)
> > +		goto bail;
> > +
> 
> I checked out a linux-next remote at the next-20151214 tag.
> 
> The allocation method is clearly kmalloc_array() not kcalloc().
> 
> Where are you seeing the kcalloc()?
> 
> While it is tempting to allocate and zero, there is a chip rev specific difference.
>
x = kmalloc_array(...)
if(!x)
   ...
memset(x...)

should be equivalent to

kcalloc - include/linux/slab.h

static inline void *kcalloc(size_t n, size_t size, gfp_t flags)
{
        return kmalloc_array(n, size, flags | __GFP_ZERO);
}

if the assumption that this is equvalent is wrong I appologize
the intent was simply API consolidation as the patch description
stated.

thx!
hofrta

[PATCH 3/3] staging/rdma/hfi1: fix build warning

[Nicholas Mc Guire]

fix the following build warning
drivers/staging/rdma/hfi1/chip.c: In function 'init_qos':
drivers/staging/rdma/hfi1/chip.c:10110:6: warning: unused variable
'rxcontext' [-Wunused-variable]

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---

Patch was compile tested with: x86_64_defconfig
CONFIG_INFINIBAND=m, CONFIG_STAGING=y, CONFIG_STAGING_RDMA=m

Patch is against linux-next (localversion-next is -next-20151214)

 drivers/staging/rdma/hfi1/chip.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/rdma/hfi1/chip.c b/drivers/staging/rdma/hfi1/chip.c
index 52d2bd7..ec368a8 100644
--- a/drivers/staging/rdma/hfi1/chip.c
+++ b/drivers/staging/rdma/hfi1/chip.c
@@ -10107,7 +10107,6 @@ static void init_qos(struct hfi1_devdata *dd, u32 first_ctxt)
 	unsigned qpns_per_vl, ctxt, i, qpn, n = 1, m;
 	u64 *rsmmap;
 	u64 reg;
-	u8  rxcontext = is_a0(dd) ? 0 : 0xff;  /* 0 is default if a0 ver. */
 
 	/* validate */
 	if (dd->n_krcv_queues <= MIN_KERNEL_KCTXTS ||
-- 
1.7.10.4

RE: [PATCH 1/3] staging/rdma/hfi1: consolidate kmalloc_array+memset into kcalloc

[Marciniszyn, Mike]

> --- a/drivers/staging/rdma/hfi1/chip.c
> +++ b/drivers/staging/rdma/hfi1/chip.c
> @@ -10128,8 +10128,7 @@ static void init_qos(struct hfi1_devdata *dd,
> u32 first_ctxt)
>  		goto bail;
>  	if (num_vls * qpns_per_vl > dd->chip_rcv_contexts)
>  		goto bail;
> -	rsmmap = kmalloc_array(NUM_MAP_REGS, sizeof(u64),
> GFP_KERNEL);
> -	memset(rsmmap, rxcontext, NUM_MAP_REGS * sizeof(u64));
> +	rsmmap = kcalloc(NUM_MAP_REGS, sizeof(u64), GFP_KERNEL);
>  	/* init the local copy of the table */
>  	for (i = 0, ctxt = first_ctxt; i < num_vls; i++) {
>  		unsigned tctxt;
> --

I'm NAKing this.

There is a chip specific difference that accounts for the current code.

Mike
--
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] staging/rdma/hfi1: consolidate kmalloc_array+memset into kcalloc

[Marciniszyn, Mike]

> --- a/drivers/staging/rdma/hfi1/chip.c
> +++ b/drivers/staging/rdma/hfi1/chip.c
> @@ -10128,8 +10128,7 @@ static void init_qos(struct hfi1_devdata *dd,
> u32 first_ctxt)
>  		goto bail;
>  	if (num_vls * qpns_per_vl > dd->chip_rcv_contexts)
>  		goto bail;
> -	rsmmap = kmalloc_array(NUM_MAP_REGS, sizeof(u64),
> GFP_KERNEL);
> -	memset(rsmmap, rxcontext, NUM_MAP_REGS * sizeof(u64));
> +	rsmmap = kcalloc(NUM_MAP_REGS, sizeof(u64), GFP_KERNEL);
>  	/* init the local copy of the table */
>  	for (i = 0, ctxt = first_ctxt; i < num_vls; i++) {
>  		unsigned tctxt;
> --

I'm NAKing this.

There is a chip specific difference that accounts for the current code.

Mike

Re: [PATCH 1/3] staging/rdma/hfi1: consolidate kmalloc_array+memset into kcalloc

[Nicholas Mc Guire]

On Mon, Dec 14, 2015 at 03:28:46PM +0000, Marciniszyn, Mike wrote:
> > --- a/drivers/staging/rdma/hfi1/chip.c
> > +++ b/drivers/staging/rdma/hfi1/chip.c
> > @@ -10128,8 +10128,7 @@ static void init_qos(struct hfi1_devdata *dd,
> > u32 first_ctxt)
> >  		goto bail;
> >  	if (num_vls * qpns_per_vl > dd->chip_rcv_contexts)
> >  		goto bail;
> > -	rsmmap = kmalloc_array(NUM_MAP_REGS, sizeof(u64),
> > GFP_KERNEL);
> > -	memset(rsmmap, rxcontext, NUM_MAP_REGS * sizeof(u64));
> > +	rsmmap = kcalloc(NUM_MAP_REGS, sizeof(u64), GFP_KERNEL);
> >  	/* init the local copy of the table */
> >  	for (i = 0, ctxt = first_ctxt; i < num_vls; i++) {
> >  		unsigned tctxt;
> > --
> 
> I'm NAKing this.
> 
> There is a chip specific difference that accounts for the current code.
>
I obviously made a real mess here.
I incorrectly concluded that rxcontext is 0 which it is not in some cases

sorry for the noise.

thx!
hofrat



--
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] staging/rdma/hfi1: consolidate kmalloc_array+memset into kcalloc

[Nicholas Mc Guire]

On Mon, Dec 14, 2015 at 03:28:46PM +0000, Marciniszyn, Mike wrote:
> > --- a/drivers/staging/rdma/hfi1/chip.c
> > +++ b/drivers/staging/rdma/hfi1/chip.c
> > @@ -10128,8 +10128,7 @@ static void init_qos(struct hfi1_devdata *dd,
> > u32 first_ctxt)
> >  		goto bail;
> >  	if (num_vls * qpns_per_vl > dd->chip_rcv_contexts)
> >  		goto bail;
> > -	rsmmap = kmalloc_array(NUM_MAP_REGS, sizeof(u64),
> > GFP_KERNEL);
> > -	memset(rsmmap, rxcontext, NUM_MAP_REGS * sizeof(u64));
> > +	rsmmap = kcalloc(NUM_MAP_REGS, sizeof(u64), GFP_KERNEL);
> >  	/* init the local copy of the table */
> >  	for (i = 0, ctxt = first_ctxt; i < num_vls; i++) {
> >  		unsigned tctxt;
> > --
> 
> I'm NAKing this.
> 
> There is a chip specific difference that accounts for the current code.
>
I obviously made a real mess here.
I incorrectly concluded that rxcontext is 0 which it is not in some cases

sorry for the noise.

thx!
hofrat

Re: [PATCH 1/3] staging/rdma/hfi1: consolidate kmalloc_array+memset into kcalloc

[Dan Carpenter]

On Mon, Dec 14, 2015 at 05:41:23PM +0000, Nicholas Mc Guire wrote:
> I obviously made a real mess here.
> I incorrectly concluded that rxcontext is 0 which it is not in some cases

Yep.  Plus you build tested it but assumed that the unused variable
warning must have been there in the original...  I've done that for
static checker warnings.  Lesson learned, hopefully.

regards,
dan carpenter

Re: [PATCH 1/3] staging/rdma/hfi1: consolidate kmalloc_array+memset into kcalloc

[Dan Carpenter]

On Mon, Dec 14, 2015 at 05:41:23PM +0000, Nicholas Mc Guire wrote:
> I obviously made a real mess here.
> I incorrectly concluded that rxcontext is 0 which it is not in some cases

Yep.  Plus you build tested it but assumed that the unused variable
warning must have been there in the original...  I've done that for
static checker warnings.  Lesson learned, hopefully.

regards,
dan carpenter