On Sun, 9 Feb 2003 12:32:01 +0100, Pavel Machek wrote:
>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.
>+static struct device_driver apic_driver = {
>+ .name = "apic",
>+ .bus = &system_bus_type,
>+ .resume = apic_resume,
>+ .suspend = apic_suspend,
>+};
...
>+static int __init init_apic_devicefs(void)
> {
>+ driver_register(&apic_driver);
> if (apic_pm_state.active)
>- pm_register(PM_SYS_DEV, 0, apic_pm_callback);
>+ return sys_device_register(&device_apic);
>+ return 0;
...
>+device_initcall(init_apic_devicefs);
init_apic_devicefs() registers apic_driver even if active is false.
This probably breaks any machine where cpu_has_apic is false, since
apic_suspend/resume will access non-existent APIC registers & MSRs.
I don't like the idea of registering apic_driver when !cpu_has_apic,
but it might be needed for the local-APIC NMI watchdog. A fix could
be to guard apic_suspend/resume with "if(!cpu_has_apic)return;".
>--- 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.
>+static int __init init_nmi_devicefs(void)
>+{
>+ driver_register(&nmi_driver);
>+
>+ device_nmi.parent = &device_apic.dev;
>+ return device_register(&device_nmi);
>+}
>
>-#define __pminit __init
>+device_initcall(init_nmi_devicefs);
Doesn't this require that init_apic_devicefs() has been done?
Can you guarantee that?
>
>+#define __pminit
> #endif /* CONFIG_PM */
__pminit is no longer defined when !CONFIG_PM, which will
cause compile errors. Possibly the remaining uses of __pminit
should be removed.
>--- 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:
static unsigned int prev_nmi_watchdog;
..
prev_nmi_watchdog = nmi_watchdog;
if (prev_nmi_watchdog == NMI_LOCAL_APIC) {
disable_apic_nmi_watchdog();
nmi_watchdog = NMI_NONE;
}
...
if (prev_nmi_watchdog == NMI_LOCAL_APIC) {
nmi_watchdog = NMI_LOCAL_APIC;
setup_apic_nmi_watchdog();
}
without having to add crap to the global namespace?
/Mikael
-
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:22 EST