* Please pull 'next' branch of 4xx tree
From: Josh Boyer @ 2012-05-02 13:51 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev
Hi Ben,
A few patches from Suzie for 47x kexec/kdump support, and some MSI patches
from Mai La.
josh
The following changes since commit ec34a6814988f17506733c1e8b058ce46602591d:
powerpc: Remove old powerpc specific ptrace getregs/setregs calls
(2012-04-30 15:37:28 +1000)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/jwboyer/powerpc-4xx.git next
for you to fetch changes up to 9c6b2353dfb80ae843b831c03fc53ddc5c3949ff:
powerpc/44x: Add PCI MSI node for Maui APM821xx SoC and Bluestone
board in DTS (2012-05-03 08:58:21 -0400)
----------------------------------------------------------------
Mai La (2):
powerpc/44x: Fix PCI MSI support for Maui APM821xx SoC and Bluestone board
powerpc/44x: Add PCI MSI node for Maui APM821xx SoC and
Bluestone board in DTS
Suzuki Poulose (3):
powerpc/44x: Fix/Initialize PID to kernel PID before the TLB search
powerpc/47x: Kernel support for KEXEC
powerpc/47x: Enable CRASH_DUMP
arch/powerpc/Kconfig | 4 +-
arch/powerpc/boot/dts/bluestone.dts | 25 +++++
arch/powerpc/kernel/misc_32.S | 203 +++++++++++++++++++++++++++++++++--
arch/powerpc/platforms/44x/Kconfig | 2 +
arch/powerpc/sysdev/ppc4xx_msi.c | 42 +++++---
5 files changed, 252 insertions(+), 24 deletions(-)
^ permalink raw reply
* Re: linux-next ppc64: RCU mods cause __might_sleep BUGs
From: Hugh Dickins @ 2012-05-02 20:25 UTC (permalink / raw)
To: Paul E. McKenney; +Cc: linuxppc-dev, linux-kernel, Paul E. McKenney
In-Reply-To: <20120501232516.GR2441@linux.vnet.ibm.com>
On Tue, 1 May 2012, Paul E. McKenney wrote:
> > > > > On Mon, 2012-04-30 at 15:37 -0700, Hugh Dickins wrote:
> > > > > >
> > > > > > BUG: sleeping function called from invalid context at include/linux/pagemap.h:354
> > > > > > in_atomic(): 0, irqs_disabled(): 0, pid: 6886, name: cc1
> > > > > > Call Trace:
> > > > > > [c0000001a99f78e0] [c00000000000f34c] .show_stack+0x6c/0x16c (unreliable)
> > > > > > [c0000001a99f7990] [c000000000077b40] .__might_sleep+0x11c/0x134
> > > > > > [c0000001a99f7a10] [c0000000000c6228] .filemap_fault+0x1fc/0x494
> > > > > > [c0000001a99f7af0] [c0000000000e7c9c] .__do_fault+0x120/0x684
> > > > > > [c0000001a99f7c00] [c000000000025790] .do_page_fault+0x458/0x664
> > > > > > [c0000001a99f7e30] [c000000000005868] handle_page_fault+0x10/0x30
Got it at last. Embarrassingly obvious. __rcu_read_lock() and
__rcu_read_unlock() are not safe to be using __this_cpu operations,
the cpu may change in between the rmw's read and write: they should
be using this_cpu operations (or, I put preempt_disable/enable in the
__rcu_read_unlock below). __this_cpus there work out fine on x86,
which was given good instructions to use; but not so well on PowerPC.
I've been running successfully for an hour now with the patch below;
but I expect you'll want to consider the tradeoffs, and may choose a
different solution.
Hugh
--- 3.4-rc4-next-20120427/include/linux/rcupdate.h 2012-04-28 09:26:38.000000000 -0700
+++ testing/include/linux/rcupdate.h 2012-05-02 11:46:06.000000000 -0700
@@ -159,7 +159,7 @@ DECLARE_PER_CPU(struct task_struct *, rc
*/
static inline void __rcu_read_lock(void)
{
- __this_cpu_inc(rcu_read_lock_nesting);
+ this_cpu_inc(rcu_read_lock_nesting);
barrier(); /* Keep code within RCU read-side critical section. */
}
--- 3.4-rc4-next-20120427/kernel/rcupdate.c 2012-04-28 09:26:40.000000000 -0700
+++ testing/kernel/rcupdate.c 2012-05-02 11:44:13.000000000 -0700
@@ -72,6 +72,7 @@ DEFINE_PER_CPU(struct task_struct *, rcu
*/
void __rcu_read_unlock(void)
{
+ preempt_disable();
if (__this_cpu_read(rcu_read_lock_nesting) != 1)
__this_cpu_dec(rcu_read_lock_nesting);
else {
@@ -83,13 +84,14 @@ void __rcu_read_unlock(void)
barrier(); /* ->rcu_read_unlock_special load before assign */
__this_cpu_write(rcu_read_lock_nesting, 0);
}
-#ifdef CONFIG_PROVE_LOCKING
+#if 1 /* CONFIG_PROVE_LOCKING */
{
int rln = __this_cpu_read(rcu_read_lock_nesting);
- WARN_ON_ONCE(rln < 0 && rln > INT_MIN / 2);
+ BUG_ON(rln < 0 && rln > INT_MIN / 2);
}
#endif /* #ifdef CONFIG_PROVE_LOCKING */
+ preempt_enable();
}
EXPORT_SYMBOL_GPL(__rcu_read_unlock);
--- 3.4-rc4-next-20120427/kernel/sched/core.c 2012-04-28 09:26:40.000000000 -0700
+++ testing/kernel/sched/core.c 2012-05-01 22:40:46.000000000 -0700
@@ -2024,7 +2024,7 @@ asmlinkage void schedule_tail(struct tas
{
struct rq *rq = this_rq();
- rcu_switch_from(prev);
+ /* rcu_switch_from(prev); */
rcu_switch_to();
finish_task_switch(rq, prev);
@@ -7093,6 +7093,10 @@ void __might_sleep(const char *file, int
"BUG: sleeping function called from invalid context at %s:%d\n",
file, line);
printk(KERN_ERR
+ "cpu=%d preempt_count=%x preempt_offset=%x rcu_nesting=%x nesting_save=%x\n",
+ raw_smp_processor_id(), preempt_count(), preempt_offset,
+ rcu_preempt_depth(), current->rcu_read_lock_nesting_save);
+ printk(KERN_ERR
"in_atomic(): %d, irqs_disabled(): %d, pid: %d, name: %s\n",
in_atomic(), irqs_disabled(),
current->pid, current->comm);
^ permalink raw reply
* Re: linux-next ppc64: RCU mods cause __might_sleep BUGs
From: Paul E. McKenney @ 2012-05-02 20:49 UTC (permalink / raw)
To: Hugh Dickins; +Cc: linuxppc-dev, linux-kernel, Paul E. McKenney
In-Reply-To: <alpine.LSU.2.00.1205021324430.24246@eggly.anvils>
On Wed, May 02, 2012 at 01:25:30PM -0700, Hugh Dickins wrote:
> On Tue, 1 May 2012, Paul E. McKenney wrote:
> > > > > > On Mon, 2012-04-30 at 15:37 -0700, Hugh Dickins wrote:
> > > > > > >
> > > > > > > BUG: sleeping function called from invalid context at include/linux/pagemap.h:354
> > > > > > > in_atomic(): 0, irqs_disabled(): 0, pid: 6886, name: cc1
> > > > > > > Call Trace:
> > > > > > > [c0000001a99f78e0] [c00000000000f34c] .show_stack+0x6c/0x16c (unreliable)
> > > > > > > [c0000001a99f7990] [c000000000077b40] .__might_sleep+0x11c/0x134
> > > > > > > [c0000001a99f7a10] [c0000000000c6228] .filemap_fault+0x1fc/0x494
> > > > > > > [c0000001a99f7af0] [c0000000000e7c9c] .__do_fault+0x120/0x684
> > > > > > > [c0000001a99f7c00] [c000000000025790] .do_page_fault+0x458/0x664
> > > > > > > [c0000001a99f7e30] [c000000000005868] handle_page_fault+0x10/0x30
>
> Got it at last. Embarrassingly obvious. __rcu_read_lock() and
> __rcu_read_unlock() are not safe to be using __this_cpu operations,
> the cpu may change in between the rmw's read and write: they should
> be using this_cpu operations (or, I put preempt_disable/enable in the
> __rcu_read_unlock below). __this_cpus there work out fine on x86,
> which was given good instructions to use; but not so well on PowerPC.
Thank you very much for tracking this down!!!
> I've been running successfully for an hour now with the patch below;
> but I expect you'll want to consider the tradeoffs, and may choose a
> different solution.
The thing that puzzles me about this is that the normal path through
the scheduler would save and restore these per-CPU variables to and
from the task structure. There must be a sneak path through the
scheduler that I failed to account for.
But given your good work, this should be easy for me to chase down
even on my x86-based laptop -- just convert from __this_cpu_inc() to a
read-inc-delay-write sequence. And check that the underlying variable
didn't change in the meantime. And dump an ftrace if it did. ;-)
Thank you again, Hugh!
Thanx, Paul
> Hugh
>
> --- 3.4-rc4-next-20120427/include/linux/rcupdate.h 2012-04-28 09:26:38.000000000 -0700
> +++ testing/include/linux/rcupdate.h 2012-05-02 11:46:06.000000000 -0700
> @@ -159,7 +159,7 @@ DECLARE_PER_CPU(struct task_struct *, rc
> */
> static inline void __rcu_read_lock(void)
> {
> - __this_cpu_inc(rcu_read_lock_nesting);
> + this_cpu_inc(rcu_read_lock_nesting);
> barrier(); /* Keep code within RCU read-side critical section. */
> }
>
> --- 3.4-rc4-next-20120427/kernel/rcupdate.c 2012-04-28 09:26:40.000000000 -0700
> +++ testing/kernel/rcupdate.c 2012-05-02 11:44:13.000000000 -0700
> @@ -72,6 +72,7 @@ DEFINE_PER_CPU(struct task_struct *, rcu
> */
> void __rcu_read_unlock(void)
> {
> + preempt_disable();
> if (__this_cpu_read(rcu_read_lock_nesting) != 1)
> __this_cpu_dec(rcu_read_lock_nesting);
> else {
> @@ -83,13 +84,14 @@ void __rcu_read_unlock(void)
> barrier(); /* ->rcu_read_unlock_special load before assign */
> __this_cpu_write(rcu_read_lock_nesting, 0);
> }
> -#ifdef CONFIG_PROVE_LOCKING
> +#if 1 /* CONFIG_PROVE_LOCKING */
> {
> int rln = __this_cpu_read(rcu_read_lock_nesting);
>
> - WARN_ON_ONCE(rln < 0 && rln > INT_MIN / 2);
> + BUG_ON(rln < 0 && rln > INT_MIN / 2);
> }
> #endif /* #ifdef CONFIG_PROVE_LOCKING */
> + preempt_enable();
> }
> EXPORT_SYMBOL_GPL(__rcu_read_unlock);
>
> --- 3.4-rc4-next-20120427/kernel/sched/core.c 2012-04-28 09:26:40.000000000 -0700
> +++ testing/kernel/sched/core.c 2012-05-01 22:40:46.000000000 -0700
> @@ -2024,7 +2024,7 @@ asmlinkage void schedule_tail(struct tas
> {
> struct rq *rq = this_rq();
>
> - rcu_switch_from(prev);
> + /* rcu_switch_from(prev); */
> rcu_switch_to();
> finish_task_switch(rq, prev);
>
> @@ -7093,6 +7093,10 @@ void __might_sleep(const char *file, int
> "BUG: sleeping function called from invalid context at %s:%d\n",
> file, line);
> printk(KERN_ERR
> + "cpu=%d preempt_count=%x preempt_offset=%x rcu_nesting=%x nesting_save=%x\n",
> + raw_smp_processor_id(), preempt_count(), preempt_offset,
> + rcu_preempt_depth(), current->rcu_read_lock_nesting_save);
> + printk(KERN_ERR
> "in_atomic(): %d, irqs_disabled(): %d, pid: %d, name: %s\n",
> in_atomic(), irqs_disabled(),
> current->pid, current->comm);
> --
> 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/
>
^ permalink raw reply
* Re: linux-next ppc64: RCU mods cause __might_sleep BUGs
From: Benjamin Herrenschmidt @ 2012-05-02 21:20 UTC (permalink / raw)
To: Hugh Dickins
Cc: Paul E. McKenney, Paul E. McKenney, linuxppc-dev, linux-kernel
In-Reply-To: <alpine.LSU.2.00.1205021324430.24246@eggly.anvils>
On Wed, 2012-05-02 at 13:25 -0700, Hugh Dickins wrote:
> Got it at last. Embarrassingly obvious. __rcu_read_lock() and
> __rcu_read_unlock() are not safe to be using __this_cpu operations,
> the cpu may change in between the rmw's read and write: they should
> be using this_cpu operations (or, I put preempt_disable/enable in the
> __rcu_read_unlock below). __this_cpus there work out fine on x86,
> which was given good instructions to use; but not so well on PowerPC.
>
> I've been running successfully for an hour now with the patch below;
> but I expect you'll want to consider the tradeoffs, and may choose a
> different solution.
Didn't Linus recently rant about these __this_cpu vs this_cpu nonsense ?
I thought that was going out..
Cheers,
Ben.
^ permalink raw reply
* Re: linux-next ppc64: RCU mods cause __might_sleep BUGs
From: Paul E. McKenney @ 2012-05-02 21:32 UTC (permalink / raw)
To: Hugh Dickins; +Cc: linuxppc-dev, linux-kernel, Paul E. McKenney
In-Reply-To: <20120502204954.GK2450@linux.vnet.ibm.com>
On Wed, May 02, 2012 at 01:49:54PM -0700, Paul E. McKenney wrote:
> On Wed, May 02, 2012 at 01:25:30PM -0700, Hugh Dickins wrote:
> > On Tue, 1 May 2012, Paul E. McKenney wrote:
> > > > > > > On Mon, 2012-04-30 at 15:37 -0700, Hugh Dickins wrote:
> > > > > > > >
> > > > > > > > BUG: sleeping function called from invalid context at include/linux/pagemap.h:354
> > > > > > > > in_atomic(): 0, irqs_disabled(): 0, pid: 6886, name: cc1
> > > > > > > > Call Trace:
> > > > > > > > [c0000001a99f78e0] [c00000000000f34c] .show_stack+0x6c/0x16c (unreliable)
> > > > > > > > [c0000001a99f7990] [c000000000077b40] .__might_sleep+0x11c/0x134
> > > > > > > > [c0000001a99f7a10] [c0000000000c6228] .filemap_fault+0x1fc/0x494
> > > > > > > > [c0000001a99f7af0] [c0000000000e7c9c] .__do_fault+0x120/0x684
> > > > > > > > [c0000001a99f7c00] [c000000000025790] .do_page_fault+0x458/0x664
> > > > > > > > [c0000001a99f7e30] [c000000000005868] handle_page_fault+0x10/0x30
> >
> > Got it at last. Embarrassingly obvious. __rcu_read_lock() and
> > __rcu_read_unlock() are not safe to be using __this_cpu operations,
> > the cpu may change in between the rmw's read and write: they should
> > be using this_cpu operations (or, I put preempt_disable/enable in the
> > __rcu_read_unlock below). __this_cpus there work out fine on x86,
> > which was given good instructions to use; but not so well on PowerPC.
>
> Thank you very much for tracking this down!!!
>
> > I've been running successfully for an hour now with the patch below;
> > but I expect you'll want to consider the tradeoffs, and may choose a
> > different solution.
>
> The thing that puzzles me about this is that the normal path through
> the scheduler would save and restore these per-CPU variables to and
> from the task structure. There must be a sneak path through the
> scheduler that I failed to account for.
Sigh... I am slow today, I guess. The preemption could of course
happen between the time that the task calculated the address of the
per-CPU variable and the time that it modified it. If this happens,
we are modifying some other CPU's per-CPU variable.
Given that Linus saw no performance benefit from this patchset, I am
strongly tempted to just drop this inlinable-__rcu_read_lock() series
at this point.
I suppose that the other option is to move preempt_count() to a
per-CPU variable, then use the space in the task_info struct.
But that didn't generate anywhere near as good of code...
Thanx, Paul
> But given your good work, this should be easy for me to chase down
> even on my x86-based laptop -- just convert from __this_cpu_inc() to a
> read-inc-delay-write sequence. And check that the underlying variable
> didn't change in the meantime. And dump an ftrace if it did. ;-)
>
> Thank you again, Hugh!
>
> Thanx, Paul
>
> > Hugh
> >
> > --- 3.4-rc4-next-20120427/include/linux/rcupdate.h 2012-04-28 09:26:38.000000000 -0700
> > +++ testing/include/linux/rcupdate.h 2012-05-02 11:46:06.000000000 -0700
> > @@ -159,7 +159,7 @@ DECLARE_PER_CPU(struct task_struct *, rc
> > */
> > static inline void __rcu_read_lock(void)
> > {
> > - __this_cpu_inc(rcu_read_lock_nesting);
> > + this_cpu_inc(rcu_read_lock_nesting);
> > barrier(); /* Keep code within RCU read-side critical section. */
> > }
> >
> > --- 3.4-rc4-next-20120427/kernel/rcupdate.c 2012-04-28 09:26:40.000000000 -0700
> > +++ testing/kernel/rcupdate.c 2012-05-02 11:44:13.000000000 -0700
> > @@ -72,6 +72,7 @@ DEFINE_PER_CPU(struct task_struct *, rcu
> > */
> > void __rcu_read_unlock(void)
> > {
> > + preempt_disable();
> > if (__this_cpu_read(rcu_read_lock_nesting) != 1)
> > __this_cpu_dec(rcu_read_lock_nesting);
> > else {
> > @@ -83,13 +84,14 @@ void __rcu_read_unlock(void)
> > barrier(); /* ->rcu_read_unlock_special load before assign */
> > __this_cpu_write(rcu_read_lock_nesting, 0);
> > }
> > -#ifdef CONFIG_PROVE_LOCKING
> > +#if 1 /* CONFIG_PROVE_LOCKING */
> > {
> > int rln = __this_cpu_read(rcu_read_lock_nesting);
> >
> > - WARN_ON_ONCE(rln < 0 && rln > INT_MIN / 2);
> > + BUG_ON(rln < 0 && rln > INT_MIN / 2);
> > }
> > #endif /* #ifdef CONFIG_PROVE_LOCKING */
> > + preempt_enable();
> > }
> > EXPORT_SYMBOL_GPL(__rcu_read_unlock);
> >
> > --- 3.4-rc4-next-20120427/kernel/sched/core.c 2012-04-28 09:26:40.000000000 -0700
> > +++ testing/kernel/sched/core.c 2012-05-01 22:40:46.000000000 -0700
> > @@ -2024,7 +2024,7 @@ asmlinkage void schedule_tail(struct tas
> > {
> > struct rq *rq = this_rq();
> >
> > - rcu_switch_from(prev);
> > + /* rcu_switch_from(prev); */
> > rcu_switch_to();
> > finish_task_switch(rq, prev);
> >
> > @@ -7093,6 +7093,10 @@ void __might_sleep(const char *file, int
> > "BUG: sleeping function called from invalid context at %s:%d\n",
> > file, line);
> > printk(KERN_ERR
> > + "cpu=%d preempt_count=%x preempt_offset=%x rcu_nesting=%x nesting_save=%x\n",
> > + raw_smp_processor_id(), preempt_count(), preempt_offset,
> > + rcu_preempt_depth(), current->rcu_read_lock_nesting_save);
> > + printk(KERN_ERR
> > "in_atomic(): %d, irqs_disabled(): %d, pid: %d, name: %s\n",
> > in_atomic(), irqs_disabled(),
> > current->pid, current->comm);
> > --
> > 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/
> >
^ permalink raw reply
* Re: linux-next ppc64: RCU mods cause __might_sleep BUGs
From: Paul E. McKenney @ 2012-05-02 21:36 UTC (permalink / raw)
To: Hugh Dickins; +Cc: linuxppc-dev, linux-kernel, Paul E. McKenney
In-Reply-To: <20120502213238.GA12153@linux.vnet.ibm.com>
On Wed, May 02, 2012 at 02:32:38PM -0700, Paul E. McKenney wrote:
> On Wed, May 02, 2012 at 01:49:54PM -0700, Paul E. McKenney wrote:
> > On Wed, May 02, 2012 at 01:25:30PM -0700, Hugh Dickins wrote:
> > > On Tue, 1 May 2012, Paul E. McKenney wrote:
> > > > > > > > On Mon, 2012-04-30 at 15:37 -0700, Hugh Dickins wrote:
> > > > > > > > >
> > > > > > > > > BUG: sleeping function called from invalid context at include/linux/pagemap.h:354
> > > > > > > > > in_atomic(): 0, irqs_disabled(): 0, pid: 6886, name: cc1
> > > > > > > > > Call Trace:
> > > > > > > > > [c0000001a99f78e0] [c00000000000f34c] .show_stack+0x6c/0x16c (unreliable)
> > > > > > > > > [c0000001a99f7990] [c000000000077b40] .__might_sleep+0x11c/0x134
> > > > > > > > > [c0000001a99f7a10] [c0000000000c6228] .filemap_fault+0x1fc/0x494
> > > > > > > > > [c0000001a99f7af0] [c0000000000e7c9c] .__do_fault+0x120/0x684
> > > > > > > > > [c0000001a99f7c00] [c000000000025790] .do_page_fault+0x458/0x664
> > > > > > > > > [c0000001a99f7e30] [c000000000005868] handle_page_fault+0x10/0x30
> > >
> > > Got it at last. Embarrassingly obvious. __rcu_read_lock() and
> > > __rcu_read_unlock() are not safe to be using __this_cpu operations,
> > > the cpu may change in between the rmw's read and write: they should
> > > be using this_cpu operations (or, I put preempt_disable/enable in the
> > > __rcu_read_unlock below). __this_cpus there work out fine on x86,
> > > which was given good instructions to use; but not so well on PowerPC.
> >
> > Thank you very much for tracking this down!!!
> >
> > > I've been running successfully for an hour now with the patch below;
> > > but I expect you'll want to consider the tradeoffs, and may choose a
> > > different solution.
> >
> > The thing that puzzles me about this is that the normal path through
> > the scheduler would save and restore these per-CPU variables to and
> > from the task structure. There must be a sneak path through the
> > scheduler that I failed to account for.
>
> Sigh... I am slow today, I guess. The preemption could of course
> happen between the time that the task calculated the address of the
> per-CPU variable and the time that it modified it. If this happens,
> we are modifying some other CPU's per-CPU variable.
>
> Given that Linus saw no performance benefit from this patchset, I am
> strongly tempted to just drop this inlinable-__rcu_read_lock() series
> at this point.
>
> I suppose that the other option is to move preempt_count() to a
> per-CPU variable, then use the space in the task_info struct.
> But that didn't generate anywhere near as good of code...
But preempt_count() would suffer exactly the same problem. The address
is calculated, the task moves to some other CPU, and then the task
is messing with some other CPU's preemption counter. Blech.
Thanx, Paul
> > But given your good work, this should be easy for me to chase down
> > even on my x86-based laptop -- just convert from __this_cpu_inc() to a
> > read-inc-delay-write sequence. And check that the underlying variable
> > didn't change in the meantime. And dump an ftrace if it did. ;-)
> >
> > Thank you again, Hugh!
> >
> > Thanx, Paul
> >
> > > Hugh
> > >
> > > --- 3.4-rc4-next-20120427/include/linux/rcupdate.h 2012-04-28 09:26:38.000000000 -0700
> > > +++ testing/include/linux/rcupdate.h 2012-05-02 11:46:06.000000000 -0700
> > > @@ -159,7 +159,7 @@ DECLARE_PER_CPU(struct task_struct *, rc
> > > */
> > > static inline void __rcu_read_lock(void)
> > > {
> > > - __this_cpu_inc(rcu_read_lock_nesting);
> > > + this_cpu_inc(rcu_read_lock_nesting);
> > > barrier(); /* Keep code within RCU read-side critical section. */
> > > }
> > >
> > > --- 3.4-rc4-next-20120427/kernel/rcupdate.c 2012-04-28 09:26:40.000000000 -0700
> > > +++ testing/kernel/rcupdate.c 2012-05-02 11:44:13.000000000 -0700
> > > @@ -72,6 +72,7 @@ DEFINE_PER_CPU(struct task_struct *, rcu
> > > */
> > > void __rcu_read_unlock(void)
> > > {
> > > + preempt_disable();
> > > if (__this_cpu_read(rcu_read_lock_nesting) != 1)
> > > __this_cpu_dec(rcu_read_lock_nesting);
> > > else {
> > > @@ -83,13 +84,14 @@ void __rcu_read_unlock(void)
> > > barrier(); /* ->rcu_read_unlock_special load before assign */
> > > __this_cpu_write(rcu_read_lock_nesting, 0);
> > > }
> > > -#ifdef CONFIG_PROVE_LOCKING
> > > +#if 1 /* CONFIG_PROVE_LOCKING */
> > > {
> > > int rln = __this_cpu_read(rcu_read_lock_nesting);
> > >
> > > - WARN_ON_ONCE(rln < 0 && rln > INT_MIN / 2);
> > > + BUG_ON(rln < 0 && rln > INT_MIN / 2);
> > > }
> > > #endif /* #ifdef CONFIG_PROVE_LOCKING */
> > > + preempt_enable();
> > > }
> > > EXPORT_SYMBOL_GPL(__rcu_read_unlock);
> > >
> > > --- 3.4-rc4-next-20120427/kernel/sched/core.c 2012-04-28 09:26:40.000000000 -0700
> > > +++ testing/kernel/sched/core.c 2012-05-01 22:40:46.000000000 -0700
> > > @@ -2024,7 +2024,7 @@ asmlinkage void schedule_tail(struct tas
> > > {
> > > struct rq *rq = this_rq();
> > >
> > > - rcu_switch_from(prev);
> > > + /* rcu_switch_from(prev); */
> > > rcu_switch_to();
> > > finish_task_switch(rq, prev);
> > >
> > > @@ -7093,6 +7093,10 @@ void __might_sleep(const char *file, int
> > > "BUG: sleeping function called from invalid context at %s:%d\n",
> > > file, line);
> > > printk(KERN_ERR
> > > + "cpu=%d preempt_count=%x preempt_offset=%x rcu_nesting=%x nesting_save=%x\n",
> > > + raw_smp_processor_id(), preempt_count(), preempt_offset,
> > > + rcu_preempt_depth(), current->rcu_read_lock_nesting_save);
> > > + printk(KERN_ERR
> > > "in_atomic(): %d, irqs_disabled(): %d, pid: %d, name: %s\n",
> > > in_atomic(), irqs_disabled(),
> > > current->pid, current->comm);
> > > --
> > > 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/
> > >
^ permalink raw reply
* Re: linux-next ppc64: RCU mods cause __might_sleep BUGs
From: Paul E. McKenney @ 2012-05-02 21:54 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Paul E. McKenney, linuxppc-dev, Hugh Dickins, linux-kernel
In-Reply-To: <1335993615.4088.1.camel@pasglop>
On Thu, May 03, 2012 at 07:20:15AM +1000, Benjamin Herrenschmidt wrote:
> On Wed, 2012-05-02 at 13:25 -0700, Hugh Dickins wrote:
> > Got it at last. Embarrassingly obvious. __rcu_read_lock() and
> > __rcu_read_unlock() are not safe to be using __this_cpu operations,
> > the cpu may change in between the rmw's read and write: they should
> > be using this_cpu operations (or, I put preempt_disable/enable in the
> > __rcu_read_unlock below). __this_cpus there work out fine on x86,
> > which was given good instructions to use; but not so well on PowerPC.
> >
> > I've been running successfully for an hour now with the patch below;
> > but I expect you'll want to consider the tradeoffs, and may choose a
> > different solution.
>
> Didn't Linus recently rant about these __this_cpu vs this_cpu nonsense ?
>
> I thought that was going out..
Linus did rant about __raw_get_cpu_var() because it cannot use the x86
%fs segement overrides a bit more than a month ago. The __this_cpu
stuff is useful if you have preemption disabled -- avoids the extra
layer of preempt_disable().
Or was this a different rant?
Thanx, Paul
^ permalink raw reply
* Re: linux-next ppc64: RCU mods cause __might_sleep BUGs
From: Hugh Dickins @ 2012-05-02 22:54 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linuxppc-dev, Christoph Lameter, linux-kernel, Paul E. McKenney
In-Reply-To: <20120502215406.GL2450@linux.vnet.ibm.com>
On Wed, May 2, 2012 at 2:54 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Thu, May 03, 2012 at 07:20:15AM +1000, Benjamin Herrenschmidt wrote:
>> On Wed, 2012-05-02 at 13:25 -0700, Hugh Dickins wrote:
>> > Got it at last. =C2=A0Embarrassingly obvious. =C2=A0__rcu_read_lock() =
and
>> > __rcu_read_unlock() are not safe to be using __this_cpu operations,
>> > the cpu may change in between the rmw's read and write: they should
>> > be using this_cpu operations (or, I put preempt_disable/enable in the
>> > __rcu_read_unlock below). =C2=A0__this_cpus there work out fine on x86=
,
>> > which was given good instructions to use; but not so well on PowerPC.
>> >
>> > I've been running successfully for an hour now with the patch below;
>> > but I expect you'll want to consider the tradeoffs, and may choose a
>> > different solution.
>>
>> Didn't Linus recently rant about these __this_cpu vs this_cpu nonsense ?
>>
>> I thought that was going out..
>
> Linus did rant about __raw_get_cpu_var() because it cannot use the x86
> %fs segement overrides a bit more than a month ago. =C2=A0The __this_cpu
> stuff is useful if you have preemption disabled -- avoids the extra
> layer of preempt_disable().
>
> Or was this a different rant?
https://lkml.org/lkml/2011/11/29/321
I think it ended up with Christoph removing the more egregious
variants, but this_cpu_that and __this_cpu_the_other remaining.
Hugh
^ permalink raw reply
* Re: linux-next ppc64: RCU mods cause __might_sleep BUGs
From: Paul E. McKenney @ 2012-05-03 0:14 UTC (permalink / raw)
To: Hugh Dickins
Cc: linuxppc-dev, Christoph Lameter, linux-kernel, Paul E. McKenney
In-Reply-To: <CANsGZ6bw4JwMRgUriooDvFB=g-HkpyL8XJa9s47RPfOEPCMcpw@mail.gmail.com>
On Wed, May 02, 2012 at 03:54:24PM -0700, Hugh Dickins wrote:
> On Wed, May 2, 2012 at 2:54 PM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> > On Thu, May 03, 2012 at 07:20:15AM +1000, Benjamin Herrenschmidt wrote:
> >> On Wed, 2012-05-02 at 13:25 -0700, Hugh Dickins wrote:
> >> > Got it at last. Embarrassingly obvious. __rcu_read_lock() and
> >> > __rcu_read_unlock() are not safe to be using __this_cpu operations,
> >> > the cpu may change in between the rmw's read and write: they should
> >> > be using this_cpu operations (or, I put preempt_disable/enable in the
> >> > __rcu_read_unlock below). __this_cpus there work out fine on x86,
> >> > which was given good instructions to use; but not so well on PowerPC.
> >> >
> >> > I've been running successfully for an hour now with the patch below;
> >> > but I expect you'll want to consider the tradeoffs, and may choose a
> >> > different solution.
> >>
> >> Didn't Linus recently rant about these __this_cpu vs this_cpu nonsense ?
> >>
> >> I thought that was going out..
> >
> > Linus did rant about __raw_get_cpu_var() because it cannot use the x86
> > %fs segement overrides a bit more than a month ago. The __this_cpu
> > stuff is useful if you have preemption disabled -- avoids the extra
> > layer of preempt_disable().
> >
> > Or was this a different rant?
>
> https://lkml.org/lkml/2011/11/29/321
>
> I think it ended up with Christoph removing the more egregious
> variants, but this_cpu_that and __this_cpu_the_other remaining.
Ah, thank you for the pointer.
It would be nice to have the CPU transparency of x86 on other
architectures, but from what I can see, that would require dedicating
a register to this purpose -- and even then requires that the arch
have indexed addressing modes. There are some other approaches, for
example, having __this_cpu_that() be located at a special address that
the scheduler treated as implicitly preempt_disable(). Or I suppose
that the arch-specific trap-handling code could fake it. A little
bit messy, but the ability to access a given CPU's per-CPU variable
while running on that CPU does appear to have at least a couple of
uses -- inlining RCU and also making preempt_disable() use per-CPU
variables.
In any case, I must confess that I feel quite silly about my series
of patches. I have reverted them aside from a couple that did useful
optimizations, and they should show up in -next shortly.
Thanx, Paul
^ permalink raw reply
* Re: linux-next ppc64: RCU mods cause __might_sleep BUGs
From: Hugh Dickins @ 2012-05-03 0:24 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linuxppc-dev, Christoph Lameter, linux-kernel, Paul E. McKenney
In-Reply-To: <20120503001433.GO2450@linux.vnet.ibm.com>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 2760 bytes --]
On Wed, 2 May 2012, Paul E. McKenney wrote:
> On Wed, May 02, 2012 at 03:54:24PM -0700, Hugh Dickins wrote:
> > On Wed, May 2, 2012 at 2:54 PM, Paul E. McKenney
> > <paulmck@linux.vnet.ibm.com> wrote:
> > > On Thu, May 03, 2012 at 07:20:15AM +1000, Benjamin Herrenschmidt wrote:
> > >> On Wed, 2012-05-02 at 13:25 -0700, Hugh Dickins wrote:
> > >> > Got it at last. Embarrassingly obvious. __rcu_read_lock() and
> > >> > __rcu_read_unlock() are not safe to be using __this_cpu operations,
> > >> > the cpu may change in between the rmw's read and write: they should
> > >> > be using this_cpu operations (or, I put preempt_disable/enable in the
> > >> > __rcu_read_unlock below). __this_cpus there work out fine on x86,
> > >> > which was given good instructions to use; but not so well on PowerPC.
> > >> >
> > >> > I've been running successfully for an hour now with the patch below;
> > >> > but I expect you'll want to consider the tradeoffs, and may choose a
> > >> > different solution.
> > >>
> > >> Didn't Linus recently rant about these __this_cpu vs this_cpu nonsense ?
> > >>
> > >> I thought that was going out..
> > >
> > > Linus did rant about __raw_get_cpu_var() because it cannot use the x86
> > > %fs segement overrides a bit more than a month ago. The __this_cpu
> > > stuff is useful if you have preemption disabled -- avoids the extra
> > > layer of preempt_disable().
> > >
> > > Or was this a different rant?
> >
> > https://lkml.org/lkml/2011/11/29/321
> >
> > I think it ended up with Christoph removing the more egregious
> > variants, but this_cpu_that and __this_cpu_the_other remaining.
>
> Ah, thank you for the pointer.
>
> It would be nice to have the CPU transparency of x86 on other
> architectures, but from what I can see, that would require dedicating
> a register to this purpose -- and even then requires that the arch
> have indexed addressing modes. There are some other approaches, for
> example, having __this_cpu_that() be located at a special address that
> the scheduler treated as implicitly preempt_disable(). Or I suppose
> that the arch-specific trap-handling code could fake it. A little
> bit messy, but the ability to access a given CPU's per-CPU variable
> while running on that CPU does appear to have at least a couple of
> uses -- inlining RCU and also making preempt_disable() use per-CPU
> variables.
>
> In any case, I must confess that I feel quite silly about my series
> of patches. I have reverted them aside from a couple that did useful
> optimizations, and they should show up in -next shortly.
A wee bit sad, but thank you - it was an experiment worth trying,
and perhaps there will be reason to come back to it future.
Hugh
^ permalink raw reply
* [PATCH] net/pasemi: fix compiler warning
From: Stephen Rothwell @ 2012-05-03 0:51 UTC (permalink / raw)
To: David Miller, netdev; +Cc: Olof Johansson, Pradeep A. Dalvi, ppc-dev
[-- Attachment #1: Type: text/plain, Size: 1449 bytes --]
Fix this compiler warning (on PowerPC) by not marking a parameter as
const:
drivers/net/ethernet/pasemi/pasemi_mac.c: In function 'pasemi_mac_replenish_rx_ring':
drivers/net/ethernet/pasemi/pasemi_mac.c:646:3: warning: passing argument 1 of 'netdev_alloc_skb' discards qualifiers from pointer target type
include/linux/skbuff.h:1706:31: note: expected 'struct net_device *' but argument is of type 'const struct net_device *'
Cc: Olof Johansson <olof@lixom.net>
Cc: Pradeep A. Dalvi <netdev@pradeepdalvi.com>
Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
drivers/net/ethernet/pasemi/pasemi_mac.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
The warning was introduced by commit dae2e9f430c4 ("netdev: ethernet
dev_alloc_skb to netdev_alloc_skb").
diff --git a/drivers/net/ethernet/pasemi/pasemi_mac.c b/drivers/net/ethernet/pasemi/pasemi_mac.c
index ddc95b0..e559dfa 100644
--- a/drivers/net/ethernet/pasemi/pasemi_mac.c
+++ b/drivers/net/ethernet/pasemi/pasemi_mac.c
@@ -623,7 +623,7 @@ static void pasemi_mac_free_rx_resources(struct pasemi_mac *mac)
mac->rx = NULL;
}
-static void pasemi_mac_replenish_rx_ring(const struct net_device *dev,
+static void pasemi_mac_replenish_rx_ring(struct net_device *dev,
const int limit)
{
const struct pasemi_mac *mac = netdev_priv(dev);
--
1.7.10.280.gaa39
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply related
* Re: [PATCH] net/pasemi: fix compiler warning
From: David Miller @ 2012-05-03 0:53 UTC (permalink / raw)
To: sfr; +Cc: olof, netdev, netdev, linuxppc-dev
In-Reply-To: <20120503105146.83f0a4a074dafad0443b875d@canb.auug.org.au>
From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Thu, 3 May 2012 10:51:46 +1000
> Fix this compiler warning (on PowerPC) by not marking a parameter as
> const:
>
> drivers/net/ethernet/pasemi/pasemi_mac.c: In function 'pasemi_mac_replenish_rx_ring':
> drivers/net/ethernet/pasemi/pasemi_mac.c:646:3: warning: passing argument 1 of 'netdev_alloc_skb' discards qualifiers from pointer target type
> include/linux/skbuff.h:1706:31: note: expected 'struct net_device *' but argument is of type 'const struct net_device *'
>
> Cc: Olof Johansson <olof@lixom.net>
> Cc: Pradeep A. Dalvi <netdev@pradeepdalvi.com>
> Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
Applied, thanks.
^ permalink raw reply
* [PATCH] powerpc: use local var instead of local_paca->irq_happened directly in __check_irq_replay
From: Wang Sheng-Hui @ 2012-05-03 1:53 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Milton Miller, Grant Likely,
Stephen Rothwell, Anton Blanchard, linuxppc-dev, linux-kernel
local_paca->irq_happened may be changed asychronously.
In my test env (IBM Power 9117-MMA), I installed the RHEL6.2 with the shipped
oprofile. Then I run into kernel v3.4-rc4, setup/start oprofile and start the
LTP test suite.
In a short while, the system would crash. Seems that oprofile may change
the irq_happened.
======================================================================
KERNEL: /boot/vmlinux-3.4.0-rc4-00104-gaf3a3ab
DUMPFILE: vmcore [PARTIAL DUMP]
CPUS: 10
DATE: Fri Apr 27 18:54:34 2012
UPTIME: 00:02:34
LOAD AVERAGE: 0.60, 0.27, 0.10
TASKS: 369
NODENAME: feastlp3.upt.austin.ibm.com
RELEASE: 3.4.0-rc4-00104-gaf3a3ab
VERSION: #4 SMP Fri Apr 27 03:13:43 CDT 2012
MACHINE: ppc64 (4704 Mhz)
MEMORY: 9.8 GB
PANIC: "kernel BUG at /usr/src/kernels/linux/arch/powerpc/kernel/irq.c:188!"
PID: 0
COMMAND: "swapper/4"
TASK: c0000002694e3cc0 (1 of 10) [THREAD_INFO: c0000002694f8000]
CPU: 4
STATE: TASK_RUNNING (PANIC)
crash> bt
PID: 0 TASK: c0000002694e3cc0 CPU: 4 COMMAND: "swapper/4"
#0 [c00000026ffcb6e0] .crash_kexec at c0000000000f22e8
#1 [c00000026ffcb8e0] .oops_end at c00000000060aed8
#2 [c00000026ffcb980] ._exception at c000000000020900
#3 [c00000026ffcbb40] program_check_common at c0000000000053b4
Breakpoint trap [700] exception frame:
R0: 0000000000000001 R1: c00000026ffcbe30 R2: c000000000edd170
R3: 0000000000000500 R4: 0000000000000000 R5: 00000000000007fd
R6: 000000000124a180 R7: 003450cf9bd1233b R8: 0000000000940000
R9: c000000003400c00 R10: 0000000000000001 R11: 0000000000000000
R12: 0000000000000002 R13: c000000003400c00 R14: c0000002694fbf90
R15: 0000000002000040 R16: 0000000000000004 R17: 0000000000000000
R18: 0000000000000000 R19: 0000000000000000 R20: c000000000f42100
R21: 0000000000000000 R22: c000000000955b80 R23: c000000000955b80
R24: 000000000000000a R25: 0000000000000004 R26: c0000002694f8100
R27: c00000026ffc8000 R28: 0000000000000000 R29: c000000000f42100
R30: c000000000e60810 R31: 0000000000000040
NIP: c00000000000ea9c MSR: 8000000000029032 OR3: c00000000000ea3c
CTR: c000000000063e40 LR: c000000000010578 XER: 0000000000000000
CCR: 0000000028000048 MQ: 0000000000000000 DAR: c000000001295d00
DSISR: 0000000000000000 Syscall Result: 0000000000000000
#4 [c00000026ffcbe30] .__check_irq_replay at c00000000000ea9c
[Link Register ] [c00000026ffcbe30] .arch_local_irq_restore at c000000000010578
#5 [c00000026ffcbea0] .__do_softirq at c000000000085724
#6 [c00000026ffcbf90] .call_do_softirq at c000000000022928
#7 [c0000002694fb8d0] .do_softirq at c0000000000106c8
#8 [c0000002694fb970] .irq_exit at c000000000085414
#9 [c0000002694fb9f0] .do_IRQ at c0000000000100a4
#10 [c0000002694fbab0] hardware_interrupt_common at c0000000000038c0
Hardware Interrupt [501] exception frame:
R0: 0000000000000001 R1: c0000002694fbda0 R2: c000000000edd170
R3: 0000000000000000 R4: 0000000000000000 R5: 0000000000000000
R6: 00000000000000e0 R7: 003450cf9bd1233b R8: 0000000000940000
R9: ffffffffffffffff R10: 0000000000243694 R11: 0000000000000001
R12: 0000000000000002 R13: c000000003400c00
NIP: c0000000000105b4 MSR: 8000000000009032 OR3: 0000000000000c00
CTR: c0000000004de3a0 LR: c0000000000105b4 XER: 0000000000000000
CCR: 0000000044000044 MQ: 0000000000000001 DAR: c0000000012990b0
DSISR: c0000002694fbce0 Syscall Result: 0000000000000000
#11 [c0000002694fbda0] .arch_local_irq_restore at c0000000000105b4 (unreliable)
#12 [c0000002694fbe10] .cpu_idle at c000000000017d20
#13 [c0000002694fbed0] .start_secondary at c00000000061a934
#14 [c0000002694fbf90] .start_secondary_prolog at c00000000000936c
Use local var instead of local_paca->irq_happened directly in this function here.
Please check this patch. Any comments are welcome.
Signed-off-by: Wang Sheng-Hui <shhuiw@gmail.com>
---
arch/powerpc/kernel/irq.c | 46 +++++++++++++++++++++++++++++---------------
1 files changed, 30 insertions(+), 16 deletions(-)
diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
index 5ec1b23..3d48b23 100644
--- a/arch/powerpc/kernel/irq.c
+++ b/arch/powerpc/kernel/irq.c
@@ -137,15 +137,17 @@ static inline notrace int decrementer_check_overflow(void)
*/
notrace unsigned int __check_irq_replay(void)
{
+ unsigned int ret_val;
/*
* We use local_paca rather than get_paca() to avoid all
* the debug_smp_processor_id() business in this low level
* function
*/
- unsigned char happened = local_paca->irq_happened;
+ unsigned char happened, irq_happened;
+ happened = irq_happened = local_paca->irq_happened;
/* Clear bit 0 which we wouldn't clear otherwise */
- local_paca->irq_happened &= ~PACA_IRQ_HARD_DIS;
+ irq_happened &= ~PACA_IRQ_HARD_DIS;
/*
* Force the delivery of pending soft-disabled interrupts on PS3.
@@ -161,33 +163,45 @@ notrace unsigned int __check_irq_replay(void)
* decrementer itself rather than the paca irq_happened field
* in case we also had a rollover while hard disabled
*/
- local_paca->irq_happened &= ~PACA_IRQ_DEC;
- if (decrementer_check_overflow())
- return 0x900;
+ irq_happened &= ~PACA_IRQ_DEC;
+ if (decrementer_check_overflow()) {
+ ret_val = 0x900;
+ goto replay;
+ }
/* Finally check if an external interrupt happened */
- local_paca->irq_happened &= ~PACA_IRQ_EE;
- if (happened & PACA_IRQ_EE)
- return 0x500;
+ irq_happened &= ~PACA_IRQ_EE;
+ if (happened & PACA_IRQ_EE) {
+ ret_val = 0x500;
+ goto replay;
+ }
#ifdef CONFIG_PPC_BOOK3E
/* Finally check if an EPR external interrupt happened
* this bit is typically set if we need to handle another
* "edge" interrupt from within the MPIC "EPR" handler
*/
- local_paca->irq_happened &= ~PACA_IRQ_EE_EDGE;
- if (happened & PACA_IRQ_EE_EDGE)
- return 0x500;
+ irq_happened &= ~PACA_IRQ_EE_EDGE;
+ if (happened & PACA_IRQ_EE_EDGE) {
+ ret_val = 0x500;
+ goto replay;
+ }
- local_paca->irq_happened &= ~PACA_IRQ_DBELL;
- if (happened & PACA_IRQ_DBELL)
- return 0x280;
+ irq_happened &= ~PACA_IRQ_DBELL;
+ if (happened & PACA_IRQ_DBELL) {
+ ret_val = 0x280;
+ goto replay;
+ }
#endif /* CONFIG_PPC_BOOK3E */
/* There should be nothing left ! */
- BUG_ON(local_paca->irq_happened != 0);
+ BUG_ON(irq_happened != 0);
+ ret_val = 0;
- return 0;
+replay:
+ local_paca->irq_happened = irq_happened;
+
+ return ret_val;
}
notrace void arch_local_irq_restore(unsigned long en)
--
1.7.1
^ permalink raw reply related
* Re: [PATCH] powerpc: use local var instead of local_paca->irq_happened directly in __check_irq_replay
From: Benjamin Herrenschmidt @ 2012-05-03 2:15 UTC (permalink / raw)
To: Wang Sheng-Hui
Cc: Stephen Rothwell, linux-kernel, Milton Miller, Anton Blanchard,
linuxppc-dev
In-Reply-To: <4FA1E527.1090807@gmail.com>
On Thu, 2012-05-03 at 09:53 +0800, Wang Sheng-Hui wrote:
> local_paca->irq_happened may be changed asychronously.
>
> In my test env (IBM Power 9117-MMA), I installed the RHEL6.2 with the shipped
> oprofile. Then I run into kernel v3.4-rc4, setup/start oprofile and start the
> LTP test suite.
>
> In a short while, the system would crash. Seems that oprofile may change
> the irq_happened.
.../...
> Use local var instead of local_paca->irq_happened directly in this function here.
>
> Please check this patch. Any comments are welcome.
It should not as __check_irq_replay() should always be called
with interrupts hard disabled... Do you see any code path
where that is not the case ?
Cheers,
Ben.
> Signed-off-by: Wang Sheng-Hui <shhuiw@gmail.com>
> ---
> arch/powerpc/kernel/irq.c | 46 +++++++++++++++++++++++++++++---------------
> 1 files changed, 30 insertions(+), 16 deletions(-)
>
> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
> index 5ec1b23..3d48b23 100644
> --- a/arch/powerpc/kernel/irq.c
> +++ b/arch/powerpc/kernel/irq.c
> @@ -137,15 +137,17 @@ static inline notrace int decrementer_check_overflow(void)
> */
> notrace unsigned int __check_irq_replay(void)
> {
> + unsigned int ret_val;
> /*
> * We use local_paca rather than get_paca() to avoid all
> * the debug_smp_processor_id() business in this low level
> * function
> */
> - unsigned char happened = local_paca->irq_happened;
> + unsigned char happened, irq_happened;
> + happened = irq_happened = local_paca->irq_happened;
>
> /* Clear bit 0 which we wouldn't clear otherwise */
> - local_paca->irq_happened &= ~PACA_IRQ_HARD_DIS;
> + irq_happened &= ~PACA_IRQ_HARD_DIS;
>
> /*
> * Force the delivery of pending soft-disabled interrupts on PS3.
> @@ -161,33 +163,45 @@ notrace unsigned int __check_irq_replay(void)
> * decrementer itself rather than the paca irq_happened field
> * in case we also had a rollover while hard disabled
> */
> - local_paca->irq_happened &= ~PACA_IRQ_DEC;
> - if (decrementer_check_overflow())
> - return 0x900;
> + irq_happened &= ~PACA_IRQ_DEC;
> + if (decrementer_check_overflow()) {
> + ret_val = 0x900;
> + goto replay;
> + }
>
> /* Finally check if an external interrupt happened */
> - local_paca->irq_happened &= ~PACA_IRQ_EE;
> - if (happened & PACA_IRQ_EE)
> - return 0x500;
> + irq_happened &= ~PACA_IRQ_EE;
> + if (happened & PACA_IRQ_EE) {
> + ret_val = 0x500;
> + goto replay;
> + }
>
> #ifdef CONFIG_PPC_BOOK3E
> /* Finally check if an EPR external interrupt happened
> * this bit is typically set if we need to handle another
> * "edge" interrupt from within the MPIC "EPR" handler
> */
> - local_paca->irq_happened &= ~PACA_IRQ_EE_EDGE;
> - if (happened & PACA_IRQ_EE_EDGE)
> - return 0x500;
> + irq_happened &= ~PACA_IRQ_EE_EDGE;
> + if (happened & PACA_IRQ_EE_EDGE) {
> + ret_val = 0x500;
> + goto replay;
> + }
>
> - local_paca->irq_happened &= ~PACA_IRQ_DBELL;
> - if (happened & PACA_IRQ_DBELL)
> - return 0x280;
> + irq_happened &= ~PACA_IRQ_DBELL;
> + if (happened & PACA_IRQ_DBELL) {
> + ret_val = 0x280;
> + goto replay;
> + }
> #endif /* CONFIG_PPC_BOOK3E */
>
> /* There should be nothing left ! */
> - BUG_ON(local_paca->irq_happened != 0);
> + BUG_ON(irq_happened != 0);
> + ret_val = 0;
>
> - return 0;
> +replay:
> + local_paca->irq_happened = irq_happened;
> +
> + return ret_val;
> }
>
> notrace void arch_local_irq_restore(unsigned long en)
^ permalink raw reply
* Re: [PATCH] powerpc: use local var instead of local_paca->irq_happened directly in __check_irq_replay
From: Wang Sheng-Hui @ 2012-05-03 2:27 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Stephen Rothwell, linux-kernel, Milton Miller, Anton Blanchard,
linuxppc-dev
In-Reply-To: <1336011306.2653.3.camel@pasglop>
On 2012年05月03日 10:15, Benjamin Herrenschmidt wrote:
> On Thu, 2012-05-03 at 09:53 +0800, Wang Sheng-Hui wrote:
>> local_paca->irq_happened may be changed asychronously.
>>
>> In my test env (IBM Power 9117-MMA), I installed the RHEL6.2 with the shipped
>> oprofile. Then I run into kernel v3.4-rc4, setup/start oprofile and start the
>> LTP test suite.
>>
>> In a short while, the system would crash. Seems that oprofile may change
>> the irq_happened.
>
> .../...
>
>> Use local var instead of local_paca->irq_happened directly in this function here.
>>
>> Please check this patch. Any comments are welcome.
>
> It should not as __check_irq_replay() should always be called
> with interrupts hard disabled... Do you see any code path
> where that is not the case ?
This is the only case.
I have run LTP test suite on my system without oprofile over 24 hours
with 3.4-rc4 kernel.
Then I started oprofile, and the system crashed quickly.
I wonder if oprofile does some special changes with the running.
But I'm not familiar with the internal of oprofile.
I tried to change BUG_ON to WARN_ON, and got lots of warnning messages
in dmesg. So I changed it to local var here.
>
> Cheers,
> Ben.
>
>> Signed-off-by: Wang Sheng-Hui <shhuiw@gmail.com>
>> ---
>> arch/powerpc/kernel/irq.c | 46 +++++++++++++++++++++++++++++---------------
>> 1 files changed, 30 insertions(+), 16 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
>> index 5ec1b23..3d48b23 100644
>> --- a/arch/powerpc/kernel/irq.c
>> +++ b/arch/powerpc/kernel/irq.c
>> @@ -137,15 +137,17 @@ static inline notrace int decrementer_check_overflow(void)
>> */
>> notrace unsigned int __check_irq_replay(void)
>> {
>> + unsigned int ret_val;
>> /*
>> * We use local_paca rather than get_paca() to avoid all
>> * the debug_smp_processor_id() business in this low level
>> * function
>> */
>> - unsigned char happened = local_paca->irq_happened;
>> + unsigned char happened, irq_happened;
>> + happened = irq_happened = local_paca->irq_happened;
>>
>> /* Clear bit 0 which we wouldn't clear otherwise */
>> - local_paca->irq_happened &= ~PACA_IRQ_HARD_DIS;
>> + irq_happened &= ~PACA_IRQ_HARD_DIS;
>>
>> /*
>> * Force the delivery of pending soft-disabled interrupts on PS3.
>> @@ -161,33 +163,45 @@ notrace unsigned int __check_irq_replay(void)
>> * decrementer itself rather than the paca irq_happened field
>> * in case we also had a rollover while hard disabled
>> */
>> - local_paca->irq_happened &= ~PACA_IRQ_DEC;
>> - if (decrementer_check_overflow())
>> - return 0x900;
>> + irq_happened &= ~PACA_IRQ_DEC;
>> + if (decrementer_check_overflow()) {
>> + ret_val = 0x900;
>> + goto replay;
>> + }
>>
>> /* Finally check if an external interrupt happened */
>> - local_paca->irq_happened &= ~PACA_IRQ_EE;
>> - if (happened & PACA_IRQ_EE)
>> - return 0x500;
>> + irq_happened &= ~PACA_IRQ_EE;
>> + if (happened & PACA_IRQ_EE) {
>> + ret_val = 0x500;
>> + goto replay;
>> + }
>>
>> #ifdef CONFIG_PPC_BOOK3E
>> /* Finally check if an EPR external interrupt happened
>> * this bit is typically set if we need to handle another
>> * "edge" interrupt from within the MPIC "EPR" handler
>> */
>> - local_paca->irq_happened &= ~PACA_IRQ_EE_EDGE;
>> - if (happened & PACA_IRQ_EE_EDGE)
>> - return 0x500;
>> + irq_happened &= ~PACA_IRQ_EE_EDGE;
>> + if (happened & PACA_IRQ_EE_EDGE) {
>> + ret_val = 0x500;
>> + goto replay;
>> + }
>>
>> - local_paca->irq_happened &= ~PACA_IRQ_DBELL;
>> - if (happened & PACA_IRQ_DBELL)
>> - return 0x280;
>> + irq_happened &= ~PACA_IRQ_DBELL;
>> + if (happened & PACA_IRQ_DBELL) {
>> + ret_val = 0x280;
>> + goto replay;
>> + }
>> #endif /* CONFIG_PPC_BOOK3E */
>>
>> /* There should be nothing left ! */
>> - BUG_ON(local_paca->irq_happened != 0);
>> + BUG_ON(irq_happened != 0);
>> + ret_val = 0;
>>
>> - return 0;
>> +replay:
>> + local_paca->irq_happened = irq_happened;
>> +
>> + return ret_val;
>> }
>>
>> notrace void arch_local_irq_restore(unsigned long en)
>
>
^ permalink raw reply
* Re: [PATCH] powerpc: use local var instead of local_paca->irq_happened directly in __check_irq_replay
From: Wang Sheng-Hui @ 2012-05-03 2:32 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Stephen Rothwell, linux-kernel, Milton Miller, Anton Blanchard,
linuxppc-dev
In-Reply-To: <1336011306.2653.3.camel@pasglop>
On 2012年05月03日 10:15, Benjamin Herrenschmidt wrote:
> On Thu, 2012-05-03 at 09:53 +0800, Wang Sheng-Hui wrote:
>> local_paca->irq_happened may be changed asychronously.
>>
>> In my test env (IBM Power 9117-MMA), I installed the RHEL6.2 with the shipped
>> oprofile. Then I run into kernel v3.4-rc4, setup/start oprofile and start the
>> LTP test suite.
>>
>> In a short while, the system would crash. Seems that oprofile may change
>> the irq_happened.
>
> .../...
>
>> Use local var instead of local_paca->irq_happened directly in this function here.
>>
>> Please check this patch. Any comments are welcome.
>
> It should not as __check_irq_replay() should always be called
> with interrupts hard disabled... Do you see any code path
> where that is not the case ?
Since __check_irq_replay() should always be called with interrupts hard disabled,
I think it's harmless to use local var here.
>
> Cheers,
> Ben.
>
>> Signed-off-by: Wang Sheng-Hui <shhuiw@gmail.com>
>> ---
>> arch/powerpc/kernel/irq.c | 46 +++++++++++++++++++++++++++++---------------
>> 1 files changed, 30 insertions(+), 16 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
>> index 5ec1b23..3d48b23 100644
>> --- a/arch/powerpc/kernel/irq.c
>> +++ b/arch/powerpc/kernel/irq.c
>> @@ -137,15 +137,17 @@ static inline notrace int decrementer_check_overflow(void)
>> */
>> notrace unsigned int __check_irq_replay(void)
>> {
>> + unsigned int ret_val;
>> /*
>> * We use local_paca rather than get_paca() to avoid all
>> * the debug_smp_processor_id() business in this low level
>> * function
>> */
>> - unsigned char happened = local_paca->irq_happened;
>> + unsigned char happened, irq_happened;
>> + happened = irq_happened = local_paca->irq_happened;
>>
>> /* Clear bit 0 which we wouldn't clear otherwise */
>> - local_paca->irq_happened &= ~PACA_IRQ_HARD_DIS;
>> + irq_happened &= ~PACA_IRQ_HARD_DIS;
>>
>> /*
>> * Force the delivery of pending soft-disabled interrupts on PS3.
>> @@ -161,33 +163,45 @@ notrace unsigned int __check_irq_replay(void)
>> * decrementer itself rather than the paca irq_happened field
>> * in case we also had a rollover while hard disabled
>> */
>> - local_paca->irq_happened &= ~PACA_IRQ_DEC;
>> - if (decrementer_check_overflow())
>> - return 0x900;
>> + irq_happened &= ~PACA_IRQ_DEC;
>> + if (decrementer_check_overflow()) {
>> + ret_val = 0x900;
>> + goto replay;
>> + }
>>
>> /* Finally check if an external interrupt happened */
>> - local_paca->irq_happened &= ~PACA_IRQ_EE;
>> - if (happened & PACA_IRQ_EE)
>> - return 0x500;
>> + irq_happened &= ~PACA_IRQ_EE;
>> + if (happened & PACA_IRQ_EE) {
>> + ret_val = 0x500;
>> + goto replay;
>> + }
>>
>> #ifdef CONFIG_PPC_BOOK3E
>> /* Finally check if an EPR external interrupt happened
>> * this bit is typically set if we need to handle another
>> * "edge" interrupt from within the MPIC "EPR" handler
>> */
>> - local_paca->irq_happened &= ~PACA_IRQ_EE_EDGE;
>> - if (happened & PACA_IRQ_EE_EDGE)
>> - return 0x500;
>> + irq_happened &= ~PACA_IRQ_EE_EDGE;
>> + if (happened & PACA_IRQ_EE_EDGE) {
>> + ret_val = 0x500;
>> + goto replay;
>> + }
>>
>> - local_paca->irq_happened &= ~PACA_IRQ_DBELL;
>> - if (happened & PACA_IRQ_DBELL)
>> - return 0x280;
>> + irq_happened &= ~PACA_IRQ_DBELL;
>> + if (happened & PACA_IRQ_DBELL) {
>> + ret_val = 0x280;
>> + goto replay;
>> + }
>> #endif /* CONFIG_PPC_BOOK3E */
>>
>> /* There should be nothing left ! */
>> - BUG_ON(local_paca->irq_happened != 0);
>> + BUG_ON(irq_happened != 0);
>> + ret_val = 0;
>>
>> - return 0;
>> +replay:
>> + local_paca->irq_happened = irq_happened;
>> +
>> + return ret_val;
>> }
>>
>> notrace void arch_local_irq_restore(unsigned long en)
>
>
^ permalink raw reply
* Re: [PATCH] powerpc: use local var instead of local_paca->irq_happened directly in __check_irq_replay
From: Benjamin Herrenschmidt @ 2012-05-03 4:22 UTC (permalink / raw)
To: Wang Sheng-Hui
Cc: Stephen Rothwell, linux-kernel, Milton Miller, Anton Blanchard,
linuxppc-dev
In-Reply-To: <1336011306.2653.3.camel@pasglop>
> It should not as __check_irq_replay() should always be called
> with interrupts hard disabled... Do you see any code path
> where that is not the case ?
More specifically, your backtrace seems to indicate that
__check_irq_repay() was called from arch_local_irq_restore() which
should have done this before calling __check_irq_replay():
if (unlikely(irq_happened != PACA_IRQ_HARD_DIS))
__hard_irq_disable();
Now, the only possibility that I can see for an interrupt to come in
and trip the problem you observed would be if for some reason we
had irq_happened set to PACA_IRQ_HARD_DIS while interrupts were
not hard disabled.
Can you try if removing the test (and thus unconditionally calling
__hard_irq_disable()) fixes the problem for you ?
If that is the case, then we need to audit the code to figure out how we
can end up with that bit in irq_happened set and interrupts hard
enabled.
Something like may_hard_irq_enable() shouldn't cause it since it should
only be called while hard disabled but adding a check in there might be
worth it (something like WARN_ON(mfmsr() & MSR_EE)).
Cheers,
Ben.
> Cheers,
> Ben.
>
> > Signed-off-by: Wang Sheng-Hui <shhuiw@gmail.com>
> > ---
> > arch/powerpc/kernel/irq.c | 46 +++++++++++++++++++++++++++++---------------
> > 1 files changed, 30 insertions(+), 16 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
> > index 5ec1b23..3d48b23 100644
> > --- a/arch/powerpc/kernel/irq.c
> > +++ b/arch/powerpc/kernel/irq.c
> > @@ -137,15 +137,17 @@ static inline notrace int decrementer_check_overflow(void)
> > */
> > notrace unsigned int __check_irq_replay(void)
> > {
> > + unsigned int ret_val;
> > /*
> > * We use local_paca rather than get_paca() to avoid all
> > * the debug_smp_processor_id() business in this low level
> > * function
> > */
> > - unsigned char happened = local_paca->irq_happened;
> > + unsigned char happened, irq_happened;
> > + happened = irq_happened = local_paca->irq_happened;
> >
> > /* Clear bit 0 which we wouldn't clear otherwise */
> > - local_paca->irq_happened &= ~PACA_IRQ_HARD_DIS;
> > + irq_happened &= ~PACA_IRQ_HARD_DIS;
> >
> > /*
> > * Force the delivery of pending soft-disabled interrupts on PS3.
> > @@ -161,33 +163,45 @@ notrace unsigned int __check_irq_replay(void)
> > * decrementer itself rather than the paca irq_happened field
> > * in case we also had a rollover while hard disabled
> > */
> > - local_paca->irq_happened &= ~PACA_IRQ_DEC;
> > - if (decrementer_check_overflow())
> > - return 0x900;
> > + irq_happened &= ~PACA_IRQ_DEC;
> > + if (decrementer_check_overflow()) {
> > + ret_val = 0x900;
> > + goto replay;
> > + }
> >
> > /* Finally check if an external interrupt happened */
> > - local_paca->irq_happened &= ~PACA_IRQ_EE;
> > - if (happened & PACA_IRQ_EE)
> > - return 0x500;
> > + irq_happened &= ~PACA_IRQ_EE;
> > + if (happened & PACA_IRQ_EE) {
> > + ret_val = 0x500;
> > + goto replay;
> > + }
> >
> > #ifdef CONFIG_PPC_BOOK3E
> > /* Finally check if an EPR external interrupt happened
> > * this bit is typically set if we need to handle another
> > * "edge" interrupt from within the MPIC "EPR" handler
> > */
> > - local_paca->irq_happened &= ~PACA_IRQ_EE_EDGE;
> > - if (happened & PACA_IRQ_EE_EDGE)
> > - return 0x500;
> > + irq_happened &= ~PACA_IRQ_EE_EDGE;
> > + if (happened & PACA_IRQ_EE_EDGE) {
> > + ret_val = 0x500;
> > + goto replay;
> > + }
> >
> > - local_paca->irq_happened &= ~PACA_IRQ_DBELL;
> > - if (happened & PACA_IRQ_DBELL)
> > - return 0x280;
> > + irq_happened &= ~PACA_IRQ_DBELL;
> > + if (happened & PACA_IRQ_DBELL) {
> > + ret_val = 0x280;
> > + goto replay;
> > + }
> > #endif /* CONFIG_PPC_BOOK3E */
> >
> > /* There should be nothing left ! */
> > - BUG_ON(local_paca->irq_happened != 0);
> > + BUG_ON(irq_happened != 0);
> > + ret_val = 0;
> >
> > - return 0;
> > +replay:
> > + local_paca->irq_happened = irq_happened;
> > +
> > + return ret_val;
> > }
> >
> > notrace void arch_local_irq_restore(unsigned long en)
>
>
> --
> 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/
^ permalink raw reply
* Re: [PATCH] powerpc: use local var instead of local_paca->irq_happened directly in __check_irq_replay
From: Benjamin Herrenschmidt @ 2012-05-03 4:26 UTC (permalink / raw)
To: Wang Sheng-Hui
Cc: Stephen Rothwell, linux-kernel, Milton Miller, Anton Blanchard,
linuxppc-dev
In-Reply-To: <4FA1EE2C.6050201@gmail.com>
On Thu, 2012-05-03 at 10:32 +0800, Wang Sheng-Hui wrote:
> > It should not as __check_irq_replay() should always be called
> > with interrupts hard disabled... Do you see any code path
> > where that is not the case ?
>
> Since __check_irq_replay() should always be called with interrupts
> hard disabled, I think it's harmless to use local var here.
No, that would be papering over the real problem. All oprofile does is
trigger perfmon interrupts (which act as some kind of NMI when
soft-disabled but should be masked by MSR:EE when hard disabled).
So there's a deeper issue here that we need to understand before we can
propose a fix. IE. It should not have happened.
Cheers,
Ben.
^ permalink raw reply
* [PATCH] powerpc/windfarm: fix compiler warning
From: Stephen Rothwell @ 2012-05-03 4:34 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: ppc-dev
[-- Attachment #1: Type: text/plain, Size: 1206 bytes --]
Fixes this warning:
drivers/macintosh/windfarm_pm91.c: In function 'wf_smu_cpu_fans_tick':
drivers/macintosh/windfarm_pm91.c:237:2: warning: passing argument 1 of 'wf_sensor_get' from incompatible pointer type
drivers/macintosh/windfarm.h:124:19: note: expected 'struct wf_sensor *' but argument is of type 'struct wf_sensor **'
Introduced by commit 33e6820b767a ("powerpc/windfarm: Add useful
accessors").
Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
drivers/macintosh/windfarm_pm91.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/macintosh/windfarm_pm91.c b/drivers/macintosh/windfarm_pm91.c
index e18002b..7653603 100644
--- a/drivers/macintosh/windfarm_pm91.c
+++ b/drivers/macintosh/windfarm_pm91.c
@@ -234,7 +234,7 @@ static void wf_smu_cpu_fans_tick(struct wf_smu_cpu_fans_state *st)
return;
}
- rc = wf_sensor_get(&sensor_cpu_power, &power);
+ rc = wf_sensor_get(sensor_cpu_power, &power);
if (rc) {
printk(KERN_WARNING "windfarm: CPU power sensor error %d\n",
rc);
--
1.7.10.280.gaa39
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply related
* Re: [PATCH] powerpc: use local var instead of local_paca->irq_happened directly in __check_irq_replay
From: Wang Sheng-Hui @ 2012-05-03 5:51 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Stephen Rothwell, linux-kernel, Milton Miller, Anton Blanchard,
linuxppc-dev
In-Reply-To: <1336018961.2653.11.camel@pasglop>
On 2012年05月03日 12:22, Benjamin Herrenschmidt wrote:
>
>> It should not as __check_irq_replay() should always be called
>> with interrupts hard disabled... Do you see any code path
>> where that is not the case ?
>
> More specifically, your backtrace seems to indicate that
> __check_irq_repay() was called from arch_local_irq_restore() which
> should have done this before calling __check_irq_replay():
>
> if (unlikely(irq_happened != PACA_IRQ_HARD_DIS))
> __hard_irq_disable();
>
> Now, the only possibility that I can see for an interrupt to come in
> and trip the problem you observed would be if for some reason we
> had irq_happened set to PACA_IRQ_HARD_DIS while interrupts were
> not hard disabled.
I have a chance to notice that the value is 0x05, not just 0x01.
So I think this is not the case.
>
> Can you try if removing the test (and thus unconditionally calling
> __hard_irq_disable()) fixes the problem for you ?
>
> If that is the case, then we need to audit the code to figure out how we
> can end up with that bit in irq_happened set and interrupts hard
> enabled.
>
> Something like may_hard_irq_enable() shouldn't cause it since it should
> only be called while hard disabled but adding a check in there might be
> worth it (something like WARN_ON(mfmsr() & MSR_EE)).
>
> Cheers,
> Ben.
>
>> Cheers,
>> Ben.
>>
>>> Signed-off-by: Wang Sheng-Hui <shhuiw@gmail.com>
>>> ---
>>> arch/powerpc/kernel/irq.c | 46 +++++++++++++++++++++++++++++---------------
>>> 1 files changed, 30 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
>>> index 5ec1b23..3d48b23 100644
>>> --- a/arch/powerpc/kernel/irq.c
>>> +++ b/arch/powerpc/kernel/irq.c
>>> @@ -137,15 +137,17 @@ static inline notrace int decrementer_check_overflow(void)
>>> */
>>> notrace unsigned int __check_irq_replay(void)
>>> {
>>> + unsigned int ret_val;
>>> /*
>>> * We use local_paca rather than get_paca() to avoid all
>>> * the debug_smp_processor_id() business in this low level
>>> * function
>>> */
>>> - unsigned char happened = local_paca->irq_happened;
>>> + unsigned char happened, irq_happened;
>>> + happened = irq_happened = local_paca->irq_happened;
>>>
>>> /* Clear bit 0 which we wouldn't clear otherwise */
>>> - local_paca->irq_happened &= ~PACA_IRQ_HARD_DIS;
>>> + irq_happened &= ~PACA_IRQ_HARD_DIS;
>>>
>>> /*
>>> * Force the delivery of pending soft-disabled interrupts on PS3.
>>> @@ -161,33 +163,45 @@ notrace unsigned int __check_irq_replay(void)
>>> * decrementer itself rather than the paca irq_happened field
>>> * in case we also had a rollover while hard disabled
>>> */
>>> - local_paca->irq_happened &= ~PACA_IRQ_DEC;
>>> - if (decrementer_check_overflow())
>>> - return 0x900;
>>> + irq_happened &= ~PACA_IRQ_DEC;
>>> + if (decrementer_check_overflow()) {
>>> + ret_val = 0x900;
>>> + goto replay;
>>> + }
>>>
>>> /* Finally check if an external interrupt happened */
>>> - local_paca->irq_happened &= ~PACA_IRQ_EE;
>>> - if (happened & PACA_IRQ_EE)
>>> - return 0x500;
>>> + irq_happened &= ~PACA_IRQ_EE;
>>> + if (happened & PACA_IRQ_EE) {
>>> + ret_val = 0x500;
>>> + goto replay;
>>> + }
>>>
>>> #ifdef CONFIG_PPC_BOOK3E
>>> /* Finally check if an EPR external interrupt happened
>>> * this bit is typically set if we need to handle another
>>> * "edge" interrupt from within the MPIC "EPR" handler
>>> */
>>> - local_paca->irq_happened &= ~PACA_IRQ_EE_EDGE;
>>> - if (happened & PACA_IRQ_EE_EDGE)
>>> - return 0x500;
>>> + irq_happened &= ~PACA_IRQ_EE_EDGE;
>>> + if (happened & PACA_IRQ_EE_EDGE) {
>>> + ret_val = 0x500;
>>> + goto replay;
>>> + }
>>>
>>> - local_paca->irq_happened &= ~PACA_IRQ_DBELL;
>>> - if (happened & PACA_IRQ_DBELL)
>>> - return 0x280;
>>> + irq_happened &= ~PACA_IRQ_DBELL;
>>> + if (happened & PACA_IRQ_DBELL) {
>>> + ret_val = 0x280;
>>> + goto replay;
>>> + }
>>> #endif /* CONFIG_PPC_BOOK3E */
>>>
>>> /* There should be nothing left ! */
>>> - BUG_ON(local_paca->irq_happened != 0);
>>> + BUG_ON(irq_happened != 0);
>>> + ret_val = 0;
>>>
>>> - return 0;
>>> +replay:
>>> + local_paca->irq_happened = irq_happened;
>>> +
>>> + return ret_val;
>>> }
>>>
>>> notrace void arch_local_irq_restore(unsigned long en)
>>
>>
>> --
>> 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/
>
>
^ permalink raw reply
* [PATCH] powerpc/windfarm: don't pass const strings to snprintf
From: Stephen Rothwell @ 2012-05-03 6:19 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: ppc-dev
[-- Attachment #1: Type: text/plain, Size: 1838 bytes --]
Fixes these build warnings:
drivers/macintosh/windfarm_smu_sat.c: In function 'wf_sat_probe':
drivers/macintosh/windfarm_smu_sat.c:290:3: warning: passing argument 1 of 'snprintf' discards qualifiers from pointer target type
include/linux/kernel.h:323:5: note: expected 'char *' but argument is of type 'const char *'
drivers/macintosh/windfarm_smu_sat.c:317:3: warning: passing argument 1 of 'snprintf' discards qualifiers from pointer target type
include/linux/kernel.h:323:5: note: expected 'char *' but argument is of type 'const char *'
Introduced by commit e074d08e2b98 ("powerpc/windfarm: const'ify and add
"priv" field to controls & sensors").
Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
drivers/macintosh/windfarm_smu_sat.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/macintosh/windfarm_smu_sat.c b/drivers/macintosh/windfarm_smu_sat.c
index 72dfe19..e2989ce 100644
--- a/drivers/macintosh/windfarm_smu_sat.c
+++ b/drivers/macintosh/windfarm_smu_sat.c
@@ -287,7 +287,7 @@ static int wf_sat_probe(struct i2c_client *client,
sens->sat = sat;
sens->sens.ops = &wf_sat_ops;
sens->sens.name = (char *) (sens + 1);
- snprintf(sens->sens.name, 16, "%s-%d", name, cpu);
+ snprintf((char *)sens->sens.name, 16, "%s-%d", name, cpu);
if (wf_register_sensor(&sens->sens))
kfree(sens);
@@ -314,7 +314,7 @@ static int wf_sat_probe(struct i2c_client *client,
sens->sat = sat;
sens->sens.ops = &wf_sat_ops;
sens->sens.name = (char *) (sens + 1);
- snprintf(sens->sens.name, 16, "cpu-power-%d", cpu);
+ snprintf((char *)sens->sens.name, 16, "cpu-power-%d", cpu);
if (wf_register_sensor(&sens->sens))
kfree(sens);
--
1.7.10.280.gaa39
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply related
* Re: [PATCH] powerpc: use local var instead of local_paca->irq_happened directly in __check_irq_replay
From: Wang Sheng-Hui @ 2012-05-03 6:33 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Stephen Rothwell, linux-kernel, Milton Miller, Anton Blanchard,
linuxppc-dev
In-Reply-To: <1336018961.2653.11.camel@pasglop>
On 2012年05月03日 12:22, Benjamin Herrenschmidt wrote:
>
>> It should not as __check_irq_replay() should always be called
>> with interrupts hard disabled... Do you see any code path
>> where that is not the case ?
>
> More specifically, your backtrace seems to indicate that
> __check_irq_repay() was called from arch_local_irq_restore() which
> should have done this before calling __check_irq_replay():
>
> if (unlikely(irq_happened != PACA_IRQ_HARD_DIS))
> __hard_irq_disable();
>
> Now, the only possibility that I can see for an interrupt to come in
> and trip the problem you observed would be if for some reason we
> had irq_happened set to PACA_IRQ_HARD_DIS while interrupts were
> not hard disabled.
>
> Can you try if removing the test (and thus unconditionally calling
> __hard_irq_disable()) fixes the problem for you ?
The system crashed before I started the LTP test run.
========================================================
kernel BUG at /usr/src/kernels/linux/arch/powerpc/kernel/irq.c:188!
cpu 0x3: Vector: 700 (Program Check) at [c00000026ffd3bb0]
pc: c00000000000ea9c: .__check_irq_replay+0x7c/0x90
lr: c00000000001058c: .arch_local_irq_restore+0x4c/0x90
sp: c00000026ffd3e30
msr: 8000000000029032
current = 0xc0000002694e0110
paca = 0xc000000003580900 softe: 0 irq_happened: 0x01
pid = 0, comm = swapper/3
kernel BUG at /usr/src/kernels/linux/arch/powerpc/kernel/irq.c:188!
enter ? for help
[link register ] c00000000001058c .arch_local_irq_restore+0x4c/0x90
[c00000026ffd3e30] c000000000f42100 softirq_vec+0x0/0x80 (unreliable)
[c00000026ffd3ea0] c0000000000857d4 .__do_softirq+0xa4/0x2a0
[c00000026ffd3f90] c000000000022958 .call_do_softirq+0x14/0x24
[c0000002694778e0] c0000000000106c8 .do_softirq+0xf8/0x130
[c000000269477980] c0000000000854c4 .irq_exit+0xc4/0xf0
[c000000269477a00] c00000000001e970 .timer_interrupt+0x120/0x290
[c000000269477ab0] c000000000003a40 decrementer_common+0x140/0x180
--- Exception: 901 (Decrementer) at c0000000000105c4 .arch_local_irq_restore+0x84/0x90
[c000000269477da0] c000000000058400 .pSeries_idle+0x10/0x40 (unreliable)
[c000000269477e10] c000000000017d50 .cpu_idle+0x190/0x290
[c000000269477ed0] c00000000061ab04 .start_secondary+0x348/0x354
[c000000269477f90] c00000000000936c .start_secondary_prolog+0x10/0x14
3:mon> e
cpu 0x3: Vector: 700 (Program Check) at [c00000026ffd3bb0]
pc: c00000000000ea9c: .__check_irq_replay+0x7c/0x90
lr: c00000000001058c: .arch_local_irq_restore+0x4c/0x90
sp: c00000026ffd3e30
msr: 8000000000029032
current = 0xc0000002694e0110
paca = 0xc000000003580900 softe: 0 irq_happened: 0x01
pid = 0, comm = swapper/3
kernel BUG at /usr/src/kernels/linux/arch/powerpc/kernel/irq.c:188!
3:mon> r
R00 = 0000000000000001 R16 = 0000000000000000
R01 = c00000026ffd3e30 R17 = 0000000000000000
R02 = c000000000edd220 R18 = 0000000000000000
R03 = 0000000000000500 R19 = 0000000000000000
R04 = 0000000000000000 R20 = c000000000f42100
R05 = 00000000000004ca R21 = 0000000000000000
R06 = 0000000002dcc370 R22 = c000000000955b80
R07 = 003524b183dca42e R23 = c000000000955b80
R08 = 0000000000920000 R24 = 000000000000000a
R09 = c000000003580900 R25 = 0000000000000003
R10 = 0000000000000008 R26 = c000000269474100
R11 = 0000000000000000 R27 = c00000026ffd0000
R12 = c000000000653ba8 R28 = 0000000000000000
R13 = c000000003580900 R29 = c000000000f42100
R14 = c000000269477f90 R30 = c000000000e60890
R15 = 000000000ef03f20 R31 = 0000000000000082
pc = c00000000000ea9c .__check_irq_replay+0x7c/0x90
cfar= c00000000000ea38 .__check_irq_replay+0x18/0x90
lr = c00000000001058c .arch_local_irq_restore+0x4c/0x90
msr = 8000000000029032 cr = 28000028
ctr = c00000000001df50 xer = 0000000000000000 trap = 700
3:mon> t
[link register ] c00000000001058c .arch_local_irq_restore+0x4c/0x90
[c00000026ffd3e30] c000000000f42100 softirq_vec+0x0/0x80 (unreliable)
[c00000026ffd3ea0] c0000000000857d4 .__do_softirq+0xa4/0x2a0
[c00000026ffd3f90] c000000000022958 .call_do_softirq+0x14/0x24
[c0000002694778e0] c0000000000106c8 .do_softirq+0xf8/0x130
[c000000269477980] c0000000000854c4 .irq_exit+0xc4/0xf0
[c000000269477a00] c00000000001e970 .timer_interrupt+0x120/0x290
[c000000269477ab0] c000000000003a40 decrementer_common+0x140/0x180
--- Exception: 901 (Decrementer) at c0000000000105c4 .arch_local_irq_restore+0x84/0x90
[c000000269477da0] c000000000058400 .pSeries_idle+0x10/0x40 (unreliable)
[c000000269477e10] c000000000017d50 .cpu_idle+0x190/0x290
[c000000269477ed0] c00000000061ab04 .start_secondary+0x348/0x354
[c000000269477f90] c00000000000936c .start_secondary_prolog+0x10/0x14
3:mon> di c00000000000ea9c
c00000000000ea9c 0b000000 tdnei r0,0
c00000000000eaa0 38600000 li r3,0
c00000000000eaa4 4bffffc4 b c00000000000ea68 # .__check_irq_replay+0x48/0x90
c00000000000eaa8 60000000 nop
...
c00000000000eab0 7c0802a6 mflr r0
c00000000000eab4 fbc1fff0 std r30,-16(r1)
c00000000000eab8 fba1ffe8 std r29,-24(r1)
c00000000000eabc fbe1fff8 std r31,-8(r1)
c00000000000eac0 ebc28128 ld r30,-32472(r2)
c00000000000eac4 3d230002 addis r9,r3,2
c00000000000eac8 f8010010 std r0,16(r1)
c00000000000eacc f821ff71 stdu r1,-144(r1)
c00000000000ead0 38a5ffd8 addi r5,r5,-40
c00000000000ead4 ebe92060 ld r31,8288(r9)
c00000000000ead8 80050048 lwz r0,72(r5)
>
> If that is the case, then we need to audit the code to figure out how we
> can end up with that bit in irq_happened set and interrupts hard
> enabled.
>
> Something like may_hard_irq_enable() shouldn't cause it since it should
> only be called while hard disabled but adding a check in there might be
> worth it (something like WARN_ON(mfmsr() & MSR_EE)).
>
> Cheers,
> Ben.
>
>> Cheers,
>> Ben.
>>
>>> Signed-off-by: Wang Sheng-Hui <shhuiw@gmail.com>
>>> ---
>>> arch/powerpc/kernel/irq.c | 46 +++++++++++++++++++++++++++++---------------
>>> 1 files changed, 30 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
>>> index 5ec1b23..3d48b23 100644
>>> --- a/arch/powerpc/kernel/irq.c
>>> +++ b/arch/powerpc/kernel/irq.c
>>> @@ -137,15 +137,17 @@ static inline notrace int decrementer_check_overflow(void)
>>> */
>>> notrace unsigned int __check_irq_replay(void)
>>> {
>>> + unsigned int ret_val;
>>> /*
>>> * We use local_paca rather than get_paca() to avoid all
>>> * the debug_smp_processor_id() business in this low level
>>> * function
>>> */
>>> - unsigned char happened = local_paca->irq_happened;
>>> + unsigned char happened, irq_happened;
>>> + happened = irq_happened = local_paca->irq_happened;
>>>
>>> /* Clear bit 0 which we wouldn't clear otherwise */
>>> - local_paca->irq_happened &= ~PACA_IRQ_HARD_DIS;
>>> + irq_happened &= ~PACA_IRQ_HARD_DIS;
>>>
>>> /*
>>> * Force the delivery of pending soft-disabled interrupts on PS3.
>>> @@ -161,33 +163,45 @@ notrace unsigned int __check_irq_replay(void)
>>> * decrementer itself rather than the paca irq_happened field
>>> * in case we also had a rollover while hard disabled
>>> */
>>> - local_paca->irq_happened &= ~PACA_IRQ_DEC;
>>> - if (decrementer_check_overflow())
>>> - return 0x900;
>>> + irq_happened &= ~PACA_IRQ_DEC;
>>> + if (decrementer_check_overflow()) {
>>> + ret_val = 0x900;
>>> + goto replay;
>>> + }
>>>
>>> /* Finally check if an external interrupt happened */
>>> - local_paca->irq_happened &= ~PACA_IRQ_EE;
>>> - if (happened & PACA_IRQ_EE)
>>> - return 0x500;
>>> + irq_happened &= ~PACA_IRQ_EE;
>>> + if (happened & PACA_IRQ_EE) {
>>> + ret_val = 0x500;
>>> + goto replay;
>>> + }
>>>
>>> #ifdef CONFIG_PPC_BOOK3E
>>> /* Finally check if an EPR external interrupt happened
>>> * this bit is typically set if we need to handle another
>>> * "edge" interrupt from within the MPIC "EPR" handler
>>> */
>>> - local_paca->irq_happened &= ~PACA_IRQ_EE_EDGE;
>>> - if (happened & PACA_IRQ_EE_EDGE)
>>> - return 0x500;
>>> + irq_happened &= ~PACA_IRQ_EE_EDGE;
>>> + if (happened & PACA_IRQ_EE_EDGE) {
>>> + ret_val = 0x500;
>>> + goto replay;
>>> + }
>>>
>>> - local_paca->irq_happened &= ~PACA_IRQ_DBELL;
>>> - if (happened & PACA_IRQ_DBELL)
>>> - return 0x280;
>>> + irq_happened &= ~PACA_IRQ_DBELL;
>>> + if (happened & PACA_IRQ_DBELL) {
>>> + ret_val = 0x280;
>>> + goto replay;
>>> + }
>>> #endif /* CONFIG_PPC_BOOK3E */
>>>
>>> /* There should be nothing left ! */
>>> - BUG_ON(local_paca->irq_happened != 0);
>>> + BUG_ON(irq_happened != 0);
>>> + ret_val = 0;
>>>
>>> - return 0;
>>> +replay:
>>> + local_paca->irq_happened = irq_happened;
>>> +
>>> + return ret_val;
>>> }
>>>
>>> notrace void arch_local_irq_restore(unsigned long en)
>>
>>
>> --
>> 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/
>
>
^ permalink raw reply
* Re: [PATCH] powerpc: use local var instead of local_paca->irq_happened directly in __check_irq_replay
From: Benjamin Herrenschmidt @ 2012-05-03 6:52 UTC (permalink / raw)
To: Wang Sheng-Hui
Cc: Stephen Rothwell, linux-kernel, Milton Miller, Anton Blanchard,
linuxppc-dev
In-Reply-To: <4FA21CD8.4060800@gmail.com>
On Thu, 2012-05-03 at 13:51 +0800, Wang Sheng-Hui wrote:
> On 2012年05月03日 12:22, Benjamin Herrenschmidt wrote:
> >
> >> It should not as __check_irq_replay() should always be called
> >> with interrupts hard disabled... Do you see any code path
> >> where that is not the case ?
> >
> > More specifically, your backtrace seems to indicate that
> > __check_irq_repay() was called from arch_local_irq_restore() which
> > should have done this before calling __check_irq_replay():
> >
> > if (unlikely(irq_happened != PACA_IRQ_HARD_DIS))
> > __hard_irq_disable();
> >
> > Now, the only possibility that I can see for an interrupt to come in
> > and trip the problem you observed would be if for some reason we
> > had irq_happened set to PACA_IRQ_HARD_DIS while interrupts were
> > not hard disabled.
>
> I have a chance to notice that the value is 0x05, not just 0x01.
> So I think this is not the case.
Well, it depends, the value could have been 0x01 before it hit there...
However 0x05 means that EE is set too which means it should never have
hard-enabled to begin with. This is all very odd, we'll need to dig.
If the value had been anything other than 0x01 it would have hard
disabled interrupts meaning that paca->irq_happened cannot change
anymore until they are re-enabled at the bottom of the function.
So please try making this disable unconditional see if that makes any
difference...
Cheers,
Ben.
> > Can you try if removing the test (and thus unconditionally calling
> > __hard_irq_disable()) fixes the problem for you ?
> >
> > If that is the case, then we need to audit the code to figure out how we
> > can end up with that bit in irq_happened set and interrupts hard
> > enabled.
> >
> > Something like may_hard_irq_enable() shouldn't cause it since it should
> > only be called while hard disabled but adding a check in there might be
> > worth it (something like WARN_ON(mfmsr() & MSR_EE)).
> >
> > Cheers,
> > Ben.
> >
> >> Cheers,
> >> Ben.
> >>
> >>> Signed-off-by: Wang Sheng-Hui <shhuiw@gmail.com>
> >>> ---
> >>> arch/powerpc/kernel/irq.c | 46 +++++++++++++++++++++++++++++---------------
> >>> 1 files changed, 30 insertions(+), 16 deletions(-)
> >>>
> >>> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
> >>> index 5ec1b23..3d48b23 100644
> >>> --- a/arch/powerpc/kernel/irq.c
> >>> +++ b/arch/powerpc/kernel/irq.c
> >>> @@ -137,15 +137,17 @@ static inline notrace int decrementer_check_overflow(void)
> >>> */
> >>> notrace unsigned int __check_irq_replay(void)
> >>> {
> >>> + unsigned int ret_val;
> >>> /*
> >>> * We use local_paca rather than get_paca() to avoid all
> >>> * the debug_smp_processor_id() business in this low level
> >>> * function
> >>> */
> >>> - unsigned char happened = local_paca->irq_happened;
> >>> + unsigned char happened, irq_happened;
> >>> + happened = irq_happened = local_paca->irq_happened;
> >>>
> >>> /* Clear bit 0 which we wouldn't clear otherwise */
> >>> - local_paca->irq_happened &= ~PACA_IRQ_HARD_DIS;
> >>> + irq_happened &= ~PACA_IRQ_HARD_DIS;
> >>>
> >>> /*
> >>> * Force the delivery of pending soft-disabled interrupts on PS3.
> >>> @@ -161,33 +163,45 @@ notrace unsigned int __check_irq_replay(void)
> >>> * decrementer itself rather than the paca irq_happened field
> >>> * in case we also had a rollover while hard disabled
> >>> */
> >>> - local_paca->irq_happened &= ~PACA_IRQ_DEC;
> >>> - if (decrementer_check_overflow())
> >>> - return 0x900;
> >>> + irq_happened &= ~PACA_IRQ_DEC;
> >>> + if (decrementer_check_overflow()) {
> >>> + ret_val = 0x900;
> >>> + goto replay;
> >>> + }
> >>>
> >>> /* Finally check if an external interrupt happened */
> >>> - local_paca->irq_happened &= ~PACA_IRQ_EE;
> >>> - if (happened & PACA_IRQ_EE)
> >>> - return 0x500;
> >>> + irq_happened &= ~PACA_IRQ_EE;
> >>> + if (happened & PACA_IRQ_EE) {
> >>> + ret_val = 0x500;
> >>> + goto replay;
> >>> + }
> >>>
> >>> #ifdef CONFIG_PPC_BOOK3E
> >>> /* Finally check if an EPR external interrupt happened
> >>> * this bit is typically set if we need to handle another
> >>> * "edge" interrupt from within the MPIC "EPR" handler
> >>> */
> >>> - local_paca->irq_happened &= ~PACA_IRQ_EE_EDGE;
> >>> - if (happened & PACA_IRQ_EE_EDGE)
> >>> - return 0x500;
> >>> + irq_happened &= ~PACA_IRQ_EE_EDGE;
> >>> + if (happened & PACA_IRQ_EE_EDGE) {
> >>> + ret_val = 0x500;
> >>> + goto replay;
> >>> + }
> >>>
> >>> - local_paca->irq_happened &= ~PACA_IRQ_DBELL;
> >>> - if (happened & PACA_IRQ_DBELL)
> >>> - return 0x280;
> >>> + irq_happened &= ~PACA_IRQ_DBELL;
> >>> + if (happened & PACA_IRQ_DBELL) {
> >>> + ret_val = 0x280;
> >>> + goto replay;
> >>> + }
> >>> #endif /* CONFIG_PPC_BOOK3E */
> >>>
> >>> /* There should be nothing left ! */
> >>> - BUG_ON(local_paca->irq_happened != 0);
> >>> + BUG_ON(irq_happened != 0);
> >>> + ret_val = 0;
> >>>
> >>> - return 0;
> >>> +replay:
> >>> + local_paca->irq_happened = irq_happened;
> >>> +
> >>> + return ret_val;
> >>> }
> >>>
> >>> notrace void arch_local_irq_restore(unsigned long en)
> >>
> >>
> >> --
> >> 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/
> >
> >
^ permalink raw reply
* Re: [PATCH] powerpc: use local var instead of local_paca->irq_happened directly in __check_irq_replay
From: Wang Sheng-Hui @ 2012-05-03 6:59 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Stephen Rothwell, linux-kernel, Milton Miller, Anton Blanchard,
linuxppc-dev
In-Reply-To: <4FA226A8.1080903@gmail.com>
On 2012年05月03日 14:33, Wang Sheng-Hui wrote:
> if (unlikely(irq_happened != PACA_IRQ_HARD_DIS))
>> __hard_irq_disable();
I have commented out the 2 lines.
FYI.
thanks,
^ permalink raw reply
* Re: [PATCH] powerpc: use local var instead of local_paca->irq_happened directly in __check_irq_replay
From: Benjamin Herrenschmidt @ 2012-05-03 8:09 UTC (permalink / raw)
To: Wang Sheng-Hui
Cc: Stephen Rothwell, linux-kernel, Milton Miller, Anton Blanchard,
linuxppc-dev
In-Reply-To: <4FA22CC5.3010107@gmail.com>
On Thu, 2012-05-03 at 14:59 +0800, Wang Sheng-Hui wrote:
> On 2012年05月03日 14:33, Wang Sheng-Hui wrote:
> > if (unlikely(irq_happened != PACA_IRQ_HARD_DIS))
> >> __hard_irq_disable();
>
> I have commented out the 2 lines.
No, Only comment the test, you must absolutely leave the
__hard_irq_disable() call ! That's the whole point of the test, make
sure we unconditionally disable to see if that fixes the problem, in
which case that will tell us that we somewhere accidentally leave
irq_happened set to 0x01 while irqs are hard enabled.
Cheers,
Ben.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox