Linux-Next discussions
 help / color / mirror / Atom feed
* Re: [BUG] next-20081216 - WARNING: at kernel/smp.c:333 smp_call_function_mask
From: Ingo Molnar @ 2008-12-27 12:26 UTC (permalink / raw)
  To: Kamalesh Babulal
  Cc: Yinghai Lu, Sam Ravnborg, David Howells, Andrew Morton,
	Stephen Rothwell, linux-next, LKML, mel
In-Reply-To: <20081227115712.GC4530@linux.vnet.ibm.com>


* Kamalesh Babulal <kamalesh@linux.vnet.ibm.com> wrote:

> > yes, indeed - updated patch below.
> > 
> > 	Ingo
> > 
> 
> Hi Ingo,
> 
> 	Sorry, for not mentioning that the previous kernel trace, was 
> after adding the return statement to the arch_init_chip_data().

ah, i missed that detail. There was a stupid typo in one of the conflict 
resolutions i had to do - could you check the updated patch below?

Thanks,

	Ingo

----------->
>From 13a0c3c269b223f60abfac8a9811d77111a8b4ba Mon Sep 17 00:00:00 2001
From: Yinghai Lu <yinghai@kernel.org>
Date: Fri, 26 Dec 2008 02:05:47 -0800
Subject: [PATCH] sparseirq: work around compiler optimizing away __weak functions

Impact: fix panic on null pointer with sparseirq

Some GCC versions seem to inline the weak global function,
when that function is empty.

Work it around, by making the functions return a (dummy) integer.

Signed-off-by: Yinghai <yinghai@kernel.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/io_apic.c |    8 ++++++--
 include/linux/irq.h       |    6 +++---
 init/main.c               |    7 ++++---
 kernel/irq/handle.c       |    7 ++++---
 4 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/io_apic.c b/arch/x86/kernel/io_apic.c
index 2fe543f..9760393 100644
--- a/arch/x86/kernel/io_apic.c
+++ b/arch/x86/kernel/io_apic.c
@@ -170,7 +170,7 @@ static struct irq_cfg irq_cfgx[NR_IRQS] = {
 	[15] = { .domain = CPU_MASK_ALL, .vector = IRQ15_VECTOR, },
 };
 
-void __init arch_early_irq_init(void)
+int __init arch_early_irq_init(void)
 {
 	struct irq_cfg *cfg;
 	struct irq_desc *desc;
@@ -184,6 +184,8 @@ void __init arch_early_irq_init(void)
 		desc = irq_to_desc(i);
 		desc->chip_data = &cfg[i];
 	}
+
+	return 0;
 }
 
 #ifdef CONFIG_SPARSE_IRQ
@@ -212,7 +214,7 @@ static struct irq_cfg *get_one_free_irq_cfg(int cpu)
 	return cfg;
 }
 
-void arch_init_chip_data(struct irq_desc *desc, int cpu)
+int arch_init_chip_data(struct irq_desc *desc, int cpu)
 {
 	struct irq_cfg *cfg;
 
@@ -224,6 +226,8 @@ void arch_init_chip_data(struct irq_desc *desc, int cpu)
 			BUG_ON(1);
 		}
 	}
+
+	return 0;
 }
 
 #ifdef CONFIG_NUMA_MIGRATE_IRQ_DESC
diff --git a/include/linux/irq.h b/include/linux/irq.h
index 69da275..0e40af4 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -193,9 +193,9 @@ struct irq_desc {
 	const char		*name;
 } ____cacheline_internodealigned_in_smp;
 
-extern void early_irq_init(void);
-extern void arch_early_irq_init(void);
-extern void arch_init_chip_data(struct irq_desc *desc, int cpu);
+extern int early_irq_init(void);
+extern int arch_early_irq_init(void);
+extern int arch_init_chip_data(struct irq_desc *desc, int cpu);
 extern void arch_init_copy_chip_data(struct irq_desc *old_desc,
 					struct irq_desc *desc, int cpu);
 extern void arch_free_chip_data(struct irq_desc *old_desc, struct irq_desc *desc);
diff --git a/init/main.c b/init/main.c
index c1f999a..c314aa1 100644
--- a/init/main.c
+++ b/init/main.c
@@ -539,13 +539,14 @@ void __init __weak thread_info_cache_init(void)
 {
 }
 
-void __init __weak arch_early_irq_init(void)
+int __init __weak arch_early_irq_init(void)
 {
+	return 0;
 }
 
-void __init __weak early_irq_init(void)
+int __init __weak early_irq_init(void)
 {
-	arch_early_irq_init();
+	return arch_early_irq_init();
 }
 
 asmlinkage void __init start_kernel(void)
diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
index 893da67..0bef3ec 100644
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -86,8 +86,9 @@ void init_kstat_irqs(struct irq_desc *desc, int cpu, int nr)
 		desc->kstat_irqs = (unsigned int *)ptr;
 }
 
-void __attribute__((weak)) arch_init_chip_data(struct irq_desc *desc, int cpu)
+int __weak arch_init_chip_data(struct irq_desc *desc, int cpu)
 {
+	return 0;
 }
 
 static void init_one_irq_desc(int irq, struct irq_desc *desc, int cpu)
@@ -132,7 +133,7 @@ static struct irq_desc irq_desc_legacy[NR_IRQS_LEGACY] __cacheline_aligned_in_sm
 /* FIXME: use bootmem alloc ...*/
 static unsigned int kstat_irqs_legacy[NR_IRQS_LEGACY][NR_CPUS];
 
-void __init early_irq_init(void)
+int __init early_irq_init(void)
 {
 	struct irq_desc *desc;
 	int legacy_count;
@@ -151,7 +152,7 @@ void __init early_irq_init(void)
 	for (i = legacy_count; i < NR_IRQS; i++)
 		irq_desc_ptrs[i] = NULL;
 
-	arch_early_irq_init();
+	return arch_early_irq_init();
 }
 
 struct irq_desc *irq_to_desc(unsigned int irq)

^ permalink raw reply related

* Re: [BUG] next-20081216 - WARNING: at kernel/smp.c:333 smp_call_function_mask
From: Kamalesh Babulal @ 2008-12-27 11:57 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Yinghai Lu, Sam Ravnborg, David Howells, Andrew Morton,
	Stephen Rothwell, linux-next, LKML, mel
In-Reply-To: <20081227112450.GA24150@elte.hu>

* Ingo Molnar <mingo@elte.hu> [2008-12-27 12:24:50]:

> 
> * Kamalesh Babulal <kamalesh@linux.vnet.ibm.com> wrote:
> 
> > * Ingo Molnar <mingo@elte.hu> [2008-12-27 09:39:36]:
> > 
> > > 
> > > * Ingo Molnar <mingo@elte.hu> wrote:
> > > 
> > > > * Yinghai Lu <yinghai@kernel.org> wrote:
> > > > 
> > > > > +extern int arch_init_chip_data(struct irq_desc *desc, int cpu);
> > > > 
> > > > when you change a function declaration's return type, you _must_ double 
> > > > check whether you properly updated all instances of that function. In 
> > > > this case you forgot about the io_apic.c instance.
> > > 
> > > the corrected patch is below.
> > > 
> > > 	Ingo
> > >
> > 
> > > --------------->
> > > From d353c528bbb29be434f7688cba9b59781e9151e3 Mon Sep 17 00:00:00 2001
> > > From: Yinghai Lu <yinghai@kernel.org>
> > > Date: Fri, 26 Dec 2008 02:05:47 -0800
> > > Subject: [PATCH] sparseirq: work around compiler optimizing away __weak functions
> > > 
> > > Impact: fix panic on null pointer with sparseirq
> > > 
> > > Some GCC versions seem to inline the weak global function,
> > > when that function is empty.
> > > 
> > > Work it around, by making the functions return a (dummy) integer.
> > > 
> > > Signed-off-by: Yinghai <yinghai@kernel.org>
> > > Signed-off-by: Ingo Molnar <mingo@elte.hu>
> > > ---
> > >  arch/x86/kernel/io_apic.c |    6 ++++--
> > >  include/linux/irq.h       |    6 +++---
> > >  init/main.c               |    7 ++++---
> > >  kernel/irq/handle.c       |    7 ++++---
> > >  4 files changed, 15 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/arch/x86/kernel/io_apic.c b/arch/x86/kernel/io_apic.c
> > > index 2fe543f..e4d4d19 100644
> > > --- a/arch/x86/kernel/io_apic.c
> > > +++ b/arch/x86/kernel/io_apic.c
> > > @@ -170,7 +170,7 @@ static struct irq_cfg irq_cfgx[NR_IRQS] = {
> > >  	[15] = { .domain = CPU_MASK_ALL, .vector = IRQ15_VECTOR, },
> > >  };
> > > 
> > > -void __init arch_early_irq_init(void)
> > > +int __init arch_early_irq_init(void)
> > >  {
> > >  	struct irq_cfg *cfg;
> > >  	struct irq_desc *desc;
> > > @@ -184,6 +184,8 @@ void __init arch_early_irq_init(void)
> > >  		desc = irq_to_desc(i);
> > >  		desc->chip_data = &cfg[i];
> > >  	}
> > > +
> > > +	return 0;
> > >  }
> > > 
> > >  #ifdef CONFIG_SPARSE_IRQ
> > > @@ -212,7 +214,7 @@ static struct irq_cfg *get_one_free_irq_cfg(int cpu)
> > >  	return cfg;
> > >  }
> > > 
> > > -void arch_init_chip_data(struct irq_desc *desc, int cpu)
> > > +int arch_init_chip_data(struct irq_desc *desc, int cpu)
> > >  {
> > >  	struct irq_cfg *cfg;
> > 
> > the return statement is missing in the patch.
> 
> yes, indeed - updated patch below.
> 
> 	Ingo
> 

Hi Ingo,

	Sorry, for not mentioning that the previous kernel trace, was after
adding the return statement to the arch_init_chip_data().

> --------------->
> From 03f20b9c3b659c191684cb93de66a20386711df4 Mon Sep 17 00:00:00 2001
> From: Yinghai Lu <yinghai@kernel.org>
> Date: Fri, 26 Dec 2008 02:05:47 -0800
> Subject: [PATCH] sparseirq: work around compiler optimizing away __weak functions
> 
> Impact: fix panic on null pointer with sparseirq
> 
> Some GCC versions seem to inline the weak global function,
> when that function is empty.
> 
> Work it around, by making the functions return a (dummy) integer.
> 
> Signed-off-by: Yinghai <yinghai@kernel.org>
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> ---
>  arch/x86/kernel/io_apic.c |    8 ++++++--
>  include/linux/irq.h       |    6 +++---
>  init/main.c               |    7 ++++---
>  kernel/irq/handle.c       |    7 ++++---
>  4 files changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/kernel/io_apic.c b/arch/x86/kernel/io_apic.c
> index 2fe543f..9760393 100644
> --- a/arch/x86/kernel/io_apic.c
> +++ b/arch/x86/kernel/io_apic.c
> @@ -170,7 +170,7 @@ static struct irq_cfg irq_cfgx[NR_IRQS] = {
>  	[15] = { .domain = CPU_MASK_ALL, .vector = IRQ15_VECTOR, },
>  };
> 
> -void __init arch_early_irq_init(void)
> +int __init arch_early_irq_init(void)
>  {
>  	struct irq_cfg *cfg;
>  	struct irq_desc *desc;
> @@ -184,6 +184,8 @@ void __init arch_early_irq_init(void)
>  		desc = irq_to_desc(i);
>  		desc->chip_data = &cfg[i];
>  	}
> +
> +	return 0;
>  }
> 
>  #ifdef CONFIG_SPARSE_IRQ
> @@ -212,7 +214,7 @@ static struct irq_cfg *get_one_free_irq_cfg(int cpu)
>  	return cfg;
>  }
> 
> -void arch_init_chip_data(struct irq_desc *desc, int cpu)
> +int arch_init_chip_data(struct irq_desc *desc, int cpu)
>  {
>  	struct irq_cfg *cfg;
> 
> @@ -224,6 +226,8 @@ void arch_init_chip_data(struct irq_desc *desc, int cpu)
>  			BUG_ON(1);
>  		}
>  	}
> +
> +	return 0;
>  }
> 
>  #ifdef CONFIG_NUMA_MIGRATE_IRQ_DESC
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index 69da275..0e40af4 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -193,9 +193,9 @@ struct irq_desc {
>  	const char		*name;
>  } ____cacheline_internodealigned_in_smp;
> 
> -extern void early_irq_init(void);
> -extern void arch_early_irq_init(void);
> -extern void arch_init_chip_data(struct irq_desc *desc, int cpu);
> +extern int early_irq_init(void);
> +extern int arch_early_irq_init(void);
> +extern int arch_init_chip_data(struct irq_desc *desc, int cpu);
>  extern void arch_init_copy_chip_data(struct irq_desc *old_desc,
>  					struct irq_desc *desc, int cpu);
>  extern void arch_free_chip_data(struct irq_desc *old_desc, struct irq_desc *desc);
> diff --git a/init/main.c b/init/main.c
> index c1f999a..b541c9a 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -539,13 +539,14 @@ void __init __weak thread_info_cache_init(void)
>  {
>  }
> 
> -void __init __weak arch_early_irq_init(void)
> +int __init __weak arch_early_irq_init(void)
>  {
> +	return 0;
>  }
> 
> -void __init __weak early_irq_init(void)
> +int __init __weak early_irq_init(void)
>  {
> -	arch_early_irq_init();
> +	return 0;
>  }
> 
>  asmlinkage void __init start_kernel(void)
> diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
> index 893da67..0bef3ec 100644
> --- a/kernel/irq/handle.c
> +++ b/kernel/irq/handle.c
> @@ -86,8 +86,9 @@ void init_kstat_irqs(struct irq_desc *desc, int cpu, int nr)
>  		desc->kstat_irqs = (unsigned int *)ptr;
>  }
> 
> -void __attribute__((weak)) arch_init_chip_data(struct irq_desc *desc, int cpu)
> +int __weak arch_init_chip_data(struct irq_desc *desc, int cpu)
>  {
> +	return 0;
>  }
> 
>  static void init_one_irq_desc(int irq, struct irq_desc *desc, int cpu)
> @@ -132,7 +133,7 @@ static struct irq_desc irq_desc_legacy[NR_IRQS_LEGACY] __cacheline_aligned_in_sm
>  /* FIXME: use bootmem alloc ...*/
>  static unsigned int kstat_irqs_legacy[NR_IRQS_LEGACY][NR_CPUS];
> 
> -void __init early_irq_init(void)
> +int __init early_irq_init(void)
>  {
>  	struct irq_desc *desc;
>  	int legacy_count;
> @@ -151,7 +152,7 @@ void __init early_irq_init(void)
>  	for (i = legacy_count; i < NR_IRQS; i++)
>  		irq_desc_ptrs[i] = NULL;
> 
> -	arch_early_irq_init();
> +	return arch_early_irq_init();
>  }
> 
>  struct irq_desc *irq_to_desc(unsigned int irq)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-next" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Thanks & Regards,
Kamalesh Babulal,
Linux Technology Center,
IBM, ISTL.

