[bug] endless loop in arch/i386/kernel/mpparse.c

[bug] endless loop in arch/i386/kernel/mpparse.c

[Bjoern A. Zeeb]

Hi Linus,

with

D 1.21 02/09/26 16:01:50-07:00 torvalds-NOXD9LnmIcc+KpMpn4J55tBPR1lH4CV8@public.gmane.org 43 42 00029/00000/01101
P arch/i386/kernel/mpparse.c
C Make the ACPI SCI interrupt get the right polarity
C when it is explicitly overridden in the MADT

you merged in following code where I added a printk:

===== mpparse.c 1.20 vs 1.21 =====
--- 1.20/arch/i386/kernel/mpparse.c     Fri Sep 20 01:14:32 2002
+++ 1.21/arch/i386/kernel/mpparse.c     Thu Sep 26 23:01:50 2002
@@ -1019,10 +1019,39 @@

 /* Ensure the ACPI SCI interrupt level is active low, edge-triggered */

+extern FADT_DESCRIPTOR acpi_fadt;
+
 void __init mp_config_ioapic_for_sci(int irq)
 {
        int ioapic;
        int ioapic_pin;
+       struct acpi_table_madt *madt;
+       struct acpi_table_int_src_ovr *entry = NULL;
+       void *madt_end;
+       acpi_status status;
+
+       /*
+        * Ensure that if there is an interrupt source override entry
+        * for the ACPI SCI, we leave it as is. Unfortunately this involves
+        * walking the MADT again.
+        */
+       status = acpi_get_firmware_table("APIC", 1, ACPI_LOGICAL_ADDRESSING,
+               (acpi_table_header **) &madt);
+       if (ACPI_SUCCESS(status)) {
+               madt_end = madt + madt->header.length;
+
+               entry = (struct acpi_table_int_src_ovr *)
+                ((unsigned long) madt + sizeof(struct acpi_table_madt));
+
+               while ((void *) entry < madt_end) {
			printk("[%s:%s:%d] madt_end=%p, entry=%p\n",
				__FILE__, __func__, __LINE__, madt_end, entry);
+                       if (entry->header.type == ACPI_MADT_INT_SRC_OVR &&
+                           acpi_fadt.sci_int == entry->global_irq)
+                               return;
+
+                       entry = (struct acpi_table_int_src_ovr *)
+                               ((unsigned long) entry + entry->header.length);
+               }
+       }

        ioapic = mp_find_ioapic(irq);
=== end ===

On boot time the kernel "hangs" (log down under).
This problem was reported on lkml without more details (i.e. any
specific code where the kernel "hangs") too.


The point is that the loop for some reason is endless.
Can anybody fix that please ? As this core code and I am not going
to touch it ;-)

simply #if 0ing out the above diff the kernel comes up fine.


--- boot log excerpt ---
...
PCI: PCI BIOS revision 2.10 entry at 0xfdb91, last bus=1
PCI: Using configuration type 1
ACPI: Subsystem revision 20020918
 tbxface-0099 [03] Acpi_load_tables      : ACPI Tables successfully loaded
Parsing Methods:......................................................................................................
Table [DSDT] - 309 Objects with 22 Devices 102 Methods 19 Regions
ACPI Namespace successfully loaded at root c03a741c
[mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d88070dc
[mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d88070e4
[mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d88070ec
[mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d88070f8
[mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d8807102
[mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d880710c
[mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d880710c
[mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d880710c
[mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d880710c
[mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d880710c
[mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d880710c
[mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d880710c
[mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d880710c
[mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d880710c
[mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d880710c
[mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d880710c
[mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d880710c
[mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d880710c
[mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d880710c
[mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d880710c
[mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d880710c
...
...
...
--- end ---


If you need further information or testing please let me know.

Thanks i advance.

-- 
Greetings

Bjoern A. Zeeb				bzeeb at Zabbadoz dot NeT
56 69 73 69 74				http://www.zabbadoz.net/




-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

RE: [bug] endless loop in arch/i386/kernel/mpparse.c

[Grover, Andrew]

Well I actually wrote that patch, so any bugs are my fault...

> From: Bjoern A. Zeeb [mailto:bzeeb-lists-mt/InyFV0sMN6vK3N8ee0qxOck334EZe@public.gmane.org] 

> On boot time the kernel "hangs" (log down under).
> This problem was reported on lkml without more details (i.e. any
> specific code where the kernel "hangs") too.

> The point is that the loop for some reason is endless.
> Can anybody fix that please ? As this core code and I am not going
> to touch it ;-)

You're the first one to report a problem with this code, so you are in the
best position to maybe diagnose this one a little more, and possibly fix
it...the code looks fine to me but maybe I'm too close to it.

Regards -- Andy


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: [bug] endless loop in arch/i386/kernel/mpparse.c

[Ernst Herzberg]

On Sonntag, 29. September 2002 23:32, Grover, Andrew wrote:

> You're the first one to report a problem with this code, so you are in the
> best position to maybe diagnose this one a little more, and possibly fix
> it...the code looks fine to me but maybe I'm too close to it.

Maybe the last entry->header.length ist 0?

<Earny>


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: [bug] endless loop in arch/i386/kernel/mpparse.c

[Matthew Wilcox]

On Sun, Sep 29, 2002 at 09:27:41PM +0200, Bjoern A. Zeeb wrote:
>  void __init mp_config_ioapic_for_sci(int irq)
>  {
>         int ioapic;
>         int ioapic_pin;
> +       struct acpi_table_madt *madt;
> +       struct acpi_table_int_src_ovr *entry = NULL;
> +       void *madt_end;
> +       acpi_status status;
> +
> +       /*
> +        * Ensure that if there is an interrupt source override entry
> +        * for the ACPI SCI, we leave it as is. Unfortunately this involves
> +        * walking the MADT again.
> +        */
> +       status = acpi_get_firmware_table("APIC", 1, ACPI_LOGICAL_ADDRESSING,
> +               (acpi_table_header **) &madt);
> +       if (ACPI_SUCCESS(status)) {
> +               madt_end = madt + madt->header.length;
> +
> +               entry = (struct acpi_table_int_src_ovr *)
> +                ((unsigned long) madt + sizeof(struct acpi_table_madt));
> +
> +               while ((void *) entry < madt_end) {
> 			printk("[%s:%s:%d] madt_end=%p, entry=%p\n",
> 				__FILE__, __func__, __LINE__, madt_end, entry);
> +                       if (entry->header.type == ACPI_MADT_INT_SRC_OVR &&
> +                           acpi_fadt.sci_int == entry->global_irq)
> +                               return;
> +
> +                       entry = (struct acpi_table_int_src_ovr *)
> +                               ((unsigned long) entry + entry->header.length);
> +               }
> +       }
> 
>         ioapic = mp_find_ioapic(irq);

If entry->header.length is zero without entry being equal to madt_end,
this will loop for ever.  Maybe:

[...]
			if (entry->header.length <= 0)
				break;
			entry = (struct acpi_table_int_src_ovr *)
				((unsigned long) entry + entry->header.length);
		}

is the right solution?

> [mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d88070dc
> [mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d88070e4
> [mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d88070ec
> [mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d88070f8
> [mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d8807102
> [mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d880710c
> [mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d880710c

You can see that entry->header.length must be 0 at this point.  Probably a
buggy MADT, but let's not _crash_ when the vendor provides a shit BIOS.

-- 
Revolutions do not require corporate support.


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: [bug] endless loop in arch/i386/kernel/mpparse.c

[Bjoern A. Zeeb]

On Sun, 29 Sep 2002, Matthew Wilcox wrote:

> If entry->header.length is zero without entry being equal to madt_end,
> this will loop for ever.  Maybe:
>
> [...]
> 			if (entry->header.length <= 0)
> 				break;

but is this really correct or does it simply hide the bug ?

> 			entry = (struct acpi_table_int_src_ovr *)
> 				((unsigned long) entry + entry->header.length);
> 		}
>
> is the right solution?


the other question: what is entry->header.length for the last entry
supposed to be ?  Should it be zero ?
Cause I think madt_end may hold the wrong value. (more down under).


> > [mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d88070dc
> > [mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d88070e4
> > [mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d88070ec
> > [mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d88070f8
> > [mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d8807102
> > [mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d880710c
> > [mpparse.c:mp_config_ioapic_for_sci:1047] madt_end=d8808080, entry=d880710c
>
> You can see that entry->header.length must be 0 at this point.  Probably a

yeah obviously.

> buggy MADT, but let's not _crash_ when the vendor provides a shit BIOS.

the other thing that I was thinking of is: entry is icremented by 8
but to reach madt_end we need to do another 494 increments. As I am
not really in that code I ask myself what is in that struct. If I
remember correctly 309 objects, ... were found; far not that much ...
(assumtion that we walk through them).
This, with the entry->header.length being zero I think (repeating
myself) madt_end may be wrong ?

I'll be off for the day now but going to pull and then check the
code in more detail esp. going back to drivers/acpi tracing
acpi_get_firmware_table().
Can anybody tell me when/where this list we walk through gets filled ?
Just to check what's coming in there ...


PS: I think on lkml in the "v2.6 vs v3.0" the posting from Murray J.
Root with answer from Zwane Mwaikambo may be related to this.
(for those to grep: original msg-id was: <20020929111918.GA1639-KLZTGNm03OFvpZiPlmaQFA@public.gmane.org>

-- 
Bjoern A. Zeeb				bzeeb at Zabbadoz dot NeT
56 69 73 69 74				http://www.zabbadoz.net/



-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

RE: [bug] endless loop in arch/i386/kernel/mpparse.c

[Grover, Andrew]

> From: Bjoern A. Zeeb [mailto:bzeeb-lists-mt/InyFV0sMN6vK3N8ee0qxOck334EZe@public.gmane.org] 
> > buggy MADT, but let's not _crash_ when the vendor provides 
> a shit BIOS.
> 
> the other thing that I was thinking of is: entry is icremented by 8
> but to reach madt_end we need to do another 494 increments. As I am
> not really in that code I ask myself what is in that struct. If I
> remember correctly 309 objects, ... were found; far not that much ...
> (assumtion that we walk through them).
> This, with the entry->header.length being zero I think (repeating
> myself) madt_end may be wrong ?

I cribbed this code from table.c acpi_table_parse_madt_family, which works
fine.

Some change must have screwed something up for this to be hanging...

Regards -- Andy


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf