>>-----Original Message-----
>>From: Ard -kwaak- van Breemen [mailto:[email protected]]
>>Sent: 2006??12??22?? 5:06
>>To: Zhang, Yanmin
>>Cc: Andrew Morton; Chuck Ebbert; Yinghai Lu; [email protected]; [email protected]; [email protected];
>>[email protected]; Eric W. Biederman
>>Subject: Re: [Bug 7505] Linux-2.6.18 fails to boot on AMD64 machine
>>
>>On Thu, Dec 21, 2006 at 04:04:04PM +0800, Zhang, Yanmin wrote:
>>> I couldn't reproduce it on my EM64T machine. I instrumented function start_kernel and
>>> didn't find irq was enabled before calling init_IRQ. It'll be better if the reporter could
>>> instrument function start_kernel to capture which function enables irq.
>>
>>Editing init/main.c:
>> preempt_disable();
>> if (!irqs_disabled())
>> printk("start_kernel(): bug: interrupts were enabled early\n");
>> printk("BLAAT17");
>> build_all_zonelists();
>> if (!irqs_disabled())
>> printk("start_kernel(): bug: interrupts were enabled early\n");
>> printk("BLAAT18");
>> page_alloc_init();
>> if (!irqs_disabled())
>> printk("start_kernel(): bug: interrupts were enabled early\n");
>> printk("BLAAT19");
>> printk(KERN_NOTICE "Kernel command line: %s\n", saved_command_line);
>> parse_early_param();
>> if (!irqs_disabled())
>> printk("start_kernel(): bug: interrupts were enabled early\n");
>> printk("BLAAT20");
>> parse_args("Booting kernel", command_line, __start___param,
>> __stop___param - __start___param,
>> &unknown_bootoption);
>> printk("BLAAT21");
>> if (!irqs_disabled())
>> printk("start_kernel(): bug: interrupts were enabled early\n");
>> sort_main_extable();
>> if (!irqs_disabled())
>> printk("start_kernel(): bug: interrupts were enabled early\n");
>> printk("BLAAT22");
>> trap_init();
>> if (!irqs_disabled())
>> printk("start_kernel(): bug: interrupts were enabled early\n");
>> printk("BLAAT23");
>>
>>Results in:
>>^MAllocating PCI resources starting at 88000000 (gap: 80000000:60000000)
>>^MBLAAT12BLAAT13<6>PERCPU: Allocating 32960 bytes of per cpu data
>>^MBLAAT14BLAAT15BLAAT16BLAAT17Built 2 zonelists. Total pages: 1032635
>>^MBLAAT18BLAAT19<5>Kernel command line: console=tty0 console=ttyS0,115200 hdb=noprobe hdc=noprobe hdd=noprobe root=/dev/md0 ro panic=30
>>earlyprintk=serial,ttyS0,115200
>>^MBLAAT20<6>ide_setup: hdb=noprobe
>>^Mide_setup: hdc=noprobe
>>^Mide_setup: hdd=noprobe
>>^MBLAAT21start_kernel(): bug: interrupts were enabled early
>>^Mstart_kernel(): bug: interrupts were enabled early
>>^MBLAAT22Initializing CPU#0
>>
>>Hmmm, that actually doesn't make sense to me (unless parse_args is able to enable irq's).
I think parse_args enables irq when it calls callbacks.
Could you try below?
1) Test Andrew's patch of sema down_write;
2) Apply below patch and see what the output is when booting. If the output has
"[BUG]..address.", Pls. map the address to function name by System.map.
--- linux-2.6.19/kernel/params.c 2006-12-08 15:32:49.000000000 +0800
+++ linux-2.6.19_work/kernel/params.c 2006-12-22 12:28:38.000000000 +0800
@@ -53,13 +53,22 @@ static int parse_one(char *param,
int (*handle_unknown)(char *param, char *val))
{
unsigned int i;
+ int result;
+ int irq_is_disabled;
/* Find parameter */
for (i = 0; i < num_params; i++) {
if (parameq(param, params[i].name)) {
DEBUGP("They are equal! Calling %p\n",
params[i].set);
- return params[i].set(val, ¶ms[i]);
+ irq_is_disabled = irqs_disabled();
+ result = params[i].set(val, ¶ms[i]);
+ if (irq_is_disabled && !irqs_disabled())
+ {
+ printk("[BUG] parse_one: function %p enabled irq!\n",
+ params[i].set);
+ }
+ return result;
}
}
--- linux-2.6.19/init/main.c 2006-12-08 15:32:49.000000000 +0800
+++ linux-2.6.19_work/init/main.c 2006-12-22 12:28:50.000000000 +0800
@@ -181,6 +181,7 @@ static int __init obsolete_checksetup(ch
{
struct obs_kernel_param *p;
int had_early_param = 0;
+ int result, irq_is_disabled;
p = __setup_start;
do {
@@ -197,8 +198,17 @@ static int __init obsolete_checksetup(ch
printk(KERN_WARNING "Parameter %s is obsolete,"
" ignored\n", p->str);
return 1;
- } else if (p->setup_func(line + n))
- return 1;
+ } else {
+ irq_is_disabled = irqs_disabled();
+ result = p->setup_func(line + n);
+ if (irq_is_disabled && !irqs_disabled())
+ {
+ printk("[BUG] obsolete_checksetup: function %p enabled irq!\n",
+ p->setup_func);
+ }
+ if (result)
+ return 1;
+ }
}
p++;
} while (p < __setup_end);
Hello,
On Fri, Dec 22, 2006 at 12:41:46PM +0800, Zhang, Yanmin wrote:
> I think parse_args enables irq when it calls callbacks.
> Could you try below?
> 1) Test Andrew's patch of sema down_write;
> 2) Apply below patch and see what the output is when booting. If the output has
> "[BUG]..address.", Pls. map the address to function name by System.map.
Without proof^H^H^H^H^Hpasting my dmesg and the "diff", I already
concluded that ide_setup was the culprit. (I've debuged
parse_one, and it barfed around the 3rd parameter which is
hdb=noprobe).
Anyway, a bad night of sleep reminds me that our EM64T boxes also
have this line (which actually is a remainder of our VA1220 boxes
;-) ), and they don't barf, so it must be either the combination
of the sata_nv together with the pata driver part, *or* just the
pata driver part. (Our opteron != nforce chipsets also works).
I will trace down the ide_setup today. First loads of coffee.
--
program signature;
begin { telegraaf.com
} writeln("<[email protected]> TEM2");
end
.
On Fri, 22 Dec 2006 09:22:48 +0100
Ard -kwaak- van Breemen <[email protected]> wrote:
> Hello,
> On Fri, Dec 22, 2006 at 12:41:46PM +0800, Zhang, Yanmin wrote:
> > I think parse_args enables irq when it calls callbacks.
> > Could you try below?
> > 1) Test Andrew's patch of sema down_write;
> > 2) Apply below patch and see what the output is when booting. If the output has
> > "[BUG]..address.", Pls. map the address to function name by System.map.
> Without proof^H^H^H^H^Hpasting my dmesg and the "diff", I already
> concluded that ide_setup was the culprit. (I've debuged
> parse_one, and it barfed around the 3rd parameter which is
> hdb=noprobe).
> Anyway, a bad night of sleep reminds me that our EM64T boxes also
> have this line (which actually is a remainder of our VA1220 boxes
> ;-) ), and they don't barf, so it must be either the combination
> of the sata_nv together with the pata driver part, *or* just the
> pata driver part. (Our opteron != nforce chipsets also works).
>
I expect that you'll find that the ide code ends up doing
down_write(pci_bus_sem), which will enable interrupts.
(We don't know which interrupt is pending this early - that'd be
interesting to find out, but we shouldn't be enabling interrupts in there).
To whom do I have to pay how much to get this darn patch tested?
--- a/lib/rwsem-spinlock.c~down_write-preserve-local-irqs
+++ a/lib/rwsem-spinlock.c
@@ -195,13 +195,14 @@ void fastcall __sched __down_write_neste
{
struct rwsem_waiter waiter;
struct task_struct *tsk;
+ unsigned long flags;
- spin_lock_irq(&sem->wait_lock);
+ spin_lock_irqsave(&sem->wait_lock, flags);
if (sem->activity == 0 && list_empty(&sem->wait_list)) {
/* granted */
sem->activity = -1;
- spin_unlock_irq(&sem->wait_lock);
+ spin_unlock_irqrestore(&sem->wait_lock, flags);
goto out;
}
@@ -216,7 +217,7 @@ void fastcall __sched __down_write_neste
list_add_tail(&waiter.list, &sem->wait_list);
/* we don't need to touch the semaphore struct anymore */
- spin_unlock_irq(&sem->wait_lock);
+ spin_unlock_irqrestore(&sem->wait_lock, flags);
/* wait to be given the lock */
for (;;) {
_
Il giorno ven, 22/12/2006 alle 00.30 -0800, Andrew Morton ha scritto:
> On Fri, 22 Dec 2006 09:22:48 +0100
> Ard -kwaak- van Breemen <[email protected]> wrote:
>
> > Hello,
> > On Fri, Dec 22, 2006 at 12:41:46PM +0800, Zhang, Yanmin wrote:
> > > I think parse_args enables irq when it calls callbacks.
> > > Could you try below?
> > > 1) Test Andrew's patch of sema down_write;
> > > 2) Apply below patch and see what the output is when booting. If the output has
> > > "[BUG]..address.", Pls. map the address to function name by System.map.
> > Without proof^H^H^H^H^Hpasting my dmesg and the "diff", I already
> > concluded that ide_setup was the culprit. (I've debuged
> > parse_one, and it barfed around the 3rd parameter which is
> > hdb=noprobe).
> > Anyway, a bad night of sleep reminds me that our EM64T boxes also
> > have this line (which actually is a remainder of our VA1220 boxes
> > ;-) ), and they don't barf, so it must be either the combination
> > of the sata_nv together with the pata driver part, *or* just the
> > pata driver part. (Our opteron != nforce chipsets also works).
> >
>
> I expect that you'll find that the ide code ends up doing
> down_write(pci_bus_sem), which will enable interrupts.
>
> (We don't know which interrupt is pending this early - that'd be
> interesting to find out, but we shouldn't be enabling interrupts in there).
>
> To whom do I have to pay how much to get this darn patch tested?
>
>
>
> --- a/lib/rwsem-spinlock.c~down_write-preserve-local-irqs
> +++ a/lib/rwsem-spinlock.c
> @@ -195,13 +195,14 @@ void fastcall __sched __down_write_neste
> {
> struct rwsem_waiter waiter;
> struct task_struct *tsk;
> + unsigned long flags;
>
> - spin_lock_irq(&sem->wait_lock);
> + spin_lock_irqsave(&sem->wait_lock, flags);
>
> if (sem->activity == 0 && list_empty(&sem->wait_list)) {
> /* granted */
> sem->activity = -1;
> - spin_unlock_irq(&sem->wait_lock);
> + spin_unlock_irqrestore(&sem->wait_lock, flags);
> goto out;
> }
>
> @@ -216,7 +217,7 @@ void fastcall __sched __down_write_neste
> list_add_tail(&waiter.list, &sem->wait_list);
>
> /* we don't need to touch the semaphore struct anymore */
> - spin_unlock_irq(&sem->wait_lock);
> + spin_unlock_irqrestore(&sem->wait_lock, flags);
>
> /* wait to be given the lock */
> for (;;) {
> _
>
Applied to 2.6.19 it doesn't change anything. It still panics.
How can I have something similar to a serial console on a laptop without
serial port but with a parallel one? Will netconsole work?
--
Stefano Takekawa
[email protected]
Frank: And why do days get longer in the summer?
Ernest: Because heat makes things expand!
On Fri, 22 Dec 2006 10:32:51 +0100
Stefano Takekawa <[email protected]> wrote:
> Applied to 2.6.19 it doesn't change anything. It still panics.
Really?
And you can confirm that converting pci_bus_sem back into a spinlock fixes
it?
> How can I have something similar to a serial console on a laptop without
> serial port but with a parallel one? Will netconsole work?
>
No, netconsole isn't available for quite some time after the kernel starts.
Your best bet would be to boot with `earlyprintk=vga vga=N', where N is
something which gives lots of rows. 0F01, perhaps.
Then, take a digital photo of the display.
Hello,
On Fri, Dec 22, 2006 at 12:30:29AM -0800, Andrew Morton wrote:
> To whom do I have to pay how much to get this darn patch tested?
I've already tested that (as I said somewhere in the bugzilla so
it probably got lost somehow :-) ): It doesn't solve the booting
problem, and I really don't have an idea what it does, nor does
it output any debug code. So I left it at: doesn't fix ;-).
Anyway: on to the ide_setup tracking....
(I've noticed that the notifier of this problem als has idebus=66
or something similar, so that explains in his case the
early call to ide_setup.)
--
program signature;
begin { telegraaf.com
} writeln("<[email protected]> TEM2");
end
.
Il giorno ven, 22/12/2006 alle 01.43 -0800, Andrew Morton ha scritto:
> On Fri, 22 Dec 2006 10:32:51 +0100
> Stefano Takekawa <[email protected]> wrote:
>
> > Applied to 2.6.19 it doesn't change anything. It still panics.
>
> Really?
>
> And you can confirm that converting pci_bus_sem back into a spinlock fixes
> it?
>
> > How can I have something similar to a serial console on a laptop without
> > serial port but with a parallel one? Will netconsole work?
> >
>
> No, netconsole isn't available for quite some time after the kernel starts.
>
> Your best bet would be to boot with `earlyprintk=vga vga=N', where N is
> something which gives lots of rows. 0F01, perhaps.
>
> Then, take a digital photo of the display.
I can't take any digital photo. Well I got this:
2.6.19 + lib/rwsem-spinlock.c patched + hdc=ide-cd or idebus=66 >> panic
2.6.19 + lib/rwsem-spinlock.c patched + no ide_setup calls >> works!!!
2.6.19 + spinlock reversed >> always works
--
Stefano Takekawa
[email protected]
Frank: And why do days get longer in the summer?
Ernest: Because heat makes things expand!
On Fri, Dec 22, 2006 at 11:30:05AM +0100, Ard -kwaak- van Breemen wrote:
> Anyway: on to the ide_setup tracking....
> (I've noticed that the notifier of this problem als has idebus=66
> or something similar, so that explains in his case the
> early call to ide_setup.)
Aaarrgh...
Somewhere between the call to ide_setup and ide_init_hwif_ports
the interrupts get enabled. But I haven't got to the point where
exactly...
include/linux/ide.h:
266 /*
267 * ide_init_hwif_ports() is OBSOLETE and will be removed in 2.7 series.
268 * New ports shouldn't define IDE_ARCH_OBSOLETE_INIT in <asm/ide.h>.
269 */
270 #ifdef IDE_ARCH_OBSOLETE_INIT
271 static inline void ide_init_hwif_ports(hw_regs_t *hw,
272 unsigned long io_addr,
273 unsigned long ctl_addr,
274 int *irq)
275 {
276 if (!irqs_disabled()) printk(__FILE__ "%s(): blaat: interrupts were enabled early@%d\n",__FUNCTION__,__LINE__);
277 if (!ctl_addr) {
278 ide_std_init_ports(hw, io_addr, ide_default_io_ctl(io_addr));
drivers/ide/ide.c:
256 static void init_hwif_default(ide_hwif_t *hwif, unsigned int index)
257 {
258 hw_regs_t hw;
259
260 if (!irqs_disabled()) printk(__FILE__ "%s(): blaat: interrupts were enabled early@%d\n",__FUNCTION__,__LINE__);
261 memset(&hw, 0, sizeof(hw_regs_t));
262 if (!irqs_disabled()) printk(__FILE__ "%s(): blaat: interrupts were enabled early@%d\n",__FUNCTION__,__LINE__);
263
264 ide_init_hwif_ports(&hw, ide_default_io_base(index), 0, &hwif->irq);
265 if (!irqs_disabled()) printk(__FILE__ " %s(): blaat: interrupts were enabled early@%d\n",__FUNCTION__,__LINE__);
266
dmesg:
BLAAT20Parsing ARGS: console=tty0 console=ttyS0,115200 hdb=noprobe hdc=noprobe hdd=noprobe root=/dev/md0 ro panic
=30 earlyprintk=serial,ttyS0,115200
Unknown argument: calling ffffffff80643380
Unknown argument: calling ffffffff80643380
Unknown argument: calling ffffffff80643380
ide_setup: hdb=noprobeinclude/linux/ide.hide_init_hwif_ports(): blaat: interrupts were enabled early@276
include/linux/ide.hide_init_hwif_ports(): blaat: interrupts were enabled early@279
include/linux/ide.hide_init_hwif_ports(): blaat: interrupts were enabled early@284
include/linux/ide.hide_init_hwif_ports(): blaat: interrupts were enabled early@289
drivers/ide/ide.c init_hwif_default(): blaat: interrupts were enabled early@265
drivers/ide/ide.cinit_hwif_default(): blaat: interrupts were enabled early@269
drivers/ide/ide.cinit_ide_data(): blaat: interrupts were enabled early@317
So as I read it: init_hwif_default calls ide_init_hwif_ports with irq's
disabled, but upon entrance, the irq's are enabled.
That really makes no sense to me.
So I will continue digging this code (there must be something recursive going
on).
--
program signature;
begin { telegraaf.com
} writeln("<[email protected]> TEM2");
end
.
On Fri, Dec 22, 2006 at 03:00:59PM +0100, Ard -kwaak- van Breemen wrote:
> 262 if (!irqs_disabled()) printk(__FILE__ "%s(): blaat: interrupts were enabled early@%d\n",__FUNCTION__,__LINE__);
> 263
> 264 ide_init_hwif_ports(&hw, ide_default_io_base(index), 0, &hwif->irq);
------------------------------------------^^^^^^^^^^^^^^^^^^^^^^^^^^^
which does a if (pci_find_device(PCI_ANY_ID, PCI_ANY_ID,
in include/asm-i386/ide.h
which should be really the part that does the irq enabling.
--
program signature;
begin { telegraaf.com
} writeln("<[email protected]> TEM2");
end
.
On Fri, Dec 22, 2006 at 12:30:29AM -0800, Andrew Morton wrote:
> I expect that you'll find that the ide code ends up doing
> down_write(pci_bus_sem), which will enable interrupts.
will: down_read(&pci_bus_sem);
also enable interrupts?
Since that is called:
init/main.c start_kernel
kernel/params.c parse_args("Booting kernel"
kernel/params.c parse_one
drivers/ide/ide.c ide_setup
drivers/ide/ide.c init_ide_data
drivers/ide/ide.c init_hwif_default
include/asm-i386/ide.h ide_default_io_base(index)
drivers/pci/search.c pci_find_device
drivers/pci/search.c pci_find_subsys
down_read(&pci_bus_sem);
--
program signature;
begin { telegraaf.com
} writeln("<[email protected]> TEM2");
end
.
On Fri, Dec 22, 2006 at 12:30:29AM -0800, Andrew Morton wrote:
> To whom do I have to pay how much to get this darn patch tested?
I've altered your patch to do the spin_lock_irqsave in down_read.
I am very ignorant and stupid. That's why I am doing it without
thinking why or why not de irqsave is ok in that region or not.
And the results are:
include/asm-i386/ide.h ide_default_io_base(): blaat: interrupts were disabled@49
include/asm-i386/ide.h ide_default_io_base(): blaat: interrupts were disabled@51
include/asm-i386/ide.h ide_default_io_base(): blaat: interrupts were disabled@59
Meaning: it works.
Repeating: I am very stupid, so I don't know if saving the irq state is ok or
not in down_read.
--
program signature;
begin { telegraaf.com
} writeln("<[email protected]> TEM2");
end
.
On Fri, Dec 22, 2006 at 03:41:34PM +0100, Ard -kwaak- van Breemen wrote:
> Repeating: I am very stupid, so I don't know if saving the irq state is ok or
> not in down_read.
The Andrew Morton patch but the rewritten for down_read makes the
symptoms go away.
The problem obviously is that the ide_setup pokes the pci
subsystem way too early.
Parsing of the ide parameters should be delayed until the next
run of parse_args I guess.
On Fri, 22 Dec 2006 15:16:55 +0100
Ard -kwaak- van Breemen <[email protected]> wrote:
> On Fri, Dec 22, 2006 at 03:00:59PM +0100, Ard -kwaak- van Breemen wrote:
> > 262 if (!irqs_disabled()) printk(__FILE__ "%s(): blaat: interrupts were enabled early@%d\n",__FUNCTION__,__LINE__);
> > 263
> > 264 ide_init_hwif_ports(&hw, ide_default_io_base(index), 0, &hwif->irq);
> ------------------------------------------^^^^^^^^^^^^^^^^^^^^^^^^^^^
> which does a if (pci_find_device(PCI_ANY_ID, PCI_ANY_ID,
> in include/asm-i386/ide.h
>
> which should be really the part that does the irq enabling.
doh, I missed down_read():
--- a/lib/rwsem-spinlock.c~down_write-preserve-local-irqs
+++ a/lib/rwsem-spinlock.c
@@ -129,13 +129,14 @@ void fastcall __sched __down_read(struct
{
struct rwsem_waiter waiter;
struct task_struct *tsk;
+ unsigned long flags;
- spin_lock_irq(&sem->wait_lock);
+ spin_lock_irqsave(&sem->wait_lock, flags);
if (sem->activity >= 0 && list_empty(&sem->wait_list)) {
/* granted */
sem->activity++;
- spin_unlock_irq(&sem->wait_lock);
+ spin_unlock_irqrestore(&sem->wait_lock, flags);
goto out;
}
@@ -150,7 +151,7 @@ void fastcall __sched __down_read(struct
list_add_tail(&waiter.list, &sem->wait_list);
/* we don't need to touch the semaphore struct anymore */
- spin_unlock_irq(&sem->wait_lock);
+ spin_unlock_irqrestore(&sem->wait_lock, flags);
/* wait to be given the lock */
for (;;) {
@@ -195,13 +196,14 @@ void fastcall __sched __down_write_neste
{
struct rwsem_waiter waiter;
struct task_struct *tsk;
+ unsigned long flags;
- spin_lock_irq(&sem->wait_lock);
+ spin_lock_irqsave(&sem->wait_lock, flags);
if (sem->activity == 0 && list_empty(&sem->wait_list)) {
/* granted */
sem->activity = -1;
- spin_unlock_irq(&sem->wait_lock);
+ spin_unlock_irqrestore(&sem->wait_lock, flags);
goto out;
}
@@ -216,7 +218,7 @@ void fastcall __sched __down_write_neste
list_add_tail(&waiter.list, &sem->wait_list);
/* we don't need to touch the semaphore struct anymore */
- spin_unlock_irq(&sem->wait_lock);
+ spin_unlock_irqrestore(&sem->wait_lock, flags);
/* wait to be given the lock */
for (;;) {
_
Could someone please test this?
From: Andrew Morton <[email protected]>
Various people have reported machines failing to boot since pci_bus_sem was
switched from a spinlock to an rwsem.
The reason for this is that these people had "ide=" on the kernel commandline,
and ide_setup() can end up calling PCI functions which do
down_read(&pci_bus_sem).
Ard has worked out the call tree:
init/main.c start_kernel
kernel/params.c parse_args("Booting kernel"
kernel/params.c parse_one
drivers/ide/ide.c ide_setup
drivers/ide/ide.c init_ide_data
drivers/ide/ide.c init_hwif_default
include/asm-i386/ide.h ide_default_io_base(index)
drivers/pci/search.c pci_find_device
drivers/pci/search.c pci_find_subsys
down_read(&pci_bus_sem);
down_read() will unconditionally enable interrupts and some early interrupt
(source unknown) comes in and whacks the machine, apparently because the LDT
isn't set up yet.
Fix that by avoiding taking the semaphore in the PCI code in this situation.
Cc: Ard -kwaak- van Breemen <[email protected]>
Cc: "Zhang, Yanmin" <[email protected]>
Cc: Chuck Ebbert <[email protected]>
Cc: Yinghai Lu <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
Cc: "Eric W. Biederman" <[email protected]>
Cc: Greg KH <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
---
drivers/pci/search.c | 10 ++++++++++
1 files changed, 10 insertions(+)
diff -puN drivers/pci/search.c~pci-avoid-taking-pci_bus_sem-early-in-boot drivers/pci/search.c
--- a/drivers/pci/search.c~pci-avoid-taking-pci_bus_sem-early-in-boot
+++ a/drivers/pci/search.c
@@ -259,6 +259,16 @@ pci_get_subsys(unsigned int vendor, unsi
struct pci_dev *dev;
WARN_ON(in_interrupt());
+
+ /*
+ * pci_get_subsys() can be called on the ide_setup() path, super-early
+ * in boot. But the down_read() will enable local interrupts, which
+ * can cause some machines to crash. So here we detect that situation
+ * and bail out early.
+ */
+ if (unlikely(list_empty(pci_devices)))
+ return NULL;
+
down_read(&pci_bus_sem);
n = from ? from->global_list.next : pci_devices.next;
_
Il giorno gio, 28/12/2006 alle 15.51 -0800, Andrew Morton ha scritto:
> Could someone please test this?
> diff -puN drivers/pci/search.c~pci-avoid-taking-pci_bus_sem-early-in-boot drivers/pci/search.c
> --- a/drivers/pci/search.c~pci-avoid-taking-pci_bus_sem-early-in-boot
> +++ a/drivers/pci/search.c
> @@ -259,6 +259,16 @@ pci_get_subsys(unsigned int vendor, unsi
> struct pci_dev *dev;
>
> WARN_ON(in_interrupt());
> +
> + /*
> + * pci_get_subsys() can be called on the ide_setup() path, super-early
> + * in boot. But the down_read() will enable local interrupts, which
> + * can cause some machines to crash. So here we detect that situation
> + * and bail out early.
> + */
> + if (unlikely(list_empty(pci_devices)))
> + return NULL;
> +
> down_read(&pci_bus_sem);
> n = from ? from->global_list.next : pci_devices.next;
>
> _
>
Applied to 2.6.19 it returns error while compiling:
CC drivers/pci/search.o
drivers/pci/search.c: In function ‘pci_get_subsys’:
drivers/pci/search.c:269: error: incompatible type for argument 1 of
‘list_empty’
make[2]: *** [drivers/pci/search.o] Error 1
make[1]: *** [drivers/pci] Error 2
make: *** [drivers] Error 2
drivers/pci/search.c
268 */
269 if (unlikely(list_empty(pci_devices)))
270 return NULL;
--
Stefano Takekawa
[email protected]
Frank: And why do days get longer in the summer?
Ernest: Because heat makes things expand!
Hello Andrew,
On Thu, Dec 28, 2006 at 03:51:48PM -0800, Andrew Morton wrote:
> Could someone please test this?
Without testing I declare it won't fix it 8-D
> Ard has worked out the call tree:
>
> init/main.c start_kernel
> kernel/params.c parse_args("Booting kernel"
> kernel/params.c parse_one
> drivers/ide/ide.c ide_setup
> drivers/ide/ide.c init_ide_data
> drivers/ide/ide.c init_hwif_default
> include/asm-i386/ide.h ide_default_io_base(index)
> drivers/pci/search.c pci_find_device
> drivers/pci/search.c pci_find_subsys
------------------------------^^^^^^^^^^^^^^
Your patch patches pci_get_subsys, while pci_find_subsys does the
down_read...
I will try it on the right function, and see what we get.
Regards,
Ard
--
program signature;
begin { telegraaf.com
} writeln("<[email protected]> TEM2");
end
.
On Fri, Dec 29, 2006 at 01:51:08PM +0100, Ard -kwaak- van Breemen wrote:
> I will try it on the right function, and see what we get.
In function: 186 static struct pci_dev * pci_find_subsys(unsigned
int vendor,
203 if (unlikely(list_empty(&pci_devices))) {
204 printk("Pci device list empty, preventing down_read\n");
205 return NULL;
206 }
delivers:
ard@supergirl:~$ sudo grep -C1 'Pci device list empty' /var/log/kern.log
Dec 29 14:17:47 localhost kernel: include/asm-i386/ide.h ide_default_io_base(): blaat: interrupts were disabled@49
Dec 29 14:17:47 localhost kernel: Pci device list empty, preventing down_read
Dec 29 14:17:47 localhost kernel: include/asm-i386/ide.h ide_default_io_base(): blaat: interrupts were disabled@51
--
Dec 29 14:17:47 localhost kernel: include/asm-i386/ide.h ide_default_io_base(): blaat: interrupts were disabled@49
Dec 29 14:17:47 localhost kernel: Pci device list empty, preventing down_read
Dec 29 14:17:47 localhost kernel: include/asm-i386/ide.h ide_default_io_base(): blaat: interrupts were disabled@51
--
Dec 29 14:17:47 localhost kernel: include/asm-i386/ide.h ide_default_io_base(): blaat: interrupts were disabled@49
Dec 29 14:17:47 localhost kernel: Pci device list empty, preventing down_read
Dec 29 14:17:47 localhost kernel: include/asm-i386/ide.h ide_default_io_base(): blaat: interrupts were disabled@51
--
Dec 29 14:17:47 localhost kernel: include/asm-i386/ide.h ide_default_io_base(): blaat: interrupts were disabled@49
Dec 29 14:17:47 localhost kernel: Pci device list empty, preventing down_read
Dec 29 14:17:47 localhost kernel: include/asm-i386/ide.h ide_default_io_base(): blaat: interrupts were disabled@51
--
Dec 29 14:17:47 localhost kernel: include/asm-i386/ide.h ide_default_io_base(): blaat: interrupts were disabled@49
Dec 29 14:17:47 localhost kernel: Pci device list empty, preventing down_read
Dec 29 14:17:47 localhost kernel: include/asm-i386/ide.h ide_default_io_base(): blaat: interrupts were disabled@51
--
Dec 29 14:17:47 localhost kernel: include/asm-i386/ide.h ide_default_io_base(): blaat: interrupts were disabled@49
Dec 29 14:17:47 localhost kernel: Pci device list empty, preventing down_read
Dec 29 14:17:47 localhost kernel: include/asm-i386/ide.h ide_default_io_base(): blaat: interrupts were disabled@51
--
Dec 29 14:17:47 localhost kernel: include/asm-i386/ide.h ide_default_io_base(): blaat: interrupts were disabled@49
Dec 29 14:17:47 localhost kernel: Pci device list empty, preventing down_read
Dec 29 14:17:47 localhost kernel: include/asm-i386/ide.h ide_default_io_base(): blaat: interrupts were disabled@51
--
Dec 29 14:17:47 localhost kernel: include/asm-i386/ide.h ide_default_io_base(): blaat: interrupts were disabled@49
Dec 29 14:17:47 localhost kernel: Pci device list empty, preventing down_read
Dec 29 14:17:47 localhost kernel: include/asm-i386/ide.h ide_default_io_base(): blaat: interrupts were disabled@51
--
Dec 29 14:17:47 localhost kernel: include/asm-i386/ide.h ide_default_io_base(): blaat: interrupts were disabled@49
Dec 29 14:17:47 localhost kernel: Pci device list empty, preventing down_read
Dec 29 14:17:47 localhost kernel: include/asm-i386/ide.h ide_default_io_base(): blaat: interrupts were disabled@51
--
Dec 29 14:17:47 localhost kernel: include/asm-i386/ide.h ide_default_io_base(): blaat: interrupts were disabled@49
Dec 29 14:17:47 localhost kernel: Pci device list empty, preventing down_read
Dec 29 14:17:47 localhost kernel: include/asm-i386/ide.h ide_default_io_base(): blaat: interrupts were disabled@51
I don't see any other warnings, so I guess the patch is working now :-).
I will clean up the patches found on this list to fix and detect this.
program signature;
begin { telegraaf.com
} writeln("<[email protected]> TEM2");
end
.
On Fri, Dec 29, 2006 at 02:27:59PM +0100, Ard -kwaak- van Breemen wrote:
> I will clean up the patches found on this list to fix and detect this.
Preliminary patches:
- pci fix of Andrews patches
- parse-one detection of Yanmin
- start_kernel detection and workaround (disable them again)
These are the patches that I am about to test in the next 2
hours... :-)
Anyway: I think it is possible that other drivers are also
potential irq enablers as soon as they are called from parse_one.
Usually I compile network drivers as modules, but in diskless
setups this might not be the case :-).
--
program signature;
begin { telegraaf.com
} writeln("<[email protected]> TEM2");
end
.
On Fri, Dec 29, 2006 at 03:10:58PM +0100, Ard -kwaak- van Breemen wrote:
> Preliminary patches:
> - pci fix of Andrews patches
The printk might be too verbose. I think removing them is ok
since the only thing that has happened is that it prevents
entering the loop and the semaphores. The only thing that bugs me
is if list_empty can be used like that. (in other words: don't we
need semaphores around that).
> - parse-one detection of Yanmin
It doesn't flag it. I am working on that.
> - start_kernel detection and workaround (disable them again)
main-irq-enable-detection-and-disable-again.patch is working
great. I love to see that one included in the kernel.
--
program signature;
begin { telegraaf.com
} writeln("<[email protected]> TEM2");
end
.
On Fri, Dec 29, 2006 at 04:01:32PM +0100, Ard -kwaak- van Breemen wrote:
> > - parse-one detection of Yanmin
> It doesn't flag it. I am working on that.
Since it goes to a callback to obsolete_checksetup()
Argh... my calltree was a little flawed :-(...
--
program signature;
begin { telegraaf.com
} writeln("<[email protected]> TEM2");
end
.
On Fri, Dec 22, 2006 at 03:35:20PM +0100, Ard -kwaak- van Breemen wrote:
> On Fri, Dec 22, 2006 at 12:30:29AM -0800, Andrew Morton wrote:
> > I expect that you'll find that the ide code ends up doing
> > down_write(pci_bus_sem), which will enable interrupts.
> will: down_read(&pci_bus_sem);
> also enable interrupts?
> Since that is called:
> init/main.c start_kernel
> kernel/params.c parse_args("Booting kernel"
> kernel/params.c parse_one
-----------------------------------------------
init/main.c unknown_bootoption
init/main.c obsolete_checksetup
-----------------------------------------------
> drivers/ide/ide.c ide_setup
> drivers/ide/ide.c init_ide_data
> drivers/ide/ide.c init_hwif_default
> include/asm-i386/ide.h ide_default_io_base(index)
> drivers/pci/search.c pci_find_device
> drivers/pci/search.c pci_find_subsys
Fixes in the calltree
--
program signature;
begin { telegraaf.com
} writeln("<[email protected]> TEM2");
end
.
On Fri, Dec 29, 2006 at 04:01:32PM +0100, Ard -kwaak- van Breemen wrote:
> > - parse-one detection of Yanmin
> It doesn't flag it. I am working on that.
As said: it was doing a callback to obsolete_...
This replaces the patch into not being bloated and still gives
enough info. It won't check voor callbacks or whatever, just
which parameter b0rked it.
Output of dmesg without the pci-patch applied:
ard@supergirl:~$ dmesg|grep -B5 -A1 'interrupts were enabled'
Kernel command line: console=tty0 console=ttyS0,115200 hdb=noprobe hdc=noprobe hdd=noprobe root=/dev/md0 ro panic=30 earlyprintk=serial,ttyS0,115200
ide_setup: hdb=noprobe
parse_args(): option 'hdb=noprobe' enabled irq's!
ide_setup: hdc=noprobe
ide_setup: hdd=noprobe
start_kernel(): bug: interrupts were enabled *very* early, fixing it
Initializing CPU#0
--
program signature;
begin { telegraaf.com
} writeln("<[email protected]> TEM2");
end
.
Hello,
On Fri, Dec 29, 2006 at 04:01:32PM +0100, Ard -kwaak- van Breemen wrote:
> On Fri, Dec 29, 2006 at 03:10:58PM +0100, Ard -kwaak- van Breemen wrote:
> > Preliminary patches:
> > - pci fix of Andrews patches
> The printk might be too verbose. I think removing them is ok
I stick with the verbose printk. Because else we will never know
that something is faul.
> since the only thing that has happened is that it prevents
> entering the loop and the semaphores. The only thing that bugs me
> is if list_empty can be used like that. (in other words: don't we
> need semaphores around that).
I was wondering about the validity of pci_devices at that time.
But on the other hand: if that was not wrong, people would have
complained much earlier.
Anyway, I think that's it: those 3 patches will fix and guard the
problems we've seen.
--
program signature;
begin { telegraaf.com
} writeln("<[email protected]> TEM2");
end
.
The calls made by parse_parms to other initialization code might
enable interrupts again way too early.
Having interrupts on this early can make systems PANIC when they
initialize the IRQ controllers (which happens later in the code).
This patch detects that irq's are enabled again, barfs about it
and disables them again as a safety net.
Signed-off-by: Ard van Breemen <[email protected]>
--- linux-2.6.20-rc2-git1/init/main.c.orig 2006-12-30 17:41:13.000000000 +0000
+++ linux-2.6.20-rc2-git1/init/main.c 2006-12-30 17:44:02.000000000 +0000
@@ -538,6 +538,10 @@ asmlinkage void __init start_kernel(void
parse_args("Booting kernel", command_line, __start___param,
__stop___param - __start___param,
&unknown_bootoption);
+ if (!irqs_disabled()) {
+ printk(KERN_WARNING "start_kernel(): bug: interrupts were enabled *very* early, fixing it\n");
+ local_irq_disable();
+ }
sort_main_extable();
trap_init();
rcu_init();
The parsing of some kernel parameters seem to enable irq's at a
stage that irq's are not supposed to be enabled (Particularly the
ide kernel parameters). Having irq's enabled before the irq
controller is initialized might lead to a kernel panic.
This patch only detects this behaviour and warns about wich
parameter caused it.
Signed-off-by: Ard van Breemen <[email protected]>
--- linux-2.6.19.vanilla/kernel/params.c 2006-11-29 21:57:37.000000000 +0000
+++ linux-2.6.19.ok/kernel/params.c 2006-12-29 15:14:26.000000000 +0000
@@ -143,9 +143,14 @@ int parse_args(const char *name,
while (*args) {
int ret;
+ int irq_was_disabled;
args = next_arg(args, ¶m, &val);
+ irq_was_disabled=irqs_disabled();
ret = parse_one(param, val, params, num, unknown);
+ if(irq_was_disabled && !irqs_disabled()) {
+ printk(KERN_WARNING "parse_args(): option '%s' enabled irq's!\n",param);
+ }
switch (ret) {
case -ENOENT:
printk(KERN_ERR "%s: Unknown parameter `%s'\n",
The pci_find_subsys gets called very early by obsolete ide setup
parameters. This is a bogus call since pci is not initialized
yet, so the list is empty. But in the mean time, interrupts get
enabled by down_read. This can result in a kernel panic when the
irq controller gets initialized.
This patch checks if the device list is empty before taking the
semaphore, and hence will not enable irq's. Furthermore it will
inform that it is called while pci_devices is empty as a reminder
that the ide code needs to be fixed.
The pci_get_subsys can get called in the same manner, and as such
is patched in the same manner.
Signed-off-by: Ard van Breemen <[email protected]>
----
This patch is an adaption of Andrew Mortons patch.
--- linux-2.6.19.vanilla/drivers/pci/search.c 2006-11-29 21:57:37.000000000 +0000
+++ linux-2.6.19.ok/drivers/pci/search.c 2006-12-29 15:38:18.000000000 +0000
@@ -193,6 +193,17 @@ static struct pci_dev * pci_find_subsys(
struct pci_dev *dev;
WARN_ON(in_interrupt());
+
+ /*
+ * pci_find_subsys() can be called on the ide_setup() path, super-early
+ * in boot. But the down_read() will enable local interrupts, which
+ * can cause some machines to crash. So here we detect and flag that
+ * situation and bail out early.
+ */
+ if(unlikely(list_empty(&pci_devices))) {
+ printk(KERN_INFO "pci_find_subsys() called while pci_devices is still empty\n");
+ return NULL;
+ }
down_read(&pci_bus_sem);
n = from ? from->global_list.next : pci_devices.next;
@@ -259,6 +270,16 @@ pci_get_subsys(unsigned int vendor, unsi
struct pci_dev *dev;
WARN_ON(in_interrupt());
+ /*
+ * pci_get_subsys() can potentially be called by drivers super-early
+ * in boot. But the down_read() will enable local interrupts, which
+ * can cause some machines to crash. So here we detect and flag that
+ * situation and bail out early.
+ */
+ if(unlikely(list_empty(&pci_devices))) {
+ printk(KERN_NOTICE "pci_get_subsys() called while pci_devices is still empty\n");
+ return NULL;
+ }
down_read(&pci_bus_sem);
n = from ? from->global_list.next : pci_devices.next;
> + printk(KERN_WARNING "start_kernel(): bug: interrupts were enabled *very* early, fixing it\n");
I built and booted the next-20080303 tag from linux-next and
found the above warning in my console log on ia64 (this is
new ... I've never seen this message before, even though
this patch was applied January 2007).
Hunting this down, I found the enabler was the lock_kernel() call
on line 536 of init/main.c ... doesn't than happen to other archs
too? We get into the first call to lock_kernel() with current->lock_depth
set to -1, so we call down(&kernel_sem) ... which does spin_lock_irq()
and then spin_unlock_irq() ... leaving interrupts enabled.
What else changed to make this suddenly kick out now? It
doesn't happen from a build from Linus' tree.
-Tony
Hi Tony,
On Mon, 3 Mar 2008 14:46:52 -0800 "Tony Luck" <[email protected]> wrote:
>
> > + printk(KERN_WARNING "start_kernel(): bug: interrupts were enabled *very* early, fixing it\n");
>
> I built and booted the next-20080303 tag from linux-next and
> found the above warning in my console log on ia64 (this is
> new ... I've never seen this message before, even though
> this patch was applied January 2007).
>
> Hunting this down, I found the enabler was the lock_kernel() call
> on line 536 of init/main.c ... doesn't than happen to other archs
> too? We get into the first call to lock_kernel() with current->lock_depth
> set to -1, so we call down(&kernel_sem) ... which does spin_lock_irq()
> and then spin_unlock_irq() ... leaving interrupts enabled.
>
> What else changed to make this suddenly kick out now? It
> doesn't happen from a build from Linus' tree.
This is Willy's generic semaphore code that is included in linux-next.
It is being discussed in another thread "linux-next: Tree for Feb 29:
WARNING: at kernel/lockdep.c:2024 trace_hardirqs_on" on the
[email protected] mailing list (archived at
http://marc.info/?l=linux-next&m=120440556729954&w=2).
--
Cheers,
Stephen Rothwell [email protected]
http://www.canb.auug.org.au/~sfr/