^ permalink raw reply

* Re: [BUG] next-20081216 - WARNING: at kernel/smp.c:333 smp_call_function_mask
From: Ingo Molnar @ 2008-12-27 11:24 UTC (permalink / raw)
  To: Kamalesh Babulal
  Cc: Yinghai Lu, Sam Ravnborg, David Howells, Andrew Morton,
	Stephen Rothwell, linux-next, LKML, mel
In-Reply-To: <20081227112125.GB4530@linux.vnet.ibm.com>


* Kamalesh Babulal <kamalesh@linux.vnet.ibm.com> wrote:

> * Ingo Molnar <mingo@elte.hu> [2008-12-27 09:39:36]:
> 
> > 
> > * Ingo Molnar <mingo@elte.hu> wrote:
> > 
> > > * Yinghai Lu <yinghai@kernel.org> wrote:
> > > 
> > > > +extern int arch_init_chip_data(struct irq_desc *desc, int cpu);
> > > 
> > > when you change a function declaration's return type, you _must_ double 
> > > check whether you properly updated all instances of that function. In 
> > > this case you forgot about the io_apic.c instance.
> > 
> > the corrected patch is below.
> > 
> > 	Ingo
> >
> 
> > --------------->
> > From d353c528bbb29be434f7688cba9b59781e9151e3 Mon Sep 17 00:00:00 2001
> > From: Yinghai Lu <yinghai@kernel.org>
> > Date: Fri, 26 Dec 2008 02:05:47 -0800
> > Subject: [PATCH] sparseirq: work around compiler optimizing away __weak functions
> > 
> > Impact: fix panic on null pointer with sparseirq
> > 
> > Some GCC versions seem to inline the weak global function,
> > when that function is empty.
> > 
> > Work it around, by making the functions return a (dummy) integer.
> > 
> > Signed-off-by: Yinghai <yinghai@kernel.org>
> > Signed-off-by: Ingo Molnar <mingo@elte.hu>
> > ---
> >  arch/x86/kernel/io_apic.c |    6 ++++--
> >  include/linux/irq.h       |    6 +++---
> >  init/main.c               |    7 ++++---
> >  kernel/irq/handle.c       |    7 ++++---
> >  4 files changed, 15 insertions(+), 11 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/io_apic.c b/arch/x86/kernel/io_apic.c
> > index 2fe543f..e4d4d19 100644
> > --- a/arch/x86/kernel/io_apic.c
> > +++ b/arch/x86/kernel/io_apic.c
> > @@ -170,7 +170,7 @@ static struct irq_cfg irq_cfgx[NR_IRQS] = {
> >  	[15] = { .domain = CPU_MASK_ALL, .vector = IRQ15_VECTOR, },
> >  };
> > 
> > -void __init arch_early_irq_init(void)
> > +int __init arch_early_irq_init(void)
> >  {
> >  	struct irq_cfg *cfg;
> >  	struct irq_desc *desc;
> > @@ -184,6 +184,8 @@ void __init arch_early_irq_init(void)
> >  		desc = irq_to_desc(i);
> >  		desc->chip_data = &cfg[i];
> >  	}
> > +
> > +	return 0;
> >  }
> > 
> >  #ifdef CONFIG_SPARSE_IRQ
> > @@ -212,7 +214,7 @@ static struct irq_cfg *get_one_free_irq_cfg(int cpu)
> >  	return cfg;
> >  }
> > 
> > -void arch_init_chip_data(struct irq_desc *desc, int cpu)
> > +int arch_init_chip_data(struct irq_desc *desc, int cpu)
> >  {
> >  	struct irq_cfg *cfg;
> 
> the return statement is missing in the patch.

yes, indeed - updated patch below.

	Ingo

--------------->
>From 03f20b9c3b659c191684cb93de66a20386711df4 Mon Sep 17 00:00:00 2001
From: Yinghai Lu <yinghai@kernel.org>
Date: Fri, 26 Dec 2008 02:05:47 -0800
Subject: [PATCH] sparseirq: work around compiler optimizing away __weak functions

Impact: fix panic on null pointer with sparseirq

Some GCC versions seem to inline the weak global function,
when that function is empty.

Work it around, by making the functions return a (dummy) integer.

Signed-off-by: Yinghai <yinghai@kernel.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/io_apic.c |    8 ++++++--
 include/linux/irq.h       |    6 +++---
 init/main.c               |    7 ++++---
 kernel/irq/handle.c       |    7 ++++---
 4 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/io_apic.c b/arch/x86/kernel/io_apic.c
index 2fe543f..9760393 100644
--- a/arch/x86/kernel/io_apic.c
+++ b/arch/x86/kernel/io_apic.c
@@ -170,7 +170,7 @@ static struct irq_cfg irq_cfgx[NR_IRQS] = {
 	[15] = { .domain = CPU_MASK_ALL, .vector = IRQ15_VECTOR, },
 };
 
-void __init arch_early_irq_init(void)
+int __init arch_early_irq_init(void)
 {
 	struct irq_cfg *cfg;
 	struct irq_desc *desc;
@@ -184,6 +184,8 @@ void __init arch_early_irq_init(void)
 		desc = irq_to_desc(i);
 		desc->chip_data = &cfg[i];
 	}
+
+	return 0;
 }
 
 #ifdef CONFIG_SPARSE_IRQ
@@ -212,7 +214,7 @@ static struct irq_cfg *get_one_free_irq_cfg(int cpu)
 	return cfg;
 }
 
-void arch_init_chip_data(struct irq_desc *desc, int cpu)
+int arch_init_chip_data(struct irq_desc *desc, int cpu)
 {
 	struct irq_cfg *cfg;
 
@@ -224,6 +226,8 @@ void arch_init_chip_data(struct irq_desc *desc, int cpu)
 			BUG_ON(1);
 		}
 	}
+
+	return 0;
 }
 
 #ifdef CONFIG_NUMA_MIGRATE_IRQ_DESC
diff --git a/include/linux/irq.h b/include/linux/irq.h
index 69da275..0e40af4 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -193,9 +193,9 @@ struct irq_desc {
 	const char		*name;
 } ____cacheline_internodealigned_in_smp;
 
-extern void early_irq_init(void);
-extern void arch_early_irq_init(void);
-extern void arch_init_chip_data(struct irq_desc *desc, int cpu);
+extern int early_irq_init(void);
+extern int arch_early_irq_init(void);
+extern int arch_init_chip_data(struct irq_desc *desc, int cpu);
 extern void arch_init_copy_chip_data(struct irq_desc *old_desc,
 					struct irq_desc *desc, int cpu);
 extern void arch_free_chip_data(struct irq_desc *old_desc, struct irq_desc *desc);
diff --git a/init/main.c b/init/main.c
index c1f999a..b541c9a 100644
--- a/init/main.c
+++ b/init/main.c
@@ -539,13 +539,14 @@ void __init __weak thread_info_cache_init(void)
 {
 }
 
-void __init __weak arch_early_irq_init(void)
+int __init __weak arch_early_irq_init(void)
 {
+	return 0;
 }
 
-void __init __weak early_irq_init(void)
+int __init __weak early_irq_init(void)
 {
-	arch_early_irq_init();
+	return 0;
 }
 
 asmlinkage void __init start_kernel(void)
diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
index 893da67..0bef3ec 100644
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -86,8 +86,9 @@ void init_kstat_irqs(struct irq_desc *desc, int cpu, int nr)
 		desc->kstat_irqs = (unsigned int *)ptr;
 }
 
-void __attribute__((weak)) arch_init_chip_data(struct irq_desc *desc, int cpu)
+int __weak arch_init_chip_data(struct irq_desc *desc, int cpu)
 {
+	return 0;
 }
 
 static void init_one_irq_desc(int irq, struct irq_desc *desc, int cpu)
@@ -132,7 +133,7 @@ static struct irq_desc irq_desc_legacy[NR_IRQS_LEGACY] __cacheline_aligned_in_sm
 /* FIXME: use bootmem alloc ...*/
 static unsigned int kstat_irqs_legacy[NR_IRQS_LEGACY][NR_CPUS];
 
-void __init early_irq_init(void)
+int __init early_irq_init(void)
 {
 	struct irq_desc *desc;
 	int legacy_count;
@@ -151,7 +152,7 @@ void __init early_irq_init(void)
 	for (i = legacy_count; i < NR_IRQS; i++)
 		irq_desc_ptrs[i] = NULL;
 
-	arch_early_irq_init();
+	return arch_early_irq_init();
 }
 
 struct irq_desc *irq_to_desc(unsigned int irq)

^ permalink raw reply related

* Re: [BUG] next-20081216 - WARNING: at kernel/smp.c:333 smp_call_function_mask
From: Kamalesh Babulal @ 2008-12-27 11:21 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Yinghai Lu, Sam Ravnborg, David Howells, Andrew Morton,
	Stephen Rothwell, linux-next, LKML, mel
In-Reply-To: <20081227083936.GA12609@elte.hu>

* Ingo Molnar <mingo@elte.hu> [2008-12-27 09:39:36]:

> 
> * Ingo Molnar <mingo@elte.hu> wrote:
> 
> > * Yinghai Lu <yinghai@kernel.org> wrote:
> > 
> > > +extern int arch_init_chip_data(struct irq_desc *desc, int cpu);
> > 
> > when you change a function declaration's return type, you _must_ double 
> > check whether you properly updated all instances of that function. In 
> > this case you forgot about the io_apic.c instance.
> 
> the corrected patch is below.
> 
> 	Ingo
>

> --------------->
> From d353c528bbb29be434f7688cba9b59781e9151e3 Mon Sep 17 00:00:00 2001
> From: Yinghai Lu <yinghai@kernel.org>
> Date: Fri, 26 Dec 2008 02:05:47 -0800
> Subject: [PATCH] sparseirq: work around compiler optimizing away __weak functions
> 
> Impact: fix panic on null pointer with sparseirq
> 
> Some GCC versions seem to inline the weak global function,
> when that function is empty.
> 
> Work it around, by making the functions return a (dummy) integer.
> 
> Signed-off-by: Yinghai <yinghai@kernel.org>
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> ---
>  arch/x86/kernel/io_apic.c |    6 ++++--
>  include/linux/irq.h       |    6 +++---
>  init/main.c               |    7 ++++---
>  kernel/irq/handle.c       |    7 ++++---
>  4 files changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/kernel/io_apic.c b/arch/x86/kernel/io_apic.c
> index 2fe543f..e4d4d19 100644
> --- a/arch/x86/kernel/io_apic.c
> +++ b/arch/x86/kernel/io_apic.c
> @@ -170,7 +170,7 @@ static struct irq_cfg irq_cfgx[NR_IRQS] = {
>  	[15] = { .domain = CPU_MASK_ALL, .vector = IRQ15_VECTOR, },
>  };
> 
> -void __init arch_early_irq_init(void)
> +int __init arch_early_irq_init(void)
>  {
>  	struct irq_cfg *cfg;
>  	struct irq_desc *desc;
> @@ -184,6 +184,8 @@ void __init arch_early_irq_init(void)
>  		desc = irq_to_desc(i);
>  		desc->chip_data = &cfg[i];
>  	}
> +
> +	return 0;
>  }
> 
>  #ifdef CONFIG_SPARSE_IRQ
> @@ -212,7 +214,7 @@ static struct irq_cfg *get_one_free_irq_cfg(int cpu)
>  	return cfg;
>  }
> 
> -void arch_init_chip_data(struct irq_desc *desc, int cpu)
> +int arch_init_chip_data(struct irq_desc *desc, int cpu)
>  {
>  	struct irq_cfg *cfg;

the return statement is missing in the patch.

> 
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index 69da275..0e40af4 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -193,9 +193,9 @@ struct irq_desc {
>  	const char		*name;
>  } ____cacheline_internodealigned_in_smp;
> 
> -extern void early_irq_init(void);
> -extern void arch_early_irq_init(void);
> -extern void arch_init_chip_data(struct irq_desc *desc, int cpu);
> +extern int early_irq_init(void);
> +extern int arch_early_irq_init(void);
> +extern int arch_init_chip_data(struct irq_desc *desc, int cpu);
>  extern void arch_init_copy_chip_data(struct irq_desc *old_desc,
>  					struct irq_desc *desc, int cpu);
>  extern void arch_free_chip_data(struct irq_desc *old_desc, struct irq_desc *desc);
> diff --git a/init/main.c b/init/main.c
> index c1f999a..b541c9a 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -539,13 +539,14 @@ void __init __weak thread_info_cache_init(void)
>  {
>  }
> 
> -void __init __weak arch_early_irq_init(void)
> +int __init __weak arch_early_irq_init(void)
>  {
> +	return 0;
>  }
> 
> -void __init __weak early_irq_init(void)
> +int __init __weak early_irq_init(void)
>  {
> -	arch_early_irq_init();
> +	return 0;
>  }
> 
>  asmlinkage void __init start_kernel(void)
> diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
> index 893da67..0bef3ec 100644
> --- a/kernel/irq/handle.c
> +++ b/kernel/irq/handle.c
> @@ -86,8 +86,9 @@ void init_kstat_irqs(struct irq_desc *desc, int cpu, int nr)
>  		desc->kstat_irqs = (unsigned int *)ptr;
>  }
> 
> -void __attribute__((weak)) arch_init_chip_data(struct irq_desc *desc, int cpu)
> +int __weak arch_init_chip_data(struct irq_desc *desc, int cpu)
>  {
> +	return 0;
>  }
> 
>  static void init_one_irq_desc(int irq, struct irq_desc *desc, int cpu)
> @@ -132,7 +133,7 @@ static struct irq_desc irq_desc_legacy[NR_IRQS_LEGACY] __cacheline_aligned_in_sm
>  /* FIXME: use bootmem alloc ...*/
>  static unsigned int kstat_irqs_legacy[NR_IRQS_LEGACY][NR_CPUS];
> 
> -void __init early_irq_init(void)
> +int __init early_irq_init(void)
>  {
>  	struct irq_desc *desc;
>  	int legacy_count;
> @@ -151,7 +152,7 @@ void __init early_irq_init(void)
>  	for (i = legacy_count; i < NR_IRQS; i++)
>  		irq_desc_ptrs[i] = NULL;
> 
> -	arch_early_irq_init();
> +	return arch_early_irq_init();
>  }
> 
>  struct irq_desc *irq_to_desc(unsigned int irq)


After applying the patch, the kernel crashes with the same back trace


Initializing CPU#0
BUG: unable to handle kernel NULL pointer dereference at 0000000000000048
IP: [<ffffffff8070e46e>] init_ISA_irqs+0x18/0x53
PGD 0 
Thread overran stack, or stack corrupted
Oops: 0002 [#1] SMP 
last sysfs file: 
CPU 0 
Modules linked in:
Pid: 0, comm: swapper Not tainted 2.6.28-rc8-autotest-tip #1
RIP: 0010:[<ffffffff8070e46e>]  [<ffffffff8070e46e>] init_ISA_irqs+0x18/0x53
RSP: 0018:ffffffff806fff68  EFLAGS: 00010093
RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffffffff80773b40
RDX: 0000000000000100 RSI: 0000000000000096 RDI: 0000000000000000
RBP: 0000000000000000 R08: ffff8800808a3000 R09: 0000000000000000
R10: ffff880001012ce0 R11: ffffffff80355720 R12: 0000000000000000
R13: ffffffff80735860 R14: ffffffff807380a0 R15: 0000000000000000
FS:  0000000000000000(0000) GS:ffffffff806f1480(0000) knlGS:0000000000000000
CS:  0010 DS: 0018 ES: 0018 CR0: 000000008005003b
CR2: 0000000000000048 CR3: 0000000000201000 CR4: 00000000000006a0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process swapper (pid: 0, threadinfo ffffffff806fe000, task ffffffff8067e3a0)
Stack:
 0000000000000003 ffffffff8070e4b2 ffffffff806e2f40 ffffffff8050e443
 0000000000000003 ffffffff80707b25 0000000000000000 00000000008127f0
 ffffffff807380a0 0000000000093370 0000000000000000 0000000000000000
Call Trace:
 [<ffffffff8070e4b2>] native_init_IRQ+0x9/0x9b5
 [<ffffffff8050e443>] register_cpu_notifier+0x1f/0x23
 [<ffffffff80707b25>] start_kernel+0x1c2/0x31d
 [<ffffffff80707394>] x86_64_start_kernel+0xde/0xe2
Code: 85 c0 75 0d 59 48 c7 c7 a0 ff 67 80 e9 78 bf cb ff 5a c3 53 31 db e8 6e 87 00 00 31 ff e8 4d 0e b0 ff 89 df e8 9f 81 b5 ff 89 df <c7> 40 48 00 02 00 00 48 c7 40 40 00 00 00 00 c7 40 4c 01 00 00 
RIP  [<ffffffff8070e46e>] init_ISA_irqs+0x18/0x53
 RSP <ffffffff806fff68>
CR2: 0000000000000048
---[ end trace 4eaa2a86a8e2da22 ]---
Kernel panic - not syncing: Attempted to kill the idle task!
Pid: 0, comm: swapper Tainted: G      D    2.6.28-rc8-autotest-tip #1
Call Trace:
 [<ffffffff80237321>] panic+0x86/0x144
 [<ffffffff80237eb1>] printk+0x4e/0x56
 [<ffffffff80239fd7>] do_exit+0x75/0x78f
 [<ffffffff8052579e>] oops_end+0xa8/0xad
 [<ffffffff80526fed>] do_page_fault+0x756/0x80f
 [<ffffffff80524daf>] page_fault+0x1f/0x30
 [<ffffffff80355720>] delay_loop+0x0/0x29
 [<ffffffff8070e46e>] init_ISA_irqs+0x18/0x53
 [<ffffffff8070e46c>] init_ISA_irqs+0x16/0x53
 [<ffffffff8070e4b2>] native_init_IRQ+0x9/0x9b5
 [<ffffffff8050e443>] register_cpu_notifier+0x1f/0x23
 [<ffffffff80707b25>] start_kernel+0x1c2/0x31d
 [<ffffffff80707394>] x86_64_start_kernel+0xde/0xe2
------------[ cut here ]------------
WARNING: at kernel/smp.c:333 smp_call_function_mask+0x36/0x224()
Hardware name: IBM eServer BladeCenter LS20 -[885055U]-
Modules linked in:
Pid: 0, comm: swapper Tainted: G      D    2.6.28-rc8-autotest-tip #1
Call Trace:
 [<ffffffff80237274>] warn_slowpath+0xd8/0xf5
 [<ffffffff80524be1>] _spin_lock_irqsave+0x9/0xe
 [<ffffffff8024c2fa>] up+0xe/0x37
 [<ffffffff80237945>] release_console_sem+0x186/0x1a1
 [<ffffffff80524be1>] _spin_lock_irqsave+0x9/0xe
 [<ffffffff8024c2fa>] up+0xe/0x37
 [<ffffffff80237945>] release_console_sem+0x186/0x1a1
 [<ffffffff80211d37>] stop_this_cpu+0x0/0x1d
 [<ffffffff80707394>] x86_64_start_kernel+0xde/0xe2
 [<ffffffff80707394>] x86_64_start_kernel+0xde/0xe2
 [<ffffffff80707394>] x86_64_start_kernel+0xde/0xe2
 [<ffffffff80237eb1>] printk+0x4e/0x56
 [<ffffffff80253dab>] smp_call_function_mask+0x36/0x224
 [<ffffffff80237eb1>] printk+0x4e/0x56
 [<ffffffff8034f386>] __next_cpu_nr+0x1a/0x21
 [<ffffffff8021e8d8>] touch_nmi_watchdog+0x43/0x53
 [<ffffffff8020f13d>] print_trace_address+0x1d/0x2d
 [<ffffffff80247747>] __kernel_text_address+0x1a/0x26
 [<ffffffff8020f20f>] print_context_stack+0x90/0xa6
 [<ffffffff80259d75>] crash_kexec+0xe6/0xef
 [<ffffffff8020e4a7>] dump_trace+0x249/0x258
 [<ffffffff80253fc3>] smp_call_function+0x2a/0x2f
 [<ffffffff8021d3e3>] native_smp_send_stop+0x1a/0x26
 [<ffffffff80237335>] panic+0x9a/0x144
 [<ffffffff80237eb1>] printk+0x4e/0x56
 [<ffffffff80239fd7>] do_exit+0x75/0x78f
 [<ffffffff8052579e>] oops_end+0xa8/0xad
 [<ffffffff80526fed>] do_page_fault+0x756/0x80f
 [<ffffffff80524daf>] page_fault+0x1f/0x30
 [<ffffffff80355720>] delay_loop+0x0/0x29
 [<ffffffff8070e46e>] init_ISA_irqs+0x18/0x53
 [<ffffffff8070e46c>] init_ISA_irqs+0x16/0x53
 [<ffffffff8070e4b2>] native_init_IRQ+0x9/0x9b5
 [<ffffffff8050e443>] register_cpu_notifier+0x1f/0x23
 [<ffffffff80707b25>] start_kernel+0x1c2/0x31d
 [<ffffffff80707394>] x86_64_start_kernel+0xde/0xe2
---[ end trace 4eaa2a86a8e2da22 ]---
-- 
Thanks & Regards,
Kamalesh Babulal,
Linux Technology Center,
IBM, ISTL.

^ permalink raw reply

* Re: [PATCH] kbuild, sparseirq: work around GCC bug with __weak aliases
From: Ingo Molnar @ 2008-12-27 10:59 UTC (permalink / raw)
  To: Paul Mackerras
  Cc: Sam Ravnborg, Yinghai Lu, David Howells, Kamalesh Babulal,
	Andrew Morton, Stephen Rothwell, linux-next, LKML, mel
In-Reply-To: <18773.43656.189438.155690@cargo.ozlabs.ibm.com>


* Paul Mackerras <paulus@samba.org> wrote:

> Ingo Molnar writes:
> 
> > * Ingo Molnar <mingo@elte.hu> wrote:
> > 
> > > > I recall David Howells had a similar issue with the bootparamter patch 
> > > > set. The workaround he used was to add a barrier(); call in the weak 
> > > > function to avoid the inline.
> > > 
> > > could we add some extra attribute to __weak that would have a similar 
> > > effect? Something like __attribute__((noinline)), or something silly 
> > > like __attribute__((deprecated)) - just to keep gcc from screwing up 
> > > __weak functions? Perhaps adding a section attribute would have a 
> > > similar effect? (putting weak definitions into an extra section is 
> > > probably helpful anyway)
> > 
> > I've applied the patch below to tip/irq/sparseirq - could someone with an 
> > affected GCC version please check whether this solves the crash?
> 
> I recall from discussions earlier that noinline doesn't fix the problem, 
> and I just tested a similar case and verified that adding noinline 
> doesn't stop some versions of gcc from inlining them.  The empty weak 
> functions in kernel/perf_counter.c were getting inlined by the cross-gcc 
> (gcc 4.1.1) I use for compiling powerpc kernels on my laptop, and adding 
> noinline doesn't help there.

hm, does Yinghai's patch below do the trick? (also in perfcounters/core 
and tip/master)

	Ingo

-------------------->
>From 01ea1ccaa24dea3552e103be13b7897211607a8b Mon Sep 17 00:00:00 2001
From: Yinghai Lu <yinghai@kernel.org>
Date: Fri, 26 Dec 2008 21:05:06 -0800
Subject: [PATCH] perf_counter: more barrier in blank weak function

Impact: fix panic possible panic

Some versions of GCC inline the weak global function if it's empty.
Add a barrier() to work it around.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/perf_counter.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/perf_counter.c b/kernel/perf_counter.c
index d7a79f3..37f7716 100644
--- a/kernel/perf_counter.c
+++ b/kernel/perf_counter.c
@@ -45,8 +45,8 @@ hw_perf_counter_init(struct perf_counter *counter)
 }
 
 u64 __weak hw_perf_save_disable(void)		{ return 0; }
-void __weak hw_perf_restore(u64 ctrl)		{ }
-void __weak hw_perf_counter_setup(void)		{ }
+void __weak hw_perf_restore(u64 ctrl)		{ barrier(); }
+void __weak hw_perf_counter_setup(void)		{ barrier(); }
 
 static void
 list_add_counter(struct perf_counter *counter, struct perf_counter_context *ctx)

^ permalink raw reply related

* Re: [BUG] next-20081216 - WARNING: at kernel/smp.c:333 smp_call_function_mask
From: Ingo Molnar @ 2008-12-27  9:50 UTC (permalink / raw)
  To: David Howells
  Cc: Sam Ravnborg, Yinghai Lu, Kamalesh Babulal, Andrew Morton,
	Stephen Rothwell, linux-next, LKML, mel
In-Reply-To: <3677.1230371218@redhat.com>


* David Howells <dhowells@redhat.com> wrote:

> Sam Ravnborg <sam@ravnborg.org> wrote:
> 
> > I recall David Howells had a similar issue with the bootparamter patch set.
> > The workaround he used was to add a barrier(); call in the weak function
> > to avoid the inline.
> 
> Yes I did.  Rusty's solution was just to move the default weak functions into
> different files.
> 
> Attempting to use the noinline attribute does not work.
> 
> Of course, you could just stick the functions into lib/ in separate .c 
> files and dispense with the weak attribute altogether.

the weak functions should be close to where the code that uses them is - 
not somewhere separate (where no-one will really be aware of their 
existence). So either we find a way to avoid such repeat bugs in the 
future, or we do away with weak functions altogether and go back to 
stone-age #ifdefs ;-)

I think we should get a Sparse check that detects empty void __weak 
functions?

	Ingo

^ permalink raw reply

* Re: [PATCH] sparc64: use unsigned long long for u64
From: Sam Ravnborg @ 2008-12-27  9:49 UTC (permalink / raw)
  To: David Miller
  Cc: akpm, sparclinux, mingo, sfr, linux-kernel, kenchen, paulus, tglx,
	hpa, linux-next
In-Reply-To: <20081227.013712.63335017.davem@davemloft.net>

On Sat, Dec 27, 2008 at 01:37:12AM -0800, David Miller wrote:
> From: Sam Ravnborg <sam@ravnborg.org>
> Date: Sat, 27 Dec 2008 10:24:04 +0100
> 
> > So it looks like my gcc does not promote:
> > ((tagval & 0x0fffffff00000000UL) >> 19UL) to unsigned long long int
> > but your gcc does?
> > 
> > My gcc (build with Dan Kegel's crosstool):
> > $ /opt/crosstool/gcc-3.4.5-glibc-2.3.6/sparc64-unknown-linux-gnu/bin/sparc64-unknown-linux-gnu-gcc -v
> > Reading specs from /opt/crosstool/gcc-3.4.5-glibc-2.3.6/sparc64-unknown-linux-gnu/lib/gcc/sparc64-unknown-linux-gnu/3.4.5/specs
> > Configured with: /home/sam/devel/crosstool-0.43/build/sparc64-unknown-linux-gnu/gcc-3.4.5-glibc-2.3.6/gcc-3.4.5/configure --target=sparc64-unknown-linux-gnu --host=i686-host_pc-linux-gnu --prefix=/opt/crosstool/gcc-3.4.5-glibc-2.3.6/sparc64-unknown-linux-gnu --disable-multilib --with-cpu=ultrasparc3 --enable-cxx-flags=-mcpu=ultrasparc3 --with-headers=/opt/crosstool/gcc-3.4.5-glibc-2.3.6/sparc64-unknown-linux-gnu/sparc64-unknown-linux-gnu/include --with-local-prefix=/opt/crosstool/gcc-3.4.5-glibc-2.3.6/sparc64-unknown-linux-gnu/sparc64-unknown-linux-gnu --disable-nls --enable-threads=posix --enable-symvers=gnu --enable-__cxa_atexit --enable-languages=c --enable-shared --enable-c99 --enable-long-long
> > Thread model: posix
> > gcc version 3.4.5
> 
> I'm using a native gcc-4.1.3 here:
> 
> gcc (GCC) 4.1.3 20070929 (prerelease) (Ubuntu 4.1.2-16ubuntu2)
> 
> When you cross build 32-bit to 64-bit, a small but non-zero
> number of warning checks and optimizations are not performed
> and I think that is what you're hitting here.

OK.
I have ordered a used Sun Blade 100(*) that should show up
before new eve.
So when I have it and it is up and running I will redo this
patch set. That will likely take a few weeks though.

(*) Any distribution recommendations? I will only use it for
    sparc kernel development so nothing fancy...

	Sam

^ permalink raw reply

* Re: [BUG] next-20081216 - WARNING: at kernel/smp.c:333 smp_call_function_mask
From: David Howells @ 2008-12-27  9:46 UTC (permalink / raw)
  To: Sam Ravnborg
  Cc: dhowells, Yinghai Lu, Kamalesh Babulal, mingo, Andrew Morton,
	Stephen Rothwell, linux-next, LKML, mel
In-Reply-To: <20081226102716.GA31450@uranus.ravnborg.org>

Sam Ravnborg <sam@ravnborg.org> wrote:

> I recall David Howells had a similar issue with the bootparamter patch set.
> The workaround he used was to add a barrier(); call in the weak function
> to avoid the inline.

Yes I did.  Rusty's solution was just to move the default weak functions into
different files.

Attempting to use the noinline attribute does not work.

Of course, you could just stick the functions into lib/ in separate .c files
and dispense with the weak attribute altogether.

David

^ permalink raw reply

* Re: [PATCH] sparc64: use unsigned long long for u64
From: David Miller @ 2008-12-27  9:37 UTC (permalink / raw)
  To: sam
  Cc: akpm, sparclinux, mingo, sfr, linux-kernel, kenchen, paulus, tglx,
	hpa, linux-next
In-Reply-To: <20081227092404.GA10375@uranus.ravnborg.org>

From: Sam Ravnborg <sam@ravnborg.org>
Date: Sat, 27 Dec 2008 10:24:04 +0100

> So it looks like my gcc does not promote:
> ((tagval & 0x0fffffff00000000UL) >> 19UL) to unsigned long long int
> but your gcc does?
> 
> My gcc (build with Dan Kegel's crosstool):
> $ /opt/crosstool/gcc-3.4.5-glibc-2.3.6/sparc64-unknown-linux-gnu/bin/sparc64-unknown-linux-gnu-gcc -v
> Reading specs from /opt/crosstool/gcc-3.4.5-glibc-2.3.6/sparc64-unknown-linux-gnu/lib/gcc/sparc64-unknown-linux-gnu/3.4.5/specs
> Configured with: /home/sam/devel/crosstool-0.43/build/sparc64-unknown-linux-gnu/gcc-3.4.5-glibc-2.3.6/gcc-3.4.5/configure --target=sparc64-unknown-linux-gnu --host=i686-host_pc-linux-gnu --prefix=/opt/crosstool/gcc-3.4.5-glibc-2.3.6/sparc64-unknown-linux-gnu --disable-multilib --with-cpu=ultrasparc3 --enable-cxx-flags=-mcpu=ultrasparc3 --with-headers=/opt/crosstool/gcc-3.4.5-glibc-2.3.6/sparc64-unknown-linux-gnu/sparc64-unknown-linux-gnu/include --with-local-prefix=/opt/crosstool/gcc-3.4.5-glibc-2.3.6/sparc64-unknown-linux-gnu/sparc64-unknown-linux-gnu --disable-nls --enable-threads=posix --enable-symvers=gnu --enable-__cxa_atexit --enable-languages=c --enable-shared --enable-c99 --enable-long-long
> Thread model: posix
> gcc version 3.4.5

I'm using a native gcc-4.1.3 here:

gcc (GCC) 4.1.3 20070929 (prerelease) (Ubuntu 4.1.2-16ubuntu2)

When you cross build 32-bit to 64-bit, a small but non-zero
number of warning checks and optimizations are not performed
and I think that is what you're hitting here.

^ permalink raw reply

* Re: [PATCH] sparc64: use unsigned long long for u64
From: Sam Ravnborg @ 2008-12-27  9:24 UTC (permalink / raw)
  To: David Miller
  Cc: akpm, sparclinux, mingo, sfr, linux-kernel, kenchen, paulus, tglx,
	hpa, linux-next
In-Reply-To: <20081227.005452.261413194.davem@davemloft.net>

On Sat, Dec 27, 2008 at 12:54:52AM -0800, David Miller wrote:
> From: Sam Ravnborg <sam@ravnborg.org>
> Date: Tue, 23 Dec 2008 14:17:46 +0100
> 
> > It may take a few days before the drivers gets fixed.
> > Christmas is approaching fast by now.
> 
> Even with this and your driver patch applied, the tree
> doesn't build successfully:
> 
> cc1: warnings being treated as errors
> arch/sparc/kernel/psycho_common.c: In function ‘psycho_check_stc_error’:
> arch/sparc/kernel/psycho_common.c:104: warning: format ‘%016lx’ expects type ‘long unsigned int’, but argument 4 has type ‘long long unsigned int’
> 
> Did you remove sparc's -Werror when doing test builds
> of these changes? :-)
No - it is worse than that.
My gcc does not complain about the above.

Just to check that I was not fouled by some preprocessor magic
here is the preprocessed code:

   printk("<3>" "%s: STC_TAG(%d)[PA(%016lx)VA(%08lx)"
          "V(%d)W(%d)]\n",
          pbm->name,
          i,
          ((tagval & 0x0fffffff00000000UL) >> 19UL),
          (tagval & 0x00000000ffffe000UL),
          ((tagval & 0x0000000000000002UL) ? 1 : 0),
          ((tagval & 0x0000000000000001UL) ? 1 : 0));


tagval is u64.

So it looks like my gcc does not promote:
((tagval & 0x0fffffff00000000UL) >> 19UL) to unsigned long long int
but your gcc does?

My gcc (build with Dan Kegel's crosstool):
$ /opt/crosstool/gcc-3.4.5-glibc-2.3.6/sparc64-unknown-linux-gnu/bin/sparc64-unknown-linux-gnu-gcc -v
Reading specs from /opt/crosstool/gcc-3.4.5-glibc-2.3.6/sparc64-unknown-linux-gnu/lib/gcc/sparc64-unknown-linux-gnu/3.4.5/specs
Configured with: /home/sam/devel/crosstool-0.43/build/sparc64-unknown-linux-gnu/gcc-3.4.5-glibc-2.3.6/gcc-3.4.5/configure --target=sparc64-unknown-linux-gnu --host=i686-host_pc-linux-gnu --prefix=/opt/crosstool/gcc-3.4.5-glibc-2.3.6/sparc64-unknown-linux-gnu --disable-multilib --with-cpu=ultrasparc3 --enable-cxx-flags=-mcpu=ultrasparc3 --with-headers=/opt/crosstool/gcc-3.4.5-glibc-2.3.6/sparc64-unknown-linux-gnu/sparc64-unknown-linux-gnu/include --with-local-prefix=/opt/crosstool/gcc-3.4.5-glibc-2.3.6/sparc64-unknown-linux-gnu/sparc64-unknown-linux-gnu --disable-nls --enable-threads=posix --enable-symvers=gnu --enable-__cxa_atexit --enable-languages=c --enable-shared --enable-c99 --enable-long-long
Thread model: posix
gcc version 3.4.5

> 
> Either way these need some work before they should be
> added to any tree.
> 
> Andrew you probably want to toss these from your tree
> since at a minimum they will break the sparc64 build.
> 
> Sam will fix them up and resubmit, and I'll apply them,
> don't worry. :)
Yup - I added Andrew on cc: only as information.

	Sam

^ permalink raw reply

* Re: [PATCH] sparc64: use unsigned long long for u64
From: David Miller @ 2008-12-27  8:54 UTC (permalink / raw)
  To: sam
  Cc: akpm, sparclinux, mingo, sfr, linux-kernel, kenchen, paulus, tglx,
	hpa, linux-next
In-Reply-To: <20081223131746.GA7456@uranus.ravnborg.org>

From: Sam Ravnborg <sam@ravnborg.org>
Date: Tue, 23 Dec 2008 14:17:46 +0100

> It may take a few days before the drivers gets fixed.
> Christmas is approaching fast by now.

Even with this and your driver patch applied, the tree
doesn't build successfully:

cc1: warnings being treated as errors
arch/sparc/kernel/psycho_common.c: In function ‘psycho_check_stc_error’:
arch/sparc/kernel/psycho_common.c:104: warning: format ‘%016lx’ expects type ‘long unsigned int’, but argument 4 has type ‘long long unsigned int’

Did you remove sparc's -Werror when doing test builds
of these changes? :-)

Either way these need some work before they should be
added to any tree.

Andrew you probably want to toss these from your tree
since at a minimum they will break the sparc64 build.

Sam will fix them up and resubmit, and I'll apply them,
don't worry. :)


^ permalink raw reply

* Re: [BUG] next-20081216 - WARNING: at kernel/smp.c:333 smp_call_function_mask
From: Ingo Molnar @ 2008-12-27  8:39 UTC (permalink / raw)
  To: Yinghai Lu
  Cc: Kamalesh Babulal, Sam Ravnborg, David Howells, Andrew Morton,
	Stephen Rothwell, linux-next, LKML, mel
In-Reply-To: <20081227083758.GA12138@elte.hu>


* Ingo Molnar <mingo@elte.hu> wrote:

> * Yinghai Lu <yinghai@kernel.org> wrote:
> 
> > +extern int arch_init_chip_data(struct irq_desc *desc, int cpu);
> 
> when you change a function declaration's return type, you _must_ double 
> check whether you properly updated all instances of that function. In 
> this case you forgot about the io_apic.c instance.

the corrected patch is below.

	Ingo

--------------->
>From d353c528bbb29be434f7688cba9b59781e9151e3 Mon Sep 17 00:00:00 2001
From: Yinghai Lu <yinghai@kernel.org>
Date: Fri, 26 Dec 2008 02:05:47 -0800
Subject: [PATCH] sparseirq: work around compiler optimizing away __weak functions

Impact: fix panic on null pointer with sparseirq

Some GCC versions seem to inline the weak global function,
when that function is empty.

Work it around, by making the functions return a (dummy) integer.

Signed-off-by: Yinghai <yinghai@kernel.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/io_apic.c |    6 ++++--
 include/linux/irq.h       |    6 +++---
 init/main.c               |    7 ++++---
 kernel/irq/handle.c       |    7 ++++---
 4 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/io_apic.c b/arch/x86/kernel/io_apic.c
index 2fe543f..e4d4d19 100644
--- a/arch/x86/kernel/io_apic.c
+++ b/arch/x86/kernel/io_apic.c
@@ -170,7 +170,7 @@ static struct irq_cfg irq_cfgx[NR_IRQS] = {
 	[15] = { .domain = CPU_MASK_ALL, .vector = IRQ15_VECTOR, },
 };
 
-void __init arch_early_irq_init(void)
+int __init arch_early_irq_init(void)
 {
 	struct irq_cfg *cfg;
 	struct irq_desc *desc;
@@ -184,6 +184,8 @@ void __init arch_early_irq_init(void)
 		desc = irq_to_desc(i);
 		desc->chip_data = &cfg[i];
 	}
+
+	return 0;
 }
 
 #ifdef CONFIG_SPARSE_IRQ
@@ -212,7 +214,7 @@ static struct irq_cfg *get_one_free_irq_cfg(int cpu)
 	return cfg;
 }
 
-void arch_init_chip_data(struct irq_desc *desc, int cpu)
+int arch_init_chip_data(struct irq_desc *desc, int cpu)
 {
 	struct irq_cfg *cfg;
 
diff --git a/include/linux/irq.h b/include/linux/irq.h
index 69da275..0e40af4 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -193,9 +193,9 @@ struct irq_desc {
 	const char		*name;
 } ____cacheline_internodealigned_in_smp;
 
-extern void early_irq_init(void);
-extern void arch_early_irq_init(void);
-extern void arch_init_chip_data(struct irq_desc *desc, int cpu);
+extern int early_irq_init(void);
+extern int arch_early_irq_init(void);
+extern int arch_init_chip_data(struct irq_desc *desc, int cpu);
 extern void arch_init_copy_chip_data(struct irq_desc *old_desc,
 					struct irq_desc *desc, int cpu);
 extern void arch_free_chip_data(struct irq_desc *old_desc, struct irq_desc *desc);
diff --git a/init/main.c b/init/main.c
index c1f999a..b541c9a 100644
--- a/init/main.c
+++ b/init/main.c
@@ -539,13 +539,14 @@ void __init __weak thread_info_cache_init(void)
 {
 }
 
-void __init __weak arch_early_irq_init(void)
+int __init __weak arch_early_irq_init(void)
 {
+	return 0;
 }
 
-void __init __weak early_irq_init(void)
+int __init __weak early_irq_init(void)
 {
-	arch_early_irq_init();
+	return 0;
 }
 
 asmlinkage void __init start_kernel(void)
diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
index 893da67..0bef3ec 100644
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -86,8 +86,9 @@ void init_kstat_irqs(struct irq_desc *desc, int cpu, int nr)
 		desc->kstat_irqs = (unsigned int *)ptr;
 }
 
-void __attribute__((weak)) arch_init_chip_data(struct irq_desc *desc, int cpu)
+int __weak arch_init_chip_data(struct irq_desc *desc, int cpu)
 {
+	return 0;
 }
 
 static void init_one_irq_desc(int irq, struct irq_desc *desc, int cpu)
@@ -132,7 +133,7 @@ static struct irq_desc irq_desc_legacy[NR_IRQS_LEGACY] __cacheline_aligned_in_sm
 /* FIXME: use bootmem alloc ...*/
 static unsigned int kstat_irqs_legacy[NR_IRQS_LEGACY][NR_CPUS];
 
-void __init early_irq_init(void)
+int __init early_irq_init(void)
 {
 	struct irq_desc *desc;
 	int legacy_count;
@@ -151,7 +152,7 @@ void __init early_irq_init(void)
 	for (i = legacy_count; i < NR_IRQS; i++)
 		irq_desc_ptrs[i] = NULL;
 
-	arch_early_irq_init();
+	return arch_early_irq_init();
 }
 
 struct irq_desc *irq_to_desc(unsigned int irq)

^ permalink raw reply related

* Re: [BUG] next-20081216 - WARNING: at kernel/smp.c:333 smp_call_function_mask
From: Ingo Molnar @ 2008-12-27  8:37 UTC (permalink / raw)
  To: Yinghai Lu
  Cc: Kamalesh Babulal, Sam Ravnborg, David Howells, Andrew Morton,
	Stephen Rothwell, linux-next, LKML, mel
In-Reply-To: <4955DBBC.4070508@kernel.org>


* Yinghai Lu <yinghai@kernel.org> wrote:

> +extern int arch_init_chip_data(struct irq_desc *desc, int cpu);

when you change a function declaration's return type, you _must_ double 
check whether you properly updated all instances of that function. In this 
case you forgot about the io_apic.c instance.

	Ingo

^ permalink raw reply

* Re: [PATCH] kgdb: more barrier in blank weak function
From: Andrew Morton @ 2008-12-27  8:34 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Yinghai Lu, Sam Ravnborg, David Howells, Kamalesh Babulal,
	Stephen Rothwell, linux-next, LKML, mel
In-Reply-To: <20081227081737.GA26218@elte.hu>

On Sat, 27 Dec 2008 09:17:37 +0100 Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Yinghai Lu <yinghai@kernel.org> wrote:
> 
> > Impact: fix panic possible panic etc
> > 
> > some compiler seems to inline the weak global function if no line in it
> 
> >  kgdb_post_primary_code(struct pt_regs *regs, int e_vector, int err_code)
> >  {
> > -	return;
> > +	barrier();
> >  }
> 
> >  void __weak kgdb_disable_hw_debug(struct pt_regs *regs)
> >  {
> > +	barrier();
> >  }
> 
> seems like the barrier() is not enough - the only safe sequence seems to 
> be to turn it into a function that returns an integer.
> 

Give up, and ban that gcc version?  Adrian had a patch a while back,
but I don't know how he worked out the affected compiler versions.

^ permalink raw reply

* Re: [PATCH] kgdb: more barrier in blank weak function
From: Ingo Molnar @ 2008-12-27  8:17 UTC (permalink / raw)
  To: Yinghai Lu
  Cc: Sam Ravnborg, David Howells, Kamalesh Babulal, Andrew Morton,
	Stephen Rothwell, linux-next, LKML, mel
In-Reply-To: <4955B7EC.7050002@kernel.org>


* Yinghai Lu <yinghai@kernel.org> wrote:

> Impact: fix panic possible panic etc
> 
> some compiler seems to inline the weak global function if no line in it

>  kgdb_post_primary_code(struct pt_regs *regs, int e_vector, int err_code)
>  {
> -	return;
> +	barrier();
>  }

>  void __weak kgdb_disable_hw_debug(struct pt_regs *regs)
>  {
> +	barrier();
>  }

seems like the barrier() is not enough - the only safe sequence seems to 
be to turn it into a function that returns an integer.

	Ingo

^ permalink raw reply

* Re: [BUG] next-20081216 - WARNING: at kernel/smp.c:333 smp_call_function_mask
From: Yinghai Lu @ 2008-12-27  7:39 UTC (permalink / raw)
  To: Kamalesh Babulal
  Cc: Ingo Molnar, Sam Ravnborg, David Howells, Andrew Morton,
	Stephen Rothwell, linux-next, LKML, mel
In-Reply-To: <20081227071138.GA4530@linux.vnet.ibm.com>

Kamalesh Babulal wrote:
> * Ingo Molnar <mingo@elte.hu> [2008-12-26 19:17:34]:
> 
>> * Kamalesh Babulal <kamalesh@linux.vnet.ibm.com> wrote:
>>
>>>> how about the patch below, does that work too? (after removing 
>>>> Yinghai's patch)
>>> Hi Ingo,
>>>
>>> 	Kernel crashes with the same trace, after the applying the patch.
>> okay, thanks - does the below cleaner variant of Yinghai's patch solve the 
>> problem too?
>>
>> 	Ingo
>>
>> -------------------->
>> From 9c93fc72ec95dc97d65b840978f9d605f4e5823a Mon Sep 17 00:00:00 2001
>> From: Yinghai Lu <yinghai@kernel.org>
>> Date: Fri, 26 Dec 2008 02:05:47 -0800
>> Subject: [PATCH] sparseirq: work around compiler optimizing away __weak functions
>>
>> Impact: fix panic on null pointer with sparseirq
>>
>> Some GCC versions seem to inline the weak global function,
>> when that function is empty.
>>
>> Work it around, by putting a barrier into the function.
>>
>> Signed-off-by: Yinghai <yinghai@kernel.org>
>> Signed-off-by: Ingo Molnar <mingo@elte.hu>
>> ---
>>  init/main.c         |    9 +++++++++
>>  kernel/irq/handle.c |    3 ++-
>>  2 files changed, 11 insertions(+), 1 deletions(-)
>>
>> diff --git a/init/main.c b/init/main.c
>> index c1f999a..841d2b2 100644
>> --- a/init/main.c
>> +++ b/init/main.c
>> @@ -531,20 +531,29 @@ static void __init boot_cpu_init(void)
>>  	cpu_set(cpu, cpu_possible_map);
>>  }
>>
>> +/*
>> + * The barriers are needed to keep a GCC bug from optimizing
>> + * away these weak functions:
>> + */
>> +
>>  void __init __weak smp_setup_processor_id(void)
>>  {
>> +	barrier();
>>  }
>>
>>  void __init __weak thread_info_cache_init(void)
>>  {
>> +	barrier();
>>  }
>>
>>  void __init __weak arch_early_irq_init(void)
>>  {
>> +	barrier();
>>  }
>>
>>  void __init __weak early_irq_init(void)
>>  {
>> +	barrier();
>>  	arch_early_irq_init();
>>  }
>>
>> diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
>> index 06b05a4..a6d16a4 100644
>> --- a/kernel/irq/handle.c
>> +++ b/kernel/irq/handle.c
>> @@ -86,8 +86,9 @@ void init_kstat_irqs(struct irq_desc *desc, int cpu, int nr)
>>  		desc->kstat_irqs = (unsigned int *)ptr;
>>  }
>>
>> -void __attribute__((weak)) arch_init_chip_data(struct irq_desc *desc, int cpu)
>> +void __weak arch_init_chip_data(struct irq_desc *desc, int cpu)
>>  {
>> +	barrier();
>>  }
>>
>>  static void init_one_irq_desc(int irq, struct irq_desc *desc, int cpu)
>> --
> 
> Hi Ingo,
> 
> 	Kernel still crashes after applying the patch,

please try it again...

diff --git a/arch/x86/kernel/io_apic.c b/arch/x86/kernel/io_apic.c
index 0fb91df..1f164a1 100644
--- a/arch/x86/kernel/io_apic.c
+++ b/arch/x86/kernel/io_apic.c
@@ -170,7 +170,7 @@ static struct irq_cfg irq_cfgx[NR_IRQS] = {
 	[15] = { .vector = IRQ15_VECTOR, },
 };
 
-void __init arch_early_irq_init(void)
+int __init arch_early_irq_init(void)
 {
 	struct irq_cfg *cfg;
 	struct irq_desc *desc;
@@ -188,6 +188,8 @@ void __init arch_early_irq_init(void)
 		if (i < NR_IRQS_LEGACY)
 			cpumask_setall(cfg[i].domain);
 	}
+
+	return 0;
 }
 
 #ifdef CONFIG_SPARSE_IRQ
diff --git a/include/linux/irq.h b/include/linux/irq.h
index 7bac2d4..8337e58 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -194,9 +194,9 @@ struct irq_desc {
 	const char		*name;
 } ____cacheline_internodealigned_in_smp;
 
-extern void early_irq_init(void);
-extern void arch_early_irq_init(void);
-extern void arch_init_chip_data(struct irq_desc *desc, int cpu);
+extern int early_irq_init(void);
+extern int arch_early_irq_init(void);
+extern int arch_init_chip_data(struct irq_desc *desc, int cpu);
 extern void arch_init_copy_chip_data(struct irq_desc *old_desc,
 					struct irq_desc *desc, int cpu);
 extern void arch_free_chip_data(struct irq_desc *old_desc, struct irq_desc *desc);
diff --git a/init/main.c b/init/main.c
index 5149dbd..9e2fafd 100644
--- a/init/main.c
+++ b/init/main.c
@@ -549,15 +549,14 @@ void __init __weak thread_info_cache_init(void)
 	barrier();
 }
 
-void __init __weak arch_early_irq_init(void)
+int __init __weak arch_early_irq_init(void)
 {
-	barrier();
+	return 0;
 }
 
-void __init __weak early_irq_init(void)
+int __init __weak early_irq_init(void)
 {
-	barrier();
-	arch_early_irq_init();
+	return arch_early_irq_init();
 }
 
 asmlinkage void __init start_kernel(void)
diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
index 925e36c..0bef3ec 100644
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -86,9 +86,9 @@ void init_kstat_irqs(struct irq_desc *desc, int cpu, int nr)
 		desc->kstat_irqs = (unsigned int *)ptr;
 }
 
-void __weak arch_init_chip_data(struct irq_desc *desc, int cpu)
+int __weak arch_init_chip_data(struct irq_desc *desc, int cpu)
 {
-	barrier();
+	return 0;
 }
 
 static void init_one_irq_desc(int irq, struct irq_desc *desc, int cpu)
@@ -133,7 +133,7 @@ static struct irq_desc irq_desc_legacy[NR_IRQS_LEGACY] __cacheline_aligned_in_sm
 /* FIXME: use bootmem alloc ...*/
 static unsigned int kstat_irqs_legacy[NR_IRQS_LEGACY][NR_CPUS];
 
-void __init early_irq_init(void)
+int __init early_irq_init(void)
 {
 	struct irq_desc *desc;
 	int legacy_count;
@@ -152,7 +152,7 @@ void __init early_irq_init(void)
 	for (i = legacy_count; i < NR_IRQS; i++)
 		irq_desc_ptrs[i] = NULL;
 
-	arch_early_irq_init();
+	return arch_early_irq_init();
 }
 
 struct irq_desc *irq_to_desc(unsigned int irq)

^ permalink raw reply related

* Re: [BUG] next-20081216 - WARNING: at kernel/smp.c:333 smp_call_function_mask
From: Yinghai Lu @ 2008-12-27  7:14 UTC (permalink / raw)
  To: Kamalesh Babulal
  Cc: Ingo Molnar, Sam Ravnborg, David Howells, Andrew Morton,
	Stephen Rothwell, linux-next, LKML, mel
In-Reply-To: <20081227071138.GA4530@linux.vnet.ibm.com>

Kamalesh Babulal wrote:
> * Ingo Molnar <mingo@elte.hu> [2008-12-26 19:17:34]:
> 
>> * Kamalesh Babulal <kamalesh@linux.vnet.ibm.com> wrote:
>>
>>>> how about the patch below, does that work too? (after removing 
>>>> Yinghai's patch)
>>> Hi Ingo,
>>>
>>> 	Kernel crashes with the same trace, after the applying the patch.
>> okay, thanks - does the below cleaner variant of Yinghai's patch solve the 
>> problem too?
>>
>> 	Ingo
>>
>> -------------------->
>> From 9c93fc72ec95dc97d65b840978f9d605f4e5823a Mon Sep 17 00:00:00 2001
>> From: Yinghai Lu <yinghai@kernel.org>
>> Date: Fri, 26 Dec 2008 02:05:47 -0800
>> Subject: [PATCH] sparseirq: work around compiler optimizing away __weak functions
>>
>> Impact: fix panic on null pointer with sparseirq
>>
>> Some GCC versions seem to inline the weak global function,
>> when that function is empty.
>>
>> Work it around, by putting a barrier into the function.
>>
>> Signed-off-by: Yinghai <yinghai@kernel.org>
>> Signed-off-by: Ingo Molnar <mingo@elte.hu>
>> ---
>>  init/main.c         |    9 +++++++++
>>  kernel/irq/handle.c |    3 ++-
>>  2 files changed, 11 insertions(+), 1 deletions(-)
>>
>> diff --git a/init/main.c b/init/main.c
>> index c1f999a..841d2b2 100644
>> --- a/init/main.c
>> +++ b/init/main.c
>> @@ -531,20 +531,29 @@ static void __init boot_cpu_init(void)
>>  	cpu_set(cpu, cpu_possible_map);
>>  }
>>
>> +/*
>> + * The barriers are needed to keep a GCC bug from optimizing
>> + * away these weak functions:
>> + */
>> +
>>  void __init __weak smp_setup_processor_id(void)
>>  {
>> +	barrier();
>>  }
>>
>>  void __init __weak thread_info_cache_init(void)
>>  {
>> +	barrier();
>>  }
>>
>>  void __init __weak arch_early_irq_init(void)
>>  {
>> +	barrier();
>>  }
>>
>>  void __init __weak early_irq_init(void)
>>  {
>> +	barrier();
>>  	arch_early_irq_init();
>>  }
>>
>> diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
>> index 06b05a4..a6d16a4 100644
>> --- a/kernel/irq/handle.c
>> +++ b/kernel/irq/handle.c
>> @@ -86,8 +86,9 @@ void init_kstat_irqs(struct irq_desc *desc, int cpu, int nr)
>>  		desc->kstat_irqs = (unsigned int *)ptr;
>>  }
>>
>> -void __attribute__((weak)) arch_init_chip_data(struct irq_desc *desc, int cpu)
>> +void __weak arch_init_chip_data(struct irq_desc *desc, int cpu)
>>  {
>> +	barrier();
>>  }
>>
>>  static void init_one_irq_desc(int irq, struct irq_desc *desc, int cpu)
>> --
> 
> Hi Ingo,
> 
> 	Kernel still crashes after applying the patch,
> 
> CPU: Physical Processor ID: 0
> CPU: Processor Core ID: 0
> ACPI: Core revision 20080926
> Setting APIC routing to flat
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
> IP: [<ffffffff8021f8fd>] add_pin_to_irq_cpu+0x14/0x73

