[PATCH 0/5] IA64: Fine-tuning for some function implementations

[PATCH 0/5] IA64: Fine-tuning for some function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 26 Aug 2016 19:54:32 +0200

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

Markus Elfring (5):
  IRQ: Use kmalloc_array() in sn_irq_lh_init()
  IRQ: Delete unnecessary braces
  TLB: Fix indentation in ia64_global_tlb_purge()
  TLB: Use kmalloc_array() in ia64_itr_entry()
  TLB: Delete unnecessary braces

 arch/ia64/mm/tlb.c        | 19 ++++++++-----------
 arch/ia64/sn/kernel/irq.c | 29 +++++++++++------------------
 2 files changed, 19 insertions(+), 29 deletions(-)

-- 
2.9.3

[PATCH 1/5] IA64-IRQ: Use kmalloc_array() in sn_irq_lh_init()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 26 Aug 2016 18:32:53 +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 "kmalloc_array".

  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>
---
 arch/ia64/sn/kernel/irq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/ia64/sn/kernel/irq.c b/arch/ia64/sn/kernel/irq.c
index 85d0951..c7f9eea 100644
--- a/arch/ia64/sn/kernel/irq.c
+++ b/arch/ia64/sn/kernel/irq.c
@@ -474,12 +474,12 @@ void __init sn_irq_lh_init(void)
 {
 	int i;
 
-	sn_irq_lh = kmalloc(sizeof(struct list_head *) * NR_IRQS, GFP_KERNEL);
+	sn_irq_lh = kmalloc_array(NR_IRQS, sizeof(*sn_irq_lh), GFP_KERNEL);
 	if (!sn_irq_lh)
 		panic("SN PCI INIT: Failed to allocate memory for PCI init\n");
 
 	for (i = 0; i < NR_IRQS; i++) {
-		sn_irq_lh[i] = kmalloc(sizeof(struct list_head), GFP_KERNEL);
+		sn_irq_lh[i] = kmalloc(*sn_irq_lh[i], GFP_KERNEL);
 		if (!sn_irq_lh[i])
 			panic("SN PCI INIT: Failed IRQ memory allocation\n");
 
-- 
2.9.3

[PATCH 2/5] IA64-IRQ: Delete unnecessary braces

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 26 Aug 2016 18:40:16 +0200

Do not use curly brackets at a few source code places
where a single statement should be sufficient.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/ia64/sn/kernel/irq.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/arch/ia64/sn/kernel/irq.c b/arch/ia64/sn/kernel/irq.c
index c7f9eea..9862a97 100644
--- a/arch/ia64/sn/kernel/irq.c
+++ b/arch/ia64/sn/kernel/irq.c
@@ -127,9 +127,8 @@ struct sn_irq_info *sn_retarget_vector(struct sn_irq_info *sn_irq_info,
 	struct sn_pcibus_provider *pci_provider;
 
 	bridge = (u64) sn_irq_info->irq_bridge;
-	if (!bridge) {
+	if (!bridge)
 		return NULL; /* irq is not a device interrupt */
-	}
 
 	local_nasid = NASID_GET(bridge);
 
@@ -274,10 +273,9 @@ void sn_irq_init(void)
 	ia64_first_device_vector = IA64_SN2_FIRST_DEVICE_VECTOR;
 	ia64_last_device_vector = IA64_SN2_LAST_DEVICE_VECTOR;
 
-	for (i = 0; i < NR_IRQS; i++) {
+	for (i = 0; i < NR_IRQS; i++)
 		if (irq_get_chip(i) == &no_irq_chip)
 			irq_set_chip(i, &irq_type_sn);
-	}
 }
 
 static void register_intr_pda(struct sn_irq_info *sn_irq_info)
@@ -285,10 +283,8 @@ static void register_intr_pda(struct sn_irq_info *sn_irq_info)
 	int irq = sn_irq_info->irq_irq;
 	int cpu = sn_irq_info->irq_cpuid;
 
-	if (pdacpu(cpu)->sn_last_irq < irq) {
+	if (pdacpu(cpu)->sn_last_irq < irq)
 		pdacpu(cpu)->sn_last_irq = irq;
-	}
-
 	if (pdacpu(cpu)->sn_first_irq == 0 || pdacpu(cpu)->sn_first_irq > irq)
 		pdacpu(cpu)->sn_first_irq = irq;
 }
@@ -304,15 +300,15 @@ static void unregister_intr_pda(struct sn_irq_info *sn_irq_info)
 	if (pdacpu(cpu)->sn_last_irq == irq) {
 		foundmatch = 0;
 		for (i = pdacpu(cpu)->sn_last_irq - 1;
-		     i && !foundmatch; i--) {
+		     i && !foundmatch;
+		     i--)
 			list_for_each_entry_rcu(tmp_irq_info,
 						sn_irq_lh[i],
-						list) {
+						list)
 				if (tmp_irq_info->irq_cpuid == cpu) {
 					foundmatch = 1;
 					break;
 				}
-			}
 		}
 		pdacpu(cpu)->sn_last_irq = i;
 	}
@@ -440,7 +436,7 @@ static void sn_check_intr(int irq, struct sn_irq_info *sn_irq_info)
 	    pdi_pcibus_info;
 	regval = pcireg_intr_status_get(pcibus_info);
 
-	if (!ia64_get_irr(irq_to_vector(irq))) {
+	if (!ia64_get_irr(irq_to_vector(irq)))
 		if (!test_bit(irq, pda->sn_in_service_ivecs)) {
 			regval &= 0xff;
 			if (sn_irq_info->irq_int_bit & regval &
@@ -449,7 +445,6 @@ static void sn_check_intr(int irq, struct sn_irq_info *sn_irq_info)
 				sn_call_force_intr_provider(sn_irq_info);
 			}
 		}
-	}
 	sn_irq_info->irq_last_intr = regval;
 }
 
@@ -462,11 +457,9 @@ void sn_lb_int_war_check(void)
 		return;
 
 	rcu_read_lock();
-	for (i = pda->sn_first_irq; i <= pda->sn_last_irq; i++) {
-		list_for_each_entry_rcu(sn_irq_info, sn_irq_lh[i], list) {
+	for (i = pda->sn_first_irq; i <= pda->sn_last_irq; i++)
+		list_for_each_entry_rcu(sn_irq_info, sn_irq_lh[i], list)
 			sn_check_intr(i, sn_irq_info);
-		}
-	}
 	rcu_read_unlock();
 }
 
-- 
2.9.3

[PATCH 3/5] ia64/mm/tlb: Fix indentation in ia64_global_tlb_purge()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 26 Aug 2016 19:36:39 +0200

The script "checkpatch.pl" pointed the following information out.

ERROR: code indent should use tabs where possible
#40: FILE: arch/ia64/mm/tlb.c:271

Thus fix the affected source code places.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/ia64/mm/tlb.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/ia64/mm/tlb.c b/arch/ia64/mm/tlb.c
index 46ecc5d..6d2f83d 100644
--- a/arch/ia64/mm/tlb.c
+++ b/arch/ia64/mm/tlb.c
@@ -269,9 +269,8 @@ ia64_global_tlb_purge (struct mm_struct *mm, unsigned long start,
 	if (need_ptcg_sem)
 		up_spin(&ptcg_sem);
 
-        if (mm != active_mm) {
-                activate_context(active_mm);
-        }
+	if (mm != active_mm)
+		activate_context(active_mm);
 }
 
 void
-- 
2.9.3

[PATCH 4/5] ia64/mm/tlb: Use kmalloc_array() in ia64_itr_entry()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 26 Aug 2016 19:43:34 +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 "kmalloc_array".

  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>
---
 arch/ia64/mm/tlb.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/ia64/mm/tlb.c b/arch/ia64/mm/tlb.c
index 6d2f83d..30870d1 100644
--- a/arch/ia64/mm/tlb.c
+++ b/arch/ia64/mm/tlb.c
@@ -429,8 +429,9 @@ int ia64_itr_entry(u64 target_mask, u64 va, u64 pte, u64 log_size)
 	int cpu = smp_processor_id();
 
 	if (!ia64_idtrs[cpu]) {
-		ia64_idtrs[cpu] = kmalloc(2 * IA64_TR_ALLOC_MAX *
-				sizeof (struct ia64_tr_entry), GFP_KERNEL);
+		ia64_idtrs[cpu] = kmalloc_array(2 * IA64_TR_ALLOC_MAX,
+						sizeof(*ia64_idtrs[cpu]),
+						GFP_KERNEL);
 		if (!ia64_idtrs[cpu])
 			return -ENOMEM;
 	}
-- 
2.9.3

[PATCH 5/5] ia64/mm/tlb: Delete unnecessary braces

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 26 Aug 2016 19:45:24 +0200

Do not use curly brackets at a few source code places
where a single statement should be sufficient.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/ia64/mm/tlb.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/ia64/mm/tlb.c b/arch/ia64/mm/tlb.c
index 30870d1..e2b7435 100644
--- a/arch/ia64/mm/tlb.c
+++ b/arch/ia64/mm/tlb.c
@@ -244,7 +244,7 @@ ia64_global_tlb_purge (struct mm_struct *mm, unsigned long start,
 
 	toolatetochangeptcgsem = 1;
 
-	if (mm != active_mm) {
+	if (mm != active_mm)
 		/* Restore region IDs for mm */
 		if (mm && active_mm) {
 			activate_context(mm);
@@ -252,7 +252,6 @@ ia64_global_tlb_purge (struct mm_struct *mm, unsigned long start,
 			flush_tlb_all();
 			return;
 		}
-	}
 
 	if (need_ptcg_sem)
 		down_spin(&ptcg_sem);
@@ -440,14 +439,13 @@ int ia64_itr_entry(u64 target_mask, u64 va, u64 pte, u64 log_size)
 	if (target_mask & 0x1) {
 		p = ia64_idtrs[cpu];
 		for (i = IA64_TR_ALLOC_BASE; i <= per_cpu(ia64_tr_used, cpu);
-								i++, p++) {
+								i++, p++)
 			if (p->pte & 0x1)
 				if (is_tr_overlap(p, va, log_size)) {
 					printk(KERN_DEBUG "Overlapped Entry"
 						"Inserted for TR Register!!\n");
 					goto out;
 			}
-		}
 	}
 	if (target_mask & 0x2) {
 		p = ia64_idtrs[cpu] + IA64_TR_ALLOC_MAX;
@@ -551,11 +549,10 @@ void ia64_ptr_entry(u64 target_mask, int slot)
 		}
 	}
 
-	for (i = per_cpu(ia64_tr_used, cpu); i >= IA64_TR_ALLOC_BASE; i--) {
+	for (i = per_cpu(ia64_tr_used, cpu); i >= IA64_TR_ALLOC_BASE; i--)
 		if (((ia64_idtrs[cpu] + i)->pte & 0x1) ||
 		    ((ia64_idtrs[cpu] + IA64_TR_ALLOC_MAX + i)->pte & 0x1))
 			break;
-	}
 	per_cpu(ia64_tr_used, cpu) = i;
 }
 EXPORT_SYMBOL_GPL(ia64_ptr_entry);
-- 
2.9.3

Re: [PATCH 1/5] IA64-IRQ: Use kmalloc_array() in sn_irq_lh_init()

[Julia Lawall]

On Fri, 26 Aug 2016, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 26 Aug 2016 18:32:53 +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 "kmalloc_array".
>
>   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>
> ---
>  arch/ia64/sn/kernel/irq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/ia64/sn/kernel/irq.c b/arch/ia64/sn/kernel/irq.c
> index 85d0951..c7f9eea 100644
> --- a/arch/ia64/sn/kernel/irq.c
> +++ b/arch/ia64/sn/kernel/irq.c
> @@ -474,12 +474,12 @@ void __init sn_irq_lh_init(void)
>  {
>  	int i;
>
> -	sn_irq_lh = kmalloc(sizeof(struct list_head *) * NR_IRQS, GFP_KERNEL);
> +	sn_irq_lh = kmalloc_array(NR_IRQS, sizeof(*sn_irq_lh), GFP_KERNEL);
>  	if (!sn_irq_lh)
>  		panic("SN PCI INIT: Failed to allocate memory for PCI init\n");
>
>  	for (i = 0; i < NR_IRQS; i++) {
> -		sn_irq_lh[i] = kmalloc(sizeof(struct list_head), GFP_KERNEL);
> +		sn_irq_lh[i] = kmalloc(*sn_irq_lh[i], GFP_KERNEL);

Did a sizeof get lost here?

julia

>  		if (!sn_irq_lh[i])
>  			panic("SN PCI INIT: Failed IRQ memory allocation\n");
>
> --
> 2.9.3
>
> --
> 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: [PATCH 1/5] IA64-IRQ: Use kmalloc_array() in sn_irq_lh_init()

[kbuild test robot]

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

Hi Markus,

[auto build test ERROR on ia64/next]
[also build test ERROR on v4.8-rc3]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/SF-Markus-Elfring/IA64-Fine-tuning-for-some-function-implementations/20160827-021651
base:   https://git.kernel.org/pub/scm/linux/kernel/git/aegl/linux.git next
config: ia64-allnoconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 4.9.0
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=ia64 

All errors (new ones prefixed by >>):

   arch/ia64/sn/kernel/irq.c: In function 'sn_irq_lh_init':
>> arch/ia64/sn/kernel/irq.c:482:18: error: incompatible type for argument 1 of 'kmalloc'
      sn_irq_lh[i] = kmalloc(*sn_irq_lh[i], GFP_KERNEL);
                     ^
   In file included from arch/ia64/sn/kernel/irq.c:15:0:
   include/linux/slab.h:466:30: note: expected 'size_t' but argument is of type 'struct list_head'
    static __always_inline void *kmalloc(size_t size, gfp_t flags)
                                 ^

vim +/kmalloc +482 arch/ia64/sn/kernel/irq.c

   476	
   477		sn_irq_lh = kmalloc_array(NR_IRQS, sizeof(*sn_irq_lh), GFP_KERNEL);
   478		if (!sn_irq_lh)
   479			panic("SN PCI INIT: Failed to allocate memory for PCI init\n");
   480	
   481		for (i = 0; i < NR_IRQS; i++) {
 > 482			sn_irq_lh[i] = kmalloc(*sn_irq_lh[i], GFP_KERNEL);
   483			if (!sn_irq_lh[i])
   484				panic("SN PCI INIT: Failed IRQ memory allocation\n");
   485	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 5608 bytes --]

Re: [PATCH 2/5] IA64-IRQ: Delete unnecessary braces

[kbuild test robot]

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

Hi Markus,

[auto build test ERROR on ia64/next]
[also build test ERROR on v4.8-rc3]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/SF-Markus-Elfring/IA64-Fine-tuning-for-some-function-implementations/20160827-021651
base:   https://git.kernel.org/pub/scm/linux/kernel/git/aegl/linux.git next
config: ia64-allnoconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 4.9.0
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=ia64 

All error/warnings (new ones prefixed by >>):

>> arch/ia64/sn/kernel/irq.c:316:2: error: expected identifier or '(' before 'if'
     if (pdacpu(cpu)->sn_first_irq == irq) {
     ^
   In file included from include/asm-generic/percpu.h:6:0,
                    from arch/ia64/include/asm/percpu.h:45,
                    from arch/ia64/include/asm/processor.h:77,
                    from arch/ia64/include/asm/thread_info.h:11,
                    from include/linux/thread_info.h:54,
                    from include/asm-generic/preempt.h:4,
                    from arch/ia64/include/generated/asm/preempt.h:1,
                    from include/linux/preempt.h:59,
                    from include/linux/spinlock.h:50,
                    from include/linux/irq.h:15,
                    from arch/ia64/sn/kernel/irq.c:11:
>> include/linux/percpu-defs.h:250:72: error: expected identifier or '(' before ')' token
    #define per_cpu_ptr(ptr, cpu) ({ (void)(cpu); VERIFY_PERCPU_PTR(ptr); })
                                                                           ^
   include/linux/percpu-defs.h:256:29: note: in expansion of macro 'per_cpu_ptr'
    #define per_cpu(var, cpu) (*per_cpu_ptr(&(var), cpu))
                                ^
>> arch/ia64/include/asm/sn/pda.h:66:23: note: in expansion of macro 'per_cpu'
    #define pdacpu(cpu) (&per_cpu(pda_percpu, cpu))
                          ^
>> arch/ia64/sn/kernel/irq.c:316:6: note: in expansion of macro 'pdacpu'
     if (pdacpu(cpu)->sn_first_irq == irq) {
         ^
>> arch/ia64/sn/kernel/irq.c:331:2: warning: data definition has no type or storage class
     rcu_read_unlock();
     ^
>> arch/ia64/sn/kernel/irq.c:331:2: error: type defaults to 'int' in declaration of 'rcu_read_unlock' [-Werror=implicit-int]
>> arch/ia64/sn/kernel/irq.c:331:2: error: function declaration isn't a prototype [-Werror=strict-prototypes]
>> arch/ia64/sn/kernel/irq.c:331:2: error: conflicting types for 'rcu_read_unlock'
   In file included from include/linux/srcu.h:33:0,
                    from include/linux/notifier.h:15,
                    from include/linux/memory_hotplug.h:6,
                    from include/linux/mmzone.h:737,
                    from include/linux/gfp.h:5,
                    from include/linux/irq.h:17,
                    from arch/ia64/sn/kernel/irq.c:11:
   include/linux/rcupdate.h:905:20: note: previous definition of 'rcu_read_unlock' was here
    static inline void rcu_read_unlock(void)
                       ^
>> arch/ia64/sn/kernel/irq.c:332:1: error: expected identifier or '(' before '}' token
    }
    ^
   arch/ia64/sn/kernel/irq.c: In function 'sn_irq_lh_init':
   arch/ia64/sn/kernel/irq.c:475:18: error: incompatible type for argument 1 of 'kmalloc'
      sn_irq_lh[i] = kmalloc(*sn_irq_lh[i], GFP_KERNEL);
                     ^
   In file included from arch/ia64/sn/kernel/irq.c:15:0:
   include/linux/slab.h:466:30: note: expected 'size_t' but argument is of type 'struct list_head'
    static __always_inline void *kmalloc(size_t size, gfp_t flags)
                                 ^
   cc1: some warnings being treated as errors

vim +316 arch/ia64/sn/kernel/irq.c

^1da177e4 Linus Torvalds  2005-04-16  310  					break;
^1da177e4 Linus Torvalds  2005-04-16  311  				}
^1da177e4 Linus Torvalds  2005-04-16  312  		}
^1da177e4 Linus Torvalds  2005-04-16  313  		pdacpu(cpu)->sn_last_irq = i;
^1da177e4 Linus Torvalds  2005-04-16  314  	}
^1da177e4 Linus Torvalds  2005-04-16  315  
^1da177e4 Linus Torvalds  2005-04-16 @316  	if (pdacpu(cpu)->sn_first_irq == irq) {
^1da177e4 Linus Torvalds  2005-04-16  317  		foundmatch = 0;
cb4cb2cb9 Prarit Bhargava 2005-07-06  318  		for (i = pdacpu(cpu)->sn_first_irq + 1;
cb4cb2cb9 Prarit Bhargava 2005-07-06  319  		     i < NR_IRQS && !foundmatch; i++) {
cb4cb2cb9 Prarit Bhargava 2005-07-06  320  			list_for_each_entry_rcu(tmp_irq_info,
cb4cb2cb9 Prarit Bhargava 2005-07-06  321  						sn_irq_lh[i],
cb4cb2cb9 Prarit Bhargava 2005-07-06  322  						list) {
^1da177e4 Linus Torvalds  2005-04-16  323  				if (tmp_irq_info->irq_cpuid == cpu) {
cb4cb2cb9 Prarit Bhargava 2005-07-06  324  					foundmatch = 1;
^1da177e4 Linus Torvalds  2005-04-16  325  					break;
^1da177e4 Linus Torvalds  2005-04-16  326  				}
^1da177e4 Linus Torvalds  2005-04-16  327  			}
^1da177e4 Linus Torvalds  2005-04-16  328  		}
^1da177e4 Linus Torvalds  2005-04-16  329  		pdacpu(cpu)->sn_first_irq = ((i == NR_IRQS) ? 0 : i);
^1da177e4 Linus Torvalds  2005-04-16  330  	}
cb4cb2cb9 Prarit Bhargava 2005-07-06 @331  	rcu_read_unlock();
^1da177e4 Linus Torvalds  2005-04-16 @332  }
^1da177e4 Linus Torvalds  2005-04-16  333  
^1da177e4 Linus Torvalds  2005-04-16  334  void sn_irq_fixup(struct pci_dev *pci_dev, struct sn_irq_info *sn_irq_info)
^1da177e4 Linus Torvalds  2005-04-16  335  {

:::::: The code at line 316 was first introduced by commit
:::::: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 Linux-2.6.12-rc2

:::::: TO: Linus Torvalds <torvalds@ppc970.osdl.org>
:::::: CC: Linus Torvalds <torvalds@ppc970.osdl.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 5608 bytes --]

Re: [PATCH 5/5] ia64/mm/tlb: Delete unnecessary braces

[kbuild test robot]

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

Hi Markus,

[auto build test WARNING on ia64/next]
[also build test WARNING on v4.8-rc3]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/SF-Markus-Elfring/IA64-Fine-tuning-for-some-function-implementations/20160827-021651
base:   https://git.kernel.org/pub/scm/linux/kernel/git/aegl/linux.git next
config: ia64-allnoconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 4.9.0
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=ia64 

All warnings (new ones prefixed by >>):

   arch/ia64/mm/tlb.c: In function 'ia64_global_tlb_purge':
>> arch/ia64/mm/tlb.c:247:5: warning: suggest explicit braces to avoid ambiguous 'else' [-Wparentheses]
     if (mm != active_mm)
        ^

vim +/else +247 arch/ia64/mm/tlb.c

   231			return;
   232		} else
   233			need_ptcg_sem = (num_possible_cpus() > nptcg);
   234	
   235	resetsema:
   236		spinaphore_init(&ptcg_sem, max_purges);
   237	}
   238	
   239	void
   240	ia64_global_tlb_purge (struct mm_struct *mm, unsigned long start,
   241			       unsigned long end, unsigned long nbits)
   242	{
   243		struct mm_struct *active_mm = current->active_mm;
   244	
   245		toolatetochangeptcgsem = 1;
   246	
 > 247		if (mm != active_mm)
   248			/* Restore region IDs for mm */
   249			if (mm && active_mm) {
   250				activate_context(mm);
   251			} else {
   252				flush_tlb_all();
   253				return;
   254			}
   255	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 5608 bytes --]

Re: [PATCH 1/5] IA64-IRQ: Use kmalloc_array() in sn_irq_lh_init()

[Joe Perches]

On Fri, 2016-08-26 at 15:57 -0400, Julia Lawall wrote:
> On Fri, 26 Aug 2016, SF Markus Elfring wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Fri, 26 Aug 2016 18:32:53 +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 "kmalloc_array".
> >
> >   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>
> > ---
> >  arch/ia64/sn/kernel/irq.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/ia64/sn/kernel/irq.c b/arch/ia64/sn/kernel/irq.c
[]
> > @@ -474,12 +474,12 @@ void __init sn_irq_lh_init(void)
> >  {
> >       int i;
> >
> > -     sn_irq_lh = kmalloc(sizeof(struct list_head *) * NR_IRQS, GFP_KERNEL);
> > +     sn_irq_lh = kmalloc_array(NR_IRQS, sizeof(*sn_irq_lh), GFP_KERNEL);
> >       if (!sn_irq_lh)
> >               panic("SN PCI INIT: Failed to allocate memory for PCI init\n");
> >
> >       for (i = 0; i < NR_IRQS; i++) {
> > -             sn_irq_lh[i] = kmalloc(sizeof(struct list_head), GFP_KERNEL);
> > +             sn_irq_lh[i] = kmalloc(*sn_irq_lh[i], GFP_KERNEL);
> 
> Did a sizeof get lost here?

Yes, thanks Julia.

This is why adding the generating spatch code is always good.

And Markus, please always compile test your code using the
appropriate cross-compilers available here:
https://www.kernel.org/pub/tools/crosstool/

And btw: using sizeof(*pp[i]) or sizeof(**pp) is not always
clearer or better than using sizeof(type)

If you _really wanted to clear up this code and make it more
robust/better, it'd probably be nicer to convert the
struct list_head **sn_irq_lh to a single struct list_head *
and do a single
	struct list_head *sn_irq_la = malloc_array(nr_irqs, sizeof(struct list_head);
instead of an malloc_array of the *sn_irq_lh and the
multiple individual struct list_head malloc entries
and change the INIT_LIST_HEAD and indexing code.

That would be less data space overall given the alignment
waste of the individual allocs.

It also appears that sn_irq_lh is extern in
arch/ia64/include/asm/sn/intr.h but could be made static
to arch/ia64/sn/kernel/irq.c.

But really, the conversion isn't worthwhile as NR_IRQS is
limited to much less than the maximum allocation / sizeof(ptr).

Re: IA64-IRQ: Use kmalloc_array() in sn_irq_lh_init()

[SF Markus Elfring]

>> -		sn_irq_lh[i] = kmalloc(sizeof(struct list_head), GFP_KERNEL);
>> +		sn_irq_lh[i] = kmalloc(*sn_irq_lh[i], GFP_KERNEL);
> 
> Did a sizeof get lost here?

Unfortunately,  yes.

It is strange that I overlooked such a hiccup yesterday.


Should I start to automate the source code transformation which I imagined here
better with further scripts for the semantic patch language?

Regards,
Markus

Re: [PATCH 1/5] IA64-IRQ: Use kmalloc_array() in sn_irq_lh_init()

[SF Markus Elfring]

>>> @@ -474,12 +474,12 @@ void __init sn_irq_lh_init(void)
>>>   {
>>>        int i;
>>>
>>> -     sn_irq_lh = kmalloc(sizeof(struct list_head *) * NR_IRQS, GFP_KERNEL);
>>> +     sn_irq_lh = kmalloc_array(NR_IRQS, sizeof(*sn_irq_lh), GFP_KERNEL);
>>>        if (!sn_irq_lh)
>>>                panic("SN PCI INIT: Failed to allocate memory for PCI init\n");
>>>
>>>        for (i = 0; i < NR_IRQS; i++) {
>>> -             sn_irq_lh[i] = kmalloc(sizeof(struct list_head), GFP_KERNEL);
>>> +             sn_irq_lh[i] = kmalloc(*sn_irq_lh[i], GFP_KERNEL);
>>
>> Did a sizeof get lost here?
> 
> Yes, thanks Julia.

Unfortunately, another copy mistake happened during a bit of
source code editing.


> This is why adding the generating spatch code is always good.

I find that this broken update suggestion can point a few details out
for further considerations.

I dared to combine some software aspects once more in this use case.
Such a combination (join point) shows interesting challenges,
doesn't it?


> And Markus, please always compile test your code using the
> appropriate cross-compilers available here:
> https://www.kernel.org/pub/tools/crosstool/

Thanks for your link.


> And btw: using sizeof(*pp[i]) or sizeof(**pp) is not always
> clearer or better than using sizeof(type)

Do you express a target conflict between your expectations
and the evolving Linux coding style documentation here?

Would any software developers insist to see the corresponding
data type directly instead of "evaluating" a pointer expression?


> If you _really wanted to clear up this code and make it more
> robust/better, it'd probably be nicer to convert the
> struct list_head **sn_irq_lh to a single struct list_head *
…
> That would be less data space overall given the alignment
> waste of the individual allocs.

Does this suggestion mean that I should drop my proposal
around the software components "IRQ" and "TLB" for the system
architecture "IA64" in such a questionable patch series?

Regards,
Markus

Re: ia64/mm/tlb: Delete unnecessary braces

[SF Markus Elfring]

>    arch/ia64/mm/tlb.c: In function 'ia64_global_tlb_purge':
>>> arch/ia64/mm/tlb.c:247:5: warning: suggest explicit braces to avoid ambiguous 'else' [-Wparentheses]
>      if (mm != active_mm)
>         ^

Does this message from the compiler mean that there are a few
corresponding details to clarify for the Linux coding style documentation?

Regards,
Markus

Re: [PATCH 1/5] IA64-IRQ: Use kmalloc_array() in sn_irq_lh_init()

[walter harms]

Am 26.08.2016 20:02, schrieb SF Markus Elfring:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 26 Aug 2016 18:32:53 +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 "kmalloc_array".
> 
>   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>
> ---
>  arch/ia64/sn/kernel/irq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/ia64/sn/kernel/irq.c b/arch/ia64/sn/kernel/irq.c
> index 85d0951..c7f9eea 100644
> --- a/arch/ia64/sn/kernel/irq.c
> +++ b/arch/ia64/sn/kernel/irq.c
> @@ -474,12 +474,12 @@ void __init sn_irq_lh_init(void)
>  {
>  	int i;
>  
> -	sn_irq_lh = kmalloc(sizeof(struct list_head *) * NR_IRQS, GFP_KERNEL);
> +	sn_irq_lh = kmalloc_array(NR_IRQS, sizeof(*sn_irq_lh), GFP_KERNEL);
>  	if (!sn_irq_lh)
>  		panic("SN PCI INIT: Failed to allocate memory for PCI init\n");
>  
>  	for (i = 0; i < NR_IRQS; i++) {
> -		sn_irq_lh[i] = kmalloc(sizeof(struct list_head), GFP_KERNEL);
> +		sn_irq_lh[i] = kmalloc(*sn_irq_lh[i], GFP_KERNEL);

perhaps a sizeof(*sn_irq_lh[i]) is here intended ?

re,
 wh

>  		if (!sn_irq_lh[i])
>  			panic("SN PCI INIT: Failed IRQ memory allocation\n");
>  

Re: [PATCH 1/5] IA64-IRQ: Use kmalloc_array() in sn_irq_lh_init()

[Joe Perches]

On Sat, 2016-08-27 at 09:02 +0200, SF Markus Elfring wrote:
> > If you _really wanted to clear up this code and make it more
> > robust/better, it'd probably be nicer to convert the
> > struct list_head **sn_irq_lh to a single struct list_head *
> > That would be less data space overall given the alignment
> > waste of the individual allocs.
> Does this suggestion mean that I should drop my proposal
> around the software components "IRQ" and "TLB" for the system
> architecture "IA64" in such a questionable patch series?

While elimination of code duplication should be good,
what it means it you should avoid making changes that
are merely mechanical and strive to make changes that
improve code execution speed or reduce overall object
size while not impacting overall execution speed.

Re: IA64-IRQ: Use kmalloc_array() in sn_irq_lh_init()

[SF Markus Elfring]

> While elimination of code duplication should be good,
> what it means it you should avoid making changes that
> are merely mechanical and strive to make changes that
> improve code execution speed or reduce overall object
> size while not impacting overall execution speed.

Do other contributors spot any further improvement opportunities
with a higher value in the source files for this software module?

Regards,
Markus

Re: [PATCH 1/5] IA64-IRQ: Use kmalloc_array() in sn_irq_lh_init()

[Julia Lawall]

On Sat, 27 Aug 2016, Joe Perches wrote:

> On Sat, 2016-08-27 at 09:02 +0200, SF Markus Elfring wrote:
> > > If you _really wanted to clear up this code and make it more
> > > robust/better, it'd probably be nicer to convert the
> > > struct list_head **sn_irq_lh to a single struct list_head *
> > > That would be less data space overall given the alignment
> > > waste of the individual allocs.
> > Does this suggestion mean that I should drop my proposal
> > around the software components "IRQ" and "TLB" for the system
> > architecture "IA64" in such a questionable patch series?
>
> While elimination of code duplication should be good,
> what it means it you should avoid making changes that
> are merely mechanical and strive to make changes that
> improve code execution speed or reduce overall object
> size while not impacting overall execution speed.

I do think that there is some value in doing similar things in a uniform
way, using meaningful names, even if in a particular case it doesn't help
performance or reduce code size.  Even duplicating code could be OK if it
is not in a critical path and it makes the code overall easier to
understand.  But if the maintainer prefers the code not to be duplicated,
then of course it should not be duplicated.

julia

Re: [PATCH 1/5] IA64-IRQ: Use kmalloc_array() in sn_irq_lh_init()

[Joe Perches]

On Sun, 2016-08-28 at 11:28 +0200, Julia Lawall wrote:

> I do think that there is some value in doing similar things in a uniform
> way, using meaningful names, even if in a particular case it doesn't help
> performance or reduce code size.  Even duplicating code could be OK if it
> is not in a critical path and it makes the code overall easier to
> understand.  But if the maintainer prefers the code not to be duplicated,
> then of course it should not be duplicated.

All true too, thanks Julia.

It's all maintainer choice.