* [PATCH] lockdep: annotate i386 apm
@ 2006-10-11 13:40 Peter Zijlstra
2006-10-11 21:18 ` Andrew Morton
0 siblings, 1 reply; 9+ messages in thread
From: Peter Zijlstra @ 2006-10-11 13:40 UTC (permalink / raw)
To: linux-kernel; +Cc: sfr, Andrew Morton, Ingo Molnar
Lockdep doesn't like to enable interrupts when they are enabled already.
BUG: warning at kernel/lockdep.c:1814/trace_hardirqs_on() (Not tainted)
[<c04051ed>] show_trace_log_lvl+0x58/0x16a
[<c04057fa>] show_trace+0xd/0x10
[<c0405913>] dump_stack+0x19/0x1b
[<c043abfb>] trace_hardirqs_on+0xa2/0x11e
[<c041463c>] apm_bios_call_simple+0xcd/0xfd
[<c0415242>] apm+0x92/0x5b1
[<c0402005>] kernel_thread_helper+0x5/0xb
DWARF2 unwinder stuck at kernel_thread_helper+0x5/0xb
Leftover inexact backtrace:
[<c04057fa>] show_trace+0xd/0x10
[<c0405913>] dump_stack+0x19/0x1b
[<c043abfb>] trace_hardirqs_on+0xa2/0x11e
[<c041463c>] apm_bios_call_simple+0xcd/0xfd
[<c0415242>] apm+0x92/0x5b1
[<c0402005>] kernel_thread_helper+0x5/0xb
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
arch/i386/kernel/apm.c | 25 ++++++++++++++++++-------
1 file changed, 18 insertions(+), 7 deletions(-)
Index: linux-2.6.18.noarch/arch/i386/kernel/apm.c
===================================================================
--- linux-2.6.18.noarch.orig/arch/i386/kernel/apm.c
+++ linux-2.6.18.noarch/arch/i386/kernel/apm.c
@@ -539,11 +539,22 @@ static inline void apm_restore_cpus(cpum
* Also, we KNOW that for the non error case of apm_bios_call, there
* is no useful data returned in the low order 8 bits of eax.
*/
-#define APM_DO_CLI \
- if (apm_info.allow_ints) \
- local_irq_enable(); \
- else \
- local_irq_disable();
+#define APM_DO_CLI \
+ do { \
+ if (apm_info.allow_ints) { \
+ if (irqs_disabled_flags(flags)) \
+ local_irq_enable(); \
+ } else \
+ local_irq_disable(); \
+ } while (0)
+
+#define APM_DO_STI \
+ do { \
+ if (irqs_disabled_flags(flags)) \
+ local_irq_disable(); \
+ else if (irqs_disabled()) \
+ local_irq_enable(); \
+ } while (0)
#ifdef APM_ZERO_SEGS
# define APM_DECL_SEGS \
@@ -600,7 +611,7 @@ static u8 apm_bios_call(u32 func, u32 eb
APM_DO_SAVE_SEGS;
apm_bios_call_asm(func, ebx_in, ecx_in, eax, ebx, ecx, edx, esi);
APM_DO_RESTORE_SEGS;
- local_irq_restore(flags);
+ APM_DO_STI;
gdt[0x40 / 8] = save_desc_40;
put_cpu();
apm_restore_cpus(cpus);
@@ -644,7 +655,7 @@ static u8 apm_bios_call_simple(u32 func,
APM_DO_SAVE_SEGS;
error = apm_bios_call_simple_asm(func, ebx_in, ecx_in, eax);
APM_DO_RESTORE_SEGS;
- local_irq_restore(flags);
+ APM_DO_STI;
gdt[0x40 / 8] = save_desc_40;
put_cpu();
apm_restore_cpus(cpus);
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] lockdep: annotate i386 apm 2006-10-11 13:40 [PATCH] lockdep: annotate i386 apm Peter Zijlstra @ 2006-10-11 21:18 ` Andrew Morton 2006-10-12 6:06 ` Peter Zijlstra 0 siblings, 1 reply; 9+ messages in thread From: Andrew Morton @ 2006-10-11 21:18 UTC (permalink / raw) To: Peter Zijlstra; +Cc: linux-kernel, sfr, Ingo Molnar On Wed, 11 Oct 2006 15:40:22 +0200 Peter Zijlstra <a.p.zijlstra@chello.nl> wrote: > > Lockdep doesn't like to enable interrupts when they are enabled already. > > ... > > --- linux-2.6.18.noarch.orig/arch/i386/kernel/apm.c > +++ linux-2.6.18.noarch/arch/i386/kernel/apm.c > @@ -539,11 +539,22 @@ static inline void apm_restore_cpus(cpum > * Also, we KNOW that for the non error case of apm_bios_call, there > * is no useful data returned in the low order 8 bits of eax. > */ > -#define APM_DO_CLI \ > - if (apm_info.allow_ints) \ > - local_irq_enable(); \ > - else \ > - local_irq_disable(); > +#define APM_DO_CLI \ > + do { \ > + if (apm_info.allow_ints) { \ > + if (irqs_disabled_flags(flags)) \ > + local_irq_enable(); \ > + } else \ > + local_irq_disable(); \ > + } while (0) > + > +#define APM_DO_STI \ > + do { \ > + if (irqs_disabled_flags(flags)) \ > + local_irq_disable(); \ > + else if (irqs_disabled()) \ > + local_irq_enable(); \ > + } while (0) > > #ifdef APM_ZERO_SEGS > # define APM_DECL_SEGS \ > @@ -600,7 +611,7 @@ static u8 apm_bios_call(u32 func, u32 eb > APM_DO_SAVE_SEGS; > apm_bios_call_asm(func, ebx_in, ecx_in, eax, ebx, ecx, edx, esi); > APM_DO_RESTORE_SEGS; > - local_irq_restore(flags); > + APM_DO_STI; > gdt[0x40 / 8] = save_desc_40; > put_cpu(); > apm_restore_cpus(cpus); > @@ -644,7 +655,7 @@ static u8 apm_bios_call_simple(u32 func, > APM_DO_SAVE_SEGS; > error = apm_bios_call_simple_asm(func, ebx_in, ecx_in, eax); > APM_DO_RESTORE_SEGS; > - local_irq_restore(flags); > + APM_DO_STI; > gdt[0x40 / 8] = save_desc_40; > put_cpu(); > apm_restore_cpus(cpus); ick. Does this work? --- a/arch/i386/kernel/apm.c~lockdep-annotate-i386-apm +++ a/arch/i386/kernel/apm.c @@ -540,12 +540,6 @@ static inline void apm_restore_cpus(cpum * Also, we KNOW that for the non error case of apm_bios_call, there * is no useful data returned in the low order 8 bits of eax. */ -#define APM_DO_CLI \ - if (apm_info.allow_ints) \ - local_irq_enable(); \ - else \ - local_irq_disable(); - #ifdef APM_ZERO_SEGS # define APM_DECL_SEGS \ unsigned int saved_fs; unsigned int saved_gs; @@ -596,8 +590,9 @@ static u8 apm_bios_call(u32 func, u32 eb save_desc_40 = gdt[0x40 / 8]; gdt[0x40 / 8] = bad_bios_desc; - local_save_flags(flags); - APM_DO_CLI; + local_irq_save(flags); + if (apm_info.allow_ints) + local_irq_enable(); APM_DO_SAVE_SEGS; apm_bios_call_asm(func, ebx_in, ecx_in, eax, ebx, ecx, edx, esi); APM_DO_RESTORE_SEGS; @@ -640,8 +635,9 @@ static u8 apm_bios_call_simple(u32 func, save_desc_40 = gdt[0x40 / 8]; gdt[0x40 / 8] = bad_bios_desc; - local_save_flags(flags); - APM_DO_CLI; + local_irq_save(flags); + if (apm_info.allow_ints) + local_irq_enable(); APM_DO_SAVE_SEGS; error = apm_bios_call_simple_asm(func, ebx_in, ecx_in, eax); APM_DO_RESTORE_SEGS; _ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] lockdep: annotate i386 apm 2006-10-11 21:18 ` Andrew Morton @ 2006-10-12 6:06 ` Peter Zijlstra 2006-10-12 6:39 ` Andrew Morton 0 siblings, 1 reply; 9+ messages in thread From: Peter Zijlstra @ 2006-10-12 6:06 UTC (permalink / raw) To: Andrew Morton; +Cc: linux-kernel, sfr, Ingo Molnar On Wed, 2006-10-11 at 14:18 -0700, Andrew Morton wrote: > On Wed, 11 Oct 2006 15:40:22 +0200 > Peter Zijlstra <a.p.zijlstra@chello.nl> wrote: > > > > > Lockdep doesn't like to enable interrupts when they are enabled already. > > > > ... > > > > --- linux-2.6.18.noarch.orig/arch/i386/kernel/apm.c > > +++ linux-2.6.18.noarch/arch/i386/kernel/apm.c > > @@ -539,11 +539,22 @@ static inline void apm_restore_cpus(cpum > > * Also, we KNOW that for the non error case of apm_bios_call, there > > * is no useful data returned in the low order 8 bits of eax. > > */ > > -#define APM_DO_CLI \ > > - if (apm_info.allow_ints) \ > > - local_irq_enable(); \ > > - else \ > > - local_irq_disable(); > > +#define APM_DO_CLI \ > > + do { \ > > + if (apm_info.allow_ints) { \ > > + if (irqs_disabled_flags(flags)) \ > > + local_irq_enable(); \ > > + } else \ > > + local_irq_disable(); \ > > + } while (0) > > + > > +#define APM_DO_STI \ > > + do { \ > > + if (irqs_disabled_flags(flags)) \ > > + local_irq_disable(); \ > > + else if (irqs_disabled()) \ > > + local_irq_enable(); \ > > + } while (0) > > > > #ifdef APM_ZERO_SEGS > > # define APM_DECL_SEGS \ > > @@ -600,7 +611,7 @@ static u8 apm_bios_call(u32 func, u32 eb > > APM_DO_SAVE_SEGS; > > apm_bios_call_asm(func, ebx_in, ecx_in, eax, ebx, ecx, edx, esi); > > APM_DO_RESTORE_SEGS; > > - local_irq_restore(flags); > > + APM_DO_STI; > > gdt[0x40 / 8] = save_desc_40; > > put_cpu(); > > apm_restore_cpus(cpus); > > @@ -644,7 +655,7 @@ static u8 apm_bios_call_simple(u32 func, > > APM_DO_SAVE_SEGS; > > error = apm_bios_call_simple_asm(func, ebx_in, ecx_in, eax); > > APM_DO_RESTORE_SEGS; > > - local_irq_restore(flags); > > + APM_DO_STI; > > gdt[0x40 / 8] = save_desc_40; > > put_cpu(); > > apm_restore_cpus(cpus); > > ick. > > Does this work? > > --- a/arch/i386/kernel/apm.c~lockdep-annotate-i386-apm > +++ a/arch/i386/kernel/apm.c > @@ -540,12 +540,6 @@ static inline void apm_restore_cpus(cpum > * Also, we KNOW that for the non error case of apm_bios_call, there > * is no useful data returned in the low order 8 bits of eax. > */ > -#define APM_DO_CLI \ > - if (apm_info.allow_ints) \ > - local_irq_enable(); \ > - else \ > - local_irq_disable(); > - > #ifdef APM_ZERO_SEGS > # define APM_DECL_SEGS \ > unsigned int saved_fs; unsigned int saved_gs; > @@ -596,8 +590,9 @@ static u8 apm_bios_call(u32 func, u32 eb > save_desc_40 = gdt[0x40 / 8]; > gdt[0x40 / 8] = bad_bios_desc; > > - local_save_flags(flags); > - APM_DO_CLI; > + local_irq_save(flags); > + if (apm_info.allow_ints) > + local_irq_enable(); > APM_DO_SAVE_SEGS; > apm_bios_call_asm(func, ebx_in, ecx_in, eax, ebx, ecx, edx, esi); > APM_DO_RESTORE_SEGS; > @@ -640,8 +635,9 @@ static u8 apm_bios_call_simple(u32 func, > save_desc_40 = gdt[0x40 / 8]; > gdt[0x40 / 8] = bad_bios_desc; > > - local_save_flags(flags); > - APM_DO_CLI; > + local_irq_save(flags); > + if (apm_info.allow_ints) > + local_irq_enable(); > APM_DO_SAVE_SEGS; > error = apm_bios_call_simple_asm(func, ebx_in, ecx_in, eax); > APM_DO_RESTORE_SEGS; > _ #define local_irq_restore(flags) \ do { \ if (raw_irqs_disabled_flags(flags)) { \ raw_local_irq_restore(flags); \ trace_hardirqs_off(); \ } else { \ trace_hardirqs_on(); \ raw_local_irq_restore(flags); \ } \ } while (0) So, say interrupts were enabled when entering apm_bios_call*(); you now save that in flags, disable interrupts, and enable them again. Upon reaching local_irq_restore(), we'll hit the else branch with irq's enabled and call trace_hardirqs_on(), which goes EEEK! ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] lockdep: annotate i386 apm 2006-10-12 6:06 ` Peter Zijlstra @ 2006-10-12 6:39 ` Andrew Morton 2006-10-12 6:50 ` Peter Zijlstra 2006-10-12 8:02 ` Ingo Molnar 0 siblings, 2 replies; 9+ messages in thread From: Andrew Morton @ 2006-10-12 6:39 UTC (permalink / raw) To: Peter Zijlstra; +Cc: linux-kernel, sfr, Ingo Molnar On Thu, 12 Oct 2006 08:06:20 +0200 Peter Zijlstra <a.p.zijlstra@chello.nl> wrote: > #define local_irq_restore(flags) \ > do { \ > if (raw_irqs_disabled_flags(flags)) { \ > raw_local_irq_restore(flags); \ > trace_hardirqs_off(); \ > } else { \ > trace_hardirqs_on(); \ > raw_local_irq_restore(flags); \ > } \ > } while (0) > > So, say interrupts were enabled when entering apm_bios_call*(); you now > save that in flags, disable interrupts, and enable them again. > Upon reaching local_irq_restore(), we'll hit the else branch with irq's > enabled and call trace_hardirqs_on(), which goes EEEK! I'd assumed lockdep was less stupid than that ;) This? Seems a bit overdone.. --- a/arch/i386/kernel/apm.c~lockdep-annotate-i386-apm +++ a/arch/i386/kernel/apm.c @@ -540,12 +540,6 @@ static inline void apm_restore_cpus(cpum * Also, we KNOW that for the non error case of apm_bios_call, there * is no useful data returned in the low order 8 bits of eax. */ -#define APM_DO_CLI \ - if (apm_info.allow_ints) \ - local_irq_enable(); \ - else \ - local_irq_disable(); - #ifdef APM_ZERO_SEGS # define APM_DECL_SEGS \ unsigned int saved_fs; unsigned int saved_gs; @@ -583,11 +577,12 @@ static u8 apm_bios_call(u32 func, u32 eb u32 *eax, u32 *ebx, u32 *ecx, u32 *edx, u32 *esi) { APM_DECL_SEGS - unsigned long flags; cpumask_t cpus; int cpu; struct desc_struct save_desc_40; struct desc_struct *gdt; + int enable_irqs = 0; + int disable_irqs = 0; cpus = apm_save_cpus(); @@ -596,12 +591,26 @@ static u8 apm_bios_call(u32 func, u32 eb save_desc_40 = gdt[0x40 / 8]; gdt[0x40 / 8] = bad_bios_desc; - local_save_flags(flags); - APM_DO_CLI; + if (apm_info.allow_ints) { + if (irqs_disabled()) { + local_irq_enable(); + disable_irqs = 1; + } + } else { + if (!irqs_disabled()) { + local_irq_disable(); + enable_irqs = 1; + } + } + + APM_DO_SAVE_SEGS; apm_bios_call_asm(func, ebx_in, ecx_in, eax, ebx, ecx, edx, esi); APM_DO_RESTORE_SEGS; - local_irq_restore(flags); + if (disable_irqs) + local_irq_disable(); + if (enable_irqs) + local_irq_enable(); gdt[0x40 / 8] = save_desc_40; put_cpu(); apm_restore_cpus(cpus); @@ -627,11 +636,12 @@ static u8 apm_bios_call_simple(u32 func, { u8 error; APM_DECL_SEGS - unsigned long flags; cpumask_t cpus; int cpu; struct desc_struct save_desc_40; struct desc_struct *gdt; + int enable_irqs = 0; + int disable_irqs = 0; cpus = apm_save_cpus(); @@ -640,12 +650,25 @@ static u8 apm_bios_call_simple(u32 func, save_desc_40 = gdt[0x40 / 8]; gdt[0x40 / 8] = bad_bios_desc; - local_save_flags(flags); - APM_DO_CLI; + if (apm_info.allow_ints) { + if (irqs_disabled()) { + local_irq_enable(); + disable_irqs = 1; + } + } else { + if (!irqs_disabled()) { + local_irq_disable(); + enable_irqs = 1; + } + } + APM_DO_SAVE_SEGS; error = apm_bios_call_simple_asm(func, ebx_in, ecx_in, eax); APM_DO_RESTORE_SEGS; - local_irq_restore(flags); + if (disable_irqs) + local_irq_disable(); + if (enable_irqs) + local_irq_enable(); gdt[0x40 / 8] = save_desc_40; put_cpu(); apm_restore_cpus(cpus); _ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] lockdep: annotate i386 apm 2006-10-12 6:39 ` Andrew Morton @ 2006-10-12 6:50 ` Peter Zijlstra 2006-10-12 7:29 ` Andrew Morton 2006-10-12 8:02 ` Ingo Molnar 1 sibling, 1 reply; 9+ messages in thread From: Peter Zijlstra @ 2006-10-12 6:50 UTC (permalink / raw) To: Andrew Morton; +Cc: linux-kernel, sfr, Ingo Molnar On Wed, 2006-10-11 at 23:39 -0700, Andrew Morton wrote: > On Thu, 12 Oct 2006 08:06:20 +0200 > Peter Zijlstra <a.p.zijlstra@chello.nl> wrote: > > > #define local_irq_restore(flags) \ > > do { \ > > if (raw_irqs_disabled_flags(flags)) { \ > > raw_local_irq_restore(flags); \ > > trace_hardirqs_off(); \ > > } else { \ > > trace_hardirqs_on(); \ > > raw_local_irq_restore(flags); \ > > } \ > > } while (0) > > > > So, say interrupts were enabled when entering apm_bios_call*(); you now > > save that in flags, disable interrupts, and enable them again. > > Upon reaching local_irq_restore(), we'll hit the else branch with irq's > > enabled and call trace_hardirqs_on(), which goes EEEK! > > I'd assumed lockdep was less stupid than that ;) This? Seems a bit > overdone.. Seems simple enough, bit on the verbose side, _however_ it depends on the BIOS call not changing the irq state. I don't know about BIOSs but I thought the general consensus was to distrust them. Was my original _that_ hard to read? > --- a/arch/i386/kernel/apm.c~lockdep-annotate-i386-apm > +++ a/arch/i386/kernel/apm.c > @@ -540,12 +540,6 @@ static inline void apm_restore_cpus(cpum > * Also, we KNOW that for the non error case of apm_bios_call, there > * is no useful data returned in the low order 8 bits of eax. > */ > -#define APM_DO_CLI \ > - if (apm_info.allow_ints) \ > - local_irq_enable(); \ > - else \ > - local_irq_disable(); > - > #ifdef APM_ZERO_SEGS > # define APM_DECL_SEGS \ > unsigned int saved_fs; unsigned int saved_gs; > @@ -583,11 +577,12 @@ static u8 apm_bios_call(u32 func, u32 eb > u32 *eax, u32 *ebx, u32 *ecx, u32 *edx, u32 *esi) > { > APM_DECL_SEGS > - unsigned long flags; > cpumask_t cpus; > int cpu; > struct desc_struct save_desc_40; > struct desc_struct *gdt; > + int enable_irqs = 0; > + int disable_irqs = 0; > > cpus = apm_save_cpus(); > > @@ -596,12 +591,26 @@ static u8 apm_bios_call(u32 func, u32 eb > save_desc_40 = gdt[0x40 / 8]; > gdt[0x40 / 8] = bad_bios_desc; > > - local_save_flags(flags); > - APM_DO_CLI; > + if (apm_info.allow_ints) { > + if (irqs_disabled()) { > + local_irq_enable(); > + disable_irqs = 1; > + } > + } else { > + if (!irqs_disabled()) { > + local_irq_disable(); > + enable_irqs = 1; > + } > + } > + > + > APM_DO_SAVE_SEGS; > apm_bios_call_asm(func, ebx_in, ecx_in, eax, ebx, ecx, edx, esi); > APM_DO_RESTORE_SEGS; > - local_irq_restore(flags); > + if (disable_irqs) > + local_irq_disable(); > + if (enable_irqs) > + local_irq_enable(); > gdt[0x40 / 8] = save_desc_40; > put_cpu(); > apm_restore_cpus(cpus); > @@ -627,11 +636,12 @@ static u8 apm_bios_call_simple(u32 func, > { > u8 error; > APM_DECL_SEGS > - unsigned long flags; > cpumask_t cpus; > int cpu; > struct desc_struct save_desc_40; > struct desc_struct *gdt; > + int enable_irqs = 0; > + int disable_irqs = 0; > > cpus = apm_save_cpus(); > > @@ -640,12 +650,25 @@ static u8 apm_bios_call_simple(u32 func, > save_desc_40 = gdt[0x40 / 8]; > gdt[0x40 / 8] = bad_bios_desc; > > - local_save_flags(flags); > - APM_DO_CLI; > + if (apm_info.allow_ints) { > + if (irqs_disabled()) { > + local_irq_enable(); > + disable_irqs = 1; > + } > + } else { > + if (!irqs_disabled()) { > + local_irq_disable(); > + enable_irqs = 1; > + } > + } > + > APM_DO_SAVE_SEGS; > error = apm_bios_call_simple_asm(func, ebx_in, ecx_in, eax); > APM_DO_RESTORE_SEGS; > - local_irq_restore(flags); > + if (disable_irqs) > + local_irq_disable(); > + if (enable_irqs) > + local_irq_enable(); > gdt[0x40 / 8] = save_desc_40; > put_cpu(); > apm_restore_cpus(cpus); > _ > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] lockdep: annotate i386 apm 2006-10-12 6:50 ` Peter Zijlstra @ 2006-10-12 7:29 ` Andrew Morton 2006-10-12 8:13 ` Peter Zijlstra 0 siblings, 1 reply; 9+ messages in thread From: Andrew Morton @ 2006-10-12 7:29 UTC (permalink / raw) To: Peter Zijlstra; +Cc: linux-kernel, sfr, Ingo Molnar On Thu, 12 Oct 2006 08:50:50 +0200 Peter Zijlstra <a.p.zijlstra@chello.nl> wrote: > Was my original _that_ hard to read? The macro was bad enough. Then you want an added another one and made it reference a caller's local variable. Another dummy-shaped dent in my wall. I'd appreciate a fixed-up patch which changes the code's niceness in a positive direction please - I'm obviously not cut out to work on apm.c. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] lockdep: annotate i386 apm 2006-10-12 7:29 ` Andrew Morton @ 2006-10-12 8:13 ` Peter Zijlstra 0 siblings, 0 replies; 9+ messages in thread From: Peter Zijlstra @ 2006-10-12 8:13 UTC (permalink / raw) To: Andrew Morton; +Cc: linux-kernel, sfr, Ingo Molnar Ok, so how about this: --- Lockdep doesn't like to enable interrupts when they are enabled already. BUG: warning at kernel/lockdep.c:1814/trace_hardirqs_on() (Not tainted) [<c04051ed>] show_trace_log_lvl+0x58/0x16a [<c04057fa>] show_trace+0xd/0x10 [<c0405913>] dump_stack+0x19/0x1b [<c043abfb>] trace_hardirqs_on+0xa2/0x11e [<c041463c>] apm_bios_call_simple+0xcd/0xfd [<c0415242>] apm+0x92/0x5b1 [<c0402005>] kernel_thread_helper+0x5/0xb DWARF2 unwinder stuck at kernel_thread_helper+0x5/0xb Leftover inexact backtrace: [<c04057fa>] show_trace+0xd/0x10 [<c0405913>] dump_stack+0x19/0x1b [<c043abfb>] trace_hardirqs_on+0xa2/0x11e [<c041463c>] apm_bios_call_simple+0xcd/0xfd [<c0415242>] apm+0x92/0x5b1 [<c0402005>] kernel_thread_helper+0x5/0xb Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl> --- arch/i386/kernel/apm.c | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-) Index: linux-2.6.18.noarch/arch/i386/kernel/apm.c =================================================================== --- linux-2.6.18.noarch.orig/arch/i386/kernel/apm.c +++ linux-2.6.18.noarch/arch/i386/kernel/apm.c @@ -539,11 +539,30 @@ static inline void apm_restore_cpus(cpum * Also, we KNOW that for the non error case of apm_bios_call, there * is no useful data returned in the low order 8 bits of eax. */ -#define APM_DO_CLI \ - if (apm_info.allow_ints) \ - local_irq_enable(); \ - else \ + +static inline unsigned long __apm_irq_save(void) +{ + unsigned long flags; + local_save_flags(flags); + if (apm_info.allow_ints) { + if (irqs_disabled_flags(flags)) + local_irq_enable(); + } else + local_irq_disable(); + + return flags; +} + +#define apm_irq_save(flags) \ + do { flags = __apm_irq_save(); } while (0) + +static inline void apm_irq_restore(unsigned long flags) +{ + if (irqs_disabled_flags(flags)) local_irq_disable(); + else if (irqs_disabled()) + local_irq_enable(); +} #ifdef APM_ZERO_SEGS # define APM_DECL_SEGS \ @@ -595,12 +614,11 @@ static u8 apm_bios_call(u32 func, u32 eb save_desc_40 = gdt[0x40 / 8]; gdt[0x40 / 8] = bad_bios_desc; - local_save_flags(flags); - APM_DO_CLI; + apm_irq_save(flags); APM_DO_SAVE_SEGS; apm_bios_call_asm(func, ebx_in, ecx_in, eax, ebx, ecx, edx, esi); APM_DO_RESTORE_SEGS; - local_irq_restore(flags); + apm_irq_restore(flags); gdt[0x40 / 8] = save_desc_40; put_cpu(); apm_restore_cpus(cpus); @@ -639,12 +657,11 @@ static u8 apm_bios_call_simple(u32 func, save_desc_40 = gdt[0x40 / 8]; gdt[0x40 / 8] = bad_bios_desc; - local_save_flags(flags); - APM_DO_CLI; + apm_irq_save(flags); APM_DO_SAVE_SEGS; error = apm_bios_call_simple_asm(func, ebx_in, ecx_in, eax); APM_DO_RESTORE_SEGS; - local_irq_restore(flags); + apm_irq_restore(flags); gdt[0x40 / 8] = save_desc_40; put_cpu(); apm_restore_cpus(cpus); ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] lockdep: annotate i386 apm 2006-10-12 6:39 ` Andrew Morton 2006-10-12 6:50 ` Peter Zijlstra @ 2006-10-12 8:02 ` Ingo Molnar 2006-10-12 8:26 ` Peter Zijlstra 1 sibling, 1 reply; 9+ messages in thread From: Ingo Molnar @ 2006-10-12 8:02 UTC (permalink / raw) To: Andrew Morton; +Cc: Peter Zijlstra, linux-kernel, sfr * Andrew Morton <akpm@osdl.org> wrote: > > So, say interrupts were enabled when entering apm_bios_call*(); you now > > save that in flags, disable interrupts, and enable them again. > > Upon reaching local_irq_restore(), we'll hit the else branch with irq's > > enabled and call trace_hardirqs_on(), which goes EEEK! > > I'd assumed lockdep was less stupid than that ;) This? Seems a bit > overdone.. the problem is not lockdep but that the BIOS enables IRQs behind the back of the kernel. Lockdep needs to be taught about that - if this happens unconditionally then i'd suggest to insert an unconditional trace_hardirqs_on() call to after the local_irq_save() that we do prior calling the BIOS. (that will be a NOP if lockdep is not enabled) Ingo ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] lockdep: annotate i386 apm 2006-10-12 8:02 ` Ingo Molnar @ 2006-10-12 8:26 ` Peter Zijlstra 0 siblings, 0 replies; 9+ messages in thread From: Peter Zijlstra @ 2006-10-12 8:26 UTC (permalink / raw) To: Ingo Molnar; +Cc: Andrew Morton, linux-kernel, sfr On Thu, 2006-10-12 at 10:02 +0200, Ingo Molnar wrote: > * Andrew Morton <akpm@osdl.org> wrote: > > > > So, say interrupts were enabled when entering apm_bios_call*(); you now > > > save that in flags, disable interrupts, and enable them again. > > > Upon reaching local_irq_restore(), we'll hit the else branch with irq's > > > enabled and call trace_hardirqs_on(), which goes EEEK! > > > > I'd assumed lockdep was less stupid than that ;) This? Seems a bit > > overdone.. > > the problem is not lockdep but that the BIOS enables IRQs behind the > back of the kernel. Lockdep needs to be taught about that - if this > happens unconditionally then i'd suggest to insert an unconditional > trace_hardirqs_on() call to after the local_irq_save() that we do prior > calling the BIOS. (that will be a NOP if lockdep is not enabled) Well, its not quite like that, the irq mess in apm does an explicit unbalanced irq action, and lockdep rightly triggers on that. And adding to that comes the fact that we do indeed call a BIOS routine which can do god knows what. So we want to save irq state and force it into a known state; normally it will only be disable - however in this case (when apm_info.allow_ints) we force enable it. All end scope routines expect irqs disabled. logically it looks something like this local_irq_save(flags) local_irq_enable() ... local_irq_disable() <--- missing local_irq_restore(flags) except that the first two statements are blended together to avoid the unneeded disable. The logic is sound because local_irq_restore will just set whatever was saved, except lockdep gets confused by the unbalanced operations - a good thing IMHO. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2006-10-12 8:26 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2006-10-11 13:40 [PATCH] lockdep: annotate i386 apm Peter Zijlstra 2006-10-11 21:18 ` Andrew Morton 2006-10-12 6:06 ` Peter Zijlstra 2006-10-12 6:39 ` Andrew Morton 2006-10-12 6:50 ` Peter Zijlstra 2006-10-12 7:29 ` Andrew Morton 2006-10-12 8:13 ` Peter Zijlstra 2006-10-12 8:02 ` Ingo Molnar 2006-10-12 8:26 ` Peter Zijlstra
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.