diff mbox

[5/5] IB/qib: Adjust two size determinations in qib_init_pportdata()

Message ID b5bc4160-aace-b919-7fe2-a4742d03c4ba@users.sourceforge.net (mailing list archive)
State Deferred
Headers show

Commit Message

SF Markus Elfring April 5, 2017, 1:55 p.m. UTC 
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 5 Apr 2017 15:00:44 +0200

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

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

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/infiniband/hw/qib/qib_init.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

Comments

Yuval Shaia April 5, 2017, 2:32 p.m. UTC | #1 
On Wed, Apr 05, 2017 at 03:55:39PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 5 Apr 2017 15:00:44 +0200
> 
> * Replace the specification of two data structures by pointer dereferences
>   to make the corresponding size determination a bit safer according to
>   the Linux coding style convention.
> 
> * Delete the local variable "size" which became unnecessary with
>   this refactoring.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/infiniband/hw/qib/qib_init.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/qib/qib_init.c b/drivers/infiniband/hw/qib/qib_init.c
> index 101580f0460a..e223226ed94d 100644
> --- a/drivers/infiniband/hw/qib/qib_init.c
> +++ b/drivers/infiniband/hw/qib/qib_init.c
> @@ -222,8 +222,6 @@ struct qib_ctxtdata *qib_create_ctxtdata(struct qib_pportdata *ppd, u32 ctxt,
>  int qib_init_pportdata(struct qib_pportdata *ppd, struct qib_devdata *dd,
>  			u8 hw_pidx, u8 port)
>  {
> -	int size;
> -
>  	ppd->dd = dd;
>  	ppd->hw_pidx = hw_pidx;
>  	ppd->port = port; /* IB port number, not index */
> @@ -270,13 +268,14 @@ int qib_init_pportdata(struct qib_pportdata *ppd, struct qib_devdata *dd,
>  	if (!ppd->congestion_entries)
>  		goto bail_1;
>  
> -	size = sizeof(struct cc_table_shadow);
> -	ppd->ccti_entries_shadow = kzalloc(size, GFP_KERNEL);
> +	ppd->ccti_entries_shadow = kzalloc(sizeof(*ppd->ccti_entries_shadow),
> +					   GFP_KERNEL);
>  	if (!ppd->ccti_entries_shadow)
>  		goto bail_2;
>  
> -	size = sizeof(struct ib_cc_congestion_setting_attr);
> -	ppd->congestion_entries_shadow = kzalloc(size, GFP_KERNEL);
> +	ppd->congestion_entries_shadow = kzalloc(sizeof(*ppd
> +							->congestion_entries_shadow),
> +						 GFP_KERNEL);

Not related to this patch but is related to your patch-set - can you check
the array allocations in lines 264 and 268?

Besides that:
Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com>

>  	if (!ppd->congestion_entries_shadow)
>  		goto bail_3;
>  
> -- 
> 2.12.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
SF Markus Elfring April 5, 2017, 3:04 p.m. UTC | #2 
>> @@ -270,13 +268,14 @@ int qib_init_pportdata(struct qib_pportdata *ppd, struct qib_devdata *dd,
>>  	if (!ppd->congestion_entries)
>>  		goto bail_1;
>>  
>> -	size = sizeof(struct cc_table_shadow);
>> -	ppd->ccti_entries_shadow = kzalloc(size, GFP_KERNEL);
>> +	ppd->ccti_entries_shadow = kzalloc(sizeof(*ppd->ccti_entries_shadow),
>> +					   GFP_KERNEL);
>>  	if (!ppd->ccti_entries_shadow)
>>  		goto bail_2;
>>  
>> -	size = sizeof(struct ib_cc_congestion_setting_attr);
>> -	ppd->congestion_entries_shadow = kzalloc(size, GFP_KERNEL);
>> +	ppd->congestion_entries_shadow = kzalloc(sizeof(*ppd
>> +							->congestion_entries_shadow),
>> +						 GFP_KERNEL);
> 
> Not related to this patch but is related to your patch-set - can you check
> the array allocations in lines 264 and 268?

Do you refer to source code places here which are affected by the update step
"[PATCH 4/5] IB/qib: Use kcalloc() in qib_init_pportdata()"?


> Besides that:
> Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com>

Do you find the proposed change for the shown data types really acceptable
in these function calls?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bart Van Assche April 5, 2017, 3:10 p.m. UTC | #3 
On Wed, 2017-04-05 at 15:55 +0200, SF Markus Elfring wrote:
> -	size = sizeof(struct ib_cc_congestion_setting_attr);
> -	ppd->congestion_entries_shadow = kzalloc(size, GFP_KERNEL);
> +	ppd->congestion_entries_shadow = kzalloc(sizeof(*ppd
> +							->congestion_entries_shadow),
> +						 GFP_KERNEL);

The way how the above line has been split looks really weird. Please
move the entire kzalloc() call to the next line such that "*ppd" and
"->congestion_entries_shadow" appear on the same line.

Thanks,

Bart.--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yuval Shaia April 5, 2017, 3:15 p.m. UTC | #4 
On Wed, Apr 05, 2017 at 05:04:35PM +0200, SF Markus Elfring wrote:
> >> @@ -270,13 +268,14 @@ int qib_init_pportdata(struct qib_pportdata *ppd, struct qib_devdata *dd,
> >>  	if (!ppd->congestion_entries)
> >>  		goto bail_1;
> >>  
> >> -	size = sizeof(struct cc_table_shadow);
> >> -	ppd->ccti_entries_shadow = kzalloc(size, GFP_KERNEL);
> >> +	ppd->ccti_entries_shadow = kzalloc(sizeof(*ppd->ccti_entries_shadow),
> >> +					   GFP_KERNEL);
> >>  	if (!ppd->ccti_entries_shadow)
> >>  		goto bail_2;
> >>  
> >> -	size = sizeof(struct ib_cc_congestion_setting_attr);
> >> -	ppd->congestion_entries_shadow = kzalloc(size, GFP_KERNEL);
> >> +	ppd->congestion_entries_shadow = kzalloc(sizeof(*ppd
> >> +							->congestion_entries_shadow),
> >> +						 GFP_KERNEL);
> > 
> > Not related to this patch but is related to your patch-set - can you check
> > the array allocations in lines 264 and 268?
> 
> Do you refer to source code places here which are affected by the update step
> "[PATCH 4/5] IB/qib: Use kcalloc() in qib_init_pportdata()"?

Oops, please ignore.

> 
> 
> > Besides that:
> > Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com>
> 
> Do you find the proposed change for the shown data types really acceptable
> in these function calls?

I found that the fix brings no harm to the existing code.

> 
> Regards,
> Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
SF Markus Elfring April 5, 2017, 3:21 p.m. UTC | #5 
> I found that the fix brings no harm to the existing code.

How do you think about to take another look at remaining update candidates
at other source code places for this Linux module?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yuval Shaia April 6, 2017, 7:48 p.m. UTC | #6 
On Wed, Apr 05, 2017 at 05:21:22PM +0200, SF Markus Elfring wrote:
> > I found that the fix brings no harm to the existing code.
> 
> How do you think about to take another look at remaining update candidates
> at other source code places for this Linux module?

You mean the other patches in this patch-set?

> 
> Regards,
> Markus
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
SF Markus Elfring April 6, 2017, 8:33 p.m. UTC | #7 
>> How do you think about to take another look at remaining update candidates
>> at other source code places for this Linux module?
> 
> You mean the other patches in this patch-set?

Partly, yes. - Do you care for adjustments around numbered jump labels
(and suggestions from the script "checkpatch.pl") for example?

Will it be more important to check return values from all calls of a function
like "kmalloc" in related source files?

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

Patch

diff --git a/drivers/infiniband/hw/qib/qib_init.c b/drivers/infiniband/hw/qib/qib_init.c
index 101580f0460a..e223226ed94d 100644
--- a/drivers/infiniband/hw/qib/qib_init.c
+++ b/drivers/infiniband/hw/qib/qib_init.c
@@ -222,8 +222,6 @@  struct qib_ctxtdata *qib_create_ctxtdata(struct qib_pportdata *ppd, u32 ctxt,
 int qib_init_pportdata(struct qib_pportdata *ppd, struct qib_devdata *dd,
 			u8 hw_pidx, u8 port)
 {
-	int size;
-
 	ppd->dd = dd;
 	ppd->hw_pidx = hw_pidx;
 	ppd->port = port; /* IB port number, not index */
@@ -270,13 +268,14 @@  int qib_init_pportdata(struct qib_pportdata *ppd, struct qib_devdata *dd,
 	if (!ppd->congestion_entries)
 		goto bail_1;
 
-	size = sizeof(struct cc_table_shadow);
-	ppd->ccti_entries_shadow = kzalloc(size, GFP_KERNEL);
+	ppd->ccti_entries_shadow = kzalloc(sizeof(*ppd->ccti_entries_shadow),
+					   GFP_KERNEL);
 	if (!ppd->ccti_entries_shadow)
 		goto bail_2;
 
-	size = sizeof(struct ib_cc_congestion_setting_attr);
-	ppd->congestion_entries_shadow = kzalloc(size, GFP_KERNEL);
+	ppd->congestion_entries_shadow = kzalloc(sizeof(*ppd
+							->congestion_entries_shadow),
+						 GFP_KERNEL);
 	if (!ppd->congestion_entries_shadow)
 		goto bail_3;