Re: Switch APIC (+nmi, +oprofile) to driver model

From: Pavel Machek (pavel@ucw.cz)
Date: Mon Feb 10 2003 - 06:01:09 EST


Hi!

> >Here's next attempt at moving APIC (+nmi, +oprofile) to the driver
> >model. If it looks good I'l submit it to Linus.
>
> I'm sorry to be a killjoy, but this still doesn't look right.
>
> >--- clean/arch/i386/kernel/apic.c 2003-01-05 22:58:18.000000000 +0100
> >+++ linux-swsusp/arch/i386/kernel/apic.c 2003-02-03 16:36:41.000000000 +0100
> >-static void apic_pm_resume(void *data)
> >+static int apic_resume(struct device *dev, u32 level)
> > {
> > unsigned int l, h;
> > unsigned long flags;
> >
> >+ if (level != RESUME_POWER_ON)
> >+ return 0;
> >+
> >+ set_fixmap_nocache(FIX_APIC_BASE, apic_phys); /* FIXME: this is needed for S3 resume, but why? */
>
> This is horrible! The only reason this might be needed is if
> the page tables weren't restored properly at resume, and that
> indicates a bug somewhere else.
>
> Also, apic_phys is (or should be) APIC_DEFAULT_PHYS_BASE, so
> you shouldn't need to make apic_phys global.

Really?

        /*
         * If no local APIC can be found then set up a fake all
         * zeroes page to simulate the local APIC and another
         * one for the IO-APIC.
         */
        if (!smp_found_config && detect_init_APIC()) {
                apic_phys = (unsigned long) alloc_bootmem_pages(PAGE_SIZE);
                apic_phys = __pa(apic_phys);
        } else
                apic_phys = mp_lapic_addr;

        set_fixmap_nocache(FIX_APIC_BASE, apic_phys);

So it seems to me it really is variable.

> >--- clean/arch/i386/kernel/nmi.c 2003-01-05 22:58:19.000000000 +0100
> >+++ linux-swsusp/arch/i386/kernel/nmi.c 2003-02-09 11:43:29.000000000 +0100
> >@@ -118,10 +121,6 @@
> > * missing bits. Right now Intel P6/P4 and AMD K7 only.
> > */
> > if ((nmi == NMI_LOCAL_APIC) &&
> >- (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) &&
> >- (boot_cpu_data.x86 == 6 || boot_cpu_data.x86 == 15))
> >- nmi_watchdog = nmi;
> >- if ((nmi == NMI_LOCAL_APIC) &&
> > (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) &&
> > (boot_cpu_data.x86 == 6 || boot_cpu_data.x86 == 15))
> > nmi_watchdog = nmi;
>
> You just killed NMI_LOCAL_APIC support on Intel.

Oops, sorry, I seen two identical blocks of code... and they were not
identical. Sorry.

> >--- clean/arch/i386/oprofile/nmi_int.c 2003-01-05 22:58:19.000000000 +0100
> >+++ linux-swsusp/arch/i386/oprofile/nmi_int.c 2003-02-09 12:16:52.000000000 +0100
> ...
> >+ if (nmi_watchdog == NMI_LOCAL_APIC) {
> >+ disable_apic_nmi_watchdog();
> >+ nmi_watchdog = NMI_LOCAL_APIC_SUSPENDED_BY_OPROFILE;
> >+ }
> ...
> >+ if (nmi_watchdog == NMI_LOCAL_APIC_SUSPENDED_BY_OPROFILE) {
> >+ nmi_watchdog = NMI_LOCAL_APIC_SUSPENDED_BY_OPROFILE;
> >+ setup_apic_nmi_watchdog();
> >+ }
> ...
> >--- clean/include/asm-i386/apic.h 2002-10-20 16:22:45.000000000 +0200
> >+++ linux-swsusp/include/asm-i386/apic.h 2003-02-09 11:46:09.000000000 +0100
> >@@ -95,6 +95,7 @@
> > #define NMI_IO_APIC 1
> > #define NMI_LOCAL_APIC 2
> > #define NMI_INVALID 3
> >+#define NMI_LOCAL_APIC_SUSPENDED_BY_OPROFILE 4
>
> This is ugly like h*ll. Surely oprofile could just do:

Yes, whole oprofile/nmi interaction is ugly like hell. This way it is
at least explicit, so people *know* its ugly.
                                                                Pavel

-- 
Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/



This archive was generated by hypermail 2b29 : Sat Feb 15 2003 - 22:00:26 EST