[patch] "big IRQ lock" removal, 2.5.27-A1

[patch] "big IRQ lock" removal, 2.5.27-A1

[Ingo Molnar]

this is the next iteration of the big-IRQ-lock removal patch, against
2.5.27 + sched-fixes + ldt-fixes:

  http://redhat.com/~mingo/remove-irqlock-patches/remove-irqlock-2.5.27-A1

one clarification wrt. disable_irq(): while it does not synchronize with
all IRQ sources globally anymore, it does synchronize with that particular
irq source (globally) - so drivers can use it safely to ensure that no
pending IRQ handler is running on another CPU after disable_irq() has
returned.

Changes:

 - Oleg Nesterov noticed a do_IRQ() bug which can cause a reschedule in
   do_IRQ().

 - George Anzinger noticed that local_bh_enable() did not re-run softirqs.

 - Linus suggested to still define cli(), sti(), save_flags(),
   restore_flags() on UP kernels, to ease the transition.

the patch compiles, boots & works just fine on UP - on SMP it boots just
fine using the following limited .config:

  http://redhat.com/~mingo/remove-irqlock-patches/config

(the serial subsystem is disabled for example.)

	Ingo

Re: [patch] "big IRQ lock" removal, 2.5.27-A1

[Linus Torvalds]

On Sun, 21 Jul 2002, Ingo Molnar wrote:
>
>   http://redhat.com/~mingo/remove-irqlock-patches/config

This seems to have tons of stuff which makes it compile, but which is just
broken. Randomly changing "cli()" to "__cli()" apparently just to make it
compile, with no warning that its now buggy.

It's doubly wrong by virtue of the fact that you apparently left
"save_flags()" and "restore_flags()" alone, and changed their semantics
rather drastically.

Ugh. That should not happen. Either it compiles and is expected to work,
or it shouldn't compile (very limited config is fine, of course).

Robert, willing to take a look too?

		Linus

Re: [patch] "big IRQ lock" removal, 2.5.27-A1

[Ingo Molnar]

On Sun, 21 Jul 2002, Linus Torvalds wrote:

> This seems to have tons of stuff which makes it compile, but which is
> just broken. Randomly changing "cli()" to "__cli()" apparently just to
> make it compile, with no warning that its now buggy.

indeed ...

fixed these, and have categorized every change whether it's safe,
known-unsafe or unknown-effect, and commented the latter two.

i also removed the save_flags() and restore_flags() macros on SMP which 
were left there by accident.

new patch is at:

  http://redhat.com/~mingo/remove-irqlock-patches/remove-irqlock-2.5.27-A3

(there's one more open bug, i can now see the 'exited with preempt_count
1' messages.)

	Ingo

Re: [patch] "big IRQ lock" removal, 2.5.27-A1

[Ingo Molnar]

> (there's one more open bug, i can now see the 'exited with preempt_count
> 1' messages.)

fixed this bug as well, new patch is at:

   http://redhat.com/~mingo/remove-irqlock-patches/remove-irqlock-2.5.27-A4

	Ingo

Re: [patch] "big IRQ lock" removal, 2.5.27-A1

[Robert Love]

On Sun, 2002-07-21 at 12:14, Ingo Molnar wrote:
> 
> On Sun, 21 Jul 2002, Linus Torvalds wrote:
> 
> > This seems to have tons of stuff which makes it compile, but which is
> > just broken. Randomly changing "cli()" to "__cli()" apparently just to
> > make it compile, with no warning that its now buggy.
> 
> indeed ...
> 
> fixed these, and have categorized every change whether it's safe,
> known-unsafe or unknown-effect, and commented the latter two.

Good.

I have brought up a machine with a config similar but not identical to
yours and I am putting it through the paces (SMP+preempt machine).  I
really think you nailed this dead on - you did it right - we just need
to clean up the mess you left behind ;)

Ingo, looking over the FIXMEs in the tty layer I think they are
definitely _broke_.  At least some of these paths have no global
synchronization now.  Someone really needs to go through this cruft and
clean it up and do some proper locking.

	Robert Love

Re: [patch] "big IRQ lock" removal, 2.5.27-A1

[Russell King]

On Sun, Jul 21, 2002 at 08:50:35PM +0200, Ingo Molnar wrote:
> (the serial subsystem is disabled for example.)

As far as the serial stuff goes:

- William Irvin and Zwane Mwaikambo have been testing it, found a
  deadlock, now fixed. (yay)

- Zwane reports that serial console doesn't work for him.  Oddly,
  it works here on a Netwinder (which has all the bits'n'pieces to
  be close enough to a PC with a PCI bus, southbridge, and standard
  serial ports at standard IO bases and standard IRQs) so I'm at a
  loss why this works for me but not Zwane.

I'm just sorting out a 2.5.26-rmk1 release, then update to 2.5.27,
make sure it builds, and then I'll be sending the serial stuff to
Linus.  Until then, I've no idea if any patch I create will apply
to 2.5.27.

Gimme about an hour or so and I'll have the patch ready.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [patch] "big IRQ lock" removal, 2.5.27-A1

[Ingo Molnar]

On Sun, 21 Jul 2002, Russell King wrote:

> - William Irvin and Zwane Mwaikambo have been testing it, found a
>   deadlock, now fixed. (yay)

good.

> - Zwane reports that serial console doesn't work for him.  Oddly,
>   it works here on a Netwinder (which has all the bits'n'pieces to
>   be close enough to a PC with a PCI bus, southbridge, and standard
>   serial ports at standard IO bases and standard IRQs) so I'm at a
>   loss why this works for me but not Zwane.

i can test this, on SMP as well.

> I'm just sorting out a 2.5.26-rmk1 release, then update to 2.5.27, make
> sure it builds, and then I'll be sending the serial stuff to Linus.  
> Until then, I've no idea if any patch I create will apply to 2.5.27.

okay - since the IRQ changes do not (suppose to) affect UP functionality,
there's time to get it fixed.

	Ingo

[patch] "big IRQ lock" removal, 2.5.27-A5

[Ingo Molnar]

reviewed and fixed the apm.c hacks, it's now using the proper spinlocks
and not cli()/sti(). Latest full patch is at:

    http://redhat.com/~mingo/remove-irqlock-patches/remove-irqlock-2.5.27-A5

(the apm.c changes are attached as well.)

	Ingo

--- linux/arch/i386/kernel/apm.c.orig	Sun Jul 21 20:37:10 2002
+++ linux/arch/i386/kernel/apm.c	Sun Jul 21 22:36:42 2002
@@ -222,6 +222,8 @@
 
 #include <linux/sysrq.h>
 
+extern rwlock_t xtime_lock;
+extern spinlock_t i8253_lock;
 extern unsigned long get_cmos_time(void);
 extern void machine_real_restart(unsigned char *, int);
 
@@ -1141,40 +1143,25 @@
 
 static void set_time(void)
 {
-	unsigned long	flags;
-
-	if (got_clock_diff) {	/* Must know time zone in order to set clock */
-		save_flags(flags);
-		cli();
+	if (got_clock_diff)	/* Must know time zone in order to set clock */
 		CURRENT_TIME = get_cmos_time() + clock_cmos_diff;
-		restore_flags(flags);
-	}
 }
 
 static void get_time_diff(void)
 {
 #ifndef CONFIG_APM_RTC_IS_GMT
-	unsigned long	flags;
-
 	/*
 	 * Estimate time zone so that set_time can update the clock
 	 */
-	save_flags(flags);
 	clock_cmos_diff = -get_cmos_time();
-	cli();
 	clock_cmos_diff += CURRENT_TIME;
 	got_clock_diff = 1;
-	restore_flags(flags);
 #endif
 }
 
-static void reinit_timer(void)
+static inline void reinit_timer(void)
 {
 #ifdef INIT_TIMER_AFTER_SUSPEND
-	unsigned long	flags;
-
-	save_flags(flags);
-	cli();
 	/* set the clock to 100 Hz */
 	outb_p(0x34,0x43);		/* binary, mode 2, LSB/MSB, ch 0 */
 	udelay(10);
@@ -1182,7 +1169,6 @@
 	udelay(10);
 	outb(LATCH >> 8 , 0x40);	/* MSB */
 	udelay(10);
-	restore_flags(flags);
 #endif
 }
 
@@ -1203,13 +1189,21 @@
 		}
 		printk(KERN_CRIT "apm: suspend was vetoed, but suspending anyway.\n");
 	}
+	/* serialize with the timer interrupt */
+	write_lock_irq(&xtime_lock);
+
+	/* protect against access to timer chip registers */
+	spin_lock(&i8253_lock);
+
 	get_time_diff();
-	cli();
 	err = set_system_power_state(APM_STATE_SUSPEND);
 	reinit_timer();
 	set_time();
 	ignore_normal_resume = 1;
-	sti();
+
+	spin_unlock(&i8253_lock);
+	write_unlock_irq(&xtime_lock);
+
 	if (err == APM_NO_ERROR)
 		err = APM_SUCCESS;
 	if (err != APM_SUCCESS)
@@ -1232,8 +1226,12 @@
 {
 	int	err;
 
+	/* serialize with the timer interrupt */
+	write_lock_irq(&xtime_lock);
 	/* If needed, notify drivers here */
 	get_time_diff();
+	write_unlock_irq(&xtime_lock);
+
 	err = set_system_power_state(APM_STATE_STANDBY);
 	if ((err != APM_SUCCESS) && (err != APM_NO_ERROR))
 		apm_error("standby", err);
@@ -1321,7 +1319,9 @@
 			ignore_bounce = 1;
 			if ((event != APM_NORMAL_RESUME)
 			    || (ignore_normal_resume == 0)) {
+				write_lock_irq(&xtime_lock);
 				set_time();
+				write_unlock_irq(&xtime_lock);
 				pm_send_all(PM_RESUME, (void *)0);
 				queue_event(event, NULL);
 			}
@@ -1336,7 +1336,9 @@
 			break;
 
 		case APM_UPDATE_TIME:
+			write_lock_irq(&xtime_lock);
 			set_time();
+			write_unlock_irq(&xtime_lock);
 			break;
 
 		case APM_CRITICAL_SUSPEND:

Re: [patch] "big IRQ lock" removal, 2.5.27-A1

[Russell King]

On Sun, Jul 21, 2002 at 08:58:01PM +0100, Russell King wrote:
> On Sun, Jul 21, 2002 at 08:50:35PM +0200, Ingo Molnar wrote:
> > (the serial subsystem is disabled for example.)
> 
> As far as the serial stuff goes:
> 
> - William Irvin and Zwane Mwaikambo have been testing it, found a
>   deadlock, now fixed. (yay)
> 
> - Zwane reports that serial console doesn't work for him.  Oddly,
>   it works here on a Netwinder (which has all the bits'n'pieces to
>   be close enough to a PC with a PCI bus, southbridge, and standard
>   serial ports at standard IO bases and standard IRQs) so I'm at a
>   loss why this works for me but not Zwane.
> 
> I'm just sorting out a 2.5.26-rmk1 release, then update to 2.5.27,
> make sure it builds, and then I'll be sending the serial stuff to
> Linus.  Until then, I've no idea if any patch I create will apply
> to 2.5.27.
> 
> Gimme about an hour or so and I'll have the patch ready.

Ok, 2.5.27 doesn't seem to touch any of the affected files; the patch
still applies.  In such a short time period, I've not been able to
confirm that it actually works with 2.5.27, only with 2.5.26.

Here's the complete patch; it's rather large, so for mortals it's
available from:

  http://www.arm.linux.org.uk/cvs/serial-2.5.26-3.diff.bz2

I'm going to send it in mail to Linus separately.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [patch] "big IRQ lock" removal, 2.5.27-A5

[Ingo Molnar]

fixed the arch/i386/kernel/mca.c hacks as well:
 
     http://redhat.com/~mingo/remove-irqlock-patches/remove-irqlock-2.5.27-A6

while there cannot be many MCA SMP boxes in existence, it should be SMP
safe as well.

 	Ingo

--- linux/arch/i386/kernel/mca.c.orig	Sun Jun  9 07:27:54 2002
+++ linux/arch/i386/kernel/mca.c	Sun Jul 21 22:49:42 2002
@@ -102,6 +102,12 @@
 
 static struct MCA_info* mca_info = NULL;
 
+/*
+ * Motherboard register spinlock. Untested on SMP at the moment, but
+ * are there any MCA SMP boxes?
+ */
+static spinlock_t mca_lock = SPIN_LOCK_UNLOCKED;
+
 /* MCA registers */
 
 #define MCA_MOTHERBOARD_SETUP_REG	0x94
@@ -213,8 +219,11 @@
 	}
 	memset(mca_info, 0, sizeof(struct MCA_info));
 
-	save_flags(flags);
-	cli();
+	/*
+	 * We do not expect many MCA interrupts during initialization,
+	 * but let us be safe:
+	 */
+	spin_lock_irq(&mca_lock);
 
 	/* Make sure adapter setup is off */
 
@@ -300,8 +309,7 @@
 	outb_p(0, MCA_ADAPTER_SETUP_REG);
 
 	/* Enable interrupts and return memory start */
-
-	restore_flags(flags);
+	spin_unlock_irq(&mca_lock);
 
 	for (i = 0; i < MCA_STANDARD_RESOURCES; i++)
 		request_resource(&ioport_resource, mca_standard_resources + i);
@@ -514,8 +522,7 @@
 	if(slot < 0 || slot >= MCA_NUMADAPTERS || mca_info == NULL) return 0;
 	if(reg < 0 || reg >= 8) return 0;
 
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(&mca_lock, flags);
 
 	/* Make sure motherboard setup is off */
 
@@ -566,7 +573,7 @@
 
 	mca_info->slot[slot].pos[reg] = byte;
 
-	restore_flags(flags);
+	spin_unlock_irqrestore(&mca_lock, flags);
 
 	return byte;
 } /* mca_read_pos() */
@@ -610,8 +617,7 @@
 	if(mca_info == NULL)
 		return;
 
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(&mca_lock, flags);
 
 	/* Make sure motherboard setup is off */
 
@@ -623,7 +629,7 @@
 	outb_p(byte, MCA_POS_REG(reg));
 	outb_p(0, MCA_ADAPTER_SETUP_REG);
 
-	restore_flags(flags);
+	spin_unlock_irqrestore(&mca_lock, flags);
 
 	/* Update the global register list, while we have the byte */
 

[patch] "big IRQ lock" removal, 2.5.27-A7

[Ingo Molnar]

the arch/i386/kernel/vm86.c hacks are fixed now as well:

      http://redhat.com/~mingo/remove-irqlock-patches/remove-irqlock-2.5.27-A7

this was relatively simple, the irqbits global variable needed to be
protected by a global spinlock. I found no other global locking assumption
in the code. In fact there was even a small bug in the code - if
free_vm86_irq()  was called outside of the global IRQ lock then there's a
irqbits corruption window in free_vm87_irq() - this is true even though
that particular IRQ source will not interfere - it's other IRQ sources
that might change the irqbits variable. The patch fixes this.

this was the last remaining x86 usage of cli()/sti().

	Ingo

--- linux/arch/i386/kernel/vm86.c.orig	Sun Jun  9 07:27:22 2002
+++ linux/arch/i386/kernel/vm86.c	Sun Jul 21 22:55:57 2002
@@ -571,6 +571,8 @@
 	struct task_struct *tsk;
 	int sig;
 } vm86_irqs[16];
+
+static spinlock_t irqbits_lock = SPIN_LOCK_UNLOCKED;
 static int irqbits;
 
 #define ALLOWED_SIGS ( 1 /* 0 = don't send a signal */ \
@@ -580,9 +582,8 @@
 static void irq_handler(int intno, void *dev_id, struct pt_regs * regs) {
 	int irq_bit;
 	unsigned long flags;
-	
-	save_flags(flags);
-	cli();
+
+	spin_lock_irqsave(&irqbits_lock, flags);	
 	irq_bit = 1 << intno;
 	if ((irqbits & irq_bit) || ! vm86_irqs[intno].tsk)
 		goto out;
@@ -591,14 +592,19 @@
 		send_sig(vm86_irqs[intno].sig, vm86_irqs[intno].tsk, 1);
 	/* else user will poll for IRQs */
 out:
-	restore_flags(flags);
+	spin_unlock_irqrestore(&irqbits_lock, flags);	
 }
 
 static inline void free_vm86_irq(int irqnumber)
 {
+	unsigned long flags;
+
 	free_irq(irqnumber,0);
 	vm86_irqs[irqnumber].tsk = 0;
+
+	spin_lock_irqsave(&irqbits_lock, flags);	
 	irqbits &= ~(1 << irqnumber);
+	spin_unlock_irqrestore(&irqbits_lock, flags);	
 }
 
 static inline int task_valid(struct task_struct *tsk)
@@ -635,11 +641,10 @@
 	
 	if ( (irqnumber<3) || (irqnumber>15) ) return 0;
 	if (vm86_irqs[irqnumber].tsk != current) return 0;
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(&irqbits_lock, flags);	
 	bit = irqbits & (1 << irqnumber);
 	irqbits &= ~bit;
-	restore_flags(flags);
+	spin_unlock_irqrestore(&irqbits_lock, flags);	
 	return bit;
 }
 

Re: [patch] "big IRQ lock" removal, 2.5.27-A1

[Ingo Molnar]

using serial-2.5.26-3.diff, i get an oops here:

(gdb) list *0xc01b1f03
0xc01b1f03 is in serial_in (serial_8250.c:189).
184                     return readb(up->port.membase + offset);
185
186             default:
187                     return inb(up->port.iobase + offset);
188             }
189     }
190
191     static _INLINE_ void
192     serial_out(struct uart_8250_port *up, int offset, int value)
193     {
(gdb)

called from:

(gdb) list *0xc01b3450
0xc01b3450 is in serial8250_interrupt (serial_8250.c:946).
941                     struct uart_8250_port *up;
942                     unsigned int iir;
943
944                     up = list_entry(l, struct uart_8250_port, list);
945
946                     iir = serial_in(up, UART_IIR);
947                     if (!(iir & UART_IIR_NO_INT)) {
948                             spin_lock(&up->port.lock);
949                             serial8250_handle_port(up, regs);
950                             spin_unlock(&up->port.lock);
(gdb)

when echo-ing into a serial port, which is also set up for kernel serial
console. (the kernel serial console produces no output.)

	Ingo

Re: [patch] "big IRQ lock" removal, 2.5.27-A1

[Russell King]

On Sun, Jul 21, 2002 at 11:09:34PM +0200, Ingo Molnar wrote:
> using serial-2.5.26-3.diff, i get an oops here:
> 
> (gdb) list *0xc01b1f03
> 0xc01b1f03 is in serial_in (serial_8250.c:189).
> 184                     return readb(up->port.membase + offset);
> 185
> 186             default:
> 187                     return inb(up->port.iobase + offset);
> 188             }
> 189     }
> 190
> 191     static _INLINE_ void
> 192     serial_out(struct uart_8250_port *up, int offset, int value)
> 193     {
> (gdb)

Interesting.  Not had a report of that thus far.  Can you send me any
kernel messages relating to serial devices please, and the bad address
that caused the oops?  (line 189 is obviously a lie...)

> when echo-ing into a serial port, which is also set up for kernel serial
> console. (the kernel serial console produces no output.)

Weird.  Currently I've no idea what's causing this; I've been booting
machines with "console=ttyS0,115200n8" fine here with no noticable
problems.

Again, any useful kernel messages?

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [patch] "big IRQ lock" removal, 2.5.27-A1

[Ingo Molnar]

On Sun, 21 Jul 2002, Russell King wrote:

> > (gdb) list *0xc01b1f03
> > 0xc01b1f03 is in serial_in (serial_8250.c:189).
> > 184                     return readb(up->port.membase + offset);
> > 185
> > 186             default:
> > 187                     return inb(up->port.iobase + offset);
> > 188             }
> > 189     }
> > 190
> > 191     static _INLINE_ void
> > 192     serial_out(struct uart_8250_port *up, int offset, int value)
> > 193     {
> > (gdb)
> 
> Interesting.  Not had a report of that thus far.  Can you send me any
> kernel messages relating to serial devices please, and the bad address
> that caused the oops?  (line 189 is obviously a lie...)

i believe it was the inb's dereference.

i dont have the oops around anymore, since serial logging is not working
;-) Had to write it down from screen.

> > when echo-ing into a serial port, which is also set up for kernel serial
> > console. (the kernel serial console produces no output.)
> 
> Weird.  Currently I've no idea what's causing this; I've been booting
> machines with "console=ttyS0,115200n8" fine here with no noticable
> problems.
> 
> Again, any useful kernel messages?

will reproduce and write down the full oops - it was an illegal
dereference on 0xfffffff8 (-8 offset of a NULL pointer) or something like
that.

my serial setup is:

        append="console=ttyS1,115200 console=tty0"

old serial driver:

 Serial driver version 5.05c (2001-07-08) with MANY_PORTS SHARE_IRQ 
 SERIAL_PCI enabled
 ttyS00 at 0x03f8 (irq = 4) is a 16550A
 ttyS01 at 0x02f8 (irq = 3) is a 16550A

i'm pretty sure it hung when i echoed to ttyS1 - not 100% sure though.

	Ingo

[patch] "big IRQ lock" removal, 2.5.27-A9

[Ingo Molnar]

the genhd.c bit is safe as well, removed the comment. Only the tty and the
ide/main.c changes are left the 'dubious' category - everything else is
supposed to be safe. (and of course there's all the other stuff that does
not compile at the moment.) Latest patch is at:
 
    http://redhat.com/~mingo/remove-irqlock-patches/remove-irqlock-2.5.27-A9

	Ingo

Re: [patch] "big IRQ lock" removal, 2.5.27-A1

[Ingo Molnar]

new-serial boot messages:

 Serial: 8250/16550 driver $Revision: 1.80 $ IRQ sharing enabled
 ttyS0 at I/O 0x3f8 (irq = 4) is a 16550A
 ttyS1 at I/O 0x2f8 (irq = 3) is a 16550A

and it does not crash on UP. (will re-try with SMP now.) Still no serial
console output though.

	Ingo

Re: [patch] "big IRQ lock" removal, 2.5.27-A1

[Ingo Molnar]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2614 bytes --]


okay, the crash happens if i boot an SMP kernel, and do the following:

	cat /dev/ttyS0

	[ ... system works at this point, no crash ... ]

	Ctrl-C

	[ ... system crashes ... ]

note that ttyS0 is not the serial console device - so this is a plain
unconnected serial port.

the oops, written down by hand:

 Unable to handle kernel paging request at virtual address ffffff8a
 c01aa193
 CPU:   1
 EIP:    0010:[<c01aa193>]   Not tainted
 Using defaults from ksymoops -t elf32-i386 -a i386
 EFLAGS: 00010292
 eax: 00000002 ebx: ffffff70 ecx: 00000000 edx: 000000ff
 esi: ffffff70 edi: 00000000 ebp: c02d3ad0 esp: c13e1edc
 Call Trace: [<c01ab696>] [<c0114210>] [<c0109d79>] [<c010a07e>] 
 [<c0113821>]
 [<c0105470>] [<c0108347>] [<c01054c0>] [<c0105470>] [<c01054eb>] 
 [<c010553a>]
 [<c011d3ac>] [<c011d1ca>]
 Aiee, killing interrupt handler!
 Warning (Oops_read): Code line not seen, dumping what data is available

>>EIP; c01aa193 <serial_in+13/80>   <=====
Trace; c01ab696 <serial8250_interrupt+66/1a0>
Trace; c0114210 <move+50/90>
Trace; c0109d79 <handle_IRQ_event+69/a0>
Trace; c010a07e <do_IRQ+ee/190>
Trace; c0113821 <smp_apic_timer_interrupt+131/140>
Trace; c0105470 <default_idle+0/50>
Trace; c0108347 <common_interrupt+1f/24>
Trace; c01054c0 <poll_idle+0/40>
Trace; c0105470 <default_idle+0/50>
Trace; c01054eb <poll_idle+2b/40>
Trace; c010553a <cpu_idle+3a/50>
Trace; c011d3ac <release_console_sem+11c/120>
Trace; c011d1ca <printk+18a/200>

ie. the second, idle CPU received a serial interrupt and crashed in it.

the .config used for this is attached. The gdb backtrace:

(gdb) list *0xc01aa193
0xc01aa193 is in serial_in (serial_8250.c:176).
171             { "RSA",        2048,   UART_CLEAR_FIFO | UART_USE_FIFO }
172     };
173
174     static _INLINE_ unsigned int serial_in(struct uart_8250_port *up, int offset)
175     {
176             offset <<= up->port.regshift;
177
178             switch (up->port.iotype) {
179             case SERIAL_IO_HUB6:
180                     outb(up->port.hub6 - 1 + offset, up->port.iobase);
(gdb)

(gdb) list *0xc01ab696
0xc01ab696 is in serial8250_interrupt (serial_8250.c:947).
942                     unsigned int iir;
943
944                     up = list_entry(l, struct uart_8250_port, list);
945
946                     iir = serial_in(up, UART_IIR);
947                     if (!(iir & UART_IIR_NO_INT)) {
948                             spin_lock(&up->port.lock);
949                             serial8250_handle_port(up, regs);
950                             spin_unlock(&up->port.lock);
951
(gdb)

this backtrace looks more credible to me.

	Ingo

[-- Attachment #2: Type: TEXT/PLAIN, Size: 15305 bytes --]

#
# Automatically generated by make menuconfig: don't edit
#
CONFIG_X86=y
CONFIG_ISA=y
# CONFIG_SBUS is not set
CONFIG_UID16=y

#
# Code maturity level options
#
CONFIG_EXPERIMENTAL=y

#
# General setup
#
CONFIG_NET=y
CONFIG_SYSVIPC=y
# CONFIG_BSD_PROCESS_ACCT is not set
CONFIG_SYSCTL=y

#
# Loadable module support
#
# CONFIG_MODULES is not set

#
# Processor type and features
#
# CONFIG_M386 is not set
# CONFIG_M486 is not set
# CONFIG_M586 is not set
# CONFIG_M586TSC is not set
# CONFIG_M586MMX is not set
CONFIG_M686=y
# CONFIG_MPENTIUMIII is not set
# CONFIG_MPENTIUM4 is not set
# CONFIG_MK6 is not set
# CONFIG_MK7 is not set
# CONFIG_MELAN is not set
# CONFIG_MCRUSOE is not set
# CONFIG_MWINCHIPC6 is not set
# CONFIG_MWINCHIP2 is not set
# CONFIG_MWINCHIP3D is not set
# CONFIG_MCYRIXIII is not set
CONFIG_X86_WP_WORKS_OK=y
CONFIG_X86_INVLPG=y
CONFIG_X86_CMPXCHG=y
CONFIG_X86_XADD=y
CONFIG_X86_BSWAP=y
CONFIG_X86_POPAD_OK=y
# CONFIG_RWSEM_GENERIC_SPINLOCK is not set
CONFIG_RWSEM_XCHGADD_ALGORITHM=y
CONFIG_X86_L1_CACHE_SHIFT=5
CONFIG_X86_TSC=y
CONFIG_X86_GOOD_APIC=y
CONFIG_X86_USE_PPRO_CHECKSUM=y
CONFIG_X86_PPRO_FENCE=y
CONFIG_SMP=y
# CONFIG_PREEMPT is not set
# CONFIG_MULTIQUAD is not set
# CONFIG_X86_MCE is not set
# CONFIG_X86_MCE_NONFATAL is not set
# CONFIG_X86_MCE_P4THERMAL is not set
# CONFIG_TOSHIBA is not set
# CONFIG_I8K is not set
# CONFIG_MICROCODE is not set
# CONFIG_X86_MSR is not set
# CONFIG_X86_CPUID is not set
CONFIG_NOHIGHMEM=y
# CONFIG_HIGHMEM4G is not set
# CONFIG_HIGHMEM64G is not set
# CONFIG_MATH_EMULATION is not set
CONFIG_MTRR=y
CONFIG_HAVE_DEC_LOCK=y

#
# Power management options (ACPI, APM)
#

#
# ACPI Support
#
# CONFIG_ACPI is not set
# CONFIG_PM is not set
# CONFIG_APM is not set

#
# Bus options (PCI, PCMCIA, EISA, MCA, ISA)
#
CONFIG_X86_IO_APIC=y
CONFIG_X86_LOCAL_APIC=y
CONFIG_PCI=y
# CONFIG_PCI_GOBIOS is not set
# CONFIG_PCI_GODIRECT is not set
CONFIG_PCI_GOANY=y
CONFIG_PCI_BIOS=y
CONFIG_PCI_DIRECT=y
# CONFIG_PCI_NAMES is not set
# CONFIG_EISA is not set
# CONFIG_MCA is not set
CONFIG_HOTPLUG=y

#
# PCMCIA/CardBus support
#
# CONFIG_PCMCIA is not set

#
# PCI Hotplug Support
#
# CONFIG_HOTPLUG_PCI is not set
# CONFIG_HOTPLUG_PCI_COMPAQ is not set
# CONFIG_HOTPLUG_PCI_COMPAQ_NVRAM is not set
# CONFIG_HOTPLUG_PCI_IBM is not set

#
# Executable file formats
#
CONFIG_KCORE_ELF=y
# CONFIG_KCORE_AOUT is not set
CONFIG_BINFMT_AOUT=y
CONFIG_BINFMT_ELF=y
CONFIG_BINFMT_MISC=y

#
# Memory Technology Devices (MTD)
#
# CONFIG_MTD is not set

#
# Parallel port support
#
# CONFIG_PARPORT is not set

#
# Plug and Play configuration
#
# CONFIG_PNP is not set
# CONFIG_ISAPNP is not set
# CONFIG_PNPBIOS is not set

#
# Block devices
#
# CONFIG_BLK_DEV_FD is not set
# CONFIG_BLK_DEV_XD is not set
# CONFIG_PARIDE is not set
# CONFIG_BLK_CPQ_DA is not set
# CONFIG_BLK_CPQ_CISS_DA is not set
# CONFIG_CISS_SCSI_TAPE is not set
# CONFIG_BLK_DEV_DAC960 is not set
# CONFIG_BLK_DEV_UMEM is not set
# CONFIG_BLK_DEV_LOOP is not set
# CONFIG_BLK_DEV_NBD is not set
# CONFIG_BLK_DEV_RAM is not set
# CONFIG_BLK_DEV_INITRD is not set

#
# Multi-device support (RAID and LVM)
#
# CONFIG_MD is not set
# CONFIG_BLK_DEV_MD is not set
# CONFIG_MD_LINEAR is not set
# CONFIG_MD_RAID0 is not set
# CONFIG_MD_RAID1 is not set
# CONFIG_MD_RAID5 is not set
# CONFIG_MD_MULTIPATH is not set
# CONFIG_BLK_DEV_LVM is not set

#
# Networking options
#
CONFIG_PACKET=y
# CONFIG_PACKET_MMAP is not set
# CONFIG_NETLINK_DEV is not set
# CONFIG_NETFILTER is not set
# CONFIG_FILTER is not set
CONFIG_UNIX=y
CONFIG_INET=y
# CONFIG_IP_MULTICAST is not set
CONFIG_IP_ADVANCED_ROUTER=y
# CONFIG_IP_MULTIPLE_TABLES is not set
# CONFIG_IP_ROUTE_MULTIPATH is not set
# CONFIG_IP_ROUTE_TOS is not set
# CONFIG_IP_ROUTE_VERBOSE is not set
# CONFIG_IP_ROUTE_LARGE_TABLES is not set
# CONFIG_IP_PNP is not set
# CONFIG_NET_IPIP is not set
# CONFIG_NET_IPGRE is not set
# CONFIG_ARPD is not set
# CONFIG_INET_ECN is not set
# CONFIG_SYN_COOKIES is not set
# CONFIG_IPV6 is not set
# CONFIG_KHTTPD is not set
# CONFIG_ATM is not set
# CONFIG_VLAN_8021Q is not set
# CONFIG_IPX is not set
# CONFIG_ATALK is not set

#
# Appletalk devices
#
# CONFIG_DEV_APPLETALK is not set
# CONFIG_DECNET is not set
# CONFIG_BRIDGE is not set
# CONFIG_X25 is not set
# CONFIG_LAPB is not set
# CONFIG_LLC is not set
# CONFIG_NET_DIVERT is not set
# CONFIG_ECONET is not set
# CONFIG_WAN_ROUTER is not set
# CONFIG_NET_FASTROUTE is not set
# CONFIG_NET_HW_FLOWCONTROL is not set

#
# QoS and/or fair queueing
#
# CONFIG_NET_SCHED is not set

#
# Telephony Support
#
# CONFIG_PHONE is not set
# CONFIG_PHONE_IXJ is not set
# CONFIG_PHONE_IXJ_PCMCIA is not set

#
# ATA/IDE/MFM/RLL support
#
CONFIG_IDE=y

#
# ATA and ATAPI Block devices
#
CONFIG_BLK_DEV_IDE=y
# CONFIG_BLK_DEV_HD_IDE is not set
# CONFIG_BLK_DEV_HD is not set
CONFIG_BLK_DEV_IDEDISK=y
CONFIG_IDEDISK_MULTI_MODE=y
# CONFIG_IDEDISK_STROKE is not set
# CONFIG_BLK_DEV_IDECS is not set
CONFIG_BLK_DEV_IDECD=y
# CONFIG_BLK_DEV_IDETAPE is not set
# CONFIG_BLK_DEV_IDEFLOPPY is not set
# CONFIG_BLK_DEV_IDESCSI is not set
# CONFIG_BLK_DEV_CMD640 is not set
# CONFIG_BLK_DEV_CMD640_ENHANCED is not set
# CONFIG_BLK_DEV_ISAPNP is not set
# CONFIG_BLK_DEV_RZ1000 is not set
# CONFIG_BLK_DEV_OFFBOARD is not set
CONFIG_IDEPCI_SHARE_IRQ=y
CONFIG_BLK_DEV_IDEDMA_PCI=y
CONFIG_IDEDMA_PCI_AUTO=y
# CONFIG_IDEDMA_ONLYDISK is not set
CONFIG_BLK_DEV_IDEDMA=y
# CONFIG_BLK_DEV_IDE_TCQ is not set
# CONFIG_BLK_DEV_IDE_TCQ_DEFAULT is not set
CONFIG_IDEDMA_NEW_DRIVE_LISTINGS=y
# CONFIG_BLK_DEV_AEC62XX is not set
# CONFIG_AEC6280_BURST is not set
# CONFIG_BLK_DEV_ALI15X3 is not set
# CONFIG_WDC_ALI15X3 is not set
# CONFIG_BLK_DEV_AMD74XX is not set
# CONFIG_BLK_DEV_CMD64X is not set
# CONFIG_BLK_DEV_CY82C693 is not set
# CONFIG_BLK_DEV_CS5530 is not set
# CONFIG_BLK_DEV_HPT34X is not set
# CONFIG_HPT34X_AUTODMA is not set
# CONFIG_BLK_DEV_HPT366 is not set
CONFIG_BLK_DEV_PIIX=y
# CONFIG_BLK_DEV_NS87415 is not set
# CONFIG_BLK_DEV_OPTI621 is not set
# CONFIG_BLK_DEV_PDC202XX is not set
# CONFIG_PDC202XX_BURST is not set
# CONFIG_PDC202XX_FORCE is not set
# CONFIG_BLK_DEV_SVWKS is not set
# CONFIG_BLK_DEV_SIS5513 is not set
# CONFIG_BLK_DEV_TRM290 is not set
# CONFIG_BLK_DEV_VIA82CXXX is not set
# CONFIG_BLK_DEV_SL82C105 is not set
# CONFIG_IDE_CHIPSETS is not set
# CONFIG_IDEDMA_IVB is not set
CONFIG_ATAPI=y
CONFIG_IDEDMA_AUTO=y
# CONFIG_BLK_DEV_ATARAID is not set
# CONFIG_BLK_DEV_ATARAID_PDC is not set
# CONFIG_BLK_DEV_ATARAID_HPT is not set

#
# SCSI support
#
# CONFIG_SCSI is not set

#
# Fusion MPT device support
#
# CONFIG_FUSION is not set
# CONFIG_FUSION_BOOT is not set
# CONFIG_FUSION_ISENSE is not set
# CONFIG_FUSION_CTL is not set
# CONFIG_FUSION_LAN is not set

#
# IEEE 1394 (FireWire) support (EXPERIMENTAL)
#
# CONFIG_IEEE1394 is not set

#
# I2O device support
#
# CONFIG_I2O is not set
# CONFIG_I2O_PCI is not set
# CONFIG_I2O_BLOCK is not set
# CONFIG_I2O_LAN is not set
# CONFIG_I2O_SCSI is not set
# CONFIG_I2O_PROC is not set

#
# Network device support
#
CONFIG_NETDEVICES=y

#
# ARCnet devices
#
# CONFIG_ARCNET is not set
# CONFIG_DUMMY is not set
# CONFIG_BONDING is not set
# CONFIG_EQUALIZER is not set
# CONFIG_TUN is not set
# CONFIG_ETHERTAP is not set

#
# Ethernet (10 or 100Mbit)
#
CONFIG_NET_ETHERNET=y
# CONFIG_SUNLANCE is not set
# CONFIG_HAPPYMEAL is not set
# CONFIG_SUNBMAC is not set
# CONFIG_SUNQE is not set
# CONFIG_SUNGEM is not set
# CONFIG_NET_VENDOR_3COM is not set
# CONFIG_LANCE is not set
# CONFIG_NET_VENDOR_SMC is not set
# CONFIG_NET_VENDOR_RACAL is not set
# CONFIG_AT1700 is not set
# CONFIG_DEPCA is not set
# CONFIG_HP100 is not set
# CONFIG_NET_ISA is not set
CONFIG_NET_PCI=y
# CONFIG_PCNET32 is not set
# CONFIG_ADAPTEC_STARFIRE is not set
# CONFIG_AC3200 is not set
# CONFIG_APRICOT is not set
# CONFIG_CS89x0 is not set
# CONFIG_DGRS is not set
CONFIG_EEPRO100=y
# CONFIG_E100 is not set
# CONFIG_LNE390 is not set
# CONFIG_FEALNX is not set
# CONFIG_NATSEMI is not set
# CONFIG_NE2K_PCI is not set
# CONFIG_NE3210 is not set
# CONFIG_ES3210 is not set
# CONFIG_8139CP is not set
# CONFIG_8139TOO is not set
# CONFIG_8139TOO_PIO is not set
# CONFIG_8139TOO_TUNE_TWISTER is not set
# CONFIG_8139TOO_8129 is not set
# CONFIG_8139_NEW_RX_RESET is not set
# CONFIG_SIS900 is not set
# CONFIG_EPIC100 is not set
# CONFIG_SUNDANCE is not set
# CONFIG_TLAN is not set
# CONFIG_VIA_RHINE is not set
# CONFIG_VIA_RHINE_MMIO is not set
# CONFIG_NET_POCKET is not set

#
# Ethernet (1000 Mbit)
#
# CONFIG_ACENIC is not set
# CONFIG_DL2K is not set
# CONFIG_E1000 is not set
# CONFIG_MYRI_SBUS is not set
# CONFIG_NS83820 is not set
# CONFIG_HAMACHI is not set
# CONFIG_YELLOWFIN is not set
# CONFIG_SK98LIN is not set
# CONFIG_TIGON3 is not set
# CONFIG_FDDI is not set
# CONFIG_HIPPI is not set
# CONFIG_PLIP is not set
# CONFIG_PPP is not set
# CONFIG_SLIP is not set

#
# Wireless LAN (non-hamradio)
#
# CONFIG_NET_RADIO is not set

#
# Token Ring devices
#
# CONFIG_TR is not set
# CONFIG_NET_FC is not set
# CONFIG_RCPCI is not set
# CONFIG_SHAPER is not set

#
# Wan interfaces
#
# CONFIG_WAN is not set

#
# Tulip family network device support
#
# CONFIG_NET_TULIP is not set

#
# Amateur Radio support
#
# CONFIG_HAMRADIO is not set

#
# IrDA (infrared) support
#
# CONFIG_IRDA is not set

#
# ISDN subsystem
#
# CONFIG_ISDN_BOOL is not set

#
# Old CD-ROM drivers (not SCSI, not IDE)
#
# CONFIG_CD_NO_IDESCSI is not set

#
# Input device support
#
# CONFIG_INPUT is not set
# CONFIG_INPUT_KEYBDEV is not set
# CONFIG_INPUT_MOUSEDEV is not set
# CONFIG_INPUT_MOUSEDEV_PSAUX is not set
# CONFIG_INPUT_JOYDEV is not set
# CONFIG_INPUT_TSDEV is not set
# CONFIG_INPUT_EVDEV is not set
# CONFIG_INPUT_EVBUG is not set
# CONFIG_INPUT_UINPUT is not set
# CONFIG_GAMEPORT is not set
CONFIG_SOUND_GAMEPORT=y
# CONFIG_GAMEPORT_NS558 is not set
# CONFIG_GAMEPORT_L4 is not set
# CONFIG_GAMEPORT_EMU10K1 is not set
# CONFIG_GAMEPORT_VORTEX is not set
# CONFIG_GAMEPORT_FM801 is not set
# CONFIG_GAMEPORT_CS461x is not set
# CONFIG_SERIO is not set
# CONFIG_SERIO_I8042 is not set
# CONFIG_SERIO_SERPORT is not set
# CONFIG_SERIO_CT82C710 is not set
# CONFIG_SERIO_Q40KBD is not set
# CONFIG_SERIO_PARKBD is not set

#
# Character devices
#
CONFIG_VT=y
CONFIG_VT_CONSOLE=y
# CONFIG_SERIAL_NONSTANDARD is not set

#
# Serial drivers
#
CONFIG_SERIAL_8250=y
CONFIG_SERIAL_8250_CONSOLE=y
# CONFIG_SERIAL_8250_CS is not set
CONFIG_SERIAL_8250_EXTENDED=y
# CONFIG_SERIAL_8250_MANY_PORTS is not set
CONFIG_SERIAL_8250_SHARE_IRQ=y
CONFIG_SERIAL_8250_DETECT_IRQ=y
# CONFIG_SERIAL_8250_MULTIPORT is not set
# CONFIG_SERIAL_8250_RSA is not set
CONFIG_SERIAL_CORE=y
CONFIG_SERIAL_CORE_CONSOLE=y
# CONFIG_UNIX98_PTYS is not set

#
# I2C support
#
# CONFIG_I2C is not set

#
# Mice
#
# CONFIG_BUSMOUSE is not set
CONFIG_PSMOUSE=y
# CONFIG_QIC02_TAPE is not set

#
# Watchdog Cards
#
CONFIG_WATCHDOG=y
# CONFIG_WATCHDOG_NOWAYOUT is not set
# CONFIG_SOFT_WATCHDOG is not set
# CONFIG_WDT is not set
# CONFIG_WDTPCI is not set
# CONFIG_PCWATCHDOG is not set
# CONFIG_ACQUIRE_WDT is not set
# CONFIG_ADVANTECH_WDT is not set
# CONFIG_EUROTECH_WDT is not set
# CONFIG_IB700_WDT is not set
# CONFIG_I810_TCO is not set
# CONFIG_MIXCOMWD is not set
# CONFIG_60XX_WDT is not set
# CONFIG_W83877F_WDT is not set
# CONFIG_MACHZ_WDT is not set
# CONFIG_INTEL_RNG is not set
# CONFIG_NVRAM is not set
# CONFIG_RTC is not set
# CONFIG_DTLK is not set
# CONFIG_R3964 is not set
# CONFIG_APPLICOM is not set
# CONFIG_SONYPI is not set

#
# Ftape, the floppy tape device driver
#
# CONFIG_FTAPE is not set
# CONFIG_AGP is not set
# CONFIG_DRM is not set
# CONFIG_MWAVE is not set

#
# Multimedia devices
#
# CONFIG_VIDEO_DEV is not set

#
# File systems
#
# CONFIG_QUOTA is not set
# CONFIG_QFMT_V1 is not set
# CONFIG_QFMT_V2 is not set
# CONFIG_AUTOFS_FS is not set
# CONFIG_AUTOFS4_FS is not set
# CONFIG_REISERFS_FS is not set
# CONFIG_REISERFS_CHECK is not set
# CONFIG_REISERFS_PROC_INFO is not set
# CONFIG_ADFS_FS is not set
# CONFIG_ADFS_FS_RW is not set
# CONFIG_AFFS_FS is not set
# CONFIG_HFS_FS is not set
# CONFIG_BFS_FS is not set
CONFIG_EXT3_FS=y
CONFIG_JBD=y
# CONFIG_JBD_DEBUG is not set
# CONFIG_FAT_FS is not set
# CONFIG_MSDOS_FS is not set
# CONFIG_UMSDOS_FS is not set
# CONFIG_VFAT_FS is not set
# CONFIG_EFS_FS is not set
# CONFIG_JFFS_FS is not set
# CONFIG_JFFS2_FS is not set
# CONFIG_CRAMFS is not set
CONFIG_TMPFS=y
CONFIG_RAMFS=y
# CONFIG_ISO9660_FS is not set
# CONFIG_JOLIET is not set
# CONFIG_ZISOFS is not set
# CONFIG_JFS_FS is not set
# CONFIG_JFS_DEBUG is not set
# CONFIG_JFS_STATISTICS is not set
# CONFIG_MINIX_FS is not set
# CONFIG_VXFS_FS is not set
# CONFIG_NTFS_FS is not set
# CONFIG_NTFS_DEBUG is not set
# CONFIG_HPFS_FS is not set
CONFIG_PROC_FS=y
# CONFIG_DEVFS_FS is not set
# CONFIG_DEVFS_MOUNT is not set
# CONFIG_DEVFS_DEBUG is not set
# CONFIG_DEVPTS_FS is not set
# CONFIG_QNX4FS_FS is not set
# CONFIG_QNX4FS_RW is not set
# CONFIG_ROMFS_FS is not set
CONFIG_EXT2_FS=y
# CONFIG_SYSV_FS is not set
# CONFIG_UDF_FS is not set
# CONFIG_UDF_RW is not set
# CONFIG_UFS_FS is not set
# CONFIG_UFS_FS_WRITE is not set

#
# Network File Systems
#
# CONFIG_CODA_FS is not set
# CONFIG_INTERMEZZO_FS is not set
# CONFIG_NFS_FS is not set
# CONFIG_NFS_V3 is not set
# CONFIG_ROOT_NFS is not set
# CONFIG_NFSD is not set
# CONFIG_NFSD_V3 is not set
# CONFIG_NFSD_TCP is not set
# CONFIG_SUNRPC is not set
# CONFIG_LOCKD is not set
# CONFIG_EXPORTFS is not set
# CONFIG_SMB_FS is not set
# CONFIG_NCP_FS is not set
# CONFIG_NCPFS_PACKET_SIGNING is not set
# CONFIG_NCPFS_IOCTL_LOCKING is not set
# CONFIG_NCPFS_STRONG is not set
# CONFIG_NCPFS_NFS_NS is not set
# CONFIG_NCPFS_OS2_NS is not set
# CONFIG_NCPFS_SMALLDOS is not set
# CONFIG_NCPFS_NLS is not set
# CONFIG_NCPFS_EXTRAS is not set
# CONFIG_ZISOFS_FS is not set

#
# Partition Types
#
# CONFIG_PARTITION_ADVANCED is not set
CONFIG_MSDOS_PARTITION=y
# CONFIG_SMB_NLS is not set
# CONFIG_NLS is not set

#
# Console drivers
#
CONFIG_VGA_CONSOLE=y
# CONFIG_VIDEO_SELECT is not set
# CONFIG_MDA_CONSOLE is not set

#
# Frame-buffer support
#
# CONFIG_FB is not set

#
# Sound
#
# CONFIG_SOUND is not set

#
# USB support
#
# CONFIG_USB is not set

#
# Bluetooth support
#
# CONFIG_BLUEZ is not set

#
# Kernel hacking
#
# CONFIG_SOFTWARE_SUSPEND is not set
CONFIG_DEBUG_KERNEL=y
CONFIG_DEBUG_SLAB=y
CONFIG_DEBUG_IOVIRT=y
CONFIG_MAGIC_SYSRQ=y
CONFIG_DEBUG_SPINLOCK=y

#
# Security options
#
CONFIG_SECURITY_CAPABILITIES=y

#
# Library routines
#
# CONFIG_CRC32 is not set
# CONFIG_ZLIB_INFLATE is not set
# CONFIG_ZLIB_DEFLATE is not set

Re: [patch] "big IRQ lock" removal, 2.5.27-A9

[Christoph Hellwig]

On Sun, Jul 21, 2002 at 11:26:40PM +0200, Ingo Molnar wrote:
> 
> the genhd.c bit is safe as well, removed the comment.

Is there any reason the sti is there at all?  In -dj almost all drivers
use module_init() now so it becomes increasingly useless..

Re: [patch] "big IRQ lock" removal, 2.5.27-A9

[Ingo Molnar]

On Sun, 21 Jul 2002, Christoph Hellwig wrote:

> > the genhd.c bit is safe as well, removed the comment.
> 
> Is there any reason the sti is there at all?  In -dj almost all drivers
> use module_init() now so it becomes increasingly useless..

well, indeed. While the sti() can be understood to a certain degree - we
used to boot with the IRQ lock on and accidentally leaving it enabled can
cause problems - but otherwise preceeding code should not disable
interrupts in an unbalanced way. I've removed the __sti() from my tree.

there's even more ancient code in the block driver init path, eg. in
drivers/block/ll_rw_blk.c:blk_dev_init():

        outb_p(0xc, 0x3f2);

i suspect this is ancient Linux code. 0x3f2 is one of the floppy
controller ports - many modern x86 boxes do not even have a floppy
controller! I've removed this from my tree as well - if this is needed at
all then it belongs into the floppy driver. Latest patch is at:
    
  http://redhat.com/~mingo/remove-irqlock-patches/remove-irqlock-2.5.27-B0

	Ingo

Re: [patch] "big IRQ lock" removal, 2.5.27-A5

[Alan Cox]

On Sun, 2002-07-21 at 21:50, Ingo Molnar wrote:
> 
> fixed the arch/i386/kernel/mca.c hacks as well:
>  
>      http://redhat.com/~mingo/remove-irqlock-patches/remove-irqlock-2.5.27-A6
> 
> while there cannot be many MCA SMP boxes in existence, it should be SMP
> safe as well.

The NCR boxes and the 9595 are probably about it. There are plenty of
9595's around but if you ever acquire one make sure someone else is
paying postage 8)

Re: [patch] "big IRQ lock" removal, 2.5.27-A1

[Ingo Molnar]

On 21 Jul 2002, Robert Love wrote:

> Ingo, looking over the FIXMEs in the tty layer I think they are
> definitely _broke_.  At least some of these paths have no global
> synchronization now.  Someone really needs to go through this cruft and
> clean it up and do some proper locking.

sure, feel free.

	Ingo

Re: [patch] "big IRQ lock" removal, 2.5.27-A9

[Linus Torvalds]

>   http://redhat.com/~mingo/remove-irqlock-patches/remove-irqlock-2.5.27-B0

Looks good. Merged into my BK tree now, so please do future patches
against this one..

		Linus

Re: [patch] "big IRQ lock" removal, 2.5.27-A1

[Ingo Molnar]

update: after some offline debugging and RMK fixing a number of (mostly
SMP) bugs, both the serial line discipline and serial kernel console
features are now fully functional under the new serial subsystem. x86 SMP
and UP works fine as well.

i also applied it to the irqlock tree, and only a single line had to be
changed [synchronize_irq()] and it worked flawlessly on SMP.

i've uploaded the patch (against sched + irqlock tree), it's at:

  http://redhat.com/~mingo/remove-irqlock-patches/newserial-2.5.27-A0.bz2

I'd vote for Russell's stuff to be considered for inclusion ...

	Ingo

Re: [patch] "big IRQ lock" removal, 2.5.27-A9

[Russell King]

On Sun, Jul 21, 2002 at 11:56:11PM +0200, Ingo Molnar wrote:
> there's even more ancient code in the block driver init path, eg. in
> drivers/block/ll_rw_blk.c:blk_dev_init():
> 
>         outb_p(0xc, 0x3f2);
> 
> i suspect this is ancient Linux code. 0x3f2 is one of the floppy
> controller ports - many modern x86 boxes do not even have a floppy
> controller! I've removed this from my tree as well - if this is needed at
> all then it belongs into the floppy driver.

Actually its to cover the case where you have a floppy drive, and you've
booted the kernel from a floppy disk, and the kernel doesn't have the
floppy driver built in.  It turns the floppy drive off, cause there's
nothing else to do that.

Obviously putting it in the floppy driver wouldn't be meaningful.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [patch] "big IRQ lock" removal, 2.5.27-A9

[Ingo Molnar]

On Mon, 22 Jul 2002, Russell King wrote:

> Actually its to cover the case where you have a floppy drive, and you've
> booted the kernel from a floppy disk, and the kernel doesn't have the
> floppy driver built in.  It turns the floppy drive off, cause there's
> nothing else to do that.

this should then be done by the floppy boot code?

	Ingo

Re: [patch] "big IRQ lock" removal, 2.5.27-A9

[Russell King]

On Mon, Jul 22, 2002 at 01:44:16AM +0200, Ingo Molnar wrote:
> On Mon, 22 Jul 2002, Russell King wrote:
> 
> > Actually its to cover the case where you have a floppy drive, and you've
> > booted the kernel from a floppy disk, and the kernel doesn't have the
> > floppy driver built in.  It turns the floppy drive off, cause there's
> > nothing else to do that.
> 
> this should then be done by the floppy boot code?

Sounds like a better idea to me.  Although I'm not one to try it out. 8)

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [patch] "big IRQ lock" removal, 2.5.27-A9

[Ingo Molnar]

On 22 Jul 2002, Alan Cox wrote:

> > > Actually its to cover the case where you have a floppy drive, and you've
> > > booted the kernel from a floppy disk, and the kernel doesn't have the
> > > floppy driver built in.  It turns the floppy drive off, cause there's
> > > nothing else to do that.
> > 
> > this should then be done by the floppy boot code?
> 
> Most definitely. On legacy free boxes there may not even be a floppy
> controller present, and on non x86 your guess is as good as mine at
> where the fdc lives.

non-x86 was covered via an #ifdef, but legacy-free is not covered.

	Ingo

Re: [patch] "big IRQ lock" removal, 2.5.27-A1

[Robert Love]

On Sun, 2002-07-21 at 15:19, Ingo Molnar wrote:

> sure, feel free.

Har Har... where is Ted? ;-)

Seriously, though, I check my mail after a couple hours and you have
solved nearly every outstanding issue.  Excellent.

	Robert Love

Re: [patch] "big IRQ lock" removal, 2.5.27-A9

[Ingo Molnar]

On Mon, 22 Jul 2002, Russell King wrote:

> > > Actually its to cover the case where you have a floppy drive, and you've
> > > booted the kernel from a floppy disk, and the kernel doesn't have the
> > > floppy driver built in.  It turns the floppy drive off, cause there's
> > > nothing else to do that.
> > 
> > this should then be done by the floppy boot code?
> 
> Sounds like a better idea to me.  Although I'm not one to try it out. 8)

i've started adding it, just to realize that bootsect.S already turns off 
the floppy motor.

so i think the issue got solved the easy way ;)

	Ingo

Re: [patch] "big IRQ lock" removal, 2.5.27-A9

[Ingo Molnar]

On Sun, 21 Jul 2002, Linus Torvalds wrote:

> Looks good. Merged into my BK tree now, so please do future patches
> against this one..

i've done a minor comment update in softirq.c, plus i've written a
cli-sti-removal.txt guide to help driver writers do the transition:

   http://redhat.com/~mingo/remove-irqlock-patches/remove-irqlock-2.5.27-C2

(also attached)

	Ingo

--- linux/Documentation/cli-sti-removal.txt.orig	Mon Jul 22 02:25:53 2002
+++ linux/Documentation/cli-sti-removal.txt	Mon Jul 22 02:31:47 2002
@@ -0,0 +1,115 @@
+
+#### cli()/sti() removal guide, started by Ingo Molnar <mingo@redhat.com>
+
+
+as of 2.5.28, four popular macros have been removed on SMP, and
+are being phased out on UP:
+
+ cli(), sti(), save_flags(flags), restore_flags(flags)
+
+until now it was possible to protect driver code against interrupt
+handlers via a cli(), but from now on other, more lightweight methods
+have to be used for synchronization, such as spinlocks or semaphores.
+
+for example, driver code that used to do something like:
+
+	struct driver_data;
+
+	irq_handler (...)
+	{
+		....
+		driver_data.finish = 1;
+		driver_data.new_work = 0;
+		....
+	}
+
+	...
+
+	ioctl_func (...)
+	{
+		...
+		cli();
+		...
+		driver_data.finish = 0;
+		driver_data.new_work = 2;
+		...
+		sti();
+		...
+	}
+
+was SMP-correct because the cli() function ensured that no
+interrupt handler (amongst them the above irq_handler()) function
+would execute while the cli()-ed section is executing.
+
+but from now on a more direct method of locking has to be used:
+
+	spinlock_t driver_lock = SPIN_LOCK_UNLOCKED;
+	struct driver_data;
+
+	irq_handler (...)
+	{
+		unsigned long flags;
+		....
+		spin_lock_irqsave(&driver_lock, flags);
+		....
+		driver_data.finish = 1;
+		driver_data.new_work = 0;
+		....
+		spin_unlock_irqrestore(&driver_lock, flags);
+		....
+	}
+
+	...
+
+	ioctl_func (...)
+	{
+		...
+		spin_lock_irq(&driver_lock);
+		...
+		driver_data.finish = 0;
+		driver_data.new_work = 2;
+		...
+		spin_unlock_irq(&driver_lock);
+		...
+	}
+
+the above code has a number of advantages:
+
+- the locking relation is easier to understand - actual lock usage
+  pinpoints the critical sections. cli() usage is too opaque.
+  Easier to understand means it's easier to debug.
+
+- it's faster, because spinlocks are faster to acquire than the
+  potentially heavily-used IRQ lock. Furthermore, your driver does
+  not have to wait eg. for a big heavy SCSI interrupt to finish,
+  because the driver_lock spinlock is only used by your driver.
+  cli() on the other hand was used by many drivers, and extended
+  the critical section to the whole IRQ handler function - creating
+  serious lock contention.
+ 
+
+to make the transition easier, we've still kept the cli(), sti(),
+save_flags() and restore_flags() macros defined on UP systems - but
+their usage will be phased out until the 2.6 kernel is released.
+
+drivers that want to disable local interrupts (interrupts on the
+current CPU), can use the following four macros:
+
+  __cli(), __sti(), __save_flags(flags), __restore_flags(flags)
+
+but beware, their meaning and semantics are much simpler, far from
+that of cli(), sti(), save_flags(flags) and restore_flags(flags).
+
+
+another related change is that synchronize_irq() now takes a parameter:
+synchronize_irq(irq). This change too has the purpose of making SMP
+synchronization more lightweight - this way you can wait for your own
+interrupt handler to finish, no need to wait for other IRQ sources.
+
+
+why were these changes done? The main reason was the architectural burden
+of maintaining the cli()/sti() interface - it became a real problem. The
+new interrupt system is much more streamlined, easier to understand, debug,
+and it's also a bit faster - the same happened to it that will happen to
+cli()/sti() using drivers once they convert to spinlocks :-)
+
--- linux/kernel/softirq.c.orig	Mon Jul 22 01:37:31 2002
+++ linux/kernel/softirq.c	Mon Jul 22 01:38:18 2002
@@ -26,10 +26,6 @@
      execution. Hence, we get something sort of weak cpu binding.
      Though it is still not clear, will it result in better locality
      or will not.
-   - These softirqs are not masked by global cli() and start_bh_atomic()
-     (by clear reasons). Hence, old parts of code still using global locks
-     MUST NOT use softirqs, but insert interfacing routines acquiring
-     global locks. F.e. look at BHs implementation.
 
    Examples:
    - NET RX softirq. It is multithreaded and does not require

Re: [patch] "big IRQ lock" removal, 2.5.27-A9

[Russell King]

On Mon, Jul 22, 2002 at 02:31:16AM +0200, Ingo Molnar wrote:
> +drivers that want to disable local interrupts (interrupts on the
> +current CPU), can use the following four macros:
> +
> +  __cli(), __sti(), __save_flags(flags), __restore_flags(flags)

Last mail before zzz (hopefully) - what about
local_irq_{enable,disable,save,restore} ?

With the exception of local_irq_save() which is actually
local_irq_save_disable(), I find these to be more "descriptive" of
their function.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [patch] "big IRQ lock" removal, 2.5.27-A9

[Robert Love]

On Sun, 2002-07-21 at 17:40, Russell King wrote:
> On Mon, Jul 22, 2002 at 02:31:16AM +0200, Ingo Molnar wrote:
> > +drivers that want to disable local interrupts (interrupts on the
> > +current CPU), can use the following four macros:
> > +
> > +  __cli(), __sti(), __save_flags(flags), __restore_flags(flags)
> 
> Last mail before zzz (hopefully) - what about
> local_irq_{enable,disable,save,restore} ?
> 
> With the exception of local_irq_save() which is actually
> local_irq_save_disable(), I find these to be more "descriptive" of
> their function.

Yes and double yes.

And for two reasons: First, the __ prefix is unnecessary now.  Second,
not all the world is an x86 (it just looks that way).

local_irq_foo is definitely preferred.

I'd make the patch and go through the effort to rename and replace if
Linus assured me it was in.

	Robert Love

Re: [patch] "big IRQ lock" removal, 2.5.27-A9

[Robert Love]

On Sun, 2002-07-21 at 17:31, Ingo Molnar wrote:

> i've done a minor comment update in softirq.c, plus i've written a
> cli-sti-removal.txt guide to help driver writers do the transition:

Nice document.

One more doc correction while we are at it...

	Robert Love

diff -urN linux-2.5.27/Documentation/preempt-locking.txt linux/Documentation/preempt-locking.txt
--- linux-2.5.27/Documentation/preempt-locking.txt	Sat Jul 20 12:11:06 2002
+++ linux/Documentation/preempt-locking.txt	Sun Jul 21 17:59:13 2002
@@ -70,7 +70,8 @@
 preempt_enable()		decrement the preempt counter
 preempt_disable()		increment the preempt counter
 preempt_enable_no_resched()	decrement, but do not immediately preempt
-preempt_get_count()		return the preempt counter
+preempt_check_resched()		if needed, reschedule
+preempt_count()			return the preempt counter
 
 The functions are nestable.  In other words, you can call preempt_disable
 n-times in a code path, and preemption will not be reenabled until the n-th

Re: [patch] "big IRQ lock" removal, 2.5.27-A9

[Alan Cox]

On Mon, 2002-07-22 at 00:44, Ingo Molnar wrote:
> 
> On Mon, 22 Jul 2002, Russell King wrote:
> 
> > Actually its to cover the case where you have a floppy drive, and you've
> > booted the kernel from a floppy disk, and the kernel doesn't have the
> > floppy driver built in.  It turns the floppy drive off, cause there's
> > nothing else to do that.
> 
> this should then be done by the floppy boot code?

Most definitely. On legacy free boxes there may not even be a floppy
controller present, and on non x86 your guess is as good as mine at
where the fdc lives.

Alan

[patch] cli()/sti() cleanup, 2.5.27-A2

[Ingo Molnar]

On Mon, 22 Jul 2002, Russell King wrote:

> On Mon, Jul 22, 2002 at 02:31:16AM +0200, Ingo Molnar wrote:
> > +drivers that want to disable local interrupts (interrupts on the
> > +current CPU), can use the following four macros:
> > +
> > +  __cli(), __sti(), __save_flags(flags), __restore_flags(flags)
> 
> Last mail before zzz (hopefully) - what about
> local_irq_{enable,disable,save,restore} ?
> 
> With the exception of local_irq_save() which is actually
> local_irq_save_disable(), I find these to be more "descriptive" of their
> function.

i actually had something much more drastic in mind! :-) Now that the
global IRQ lock is no more, there's no 'local' and 'global' distinction
anymore between various irq disabling mechanizms.

So what i did in my tree was to introduce the following 5 core means of
manipulating the local interrupt flags:

	irq_off()
	irq_on()
	irq_save(flags)
	irq_save_off(flags)
	irq_restore(flags)

the equivalencies are:

	local_irq_save(flags)	 == irq_save_off(flags)
	local_irq_restore(flags) == irq_restore(flags)
	local_irq_disable()	 == irq_off()
	local_irq_enable()	 == irq_on()
	__cli()			 == irq_off()
	__sti()			 == irq_on()
	__save_flags(flags)	 == irq_save(flags)
	__restore_flags(flags)	 == irq_restore(flags)
	__save_and_cli(flags)	 == irq_save_off(flags)

the advantages this rename has:

- consistency - no dual naming for the same thing, we had 9 names for 5 
  entities.

- __cli, __sti are based on similarly named x86 instruction names which
  are often named differently on other architectures. 'irq_off()' and
  'irq_on()' on the other hand is a generic description. Also, cli and sti 
  are more cryptic than need to be.

- the names are actually shorter, more compact, making the source easier
  to read and understand.

- there's no conceptual confusion between local_irq_disable() and
  irq_disable(), or local_irq_enable() and irq_enable(), which are a
  completely separate set of APIs.

- there's no local_ prefix either - we know that interrupts can only be
  disabled locally.

- the confusion wrt. local_irq_save() is fixed too. local_irq_save() used
  to disable interrupt, although intuition tells that it saves flags only.

- the 5 new names do not create any namespace collision either.

some of these advantages are pretty 'small' in scope - but they all add
up. So in my opinion this is something we should do.

to ease the decision further, here's a patch against Linus' latest BK tree
that does all of this, in the whole kernel tree, for every architecture
and driver:

  http://redhat.com/~mingo/remove-irqlock-patches/cli-sti-cleanup-2.5.27-A2

(the patch also cleans up every architecture's include/asm/system.h's irq
macros.)

the patch shaves off a total of 4 KB from the kernel source code size. It
compiles, boots & works just fine on x86 UP and SMP.

	Ingo

Re: [patch] cli()/sti() cleanup, 2.5.27-A2

[Christoph Hellwig]

On Mon, Jul 22, 2002 at 03:01:48PM +0200, Ingo Molnar wrote:
> So what i did in my tree was to introduce the following 5 core means of
> manipulating the local interrupt flags:
> 
> 	irq_off()
> 	irq_on()
> 	irq_save(flags)
> 	irq_save_off(flags)
> 	irq_restore(flags)

I'd prefer the following:

void irq_off(void);
void irq_on(void);

flags_t irq_save();		/* the old irq_save_off() */
void irq_restore(flags_t);

void __irq_save(void);		/* without saveing */

rational:  proper function-like API (should be inlines), irq save
without disableing is very uncommon, better make the API symmetric.

Re: [patch] cli()/sti() cleanup, 2.5.27-A2

[Ingo Molnar]

On Mon, 22 Jul 2002, Christoph Hellwig wrote:

> > 	irq_off()
> > 	irq_on()
> > 	irq_save(flags)
> > 	irq_save_off(flags)
> > 	irq_restore(flags)
> 
> I'd prefer the following:
> 
> void irq_off(void);
> void irq_on(void);
> 
> flags_t irq_save();		/* the old irq_save_off() */
> void irq_restore(flags_t);
> 
> void __irq_save(void);		/* without saveing */
> 
> rational:  proper function-like API (should be inlines), irq save
> without disableing is very uncommon, better make the API symmetric.

i agree mostly, but i do not agree with __irq_save() and irq_save().  
What's wrong with "flags_t irq_save_off()" - the name carries the proper
meaning, and it also harmonizes with irq_off().

	Ingo

Re: [patch] cli()/sti() cleanup, 2.5.27-A2

[Christoph Hellwig]

On Mon, Jul 22, 2002 at 03:20:56PM +0200, Christoph Hellwig wrote:
> I'd prefer the following:
> 
> void irq_off(void);
> void irq_on(void);
> 
> flags_t irq_save();		/* the old irq_save_off() */
> void irq_restore(flags_t);
> 
> void __irq_save(void);		/* without saveing */

					^^^^^ stupid ^^^^^

rmk's sanity checker caught this, should be without disabling.

Re: [patch] cli()/sti() cleanup, 2.5.27-A2

[Christoph Hellwig]

On Mon, Jul 22, 2002 at 03:23:40PM +0200, Ingo Molnar wrote:
> i agree mostly, but i do not agree with __irq_save() and irq_save().  
> What's wrong with "flags_t irq_save_off()" - the name carries the proper
> meaning, and it also harmonizes with irq_off().

but not with irq_restore :)  maybe irq_restore_on() although the on
is implicit.

Re: [patch] cli()/sti() cleanup, 2.5.27-A2

[Ingo Molnar]

On Mon, 22 Jul 2002, Christoph Hellwig wrote:

> > i agree mostly, but i do not agree with __irq_save() and irq_save().  
> > What's wrong with "flags_t irq_save_off()" - the name carries the proper
> > meaning, and it also harmonizes with irq_off().
> 
> but not with irq_restore :)  maybe irq_restore_on() although the on
> is implicit.

no, the on is not implicit at all. If you restore into a disabled state
then it will go from on to off.

	Ingo

Re: [patch] cli()/sti() cleanup, 2.5.27-A2

[Christoph Hellwig]

On Mon, Jul 22, 2002 at 03:26:57PM +0200, Ingo Molnar wrote:
> no, the on is not implicit at all. If you restore into a disabled state
> then it will go from on to off.

well, for the normal use of it.  Okay, I give up, even if the nameing
looks strange in the beginning is is consistant :)

[patch] remove-irqlock-2.5.27-D7

[Ingo Molnar]

On Mon, 22 Jul 2002, Christoph Hellwig wrote:

> > no, the on is not implicit at all. If you restore into a disabled state
> > then it will go from on to off.
> 
> well, for the normal use of it.  Okay, I give up, even if the nameing
> looks strange in the beginning is is consistant :)

it does precisely what it says:
	
    irq_off()           => turn local IRQs off

    irq_on()            => turn local IRQs on

    irq_save(flags)     => save the current IRQ state into flags. The 
                           state can be on or off. (on some 
                           architectures there's even more bits in it.)

    irq_save_off(flags) => save the current IRQ state into flags and 
                           disable interrupts.

    irq_restore(flags)  => restore the IRQ state from flags.

while it's true that 'normally' we save irq-enabled state, it's not at all
sure, eg. when nested irq_save() is done then the first flags will carry
an irqs-on bit, the other nested flags will carry an irqs-off flag - and
the nested irq_restore() will restore to irqs-off state.

this is how it has worked in the past 10 years or so.

but i've added this description to the cli-sti guide :-) Check out the -D7
patch:
 
  http://redhat.com/~mingo/remove-irqlock-patches/remove-irqlock-2.5.27-D7

	Ingo

Re: [patch] cli()/sti() cleanup, 2.5.27-A2

[Ingo Molnar]

On Mon, 22 Jul 2002, Christoph Hellwig wrote:

> > i agree mostly, but i do not agree with __irq_save() and irq_save().  
> > What's wrong with "flags_t irq_save_off()" - the name carries the proper
> > meaning, and it also harmonizes with irq_off().
> 
> but not with irq_restore :)  maybe irq_restore_on() although the on is
> implicit.

think about it - if this was true then irq_restore_on() would be
equivalent to irq_on().

	Ingo

Re: [patch] cli()/sti() cleanup, 2.5.27-A2

[Christoph Hellwig]

On Mon, Jul 22, 2002 at 03:38:29PM +0200, Ingo Molnar wrote:
> > but not with irq_restore :)  maybe irq_restore_on() although the on is
> > implicit.
> 
> think about it - if this was true then irq_restore_on() would be
> equivalent to irq_on().

yupp, you're right.  could you take the prototype changes anyway?

Re: [patch] cli()/sti() cleanup, 2.5.27-A2

[Ingo Molnar]

On Mon, 22 Jul 2002, Christoph Hellwig wrote:

> void irq_off(void);
> void irq_on(void);
> 
> flags_t irq_save();
> void irq_restore(flags_t);

i'm not so sure about flags_t. 'unsigned long' worked pretty well so far,
and i do not see the need for a more complex (or more opaque) irqflags
type. It's not that we confuse flags with some other flag all that
frequently that would necessiate some structure-based more abstract
protection of these variables.

(wrt. inline functions, every architecture is free to define them as
inline functions as they see fit.)

	Ingo

Re: [patch] cli()/sti() cleanup, 2.5.27-A2

[Russell King]

On Mon, Jul 22, 2002 at 03:43:50PM +0200, Ingo Molnar wrote:
> i'm not so sure about flags_t. 'unsigned long' worked pretty well so far,
> and i do not see the need for a more complex (or more opaque) irqflags
> type.

A feature request then.  Type checking.  Too many people try to stuff
the value into an int or signed long.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [patch] cli()/sti() cleanup, 2.5.27-A2

[Christoph Hellwig]

On Mon, Jul 22, 2002 at 03:43:50PM +0200, Ingo Molnar wrote:
> i'm not so sure about flags_t. 'unsigned long' worked pretty well so far,
> and i do not see the need for a more complex (or more opaque) irqflags
> type. It's not that we confuse flags with some other flag all that
> frequently that would necessiate some structure-based more abstract
> protection of these variables.

It's just that we don't really care what it is.  But the type change
wasn't my main point, rather the returning of the flags value in
irq_save to allow implementing it as non-macro.

Re: [patch] cli()/sti() cleanup, 2.5.27-A2

[Ingo Molnar]

On Mon, 22 Jul 2002, Russell King wrote:

> > i'm not so sure about flags_t. 'unsigned long' worked pretty well so far,
> > and i do not see the need for a more complex (or more opaque) irqflags
> > type.
> 
> A feature request then.  Type checking.  Too many people try to stuff
> the value into an int or signed long.

the next portion of the quote deals with this:

> > It's not that we confuse flags with some other flag all that
> > frequently that would necessiate some structure-based more abstract
> > protection of these variables.

are you sure type-checking is really needed? Sure people can mess up the
flags variable, but 64-bit archs could do a sizeof at compile-time.

	Ingo

Re: [patch] cli()/sti() cleanup, 2.5.27-A2

[Ingo Molnar]

On Mon, 22 Jul 2002, Christoph Hellwig wrote:

> yupp, you're right.  could you take the prototype changes anyway?

i'm hesitant for a number of reasons. Eg. irq_save_off(flags) has to be a
macro, otherwise we move the assignment into the irqs-off section.  
Compare:

	flags = irq_save_off();

with:
	irq_flags_off(flags);

sure, it could be written as:

	flags = irq_save();
	irq_off();

but then again the macro form is more compact.

	Ingo

Re: [patch] cli()/sti() cleanup, 2.5.27-A2

[Ingo Molnar]

we abstracted simple types such as pte_t, pmd_t and pgd_t for one good
reason: it's 3 *distinct* entities that can be confused quite easily,
causing subtle VM bugs (eg. for quite some time the x86 arch had 2-level
paging only, so to have a proper 3-level paging VM we needed these type
checks).

i dont sense the same type of urge (and danger of mixup) wrt. the
interrupt flags.

	Ingo

Re: [patch] cli()/sti() cleanup, 2.5.27-A2

[Marcin Dalecki]

Russell King wrote:
> On Mon, Jul 22, 2002 at 03:43:50PM +0200, Ingo Molnar wrote:
> 
>>i'm not so sure about flags_t. 'unsigned long' worked pretty well so far,
>>and i do not see the need for a more complex (or more opaque) irqflags
>>type.
> 
> 
> A feature request then.  Type checking.  Too many people try to stuff
> the value into an int or signed long.

Amen.

Re: [patch] cli()/sti() cleanup, 2.5.27-A2

[Marcin Dalecki]

Ingo Molnar wrote:
> On Mon, 22 Jul 2002, Christoph Hellwig wrote:
> 
> 
>>yupp, you're right.  could you take the prototype changes anyway?
> 
> 
> i'm hesitant for a number of reasons. Eg. irq_save_off(flags) has to be a
> macro, otherwise we move the assignment into the irqs-off section.  
> Compare:
> 
> 	flags = irq_save_off();
> 
> with:
> 	irq_flags_off(flags);
> 
> sure, it could be written as:
> 
> 	flags = irq_save();
> 	irq_off();
> 
> but then again the macro form is more compact.


By 2 characters. And hiding the side-effect. We don't have the notion of
var argument passing like in pascal or C++ here.

Re: [patch] cli()/sti() cleanup, 2.5.27-A2

[Ingo Molnar]

well, if/when there's a concensus i'll do the type checking change in a
'second wave' patch, since it's a distinct issue not directly connected to
the naming cleanup.

there are some more IRQ subsystem cleanups for which i have patches: such
as the removal of the pt_regs parameter from the irq handler function,
it's unused in 99% of the drivers - and the remaining 1% can get at it via
other means.

	Ingo

Re: [patch] cli()/sti() cleanup, 2.5.27-A2

[Ingo Molnar]

On Mon, 22 Jul 2002, Marcin Dalecki wrote:

> > i'm hesitant for a number of reasons. Eg. irq_save_off(flags) has to be a
> > macro, otherwise we move the assignment into the irqs-off section.  
> > Compare:
> > 
> > 	flags = irq_save_off();
> > 
> > with:
> > 	irq_flags_off(flags);
> > 
> > sure, it could be written as:
> > 
> > 	flags = irq_save();
> > 	irq_off();
> > 
> > but then again the macro form is more compact.
> 
> By 2 characters. [...]

and a full line ...

> [...] And hiding the side-effect. We don't have the notion of var
> argument passing like in pascal or C++ here.

well, it's a well-known side effect on the other hand.

	Ingo

Re: [patch] cli()/sti() cleanup, 2.5.27-A2

[Russell King]

On Mon, Jul 22, 2002 at 04:05:00PM +0200, Ingo Molnar wrote:
> there are some more IRQ subsystem cleanups for which i have patches: such
> as the removal of the pt_regs parameter from the irq handler function,
> it's unused in 99% of the drivers - and the remaining 1% can get at it via
> other means.

If "other means" means knowing that its located in a certain place on the
stack, that's actually bogus.  Any user space task started via exec from
a kernel thread has extra junk on the kernel stack.  Been there already.
;(

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [patch] cli()/sti() cleanup, 2.5.27-A2

[Ingo Molnar]

On Mon, 22 Jul 2002, Russell King wrote:

> If "other means" means knowing that its located in a certain place on
> the stack, that's actually bogus.  Any user space task started via exec
> from a kernel thread has extra junk on the kernel stack.  Been there
> already. ;(

no, i've added it to the irq descriptor structure, where it can be
accessed in normal ways by the driver. [the stack position thing doesnt
fly with vm86 tasks either.]

	Ingo

Re: [patch] cli()/sti() cleanup, 2.5.27-A2

[Linus Torvalds]

On Mon, 22 Jul 2002, Ingo Molnar wrote:
>
> So what i did in my tree was to introduce the following 5 core means of
> manipulating the local interrupt flags:
>
> 	irq_off()
> 	irq_on()
> 	irq_save(flags)
> 	irq_save_off(flags)
> 	irq_restore(flags)

Ugh.

I'd much rather keep the current "local_xxx" versions, since they clearly
say that it's local to the CPU. Let's face it, people SHOULD NOT USE
THESE!

You should use "spin_lock_irq()" and friends, since those are the only
sane interfaces for doing real irq-safe locking.

So what's wrong with just keeping the things that we've advocated for a
long while, and not try to break source compatibility "just because".

Keeping the old names will make it a lot easier to maintain drivers that
do want to use them, and it means not having to change old drivers that do
the right thing.

So I vote for

	local_irq_save(flags)		- save and disable
	local_irq_restore(flags)	- restore
	local_irq_disable()		- disable
	local_irq_enable()		- enable

and that's it. Yes, the "calling convention" for local_irq_save() is
strange, but it makes it easier for some architectures, and other
architectures can just always make it

	#define local_irq_save(flags) \
		do { (flags) = arch_irq_save(); } while (0)

and it's not worth breaking existing practices over (besides, that's the
calling convention that "read_lock_irqsave()" also has, and I do _not_
want to change all of that _too_).

As to needing to do a save without a disable, show me where that really
matters..

I agree that we should get rid of __cli / __sti / __restore_flags /
__save_flags and company, but that is no excuse for breaking backwards
compatibility for stuff that has used the new interfaces

I really think that "local_" prefix is worth it. It makes people who are
used to, and work exclusively with, UP think twice about what the thing
actually does.

		Linus

Re: [patch] cli()/sti() cleanup, 2.5.27-A2

[Ingo Molnar]

On Mon, 22 Jul 2002, Linus Torvalds wrote:

> I'd much rather keep the current "local_xxx" versions, since they
> clearly say that it's local to the CPU. Let's face it, people SHOULD NOT
> USE THESE!

okay, agreed.

> So I vote for
> 
> 	local_irq_save(flags)		- save and disable
> 	local_irq_restore(flags)	- restore
> 	local_irq_disable()		- disable
> 	local_irq_enable()		- enable

i've added this one as well:

	local_save_flags(flags)		- save

a fair number of places want (and need) to use __save_flags(flags)-type of
functionality, without the irq-disabling side-effect.

But also, a number of places now do:

	local_save_flags(flags);
	local_irq_disable();

which should be:

	local_irq_save(flags);

(these places will be simplified in an upcoming patch.)

The new __cli/__sti cleanup patch is at:

  http://redhat.com/~mingo/remove-irqlock-patches/cli-sti-cleanup-2.5.27-B2

compiles, boots & works just fine on x86 UP and SMP.

	Ingo

Re: [patch] cli()/sti() cleanup, 2.5.27-A2

[Zwane Mwaikambo]

On Mon, 22 Jul 2002, Linus Torvalds wrote:

> As to needing to do a save without a disable, show me where that really
> matters..

Heres an interesting one;

void setup_APIC_timer(void * data)
{
	[...]
        __save_flags(flags);
        __sti();
	[...]
}

Regards,
	Zwane
-- 
function.linuxpower.ca

Re: [patch] cli()/sti() cleanup, 2.5.27-A2

[Pavel Machek]

Hi!

> > > It's not that we confuse flags with some other flag all that
> > > frequently that would necessiate some structure-based more abstract
> > > protection of these variables.
> 
> are you sure type-checking is really needed? Sure people can mess up the
> flags variable, but 64-bit archs could do a sizeof at compile-time.

Yes, it is needed; type-checking can be easily implemented as 

{
	unsigned long foo;
	(&foo == &arg);
}

 -- that gives warning when arg is not unsigned long.
								Pavel


-- 
Worst form of spam? Adding advertisment signatures ala sourceforge.net.
What goes next? Inserting advertisment *into* email?