[PATCH 0/5] IB/qib: Fine-tuning for four function implementations

[PATCH 0/5] IB/qib: Fine-tuning for four function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 5 Apr 2017 15:34:32 +0200

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

Markus Elfring (5):
  Use kcalloc() in qib_init_iba7322_funcs()
  Use kmalloc_array() in qib_init_7322_variables()
  Use kcalloc() in qib_alloc_devdata()
  Use kcalloc() in qib_init_pportdata()
  Adjust two size determinations in qib_init_pportdata()

 drivers/infiniband/hw/qib/qib_iba7322.c | 20 ++++++++++++--------
 drivers/infiniband/hw/qib/qib_init.c    | 27 +++++++++++++--------------
 2 files changed, 25 insertions(+), 22 deletions(-)

-- 
2.12.2

[PATCH 1/5] IB/qib: Use kcalloc() in qib_init_iba7322_funcs()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 5 Apr 2017 09:43:54 +0200

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

  This issue was detected by using the Coccinelle software.

* Replace the specification of a data structure 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/qib/qib_iba7322.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/hw/qib/qib_iba7322.c b/drivers/infiniband/hw/qib/qib_iba7322.c
index af9f596bb68b..e4e4e675c89c 100644
--- a/drivers/infiniband/hw/qib/qib_iba7322.c
+++ b/drivers/infiniband/hw/qib/qib_iba7322.c
@@ -7324,8 +7324,9 @@ struct qib_devdata *qib_init_iba7322_funcs(struct pci_dev *pdev,
 		actual_cnt -= dd->num_pports;
 
 	tabsize = actual_cnt;
-	dd->cspec->msix_entries = kzalloc(tabsize *
-			sizeof(struct qib_msix_entry), GFP_KERNEL);
+	dd->cspec->msix_entries = kcalloc(tabsize,
+					  sizeof(*dd->cspec->msix_entries),
+					  GFP_KERNEL);
 	if (!dd->cspec->msix_entries)
 		tabsize = 0;
 
-- 
2.12.2

Re: [PATCH 1/5] IB/qib: Use kcalloc() in qib_init_iba7322_funcs()

[Yuval Shaia]

On Wed, Apr 05, 2017 at 03:51:52PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 5 Apr 2017 09:43:54 +0200
> 
> * A multiplication for the size determination of a memory allocation
>   indicated that an array data structure should be processed.
>   Thus use the corresponding function "kcalloc".
> 
>   This issue was detected by using the Coccinelle software.
> 
> * Replace the specification of a data structure 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/qib/qib_iba7322.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/qib/qib_iba7322.c b/drivers/infiniband/hw/qib/qib_iba7322.c
> index af9f596bb68b..e4e4e675c89c 100644
> --- a/drivers/infiniband/hw/qib/qib_iba7322.c
> +++ b/drivers/infiniband/hw/qib/qib_iba7322.c
> @@ -7324,8 +7324,9 @@ struct qib_devdata *qib_init_iba7322_funcs(struct pci_dev *pdev,
>  		actual_cnt -= dd->num_pports;
>  
>  	tabsize = actual_cnt;
> -	dd->cspec->msix_entries = kzalloc(tabsize *
> -			sizeof(struct qib_msix_entry), GFP_KERNEL);
> +	dd->cspec->msix_entries = kcalloc(tabsize,
> +					  sizeof(*dd->cspec->msix_entries),
> +					  GFP_KERNEL);

Are we fine with loosing the zeroing of the entries?

>  	if (!dd->cspec->msix_entries)
>  		tabsize = 0;
>  
> -- 
> 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

Re: [PATCH 1/5] IB/qib: Use kcalloc() in qib_init_iba7322_funcs()

[SF Markus Elfring]

>> @@ -7324,8 +7324,9 @@ struct qib_devdata *qib_init_iba7322_funcs(struct pci_dev *pdev,
>>  		actual_cnt -= dd->num_pports;
>>  
>>  	tabsize = actual_cnt;
>> -	dd->cspec->msix_entries = kzalloc(tabsize *
>> -			sizeof(struct qib_msix_entry), GFP_KERNEL);
>> +	dd->cspec->msix_entries = kcalloc(tabsize,
>> +					  sizeof(*dd->cspec->msix_entries),
>> +					  GFP_KERNEL);
> 
> Are we fine with loosing the zeroing of the entries?

How did you get this concern?

Do you really miss such functionality from the other interface?

Regards,
Markus

Re: [PATCH 1/5] IB/qib: Use kcalloc() in qib_init_iba7322_funcs()

[Johannes Thumshirn]

On Wed, Apr 05, 2017 at 04:54:40PM +0200, SF Markus Elfring wrote:
> >> @@ -7324,8 +7324,9 @@ struct qib_devdata *qib_init_iba7322_funcs(struct pci_dev *pdev,
> >>  		actual_cnt -= dd->num_pports;
> >>  
> >>  	tabsize = actual_cnt;
> >> -	dd->cspec->msix_entries = kzalloc(tabsize *
> >> -			sizeof(struct qib_msix_entry), GFP_KERNEL);
> >> +	dd->cspec->msix_entries = kcalloc(tabsize,
> >> +					  sizeof(*dd->cspec->msix_entries),
> >> +					  GFP_KERNEL);
> > 
> > Are we fine with loosing the zeroing of the entries?
> 
> How did you get this concern?
> 
> Do you really miss such functionality from the other interface?

Ahm... Don't kzalloc() and kcalloc() both pass in __GFP_ZERO?

-- 
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 1/5] IB/qib: Use kcalloc() in qib_init_iba7322_funcs()

[Yuval Shaia]

On Wed, Apr 05, 2017 at 05:06:58PM +0200, Johannes Thumshirn wrote:
> On Wed, Apr 05, 2017 at 04:54:40PM +0200, SF Markus Elfring wrote:
> > >> @@ -7324,8 +7324,9 @@ struct qib_devdata *qib_init_iba7322_funcs(struct pci_dev *pdev,
> > >>  		actual_cnt -= dd->num_pports;
> > >>  
> > >>  	tabsize = actual_cnt;
> > >> -	dd->cspec->msix_entries = kzalloc(tabsize *
> > >> -			sizeof(struct qib_msix_entry), GFP_KERNEL);
> > >> +	dd->cspec->msix_entries = kcalloc(tabsize,
> > >> +					  sizeof(*dd->cspec->msix_entries),
> > >> +					  GFP_KERNEL);
> > > 
> > > Are we fine with loosing the zeroing of the entries?
> > 
> > How did you get this concern?
> > 
> > Do you really miss such functionality from the other interface?
> 
> Ahm... Don't kzalloc() and kcalloc() both pass in __GFP_ZERO?

Yes.
Please ignore my comment.

> 
> -- 
> 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 2/5] IB/qib: Use kmalloc_array() in qib_init_7322_variables()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 5 Apr 2017 09:51:33 +0200

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

This issue was detected by using the Coccinelle software.

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

diff --git a/drivers/infiniband/hw/qib/qib_iba7322.c b/drivers/infiniband/hw/qib/qib_iba7322.c
index e4e4e675c89c..490f432809b8 100644
--- a/drivers/infiniband/hw/qib/qib_iba7322.c
+++ b/drivers/infiniband/hw/qib/qib_iba7322.c
@@ -6442,12 +6442,15 @@ static int qib_init_7322_variables(struct qib_devdata *dd)
 	sbufcnt = dd->piobcnt2k + dd->piobcnt4k +
 		NUM_VL15_BUFS + BITS_PER_LONG - 1;
 	sbufcnt /= BITS_PER_LONG;
-	dd->cspec->sendchkenable = kmalloc(sbufcnt *
-		sizeof(*dd->cspec->sendchkenable), GFP_KERNEL);
-	dd->cspec->sendgrhchk = kmalloc(sbufcnt *
-		sizeof(*dd->cspec->sendgrhchk), GFP_KERNEL);
-	dd->cspec->sendibchk = kmalloc(sbufcnt *
-		sizeof(*dd->cspec->sendibchk), GFP_KERNEL);
+	dd->cspec->sendchkenable = kmalloc_array(sbufcnt,
+						 sizeof(*dd->cspec->sendchkenable),
+						 GFP_KERNEL);
+	dd->cspec->sendgrhchk = kmalloc_array(sbufcnt,
+					      sizeof(*dd->cspec->sendgrhchk),
+					      GFP_KERNEL);
+	dd->cspec->sendibchk = kmalloc_array(sbufcnt,
+					     sizeof(*dd->cspec->sendibchk),
+					     GFP_KERNEL);
 	if (!dd->cspec->sendchkenable || !dd->cspec->sendgrhchk ||
 		!dd->cspec->sendibchk) {
 		ret = -ENOMEM;
-- 
2.12.2

Re: [PATCH 2/5] IB/qib: Use kmalloc_array() in qib_init_7322_variables()

[Yuval Shaia]

On Wed, Apr 05, 2017 at 03:52:48PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 5 Apr 2017 09:51:33 +0200
> 
> Multiplications for the size determination of memory allocations
> indicated that array data structures should be processed.
> Thus use the corresponding function "kmalloc_array".
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/infiniband/hw/qib/qib_iba7322.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/qib/qib_iba7322.c b/drivers/infiniband/hw/qib/qib_iba7322.c
> index e4e4e675c89c..490f432809b8 100644
> --- a/drivers/infiniband/hw/qib/qib_iba7322.c
> +++ b/drivers/infiniband/hw/qib/qib_iba7322.c
> @@ -6442,12 +6442,15 @@ static int qib_init_7322_variables(struct qib_devdata *dd)
>  	sbufcnt = dd->piobcnt2k + dd->piobcnt4k +
>  		NUM_VL15_BUFS + BITS_PER_LONG - 1;
>  	sbufcnt /= BITS_PER_LONG;
> -	dd->cspec->sendchkenable = kmalloc(sbufcnt *
> -		sizeof(*dd->cspec->sendchkenable), GFP_KERNEL);
> -	dd->cspec->sendgrhchk = kmalloc(sbufcnt *
> -		sizeof(*dd->cspec->sendgrhchk), GFP_KERNEL);
> -	dd->cspec->sendibchk = kmalloc(sbufcnt *
> -		sizeof(*dd->cspec->sendibchk), GFP_KERNEL);
> +	dd->cspec->sendchkenable = kmalloc_array(sbufcnt,
> +						 sizeof(*dd->cspec->sendchkenable),
> +						 GFP_KERNEL);
> +	dd->cspec->sendgrhchk = kmalloc_array(sbufcnt,
> +					      sizeof(*dd->cspec->sendgrhchk),
> +					      GFP_KERNEL);
> +	dd->cspec->sendibchk = kmalloc_array(sbufcnt,
> +					     sizeof(*dd->cspec->sendibchk),
> +					     GFP_KERNEL);
>  	if (!dd->cspec->sendchkenable || !dd->cspec->sendgrhchk ||
>  		!dd->cspec->sendibchk) {
>  		ret = -ENOMEM;

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

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

[PATCH 3/5] IB/qib: Use kcalloc() in qib_alloc_devdata()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 5 Apr 2017 14:15:45 +0200

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

This issue was detected by using the Coccinelle software.

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

diff --git a/drivers/infiniband/hw/qib/qib_init.c b/drivers/infiniband/hw/qib/qib_init.c
index b50240b1d5a4..9e680ca971e3 100644
--- a/drivers/infiniband/hw/qib/qib_init.c
+++ b/drivers/infiniband/hw/qib/qib_init.c
@@ -1140,8 +1140,8 @@ struct qib_devdata *qib_alloc_devdata(struct pci_dev *pdev, size_t extra)
 	if (!qib_cpulist_count) {
 		u32 count = num_online_cpus();
 
-		qib_cpulist = kzalloc(BITS_TO_LONGS(count) *
-				      sizeof(long), GFP_KERNEL);
+		qib_cpulist = kcalloc(BITS_TO_LONGS(count), sizeof(long),
+				      GFP_KERNEL);
 		if (qib_cpulist)
 			qib_cpulist_count = count;
 	}
-- 
2.12.2

[PATCH 4/5] IB/qib: Use kcalloc() in qib_init_pportdata()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 5 Apr 2017 14:20:10 +0200

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

  This issue was detected by using the Coccinelle software.

* Replace the specification of data structures by pointer dereferences
  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/qib/qib_init.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/infiniband/hw/qib/qib_init.c b/drivers/infiniband/hw/qib/qib_init.c
index 9e680ca971e3..101580f0460a 100644
--- a/drivers/infiniband/hw/qib/qib_init.c
+++ b/drivers/infiniband/hw/qib/qib_init.c
@@ -258,15 +258,15 @@ int qib_init_pportdata(struct qib_pportdata *ppd, struct qib_devdata *dd,
 
 	ppd->cc_max_table_entries =
 		ppd->cc_supported_table_entries/IB_CCT_ENTRIES;
-
-	size = IB_CC_TABLE_CAP_DEFAULT * sizeof(struct ib_cc_table_entry)
-		* IB_CCT_ENTRIES;
-	ppd->ccti_entries = kzalloc(size, GFP_KERNEL);
+	ppd->ccti_entries = kcalloc(IB_CC_TABLE_CAP_DEFAULT * IB_CCT_ENTRIES,
+				    sizeof(*ppd->ccti_entries),
+				    GFP_KERNEL);
 	if (!ppd->ccti_entries)
 		goto bail;
 
-	size = IB_CC_CCS_ENTRIES * sizeof(struct ib_cc_congestion_entry);
-	ppd->congestion_entries = kzalloc(size, GFP_KERNEL);
+	ppd->congestion_entries = kcalloc(IB_CC_CCS_ENTRIES,
+					  sizeof(*ppd->congestion_entries),
+					  GFP_KERNEL);
 	if (!ppd->congestion_entries)
 		goto bail_1;
 
-- 
2.12.2

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

[SF Markus Elfring]

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);
 	if (!ppd->congestion_entries_shadow)
 		goto bail_3;
 
-- 
2.12.2

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

[Yuval Shaia]

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

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

[SF Markus Elfring]

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

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

[Yuval Shaia]

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

Re: IB/qib: Adjust two size determinations in qib_init_pportdata()

[SF Markus Elfring]

> 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

Re: IB/qib: Adjust two size determinations in qib_init_pportdata()

[Yuval Shaia]

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

Re: IB/qib: Adjust two size determinations in qib_init_pportdata()

[SF Markus Elfring]

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

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

[Bart Van Assche]

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.

Re: [PATCH 0/5] IB/qib: Fine-tuning for four function implementations

[Dennis Dalessandro]

On 04/05/2017 09:50 AM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 5 Apr 2017 15:34:32 +0200
>
> A few update suggestions were taken into account
> from static source code analysis.
>
> Markus Elfring (5):
>   Use kcalloc() in qib_init_iba7322_funcs()
>   Use kmalloc_array() in qib_init_7322_variables()
>   Use kcalloc() in qib_alloc_devdata()
>   Use kcalloc() in qib_init_pportdata()
>   Adjust two size determinations in qib_init_pportdata()
>
>  drivers/infiniband/hw/qib/qib_iba7322.c | 20 ++++++++++++--------
>  drivers/infiniband/hw/qib/qib_init.c    | 27 +++++++++++++--------------
>  2 files changed, 25 insertions(+), 22 deletions(-)
>

Doug,

I haven't reviewed these in detail yet but seems be along the lines of 
the patchset you decided not to take for cxgb [1]. I recommend not 
picking this up either. If something in here catches our eye Mike or I 
one will Ack it.

[1] http://marc.info/?l=linux-rdma&m=149141638506039&w=2

-Denny