Bogus ioapic entry

[John Baldwin]

On Wednesday 16 November 2005 05:48 am, Nikos Ntarmos wrote:
> Hi all.
>
> I spent a couple of hours last night trying to get ACPI to function
> correctly on my new Acer Aspire 1692WLMi. I found out that with apic
> enabled, the system hangs when returning from S3, while with apic
> disabled it suffers from interrupt storms on irq9 and the screen is not
> reinitialized on return from S3 (with hw.acpi.reset_video set to both
> "0" and "1"). While trying to hunt this thing down, I noticed that the
> GENERIC kernel of 6.0-STABLE (and probably of other branches as well)
> incorrectly discovers two ioapic devices:
>
> ioapic1: Changing APIC ID to 2
> ioapic0 <Version 2.0> irqs 0-23 on motherboard
> ioapic1 <Version 15.15> irqs 24-23 on motherboard
>
> There is something obviously wrong with ioapic1. The cause seems to be
> the handling of value and numintr in ioapic_create(...). value is a
> uint32_t, while numintr is a u_int, both of which are 32-bits long (per
> <sys/types.h> and <machine/_types.h> at least on my i386). The code in
> ioapic_create(...), after the call to ioapic_read(apic, IOAPIC_VER),
> uses the latter's return value to compute the number of interrupts
> assigned to the current ioapic. This return value is checked against
> 0xffffff (that's 24 1-bits) to detect an error and bail out. This code
> chunk is also in -current (so perhaps I'm missing something here...)
>
> The (buggy?) BIOS implementation of my laptop returns a value of
> 0xffffffff (32 1-bits) in ioapic_read(...). This, however, doesn't
> trigger the current if-clause, so the code proceeds and creates ioapic1
> with -1(!) assigned IRQs (due to the and-and-shift-plus-one formula of
> computing numintr).
>
> The attached diff fixes this situation and seems more correct to me than
> the current code, given that io_numintr is 8 bits and u_int is 32 bits
> per <cannot-remember> C standard. The problems with resuming from S3
> still haunt me though. I guess I should have a look at acpi@ later
> today...
>
> Cheers.
>
> \n\n

How about this diff instead.  It is supposed to be explicitly checking for -1 
since a read of an invalid memory or device location returns -1.  I just 
typo'd and didn't put the right value in for the check.

Index: io_apic.c
===================================================================
RCS file: /usr/cvs/src/sys/i386/i386/io_apic.c,v
retrieving revision 1.23
diff -u -r1.23 io_apic.c
--- io_apic.c   2 Nov 2005 20:11:47 -0000       1.23
+++ io_apic.c   16 Nov 2005 16:09:19 -0000
@@ -502,7 +502,7 @@
        mtx_unlock_spin(&icu_lock);

        /* If it's version register doesn't seem to work, punt. */
-       if (value == 0xffffff) {
+       if (value == 0xffffffff) {
                pmap_unmapdev((vm_offset_t)apic, IOAPIC_MEM_REGION);
                return (NULL);
        }

-- 
John Baldwin <jhb at FreeBSD.org>  <><  http://www.FreeBSD.org/~jhb/
"Power Users Use the Power to Serve"  =  http://www.FreeBSD.org