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

[Pavel Machek]

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.