that is interesting, barrier() doesn't work, we do need that printk?

YH

^ permalink raw reply

* Re: [BUG] next-20081216 - WARNING: at kernel/smp.c:333 smp_call_function_mask
From: Kamalesh Babulal @ 2008-12-27  7:11 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Sam Ravnborg, Yinghai Lu, David Howells, Andrew Morton,
	Stephen Rothwell, linux-next, LKML, mel
In-Reply-To: <20081226181733.GA24439@elte.hu>

* Ingo Molnar <mingo@elte.hu> [2008-12-26 19:17:34]:

> 
> * Kamalesh Babulal <kamalesh@linux.vnet.ibm.com> wrote:
> 
> > > how about the patch below, does that work too? (after removing 
> > > Yinghai's patch)
> > 
> > Hi Ingo,
> > 
> > 	Kernel crashes with the same trace, after the applying the patch.
> 
> okay, thanks - does the below cleaner variant of Yinghai's patch solve the 
> problem too?
> 
> 	Ingo
> 
> -------------------->
> From 9c93fc72ec95dc97d65b840978f9d605f4e5823a Mon Sep 17 00:00:00 2001
> From: Yinghai Lu <yinghai@kernel.org>
> Date: Fri, 26 Dec 2008 02:05:47 -0800
> Subject: [PATCH] sparseirq: work around compiler optimizing away __weak functions
> 
> Impact: fix panic on null pointer with sparseirq
> 
> Some GCC versions seem to inline the weak global function,
> when that function is empty.
> 
> Work it around, by putting a barrier into the function.
> 
> Signed-off-by: Yinghai <yinghai@kernel.org>
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> ---
>  init/main.c         |    9 +++++++++
>  kernel/irq/handle.c |    3 ++-
>  2 files changed, 11 insertions(+), 1 deletions(-)
> 
> diff --git a/init/main.c b/init/main.c
> index c1f999a..841d2b2 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -531,20 +531,29 @@ static void __init boot_cpu_init(void)
>  	cpu_set(cpu, cpu_possible_map);
>  }
> 
> +/*
> + * The barriers are needed to keep a GCC bug from optimizing
> + * away these weak functions:
> + */
> +
>  void __init __weak smp_setup_processor_id(void)
>  {
> +	barrier();
>  }
> 
>  void __init __weak thread_info_cache_init(void)
>  {
> +	barrier();
>  }
> 
>  void __init __weak arch_early_irq_init(void)
>  {
> +	barrier();
>  }
> 
>  void __init __weak early_irq_init(void)
>  {
> +	barrier();
>  	arch_early_irq_init();
>  }
> 
> diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
> index 06b05a4..a6d16a4 100644
> --- a/kernel/irq/handle.c
> +++ b/kernel/irq/handle.c
> @@ -86,8 +86,9 @@ void init_kstat_irqs(struct irq_desc *desc, int cpu, int nr)
>  		desc->kstat_irqs = (unsigned int *)ptr;
>  }
> 
> -void __attribute__((weak)) arch_init_chip_data(struct irq_desc *desc, int cpu)
> +void __weak arch_init_chip_data(struct irq_desc *desc, int cpu)
>  {
> +	barrier();
>  }
> 
>  static void init_one_irq_desc(int irq, struct irq_desc *desc, int cpu)
> --

Hi Ingo,

	Kernel still crashes after applying the patch,

CPU: Physical Processor ID: 0
CPU: Processor Core ID: 0
ACPI: Core revision 20080926
Setting APIC routing to flat
BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
IP: [<ffffffff8021f8fd>] add_pin_to_irq_cpu+0x14/0x73
PGD 0 
Oops: 0000 [#1] SMP 
last sysfs file: 
CPU 0 
Modules linked in:
Pid: 1, comm: swapper Not tainted 2.6.28-rc8-autotest-tip #1
RIP: 0010:[<ffffffff8021f8fd>]  [<ffffffff8021f8fd>] add_pin_to_irq_cpu+0x14/0x73
RSP: 0018:ffff88003f9d7e40  EFLAGS: 00010282
RAX: ffffffff806e7300 RBX: 0000000000000001 RCX: 0000000000000001
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
RBP: 0000000000000000 R08: 0000000000000018 R09: 0000000000000002
R10: 000000000000000f R11: ffffffff8021e74a R12: 0000000000000001
R13: 0000000000000000 R14: 0000000000000002 R15: 0000000000000000
FS:  0000000000000000(0000) GS:ffffffff806f1480(0000) knlGS:0000000000000000
CS:  0010 DS: 0018 ES: 0018 CR0: 000000008005003b
CR2: 0000000000000000 CR3: 0000000000201000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process swapper (pid: 1, threadinfo ffff88003f9d6000, task ffff88003f9d8000)
Stack:
 0000000000000000 0000000000000001 ffffffff806e7300 0000000000000001
 0000000000000001 ffffffff8071819e 0000000000000286 0000000000000286
 0000000000010000 0000000000000206 0000000000000002 0000000000000002
Call Trace:
 [<ffffffff8071819e>] setup_IO_APIC+0x12f/0x720
 [<ffffffff80524bf1>] _spin_lock_irqsave+0x9/0xe
 [<ffffffff8022221b>] get_apic_id+0x0/0x9
 [<ffffffff80715087>] native_smp_prepare_cpus+0x256/0x2f1
 [<ffffffff807077c8>] kernel_init+0x50/0x1eb
 [<ffffffff8020ceda>] child_rip+0xa/0x20
 [<ffffffff80707778>] kernel_init+0x0/0x1eb
 [<ffffffff8020ced0>] child_rip+0x0/0x20
Code: 48 c7 c7 9a 29 5f 80 31 c0 e8 83 85 01 00 5b 4c 89 e0 41 5c 41 5d c3 41 55 49 89 fd 89 f7 41 54 41 89 cc 55 89 d5 53 48 83 ec 08 <49> 8b 5d 00 48 85 db 75 35 e8 88 ff ff ff 48 85 c0 75 18 5b 5b 
RIP  [<ffffffff8021f8fd>] add_pin_to_irq_cpu+0x14/0x73
 RSP <ffff88003f9d7e40>
CR2: 0000000000000000
---[ end trace 4eaa2a86a8e2da22 ]---
Kernel panic - not syncing: Attempted to kill init!
Pid: 1, comm: swapper Tainted: G      D    2.6.28-rc8-autotest-tip #1
Call Trace:
 [<ffffffff80237321>] panic+0x86/0x144
 [<ffffffff80236bc8>] mm_release+0x3f/0xd3
 [<ffffffff80238b4f>] exit_fs+0x35/0x46
 [<ffffffff80239fd7>] do_exit+0x75/0x78f
 [<ffffffff805257ae>] oops_end+0xa8/0xad
 [<ffffffff80526ffd>] do_page_fault+0x756/0x80f
 [<ffffffff80524dbf>] page_fault+0x1f/0x30
 [<ffffffff8021e74a>] native_apic_mem_write+0x0/0x9
 [<ffffffff8021f8fd>] add_pin_to_irq_cpu+0x14/0x73
 [<ffffffff8071819e>] setup_IO_APIC+0x12f/0x720
 [<ffffffff80524bf1>] _spin_lock_irqsave+0x9/0xe
 [<ffffffff8022221b>] get_apic_id+0x0/0x9
 [<ffffffff80715087>] native_smp_prepare_cpus+0x256/0x2f1
 [<ffffffff807077c8>] kernel_init+0x50/0x1eb
 [<ffffffff8020ceda>] child_rip+0xa/0x20
 [<ffffffff80707778>] kernel_init+0x0/0x1eb
 [<ffffffff8020ced0>] child_rip+0x0/0x20
-- 
Thanks & Regards,
Kamalesh Babulal,
Linux Technology Center,
IBM, ISTL.

^ permalink raw reply

* Re: [PATCH] perf_counter: more barrier in blank weak function
From: KOSAKI Motohiro @ 2008-12-27  6:07 UTC (permalink / raw)
  To: Yinghai Lu
  Cc: kosaki.motohiro, Sam Ravnborg, David Howells, mingo,
	Kamalesh Babulal, Andrew Morton, Stephen Rothwell, linux-next,
	LKML, mel
In-Reply-To: <4955B782.3030407@kernel.org>

> 
> Impact: fix panic possible panic etc
> 
> some compiler seems to inline the weak global function if no line in it
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> 
> diff --git a/kernel/perf_counter.c b/kernel/perf_counter.c
> index d7a79f3..37f7716 100644
> --- a/kernel/perf_counter.c
> +++ b/kernel/perf_counter.c
> @@ -45,8 +45,8 @@ hw_perf_counter_init(struct perf_counter *counter)
>  }
>  
>  u64 __weak hw_perf_save_disable(void)		{ return 0; }
> -void __weak hw_perf_restore(u64 ctrl)		{ }
> -void __weak hw_perf_counter_setup(void)		{ }
> +void __weak hw_perf_restore(u64 ctrl)		{ barrier(); }
> +void __weak hw_perf_counter_setup(void)		{ barrier(); }

We need comment into the function.
I expect anyone think "hm, this barrier() is meaningless. let's cleanup." in the future.
we should avoid it.

^ permalink raw reply

* [PATCH] kgdb: more barrier in blank weak function
From: Yinghai Lu @ 2008-12-27  5:06 UTC (permalink / raw)
  To: Sam Ravnborg, David Howells, mingo
  Cc: Kamalesh Babulal, Andrew Morton, Stephen Rothwell, linux-next,
	LKML, mel
In-Reply-To: <4955B782.3030407@kernel.org>


Impact: fix panic possible panic etc

some compiler seems to inline the weak global function if no line in it

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

diff --git a/kernel/kgdb.c b/kernel/kgdb.c
index e4dcfb2..70d2e36 100644
--- a/kernel/kgdb.c
+++ b/kernel/kgdb.c
@@ -223,7 +223,7 @@ int __weak kgdb_skipexception(int exception, struct pt_regs *regs)
 void __weak
 kgdb_post_primary_code(struct pt_regs *regs, int e_vector, int err_code)
 {
-	return;
+	barrier();
 }
 
 /**
@@ -236,6 +236,7 @@ kgdb_post_primary_code(struct pt_regs *regs, int e_vector, int err_code)
  */
 void __weak kgdb_disable_hw_debug(struct pt_regs *regs)
 {
+	barrier();
 }
 
 /*

^ permalink raw reply related

* [PATCH] perf_counter: more barrier in blank weak function
From: Yinghai Lu @ 2008-12-27  5:05 UTC (permalink / raw)
  To: Sam Ravnborg, David Howells, mingo
  Cc: Kamalesh Babulal, Andrew Morton, Stephen Rothwell, linux-next,
	LKML, mel
In-Reply-To: <20081226102716.GA31450@uranus.ravnborg.org>


Impact: fix panic possible panic etc

some compiler seems to inline the weak global function if no line in it

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

diff --git a/kernel/perf_counter.c b/kernel/perf_counter.c
index d7a79f3..37f7716 100644
--- a/kernel/perf_counter.c
+++ b/kernel/perf_counter.c
@@ -45,8 +45,8 @@ hw_perf_counter_init(struct perf_counter *counter)
 }
 
 u64 __weak hw_perf_save_disable(void)		{ return 0; }
-void __weak hw_perf_restore(u64 ctrl)		{ }
-void __weak hw_perf_counter_setup(void)		{ }
+void __weak hw_perf_restore(u64 ctrl)		{ barrier(); }
+void __weak hw_perf_counter_setup(void)		{ barrier(); }
 
 static void
 list_add_counter(struct perf_counter *counter, struct perf_counter_context *ctx)

^ permalink raw reply related

* Re: [PATCH] kbuild, sparseirq: work around GCC bug with __weak aliases
From: Paul Mackerras @ 2008-12-27  4:09 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Sam Ravnborg, Yinghai Lu, David Howells, Kamalesh Babulal,
	Andrew Morton, Stephen Rothwell, linux-next, LKML, mel
In-Reply-To: <20081226134218.GA1054@elte.hu>

Ingo Molnar writes:

> * Ingo Molnar <mingo@elte.hu> wrote:
> 
> > > I recall David Howells had a similar issue with the bootparamter patch 
> > > set. The workaround he used was to add a barrier(); call in the weak 
> > > function to avoid the inline.
> > 
> > could we add some extra attribute to __weak that would have a similar 
> > effect? Something like __attribute__((noinline)), or something silly 
> > like __attribute__((deprecated)) - just to keep gcc from screwing up 
> > __weak functions? Perhaps adding a section attribute would have a 
> > similar effect? (putting weak definitions into an extra section is 
> > probably helpful anyway)
> 
> I've applied the patch below to tip/irq/sparseirq - could someone with an 
> affected GCC version please check whether this solves the crash?

I recall from discussions earlier that noinline doesn't fix the
problem, and I just tested a similar case and verified that adding
noinline doesn't stop some versions of gcc from inlining them.  The
empty weak functions in kernel/perf_counter.c were getting inlined by
the cross-gcc (gcc 4.1.1) I use for compiling powerpc kernels on my
laptop, and adding noinline doesn't help there.

Paul.

^ permalink raw reply

* Re: [BUG] next-20081216 - WARNING: at kernel/smp.c:333 smp_call_function_mask
From: Ingo Molnar @ 2008-12-26 18:17 UTC (permalink / raw)
  To: Kamalesh Babulal
  Cc: Sam Ravnborg, Yinghai Lu, David Howells, Andrew Morton,
	Stephen Rothwell, linux-next, LKML, mel
In-Reply-To: <20081226170452.GC5100@linux.vnet.ibm.com>


* Kamalesh Babulal <kamalesh@linux.vnet.ibm.com> wrote:

> > how about the patch below, does that work too? (after removing 
> > Yinghai's patch)
> 
> Hi Ingo,
> 
> 	Kernel crashes with the same trace, after the applying the patch.

okay, thanks - does the below cleaner variant of Yinghai's patch solve the 
problem too?

	Ingo

-------------------->
>From 9c93fc72ec95dc97d65b840978f9d605f4e5823a Mon Sep 17 00:00:00 2001
From: Yinghai Lu <yinghai@kernel.org>
Date: Fri, 26 Dec 2008 02:05:47 -0800
Subject: [PATCH] sparseirq: work around compiler optimizing away __weak functions

Impact: fix panic on null pointer with sparseirq

Some GCC versions seem to inline the weak global function,
when that function is empty.

Work it around, by putting a barrier into the function.

Signed-off-by: Yinghai <yinghai@kernel.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 init/main.c         |    9 +++++++++
 kernel/irq/handle.c |    3 ++-
 2 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/init/main.c b/init/main.c
index c1f999a..841d2b2 100644
--- a/init/main.c
+++ b/init/main.c
@@ -531,20 +531,29 @@ static void __init boot_cpu_init(void)
 	cpu_set(cpu, cpu_possible_map);
 }
 
+/*
+ * The barriers are needed to keep a GCC bug from optimizing
+ * away these weak functions:
+ */
+
 void __init __weak smp_setup_processor_id(void)
 {
+	barrier();
 }
 
 void __init __weak thread_info_cache_init(void)
 {
+	barrier();
 }
 
 void __init __weak arch_early_irq_init(void)
 {
+	barrier();
 }
 
 void __init __weak early_irq_init(void)
 {
+	barrier();
 	arch_early_irq_init();
 }
 
diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
index 06b05a4..a6d16a4 100644
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -86,8 +86,9 @@ void init_kstat_irqs(struct irq_desc *desc, int cpu, int nr)
 		desc->kstat_irqs = (unsigned int *)ptr;
 }
 
-void __attribute__((weak)) arch_init_chip_data(struct irq_desc *desc, int cpu)
+void __weak arch_init_chip_data(struct irq_desc *desc, int cpu)
 {
+	barrier();
 }
 
 static void init_one_irq_desc(int irq, struct irq_desc *desc, int cpu)

^ permalink raw reply related

* Re: [BUG] next-20081216 - WARNING: at kernel/smp.c:333 smp_call_function_mask
From: Kamalesh Babulal @ 2008-12-26 17:04 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Sam Ravnborg, Yinghai Lu, David Howells, Andrew Morton,
	Stephen Rothwell, linux-next, LKML, mel
In-Reply-To: <20081226153318.GA4514@elte.hu>

* Ingo Molnar <mingo@elte.hu> [2008-12-26 16:33:18]:

> 
> * Kamalesh Babulal <kamalesh@linux.vnet.ibm.com> wrote:
> 
> > * Sam Ravnborg <sam@ravnborg.org> [2008-12-26 11:27:16]:
> > 
> > > On Fri, Dec 26, 2008 at 02:05:47AM -0800, Yinghai Lu wrote:
> > > > Kamalesh Babulal wrote:
> > > > > * Yinghai Lu <yinghai@kernel.org> [2008-12-24 12:34:41]:
> > > > > 
> > > > >> --
> > > > > 
> > > > > After the applying the patch, the kernel panic's with the same backtrace. The
> > > > > box is running Fedora 5 on it.
> > > > > 
> > > > 
> > > > please try...
> > > > Ingo, do we need to switch to use #idef and inline function instead?
> > > 
> > > I recall David Howells had a similar issue with the bootparamter patch set.
> > > The workaround he used was to add a barrier(); call in the weak function
> > > to avoid the inline.
> > > 
> > > [full context kept as I added David to to: list].
> > > 	Sam
> > > 
> > 
> > Hi Yinghai,
> > 
> > 	Thanks, after applying the patch, the kernel boot's up fine.
> 
> how about the patch below, does that work too? (after removing Yinghai's 
> patch)
> 
> 	Ingo


Hi Ingo,

	Kernel crashes with the same trace, after the applying the patch.


> 
> ------------->
> From 0d41e654c877a0de441448ca2f63a72ed11c5d84 Mon Sep 17 00:00:00 2001
> From: Ingo Molnar <mingo@elte.hu>
> Date: Fri, 26 Dec 2008 14:39:51 +0100
> Subject: [PATCH] kbuild, sparseirq: work around GCC bug with __weak aliases
> 
> Impact: fix panic on null pointer with sparseirq
> 
> Certain versions of GCC inline the weak global functions.
> Work it around by marking __weak functions as noinline.
> 
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> ---
>  include/linux/compiler-gcc.h |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
> index 5c8351b..1ad72e6 100644
> --- a/include/linux/compiler-gcc.h
> +++ b/include/linux/compiler-gcc.h
> @@ -41,11 +41,17 @@
> 
>  #define __deprecated			__attribute__((deprecated))
>  #define __packed			__attribute__((packed))
> -#define __weak				__attribute__((weak))
>  #define __naked				__attribute__((naked))
>  #define __noreturn			__attribute__((noreturn))
> 
>  /*
> + * Mark weak aliases also noinline, because some GCC versions
> + * incorrectly inline weak aliases (even when a non-weak variant
> + * is there) and thus cause crashes:
> + */
> +#define __weak				__attribute__((weak)) noinline
> +
> +/*
>   * From the GCC manual:
>   *
>   * Many functions have no effects except the return value and their
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Thanks & Regards,
Kamalesh Babulal,
Linux Technology Center,
IBM, ISTL.

^ permalink raw reply

* Re: [BUG] next-20081216 - WARNING: at kernel/smp.c:333 smp_call_function_mask
From: Ingo Molnar @ 2008-12-26 15:33 UTC (permalink / raw)
  To: Kamalesh Babulal
  Cc: Sam Ravnborg, Yinghai Lu, David Howells, Andrew Morton,
	Stephen Rothwell, linux-next, LKML, mel
In-Reply-To: <20081226152804.GB5100@linux.vnet.ibm.com>


* Kamalesh Babulal <kamalesh@linux.vnet.ibm.com> wrote:

> * Sam Ravnborg <sam@ravnborg.org> [2008-12-26 11:27:16]:
> 
> > On Fri, Dec 26, 2008 at 02:05:47AM -0800, Yinghai Lu wrote:
> > > Kamalesh Babulal wrote:
> > > > * Yinghai Lu <yinghai@kernel.org> [2008-12-24 12:34:41]:
> > > > 
> > > >> --
> > > > 
> > > > After the applying the patch, the kernel panic's with the same backtrace. The
> > > > box is running Fedora 5 on it.
> > > > 
> > > 
> > > please try...
> > > Ingo, do we need to switch to use #idef and inline function instead?
> > 
> > I recall David Howells had a similar issue with the bootparamter patch set.
> > The workaround he used was to add a barrier(); call in the weak function
> > to avoid the inline.
> > 
> > [full context kept as I added David to to: list].
> > 	Sam
> > 
> 
> Hi Yinghai,
> 
> 	Thanks, after applying the patch, the kernel boot's up fine.

how about the patch below, does that work too? (after removing Yinghai's 
patch)

	Ingo

------------->
>From 0d41e654c877a0de441448ca2f63a72ed11c5d84 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@elte.hu>
Date: Fri, 26 Dec 2008 14:39:51 +0100
Subject: [PATCH] kbuild, sparseirq: work around GCC bug with __weak aliases

Impact: fix panic on null pointer with sparseirq

Certain versions of GCC inline the weak global functions.
Work it around by marking __weak functions as noinline.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 include/linux/compiler-gcc.h |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index 5c8351b..1ad72e6 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -41,11 +41,17 @@
 
 #define __deprecated			__attribute__((deprecated))
 #define __packed			__attribute__((packed))
-#define __weak				__attribute__((weak))
 #define __naked				__attribute__((naked))
 #define __noreturn			__attribute__((noreturn))
 
 /*
+ * Mark weak aliases also noinline, because some GCC versions
+ * incorrectly inline weak aliases (even when a non-weak variant
+ * is there) and thus cause crashes:
+ */
+#define __weak				__attribute__((weak)) noinline
+
+/*
  * From the GCC manual:
  *
  * Many functions have no effects except the return value and their

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox