Hi!
This switches apic code to driver model, cleans code up a lot, and
makes S3 while apic is used work. Please apply,
Pavel
--- clean/arch/i386/kernel/apic.c 2003-01-05 22:58:18.000000000 +0100
+++ linux-swsusp/arch/i386/kernel/apic.c 2003-01-20 23:19:50.000000000 +0100
@@ -54,6 +55,8 @@
int prof_old_multiplier[NR_CPUS] = { 1, };
int prof_counter[NR_CPUS] = { 1, };
+unsigned long apic_phys;
+
int get_maxlvt(void)
{
unsigned int v, ver, maxlvt;
@@ -438,8 +441,8 @@
}
#ifdef CONFIG_PM
-
#include <linux/slab.h>
+#include <linux/device.h>
#include <linux/pm.h>
static struct {
@@ -447,9 +450,6 @@
not the BIOS; this signifies that we are also responsible
for disabling it before entering apm/acpi suspend */
int active;
- /* 'perfctr_pmdev' is here because the current (2.4.1) PM
- callback system doesn't handle hierarchical dependencies */
- struct pm_dev *perfctr_pmdev;
/* r/w apic fields */
unsigned int apic_id;
unsigned int apic_taskpri;
@@ -466,13 +466,15 @@
unsigned int apic_thmr;
} apic_pm_state;
-static void apic_pm_suspend(void *data)
+static int apic_suspend(struct device *dev, u32 state, u32 level)
{
unsigned int l, h;
unsigned long flags;
- if (apic_pm_state.perfctr_pmdev)
- pm_send(apic_pm_state.perfctr_pmdev, PM_SUSPEND, data);
+ if (level != SUSPEND_DISABLE)
+ return 0;
+
+ disable_apic_nmi_watchdog();
apic_pm_state.apic_id = apic_read(APIC_ID);
apic_pm_state.apic_taskpri = apic_read(APIC_TASKPRI);
apic_pm_state.apic_ldr = apic_read(APIC_LDR);
@@ -493,13 +495,19 @@
l &= ~MSR_IA32_APICBASE_ENABLE;
wrmsr(MSR_IA32_APICBASE, l, h);
local_irq_restore(flags);
+ return 0;
}
-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? */
+
local_irq_save(flags);
rdmsr(MSR_IA32_APICBASE, l, h);
l &= ~MSR_IA32_APICBASE_BASE;
@@ -524,74 +532,33 @@
apic_write(APIC_ESR, 0);
apic_read(APIC_ESR);
local_irq_restore(flags);
- if (apic_pm_state.perfctr_pmdev)
- pm_send(apic_pm_state.perfctr_pmdev, PM_RESUME, data);
-}
-
-static int apic_pm_callback(struct pm_dev *dev, pm_request_t rqst, void *data)
-{
- switch (rqst) {
- case PM_SUSPEND:
- apic_pm_suspend(data);
- break;
- case PM_RESUME:
- apic_pm_resume(data);
- break;
- }
+ setup_apic_nmi_watchdog();
return 0;
}
-/* perfctr driver should call this instead of pm_register() */
-struct pm_dev *apic_pm_register(pm_dev_t type,
- unsigned long id,
- pm_callback callback)
-{
- struct pm_dev *dev;
-
- if (!apic_pm_state.active)
- return pm_register(type, id, callback);
- if (apic_pm_state.perfctr_pmdev)
- return NULL; /* we're busy */
- dev = kmalloc(sizeof(struct pm_dev), GFP_KERNEL);
- if (dev) {
- memset(dev, 0, sizeof(*dev));
- dev->type = type;
- dev->id = id;
- dev->callback = callback;
- apic_pm_state.perfctr_pmdev = dev;
- }
- return dev;
-}
+static struct device_driver apic_driver = {
+ .name = "apic",
+ .bus = &system_bus_type,
+ .resume = apic_resume,
+ .suspend = apic_suspend,
+};
+
+static struct sys_device device_apic = {
+ .name = "apic",
+ .id = 0,
+ .dev = {
+ .name = "APIC",
+ .driver = &apic_driver,
+ },
+};
-/* perfctr driver should call this instead of pm_unregister() */
-void apic_pm_unregister(struct pm_dev *dev)
+static int __init init_apic_devicefs(void)
{
- if (!apic_pm_state.active) {
- pm_unregister(dev);
- } else if (dev == apic_pm_state.perfctr_pmdev) {
- apic_pm_state.perfctr_pmdev = NULL;
- kfree(dev);
- }
-}
-
-static void __init apic_pm_init1(void)
-{
- /* can't pm_register() at this early stage in the boot process
- (causes an immediate reboot), so just set the flag */
- apic_pm_state.active = 1;
-}
-
-static void __init apic_pm_init2(void)
-{
- if (apic_pm_state.active)
- pm_register(PM_SYS_DEV, 0, apic_pm_callback);
+ driver_register(&apic_driver);
+ return sys_device_register(&device_apic);
}
-#else /* CONFIG_PM */
-
-static inline void apic_pm_init1(void) { }
-static inline void apic_pm_init2(void) { }
-
+device_initcall(init_apic_devicefs);
#endif /* CONFIG_PM */
/*
@@ -658,9 +625,6 @@
nmi_watchdog = NMI_LOCAL_APIC;
printk("Found and enabled local APIC!\n");
-
- apic_pm_init1();
-
return 0;
no_apic:
@@ -670,8 +634,6 @@
void __init init_apic_mappings(void)
{
- unsigned long apic_phys;
-
/*
* If no local APIC can be found then set up a fake all
* zeroes page to simulate the local APIC and another
@@ -1141,8 +1103,6 @@
phys_cpu_present_map = 1;
apic_write_around(APIC_ID, boot_cpu_physical_apicid);
- apic_pm_init2();
-
setup_local_APIC();
if (nmi_watchdog == NMI_LOCAL_APIC)
--- clean/arch/i386/kernel/i386_ksyms.c 2003-01-17 23:09:51.000000000 +0100
+++ linux-swsusp/arch/i386/kernel/i386_ksyms.c 2003-01-19 19:58:34.000000000 +0100
@@ -161,10 +161,6 @@
EXPORT_SYMBOL(flush_tlb_page);
#endif
-#if defined(CONFIG_X86_LOCAL_APIC) && defined(CONFIG_PM)
-EXPORT_SYMBOL_GPL(set_nmi_pm_callback);
-EXPORT_SYMBOL_GPL(unset_nmi_pm_callback);
-#endif
#ifdef CONFIG_X86_IO_APIC
EXPORT_SYMBOL(IO_APIC_get_PCI_irq_vector);
#endif
--- clean/arch/i386/kernel/nmi.c 2003-01-05 22:58:19.000000000 +0100
+++ linux-swsusp/arch/i386/kernel/nmi.c 2003-01-19 20:02:27.000000000 +0100
@@ -138,11 +138,7 @@
#ifdef CONFIG_PM
-#include <linux/pm.h>
-
-struct pm_dev *nmi_pmdev;
-
-static void disable_apic_nmi_watchdog(void)
+void disable_apic_nmi_watchdog(void)
{
switch (boot_cpu_data.x86_vendor) {
case X86_VENDOR_AMD:
@@ -162,45 +158,7 @@
}
}
-static int nmi_pm_callback(struct pm_dev *dev, pm_request_t rqst, void *data)
-{
- switch (rqst) {
- case PM_SUSPEND:
- disable_apic_nmi_watchdog();
- break;
- case PM_RESUME:
- setup_apic_nmi_watchdog();
- break;
- }
- return 0;
-}
-
-struct pm_dev * set_nmi_pm_callback(pm_callback callback)
-{
- apic_pm_unregister(nmi_pmdev);
- return apic_pm_register(PM_SYS_DEV, 0, callback);
-}
-
-void unset_nmi_pm_callback(struct pm_dev * dev)
-{
- apic_pm_unregister(dev);
- nmi_pmdev = apic_pm_register(PM_SYS_DEV, 0, nmi_pm_callback);
-}
-
-static void nmi_pm_init(void)
-{
- if (!nmi_pmdev)
- nmi_pmdev = apic_pm_register(PM_SYS_DEV, 0, nmi_pm_callback);
-}
-
-#define __pminit /*empty*/
-
-#else /* CONFIG_PM */
-
-static inline void nmi_pm_init(void) { }
-
-#define __pminit __init
-
+#define __pminit
#endif /* CONFIG_PM */
/*
@@ -314,7 +272,6 @@
default:
return;
}
- nmi_pm_init();
}
static spinlock_t nmi_print_lock = SPIN_LOCK_UNLOCKED;
--
Worst form of spam? Adding advertisment signatures ala sourceforge.net.
What goes next? Inserting advertisment *into* email?
On Mon, 20 Jan 2003 23:25:27 +0100, Pavel Machek wrote:
>This switches apic code to driver model, cleans code up a lot, and
>makes S3 while apic is used work. Please apply,
Please don't apply this. It breaks stuff:
1. apic_suspend() unconditionally calls disable_apic_nmi_watchdog()
apic_resume() unconditionally calls setup_apic_nmi_watchdog()
apic_pm_state.perfctr_pmdev removed
- You're calling local-APIC NMI watchdog procedures even if
the local-APIC NMI watchdog isn't active. Bad.
- You're hardcoding that the local-APIC NMI watchdog is the
only possible sub-client of the local APIC. Not true.
- perfctr_pmdev exists precisely to handle both these cases
in a clean way.
2. You unconditionally register apic_driver with its suspend/resume
methods through a device_initcall().
This breaks if a UP_APIC or SMP kernel runs on a CPU with no or
an unusable local APIC. apic_pm_init2() does a runtime check
for successful init before doing a pm_register().
3. You severed the link between the PM API and the local APIC.
This breaks APM suspend when the local APIC is enabled. The
machine will hang (or immediately resume). I tested this, and
the driver model "stuff" simply doesn't do the right thing yet.
I you just want SOFTWARE_SUSPEND to work, why not simply post the
appropriate PM_SUSPEND and PM_RESUME events?
That should work without any changes to apic.c or nmi.c.
/Mikael
Hi!
> >This switches apic code to driver model, cleans code up a lot, and
> >makes S3 while apic is used work. Please apply,
>
> Please don't apply this. It breaks stuff:
>
> 1. apic_suspend() unconditionally calls disable_apic_nmi_watchdog()
> apic_resume() unconditionally calls setup_apic_nmi_watchdog()
> apic_pm_state.perfctr_pmdev removed
>
> - You're calling local-APIC NMI watchdog procedures even if
> the local-APIC NMI watchdog isn't active. Bad.
Fixed.
> - You're hardcoding that the local-APIC NMI watchdog is the
> only possible sub-client of the local APIC. Not true.
> - perfctr_pmdev exists precisely to handle both these cases
> in a clean way.
While being as ugly as night, which is even noted in sources:
- /* 'perfctr_pmdev' is here because the current (2.4.1) PM
- callback system doesn't handle hierarchical dependencies */
Nothing prevents more clients from registering as subtrees to APIC. I
did not do that for NMI watchdog because it is hardcoded in Makefile,
anyway.
> 2. You unconditionally register apic_driver with its suspend/resume
> methods through a device_initcall().
>
> This breaks if a UP_APIC or SMP kernel runs on a CPU with no or
> an unusable local APIC. apic_pm_init2() does a runtime check
> for successful init before doing a pm_register().
Fixed.
> 3. You severed the link between the PM API and the local APIC.
>
> This breaks APM suspend when the local APIC is enabled. The
> machine will hang (or immediately resume). I tested this, and
> the driver model "stuff" simply doesn't do the right thing yet.
I'll fix APM to call device model methods.
> I you just want SOFTWARE_SUSPEND to work, why not simply post the
> appropriate PM_SUSPEND and PM_RESUME events?
> That should work without any changes to apic.c or nmi.c.
Because PM_SUSPEND/PM_RESUME is ugly and can not be made to work
(devices are hierarchical, and PM_SUSPEND/PM_RESUME system does not
honour that).
Pavel
--
Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.
On Tue, 28 Jan 2003 10:26:09 +0100, Pavel Machek wrote:
>> - You're hardcoding that the local-APIC NMI watchdog is the
>> only possible sub-client of the local APIC. Not true.
>> - perfctr_pmdev exists precisely to handle both these cases
>> in a clean way.
>
>While being as ugly as night, which is even noted in sources:
>
>- /* 'perfctr_pmdev' is here because the current (2.4.1) PM
>- callback system doesn't handle hierarchical dependencies */
>
>Nothing prevents more clients from registering as subtrees to APIC. I
>did not do that for NMI watchdog because it is hardcoded in Makefile,
>anyway.
Not "more" clients, OTHER clients. They're exclusive.
The NMI watchdog simply happens to be the default client, but it
needs to unregister itself before any other client can take over
the performance counters and the local APIC's LVTPC entry.
(And that's what happens today.)
If the device model handles hierarchical dependencies correctly,
there should be no need to hardcode calls from the local APIC's
PM routines to whoever happens to be its current sub-client.
(And if it doesn't do this correctly, please fix the device
model first before migrating apic.c/nmi.c to it.)
>I'll fix APM to call device model methods.
Good.
>Because PM_SUSPEND/PM_RESUME is ugly and can not be made to work
>(devices are hierarchical, and PM_SUSPEND/PM_RESUME system does not
>honour that).
Agreed, but existing PM users do work. Most are leaves in the
dependency tree (e.g. sound cards). The only one I know of that
isn't is apic.c, and it has a local workaround as you noted.
Given that we're supposed to be in a feature freeze getting 2.5
into some kind of 2.6-worthy shape soonish, I think PM should
be hooked into the device model as a legacy API.
/Mikael
Hi!
> >> - You're hardcoding that the local-APIC NMI watchdog is the
> >> only possible sub-client of the local APIC. Not true.
> >> - perfctr_pmdev exists precisely to handle both these cases
> >> in a clean way.
> >
> >While being as ugly as night, which is even noted in sources:
> >
> >- /* 'perfctr_pmdev' is here because the current (2.4.1) PM
> >- callback system doesn't handle hierarchical dependencies */
> >
> >Nothing prevents more clients from registering as subtrees to APIC. I
> >did not do that for NMI watchdog because it is hardcoded in Makefile,
> >anyway.
>
> Not "more" clients, OTHER clients. They're exclusive.
> The NMI watchdog simply happens to be the default client, but it
> needs to unregister itself before any other client can take over
> the performance counters and the local APIC's LVTPC entry.
> (And that's what happens today.)
How can this be? I see nmi.c being unconditionaly compiled-in. Where
are the other clients you are talking about?
> If the device model handles hierarchical dependencies correctly,
> there should be no need to hardcode calls from the local APIC's
> PM routines to whoever happens to be its current sub-client.
>
> (And if it doesn't do this correctly, please fix the device
> model first before migrating apic.c/nmi.c to it.)
>
> >I'll fix APM to call device model methods.
>
> Good.
>
> >Because PM_SUSPEND/PM_RESUME is ugly and can not be made to work
> >(devices are hierarchical, and PM_SUSPEND/PM_RESUME system does not
> >honour that).
>
> Agreed, but existing PM users do work. Most are leaves in the
> dependency tree (e.g. sound cards). The only one I know of that
> isn't is apic.c, and it has a local workaround as you noted.
>
> Given that we're supposed to be in a feature freeze getting 2.5
> into some kind of 2.6-worthy shape soonish, I think PM should
> be hooked into the device model as a legacy API.
Well, PM will be left there for APM. I don't want to call it from
swsusp...
Do you think you could try this patch to see if it works for you? [It
should address everything except "modular nmi", where I am not
convinced it is needed.]
Pavel
--- clean/arch/i386/kernel/apic.c 2003-01-05 22:58:18.000000000 +0100
+++ linux-swsusp/arch/i386/kernel/apic.c 2003-01-28 10:28:05.000000000 +0100
@@ -23,6 +23,7 @@
#include <linux/interrupt.h>
#include <linux/mc146818rtc.h>
#include <linux/kernel_stat.h>
+#include <linux/delay.h>
#include <asm/atomic.h>
#include <asm/smp.h>
@@ -54,6 +55,8 @@
int prof_old_multiplier[NR_CPUS] = { 1, };
int prof_counter[NR_CPUS] = { 1, };
+unsigned long apic_phys;
+
int get_maxlvt(void)
{
unsigned int v, ver, maxlvt;
@@ -438,8 +441,8 @@
}
#ifdef CONFIG_PM
-
#include <linux/slab.h>
+#include <linux/device.h>
#include <linux/pm.h>
static struct {
@@ -447,9 +450,6 @@
not the BIOS; this signifies that we are also responsible
for disabling it before entering apm/acpi suspend */
int active;
- /* 'perfctr_pmdev' is here because the current (2.4.1) PM
- callback system doesn't handle hierarchical dependencies */
- struct pm_dev *perfctr_pmdev;
/* r/w apic fields */
unsigned int apic_id;
unsigned int apic_taskpri;
@@ -466,13 +466,15 @@
unsigned int apic_thmr;
} apic_pm_state;
-static void apic_pm_suspend(void *data)
+static int apic_suspend(struct device *dev, u32 state, u32 level)
{
unsigned int l, h;
unsigned long flags;
- if (apic_pm_state.perfctr_pmdev)
- pm_send(apic_pm_state.perfctr_pmdev, PM_SUSPEND, data);
+ if (level != SUSPEND_DISABLE)
+ return 0;
+
+ disable_apic_nmi_watchdog();
apic_pm_state.apic_id = apic_read(APIC_ID);
apic_pm_state.apic_taskpri = apic_read(APIC_TASKPRI);
apic_pm_state.apic_ldr = apic_read(APIC_LDR);
@@ -493,13 +495,19 @@
l &= ~MSR_IA32_APICBASE_ENABLE;
wrmsr(MSR_IA32_APICBASE, l, h);
local_irq_restore(flags);
+ return 0;
}
-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? */
+
local_irq_save(flags);
rdmsr(MSR_IA32_APICBASE, l, h);
l &= ~MSR_IA32_APICBASE_BASE;
@@ -524,74 +532,35 @@
apic_write(APIC_ESR, 0);
apic_read(APIC_ESR);
local_irq_restore(flags);
- if (apic_pm_state.perfctr_pmdev)
- pm_send(apic_pm_state.perfctr_pmdev, PM_RESUME, data);
-}
-
-static int apic_pm_callback(struct pm_dev *dev, pm_request_t rqst, void *data)
-{
- switch (rqst) {
- case PM_SUSPEND:
- apic_pm_suspend(data);
- break;
- case PM_RESUME:
- apic_pm_resume(data);
- break;
- }
+ setup_apic_nmi_watchdog();
return 0;
}
-/* perfctr driver should call this instead of pm_register() */
-struct pm_dev *apic_pm_register(pm_dev_t type,
- unsigned long id,
- pm_callback callback)
-{
- struct pm_dev *dev;
-
- if (!apic_pm_state.active)
- return pm_register(type, id, callback);
- if (apic_pm_state.perfctr_pmdev)
- return NULL; /* we're busy */
- dev = kmalloc(sizeof(struct pm_dev), GFP_KERNEL);
- if (dev) {
- memset(dev, 0, sizeof(*dev));
- dev->type = type;
- dev->id = id;
- dev->callback = callback;
- apic_pm_state.perfctr_pmdev = dev;
- }
- return dev;
-}
-
-/* perfctr driver should call this instead of pm_unregister() */
-void apic_pm_unregister(struct pm_dev *dev)
-{
- if (!apic_pm_state.active) {
- pm_unregister(dev);
- } else if (dev == apic_pm_state.perfctr_pmdev) {
- apic_pm_state.perfctr_pmdev = NULL;
- kfree(dev);
- }
-}
-
-static void __init apic_pm_init1(void)
-{
- /* can't pm_register() at this early stage in the boot process
- (causes an immediate reboot), so just set the flag */
- apic_pm_state.active = 1;
-}
+static struct device_driver apic_driver = {
+ .name = "apic",
+ .bus = &system_bus_type,
+ .resume = apic_resume,
+ .suspend = apic_suspend,
+};
+
+static struct sys_device device_apic = {
+ .name = "apic",
+ .id = 0,
+ .dev = {
+ .name = "APIC",
+ .driver = &apic_driver,
+ },
+};
-static void __init apic_pm_init2(void)
+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;
}
-#else /* CONFIG_PM */
-
-static inline void apic_pm_init1(void) { }
-static inline void apic_pm_init2(void) { }
-
+device_initcall(init_apic_devicefs);
#endif /* CONFIG_PM */
/*
@@ -658,9 +627,6 @@
nmi_watchdog = NMI_LOCAL_APIC;
printk("Found and enabled local APIC!\n");
-
- apic_pm_init1();
-
return 0;
no_apic:
@@ -670,8 +636,6 @@
void __init init_apic_mappings(void)
{
- unsigned long apic_phys;
-
/*
* If no local APIC can be found then set up a fake all
* zeroes page to simulate the local APIC and another
@@ -1141,8 +1105,6 @@
phys_cpu_present_map = 1;
apic_write_around(APIC_ID, boot_cpu_physical_apicid);
- apic_pm_init2();
-
setup_local_APIC();
if (nmi_watchdog == NMI_LOCAL_APIC)
--- clean/arch/i386/kernel/apm.c 2003-01-09 22:16:11.000000000 +0100
+++ linux-swsusp/arch/i386/kernel/apm.c 2003-01-28 10:35:51.000000000 +0100
@@ -1263,6 +1263,11 @@
}
printk(KERN_CRIT "apm: suspend was vetoed, but suspending anyway.\n");
}
+
+ device_suspend(3, SUSPEND_NOTIFY);
+ device_suspend(3, SUSPEND_SAVE_STATE);
+ device_suspend(3, SUSPEND_DISABLE);
+
/* serialize with the timer interrupt */
write_lock_irq(&xtime_lock);
@@ -1283,6 +1288,8 @@
if (err != APM_SUCCESS)
apm_error("suspend", err);
err = (err == APM_SUCCESS) ? 0 : -EIO;
+ device_resume(RESUME_RESTORE_STATE);
+ device_resume(RESUME_ENABLE);
pm_send_all(PM_RESUME, (void *)0);
queue_event(APM_NORMAL_RESUME, NULL);
out:
@@ -1396,6 +1403,8 @@
write_lock_irq(&xtime_lock);
set_time();
write_unlock_irq(&xtime_lock);
+ device_resume(RESUME_RESTORE_STATE);
+ device_resume(RESUME_ENABLE);
pm_send_all(PM_RESUME, (void *)0);
queue_event(event, NULL);
}
--- clean/arch/i386/kernel/i386_ksyms.c 2003-01-17 23:09:51.000000000 +0100
+++ linux-swsusp/arch/i386/kernel/i386_ksyms.c 2003-01-19 19:58:34.000000000 +0100
@@ -161,10 +161,6 @@
EXPORT_SYMBOL(flush_tlb_page);
#endif
-#if defined(CONFIG_X86_LOCAL_APIC) && defined(CONFIG_PM)
-EXPORT_SYMBOL_GPL(set_nmi_pm_callback);
-EXPORT_SYMBOL_GPL(unset_nmi_pm_callback);
-#endif
#ifdef CONFIG_X86_IO_APIC
EXPORT_SYMBOL(IO_APIC_get_PCI_irq_vector);
#endif
--- clean/arch/i386/kernel/nmi.c 2003-01-05 22:58:19.000000000 +0100
+++ linux-swsusp/arch/i386/kernel/nmi.c 2003-01-28 10:29:56.000000000 +0100
@@ -29,6 +29,7 @@
static unsigned int nmi_hz = HZ;
unsigned int nmi_perfctr_msr; /* the MSR to reset in NMI handler */
extern void show_registers(struct pt_regs *regs);
+static int nmi_active;
#define K7_EVNTSEL_ENABLE (1 << 22)
#define K7_EVNTSEL_INT (1 << 20)
@@ -138,12 +139,10 @@
#ifdef CONFIG_PM
-#include <linux/pm.h>
-
-struct pm_dev *nmi_pmdev;
-
-static void disable_apic_nmi_watchdog(void)
+void disable_apic_nmi_watchdog(void)
{
+ if (!nmi_active)
+ return;
switch (boot_cpu_data.x86_vendor) {
case X86_VENDOR_AMD:
wrmsr(MSR_K7_EVNTSEL0, 0, 0);
@@ -160,47 +159,10 @@
}
break;
}
+ nmi_active = 1;
}
-static int nmi_pm_callback(struct pm_dev *dev, pm_request_t rqst, void *data)
-{
- switch (rqst) {
- case PM_SUSPEND:
- disable_apic_nmi_watchdog();
- break;
- case PM_RESUME:
- setup_apic_nmi_watchdog();
- break;
- }
- return 0;
-}
-
-struct pm_dev * set_nmi_pm_callback(pm_callback callback)
-{
- apic_pm_unregister(nmi_pmdev);
- return apic_pm_register(PM_SYS_DEV, 0, callback);
-}
-
-void unset_nmi_pm_callback(struct pm_dev * dev)
-{
- apic_pm_unregister(dev);
- nmi_pmdev = apic_pm_register(PM_SYS_DEV, 0, nmi_pm_callback);
-}
-
-static void nmi_pm_init(void)
-{
- if (!nmi_pmdev)
- nmi_pmdev = apic_pm_register(PM_SYS_DEV, 0, nmi_pm_callback);
-}
-
-#define __pminit /*empty*/
-
-#else /* CONFIG_PM */
-
-static inline void nmi_pm_init(void) { }
-
-#define __pminit __init
-
+#define __pminit
#endif /* CONFIG_PM */
/*
@@ -314,7 +276,7 @@
default:
return;
}
- nmi_pm_init();
+ nmi_active = 1;
}
static spinlock_t nmi_print_lock = SPIN_LOCK_UNLOCKED;
--
Worst form of spam? Adding advertisment signatures ala sourceforge.net.
What goes next? Inserting advertisment *into* email?
On Wed, Jan 29, 2003 at 09:18:44PM +0100, Pavel Machek wrote:
> How can this be? I see nmi.c being unconditionaly compiled-in. Where
> are the other clients you are talking about?
Is grep broken ?
> -#if defined(CONFIG_X86_LOCAL_APIC) && defined(CONFIG_PM)
> -EXPORT_SYMBOL_GPL(set_nmi_pm_callback);
> -EXPORT_SYMBOL_GPL(unset_nmi_pm_callback);
> -#endif
You removed these but didn't check where they were used ?
regards
john
Hi!
> > How can this be? I see nmi.c being unconditionaly compiled-in. Where
> > are the other clients you are talking about?
>
> Is grep broken ?
>
> > -#if defined(CONFIG_X86_LOCAL_APIC) && defined(CONFIG_PM)
> > -EXPORT_SYMBOL_GPL(set_nmi_pm_callback);
> > -EXPORT_SYMBOL_GPL(unset_nmi_pm_callback);
> > -#endif
>
> You removed these but didn't check where they were used ?
Well, set_nmi... strongly suggests nmi, so I just killed them.. And
ouch its used by oprofile :-(.
Pavel
--
Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.
Hi!
> >This switches apic code to driver model, cleans code up a lot, and
> >makes S3 while apic is used work. Please apply,
>
> Please don't apply this. It breaks stuff:
This one should be okay. [oprofile not tested because I don't know how
to use it... If someone could test it that would be cool.]
Pavel
--- clean/arch/i386/kernel/apic.c 2003-01-05 22:58:18.000000000 +0100
+++ linux-swsusp/arch/i386/kernel/apic.c 2003-01-30 21:05:06.000000000 +0100
@@ -10,6 +10,7 @@
* for testing these extensively.
* Maciej W. Rozycki : Various updates and fixes.
* Mikael Pettersson : Power Management for UP-APIC.
+ * Pavel Machek : Converted to driver model.
*/
#include <linux/config.h>
@@ -23,6 +24,10 @@
#include <linux/interrupt.h>
#include <linux/mc146818rtc.h>
#include <linux/kernel_stat.h>
+#include <linux/delay.h>
+#include <linux/slab.h>
+#include <linux/device.h>
+#include <linux/pm.h>
#include <asm/atomic.h>
#include <asm/smp.h>
@@ -54,6 +59,8 @@
int prof_old_multiplier[NR_CPUS] = { 1, };
int prof_counter[NR_CPUS] = { 1, };
+unsigned long apic_phys;
+
int get_maxlvt(void)
{
unsigned int v, ver, maxlvt;
@@ -292,6 +299,27 @@
apic_write_around(APIC_LVT1, value);
}
+static struct {
+ /* 'active' is true if the local APIC was enabled by us and
+ not the BIOS; this signifies that we are also responsible
+ for disabling it before entering apm/acpi suspend */
+ int active;
+ /* r/w apic fields */
+ unsigned int apic_id;
+ unsigned int apic_taskpri;
+ unsigned int apic_ldr;
+ unsigned int apic_dfr;
+ unsigned int apic_spiv;
+ unsigned int apic_lvtt;
+ unsigned int apic_lvtpc;
+ unsigned int apic_lvt0;
+ unsigned int apic_lvt1;
+ unsigned int apic_lvterr;
+ unsigned int apic_tmict;
+ unsigned int apic_tdcr;
+ unsigned int apic_thmr;
+} apic_pm_state;
+
void __init setup_local_APIC (void)
{
unsigned long value, ver, maxlvt;
@@ -435,44 +463,19 @@
if (nmi_watchdog == NMI_LOCAL_APIC)
setup_apic_nmi_watchdog();
+
+ apic_pm_state.active = 1;
}
#ifdef CONFIG_PM
-
-#include <linux/slab.h>
-#include <linux/pm.h>
-
-static struct {
- /* 'active' is true if the local APIC was enabled by us and
- not the BIOS; this signifies that we are also responsible
- for disabling it before entering apm/acpi suspend */
- int active;
- /* 'perfctr_pmdev' is here because the current (2.4.1) PM
- callback system doesn't handle hierarchical dependencies */
- struct pm_dev *perfctr_pmdev;
- /* r/w apic fields */
- unsigned int apic_id;
- unsigned int apic_taskpri;
- unsigned int apic_ldr;
- unsigned int apic_dfr;
- unsigned int apic_spiv;
- unsigned int apic_lvtt;
- unsigned int apic_lvtpc;
- unsigned int apic_lvt0;
- unsigned int apic_lvt1;
- unsigned int apic_lvterr;
- unsigned int apic_tmict;
- unsigned int apic_tdcr;
- unsigned int apic_thmr;
-} apic_pm_state;
-
-static void apic_pm_suspend(void *data)
+static int apic_suspend(struct device *dev, u32 state, u32 level)
{
unsigned int l, h;
unsigned long flags;
- if (apic_pm_state.perfctr_pmdev)
- pm_send(apic_pm_state.perfctr_pmdev, PM_SUSPEND, data);
+ if (level != SUSPEND_DISABLE)
+ return 0;
+
apic_pm_state.apic_id = apic_read(APIC_ID);
apic_pm_state.apic_taskpri = apic_read(APIC_TASKPRI);
apic_pm_state.apic_ldr = apic_read(APIC_LDR);
@@ -493,13 +496,19 @@
l &= ~MSR_IA32_APICBASE_ENABLE;
wrmsr(MSR_IA32_APICBASE, l, h);
local_irq_restore(flags);
+ return 0;
}
-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? */
+
local_irq_save(flags);
rdmsr(MSR_IA32_APICBASE, l, h);
l &= ~MSR_IA32_APICBASE_BASE;
@@ -524,74 +533,34 @@
apic_write(APIC_ESR, 0);
apic_read(APIC_ESR);
local_irq_restore(flags);
- if (apic_pm_state.perfctr_pmdev)
- pm_send(apic_pm_state.perfctr_pmdev, PM_RESUME, data);
-}
-
-static int apic_pm_callback(struct pm_dev *dev, pm_request_t rqst, void *data)
-{
- switch (rqst) {
- case PM_SUSPEND:
- apic_pm_suspend(data);
- break;
- case PM_RESUME:
- apic_pm_resume(data);
- break;
- }
return 0;
}
-/* perfctr driver should call this instead of pm_register() */
-struct pm_dev *apic_pm_register(pm_dev_t type,
- unsigned long id,
- pm_callback callback)
-{
- struct pm_dev *dev;
-
- if (!apic_pm_state.active)
- return pm_register(type, id, callback);
- if (apic_pm_state.perfctr_pmdev)
- return NULL; /* we're busy */
- dev = kmalloc(sizeof(struct pm_dev), GFP_KERNEL);
- if (dev) {
- memset(dev, 0, sizeof(*dev));
- dev->type = type;
- dev->id = id;
- dev->callback = callback;
- apic_pm_state.perfctr_pmdev = dev;
- }
- return dev;
-}
-
-/* perfctr driver should call this instead of pm_unregister() */
-void apic_pm_unregister(struct pm_dev *dev)
-{
- if (!apic_pm_state.active) {
- pm_unregister(dev);
- } else if (dev == apic_pm_state.perfctr_pmdev) {
- apic_pm_state.perfctr_pmdev = NULL;
- kfree(dev);
- }
-}
+static struct device_driver apic_driver = {
+ .name = "apic",
+ .bus = &system_bus_type,
+ .resume = apic_resume,
+ .suspend = apic_suspend,
+};
+
+struct sys_device device_apic = {
+ .name = "apic",
+ .id = 0,
+ .dev = {
+ .name = "APIC",
+ .driver = &apic_driver,
+ },
+};
-static void __init apic_pm_init1(void)
-{
- /* can't pm_register() at this early stage in the boot process
- (causes an immediate reboot), so just set the flag */
- apic_pm_state.active = 1;
-}
-
-static void __init apic_pm_init2(void)
+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;
}
-#else /* CONFIG_PM */
-
-static inline void apic_pm_init1(void) { }
-static inline void apic_pm_init2(void) { }
-
+device_initcall(init_apic_devicefs);
#endif /* CONFIG_PM */
/*
@@ -658,9 +627,6 @@
nmi_watchdog = NMI_LOCAL_APIC;
printk("Found and enabled local APIC!\n");
-
- apic_pm_init1();
-
return 0;
no_apic:
@@ -670,8 +636,6 @@
void __init init_apic_mappings(void)
{
- unsigned long apic_phys;
-
/*
* If no local APIC can be found then set up a fake all
* zeroes page to simulate the local APIC and another
@@ -1141,8 +1105,6 @@
phys_cpu_present_map = 1;
apic_write_around(APIC_ID, boot_cpu_physical_apicid);
- apic_pm_init2();
-
setup_local_APIC();
if (nmi_watchdog == NMI_LOCAL_APIC)
--- clean/arch/i386/kernel/apm.c 2003-01-09 22:16:11.000000000 +0100
+++ linux-swsusp/arch/i386/kernel/apm.c 2003-01-28 10:35:51.000000000 +0100
@@ -1263,6 +1263,11 @@
}
printk(KERN_CRIT "apm: suspend was vetoed, but suspending anyway.\n");
}
+
+ device_suspend(3, SUSPEND_NOTIFY);
+ device_suspend(3, SUSPEND_SAVE_STATE);
+ device_suspend(3, SUSPEND_DISABLE);
+
/* serialize with the timer interrupt */
write_lock_irq(&xtime_lock);
@@ -1283,6 +1288,8 @@
if (err != APM_SUCCESS)
apm_error("suspend", err);
err = (err == APM_SUCCESS) ? 0 : -EIO;
+ device_resume(RESUME_RESTORE_STATE);
+ device_resume(RESUME_ENABLE);
pm_send_all(PM_RESUME, (void *)0);
queue_event(APM_NORMAL_RESUME, NULL);
out:
@@ -1396,6 +1403,8 @@
write_lock_irq(&xtime_lock);
set_time();
write_unlock_irq(&xtime_lock);
+ device_resume(RESUME_RESTORE_STATE);
+ device_resume(RESUME_ENABLE);
pm_send_all(PM_RESUME, (void *)0);
queue_event(event, NULL);
}
--- clean/arch/i386/kernel/i386_ksyms.c 2003-01-17 23:09:51.000000000 +0100
+++ linux-swsusp/arch/i386/kernel/i386_ksyms.c 2003-01-19 19:58:34.000000000 +0100
@@ -161,10 +161,6 @@
EXPORT_SYMBOL(flush_tlb_page);
#endif
-#if defined(CONFIG_X86_LOCAL_APIC) && defined(CONFIG_PM)
-EXPORT_SYMBOL_GPL(set_nmi_pm_callback);
-EXPORT_SYMBOL_GPL(unset_nmi_pm_callback);
-#endif
#ifdef CONFIG_X86_IO_APIC
EXPORT_SYMBOL(IO_APIC_get_PCI_irq_vector);
#endif
--- clean/arch/i386/kernel/nmi.c 2003-01-05 22:58:19.000000000 +0100
+++ linux-swsusp/arch/i386/kernel/nmi.c 2003-01-31 00:21:02.000000000 +0100
@@ -9,6 +9,7 @@
* Mikael Pettersson : AMD K7 support for local APIC NMI watchdog.
* Mikael Pettersson : Power Management for local APIC NMI watchdog.
* Mikael Pettersson : Pentium 4 support for local APIC NMI watchdog.
+ * Pavel Machek : Driver model here, too.
*/
#include <linux/config.h>
@@ -20,6 +21,7 @@
#include <linux/interrupt.h>
#include <linux/mc146818rtc.h>
#include <linux/kernel_stat.h>
+#include <linux/device.h>
#include <asm/smp.h>
#include <asm/mtrr.h>
@@ -29,6 +31,7 @@
static unsigned int nmi_hz = HZ;
unsigned int nmi_perfctr_msr; /* the MSR to reset in NMI handler */
extern void show_registers(struct pt_regs *regs);
+static int nmi_active;
#define K7_EVNTSEL_ENABLE (1 << 22)
#define K7_EVNTSEL_INT (1 << 20)
@@ -138,12 +141,10 @@
#ifdef CONFIG_PM
-#include <linux/pm.h>
-
-struct pm_dev *nmi_pmdev;
-
-static void disable_apic_nmi_watchdog(void)
+void disable_apic_nmi_watchdog(void)
{
+ if (!nmi_active)
+ return;
switch (boot_cpu_data.x86_vendor) {
case X86_VENDOR_AMD:
wrmsr(MSR_K7_EVNTSEL0, 0, 0);
@@ -160,47 +161,54 @@
}
break;
}
+ nmi_active = 1;
}
-static int nmi_pm_callback(struct pm_dev *dev, pm_request_t rqst, void *data)
+static int nmi_suspend(struct device *dev, u32 state, u32 level)
{
- switch (rqst) {
- case PM_SUSPEND:
- disable_apic_nmi_watchdog();
- break;
- case PM_RESUME:
- setup_apic_nmi_watchdog();
- break;
- }
+ if (level != SUSPEND_DISABLE)
+ return 0;
+
+ disable_apic_nmi_watchdog();
return 0;
}
-struct pm_dev * set_nmi_pm_callback(pm_callback callback)
+static int nmi_resume(struct device *dev, u32 level)
{
- apic_pm_unregister(nmi_pmdev);
- return apic_pm_register(PM_SYS_DEV, 0, callback);
-}
+ if (level != RESUME_POWER_ON)
+ return 0;
-void unset_nmi_pm_callback(struct pm_dev * dev)
-{
- apic_pm_unregister(dev);
- nmi_pmdev = apic_pm_register(PM_SYS_DEV, 0, nmi_pm_callback);
-}
-
-static void nmi_pm_init(void)
-{
- if (!nmi_pmdev)
- nmi_pmdev = apic_pm_register(PM_SYS_DEV, 0, nmi_pm_callback);
+ setup_apic_nmi_watchdog();
+ return 0;
}
-#define __pminit /*empty*/
-#else /* CONFIG_PM */
+static struct device_driver nmi_driver = {
+ .name = "nmi",
+ .bus = &system_bus_type,
+ .resume = nmi_resume,
+ .suspend = nmi_suspend,
+};
+
+static struct device device_nmi = {
+ .name = "NMI",
+ .bus_id = "NMI",
+ .driver = &nmi_driver,
+};
-static inline void nmi_pm_init(void) { }
+extern struct sys_device device_apic;
+
+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);
+#define __pminit
#endif /* CONFIG_PM */
/*
@@ -314,7 +322,7 @@
default:
return;
}
- nmi_pm_init();
+ nmi_active = 1;
}
static spinlock_t nmi_print_lock = SPIN_LOCK_UNLOCKED;
--- clean/arch/i386/oprofile/nmi_int.c 2003-01-05 22:58:19.000000000 +0100
+++ linux-swsusp/arch/i386/oprofile/nmi_int.c 2003-01-31 00:30:27.000000000 +0100
@@ -11,6 +11,7 @@
#include <linux/notifier.h>
#include <linux/smp.h>
#include <linux/oprofile.h>
+#include <linux/device.h>
#include <asm/nmi.h>
#include <asm/msr.h>
#include <asm/apic.h>
@@ -24,27 +25,6 @@
static int nmi_start(void);
static void nmi_stop(void);
-
-static struct pm_dev * oprofile_pmdev;
-
-/* We're at risk of causing big trouble unless we
- * make sure to not cause any NMI interrupts when
- * suspended.
- */
-static int oprofile_pm_callback(struct pm_dev * dev,
- pm_request_t rqst, void * data)
-{
- switch (rqst) {
- case PM_SUSPEND:
- nmi_stop();
- break;
- case PM_RESUME:
- nmi_start();
- break;
- }
- return 0;
-}
-
static int nmi_callback(struct pt_regs * regs, int cpu)
{
@@ -86,7 +66,42 @@
saved_lvtpc[cpu] = apic_read(APIC_LVTPC);
apic_write(APIC_LVTPC, APIC_DM_NMI);
}
-
+
+static int nmi_enabled = -1; /* -1 == Not yet registered, 0 == registered but off, 1 == registered and on */
+
+static int nmi_suspend(struct device *dev, u32 state, u32 level)
+{
+ if (level != SUSPEND_DISABLE)
+ return 0;
+ if (nmi_enabled == 1)
+ nmi_stop();
+ return 0;
+}
+
+static int nmi_resume(struct device *dev, u32 level)
+{
+ if (level != RESUME_POWER_ON)
+ return 0;
+ if (nmi_enabled == 1)
+ nmi_start();
+ return 0;
+}
+
+
+static struct device_driver nmi_driver = {
+ .name = "oprofile",
+ .bus = &system_bus_type,
+ .resume = nmi_resume,
+ .suspend = nmi_suspend,
+};
+
+static struct device device_nmi = {
+ .name = "oprofile",
+ .bus_id = "oprofile",
+ .driver = &nmi_driver,
+};
+
+extern struct sys_device device_apic;
static int nmi_setup(void)
{
@@ -98,8 +113,13 @@
smp_call_function(nmi_cpu_setup, NULL, 0, 1);
nmi_cpu_setup(0);
set_nmi_callback(nmi_callback);
- oprofile_pmdev = set_nmi_pm_callback(oprofile_pm_callback);
- return 0;
+
+ if (nmi_enabled == -1) {
+ driver_register(&nmi_driver);
+ device_nmi.parent = &device_apic.dev;
+ }
+ nmi_enabled = 1;
+ return device_register(&device_nmi);
}
@@ -146,7 +166,7 @@
static void nmi_shutdown(void)
{
- unset_nmi_pm_callback(oprofile_pmdev);
+ nmi_enabled = 0;
unset_nmi_callback();
smp_call_function(nmi_cpu_shutdown, NULL, 0, 1);
nmi_cpu_shutdown(0);
@@ -217,9 +237,9 @@
int __init nmi_init(struct oprofile_operations ** ops, enum oprofile_cpu * cpu)
{
- __u8 vendor = current_cpu_data.x86_vendor;
- __u8 family = current_cpu_data.x86;
- __u8 cpu_model = current_cpu_data.x86_model;
+ u8 vendor = current_cpu_data.x86_vendor;
+ u8 family = current_cpu_data.x86;
+ u8 cpu_model = current_cpu_data.x86_model;
if (!cpu_has_apic)
return 0;
--
Worst form of spam? Adding advertisment signatures ala sourceforge.net.
What goes next? Inserting advertisment *into* email?
On Sun, Feb 02, 2003 at 01:42:35PM +0100, Pavel Machek wrote:
> This one should be okay. [oprofile not tested because I don't know how
> to use it...
It's not hard you know[1].
> -struct pm_dev * set_nmi_pm_callback(pm_callback callback)
> +static int nmi_resume(struct device *dev, u32 level)
> {
> - apic_pm_unregister(nmi_pmdev);
> - return apic_pm_register(PM_SYS_DEV, 0, callback);
> -}
> + if (level != RESUME_POWER_ON)
> + return 0;
> + setup_apic_nmi_watchdog();
> + return 0;
I don't pretend to understand the PM layer at all, but it looks like
that both nmi.c's and oprofile's resume functions will get called. This
won't work: if oprofile has control of the perfctr's/nmi stuff, you
can't let the NMI watchdog's resume() be called, as it may conflict with
what oprofile is trying to resume.
> + return device_register(&device_nmi);
There's some evil spaces in your patch.
regards
john
[1] opcontrol --setup --vmlinux=/boot/vmlinux --ctr0-count=20000
--ctr0-event=CPU_CLK_UNHALTED && opcontrol --start
On Sun, 2 Feb 2003, Pavel Machek wrote:
> + set_fixmap_nocache(FIX_APIC_BASE, apic_phys); /* FIXME: this is needed for S3 resume, but why? */
Intel recommends a strong uncacheable mapping otherwise it may exhibit
undeterministic behaviour. Perhaps that could be it?
Zwane
Hi!
> > This one should be okay. [oprofile not tested because I don't know how
> > to use it...
>
> It's not hard you know[1].
should apt-get install oprofile then opcontrol --setup
--vmlinux=/boot/vmlinux --ctr0-count=20000
--ctr0-event=CPU_CLK_UNHALTED && opcontrol --start
do the trick? If that works does it mean oprofile is okay?
> > -struct pm_dev * set_nmi_pm_callback(pm_callback callback)
> > +static int nmi_resume(struct device *dev, u32 level)
> > {
> > - apic_pm_unregister(nmi_pmdev);
> > - return apic_pm_register(PM_SYS_DEV, 0, callback);
> > -}
> > + if (level != RESUME_POWER_ON)
> > + return 0;
> > + setup_apic_nmi_watchdog();
> > + return 0;
>
> I don't pretend to understand the PM layer at all, but it looks like
> that both nmi.c's and oprofile's resume functions will get called. This
> won't work: if oprofile has control of the perfctr's/nmi stuff, you
> can't let the NMI watchdog's resume() be called, as it may conflict with
> what oprofile is trying to resume.
oprofile() should already have checks to prevent that, and I added one
[ if (nmi_watchdog == NMI_LOCAL_APIC)
]
to nmi.c. I hope that's okay.
Pavel
--
Worst form of spam? Adding advertisment signatures ala sourceforge.net.
What goes next? Inserting advertisment *into* email?
On Mon, Feb 03, 2003 at 04:40:08PM +0100, Pavel Machek wrote:
> should apt-get install
not packaged yet I'm afraid. Get
http://movementarian.org/oprofile-0.5.tar.gz, ./configure \
--with-kernel-support && make && make install
> --ctr0-event=CPU_CLK_UNHALTED && opcontrol --start
If it's an Intel CPU (not pentium 4) this should be OK
For Athlon, use INST_RETIRED
> do the trick? If that works does it mean oprofile is okay?
Run a few kernel compiles and stress it a bit. Run "op_time" to check
you get output...
> > I don't pretend to understand the PM layer at all, but it looks like
> > that both nmi.c's and oprofile's resume functions will get called. This
> > won't work: if oprofile has control of the perfctr's/nmi stuff, you
> > can't let the NMI watchdog's resume() be called, as it may conflict with
> > what oprofile is trying to resume.
>
> oprofile() should already have checks to prevent that, and I added one
I haven't seen where you added these checks. They did not exist before:
oprofile took responsibility for the NMI/perfctr handling off the NMI
watchdog entirely, then handed it back when oprofile finished.
Basically you have to ensure that only the right pm callback is called,
not both.
> [ if (nmi_watchdog == NMI_LOCAL_APIC)
I don't see an added line like this ?
regards
john
Hi!
> > should apt-get install
>
> not packaged yet I'm afraid. Get
> http://movementarian.org/oprofile-0.5.tar.gz, ./configure \
> --with-kernel-support && make && make install
>
> > --ctr0-event=CPU_CLK_UNHALTED && opcontrol --start
>
> If it's an Intel CPU (not pentium 4) this should be OK
> For Athlon, use INST_RETIRED
Unfortunately my systems seems to miss bfd.h, required by libutil++.
> > > I don't pretend to understand the PM layer at all, but it looks like
> > > that both nmi.c's and oprofile's resume functions will get called. This
> > > won't work: if oprofile has control of the perfctr's/nmi stuff, you
> > > can't let the NMI watchdog's resume() be called, as it may conflict with
> > > what oprofile is trying to resume.
> >
> > oprofile() should already have checks to prevent that, and I added one
>
> I haven't seen where you added these checks. They did not exist before:
> oprofile took responsibility for the NMI/perfctr handling off the NMI
> watchdog entirely, then handed it back when oprofile finished.
Ouch, I did not see that code. It seems not to be in nmi_int.c; where
is that?
> Basically you have to ensure that only the right pm callback is called,
> not both.
>
> > [ if (nmi_watchdog == NMI_LOCAL_APIC)
>
> I don't see an added line like this ?
I added it to my local tree only (sorry). (Updated patch is attached).
Pavel
--- 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
@@ -10,6 +10,7 @@
* for testing these extensively.
* Maciej W. Rozycki : Various updates and fixes.
* Mikael Pettersson : Power Management for UP-APIC.
+ * Pavel Machek : Converted to driver model.
*/
#include <linux/config.h>
@@ -23,6 +24,10 @@
#include <linux/interrupt.h>
#include <linux/mc146818rtc.h>
#include <linux/kernel_stat.h>
+#include <linux/delay.h>
+#include <linux/slab.h>
+#include <linux/device.h>
+#include <linux/pm.h>
#include <asm/atomic.h>
#include <asm/smp.h>
@@ -54,6 +59,8 @@
int prof_old_multiplier[NR_CPUS] = { 1, };
int prof_counter[NR_CPUS] = { 1, };
+unsigned long apic_phys;
+
int get_maxlvt(void)
{
unsigned int v, ver, maxlvt;
@@ -292,6 +299,27 @@
apic_write_around(APIC_LVT1, value);
}
+static struct {
+ /* 'active' is true if the local APIC was enabled by us and
+ not the BIOS; this signifies that we are also responsible
+ for disabling it before entering apm/acpi suspend */
+ int active;
+ /* r/w apic fields */
+ unsigned int apic_id;
+ unsigned int apic_taskpri;
+ unsigned int apic_ldr;
+ unsigned int apic_dfr;
+ unsigned int apic_spiv;
+ unsigned int apic_lvtt;
+ unsigned int apic_lvtpc;
+ unsigned int apic_lvt0;
+ unsigned int apic_lvt1;
+ unsigned int apic_lvterr;
+ unsigned int apic_tmict;
+ unsigned int apic_tdcr;
+ unsigned int apic_thmr;
+} apic_pm_state;
+
void __init setup_local_APIC (void)
{
unsigned long value, ver, maxlvt;
@@ -435,44 +463,19 @@
if (nmi_watchdog == NMI_LOCAL_APIC)
setup_apic_nmi_watchdog();
+
+ apic_pm_state.active = 1;
}
#ifdef CONFIG_PM
-
-#include <linux/slab.h>
-#include <linux/pm.h>
-
-static struct {
- /* 'active' is true if the local APIC was enabled by us and
- not the BIOS; this signifies that we are also responsible
- for disabling it before entering apm/acpi suspend */
- int active;
- /* 'perfctr_pmdev' is here because the current (2.4.1) PM
- callback system doesn't handle hierarchical dependencies */
- struct pm_dev *perfctr_pmdev;
- /* r/w apic fields */
- unsigned int apic_id;
- unsigned int apic_taskpri;
- unsigned int apic_ldr;
- unsigned int apic_dfr;
- unsigned int apic_spiv;
- unsigned int apic_lvtt;
- unsigned int apic_lvtpc;
- unsigned int apic_lvt0;
- unsigned int apic_lvt1;
- unsigned int apic_lvterr;
- unsigned int apic_tmict;
- unsigned int apic_tdcr;
- unsigned int apic_thmr;
-} apic_pm_state;
-
-static void apic_pm_suspend(void *data)
+static int apic_suspend(struct device *dev, u32 state, u32 level)
{
unsigned int l, h;
unsigned long flags;
- if (apic_pm_state.perfctr_pmdev)
- pm_send(apic_pm_state.perfctr_pmdev, PM_SUSPEND, data);
+ if (level != SUSPEND_DISABLE)
+ return 0;
+
apic_pm_state.apic_id = apic_read(APIC_ID);
apic_pm_state.apic_taskpri = apic_read(APIC_TASKPRI);
apic_pm_state.apic_ldr = apic_read(APIC_LDR);
@@ -493,13 +496,19 @@
l &= ~MSR_IA32_APICBASE_ENABLE;
wrmsr(MSR_IA32_APICBASE, l, h);
local_irq_restore(flags);
+ return 0;
}
-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? */
+
local_irq_save(flags);
rdmsr(MSR_IA32_APICBASE, l, h);
l &= ~MSR_IA32_APICBASE_BASE;
@@ -524,74 +533,34 @@
apic_write(APIC_ESR, 0);
apic_read(APIC_ESR);
local_irq_restore(flags);
- if (apic_pm_state.perfctr_pmdev)
- pm_send(apic_pm_state.perfctr_pmdev, PM_RESUME, data);
-}
-
-static int apic_pm_callback(struct pm_dev *dev, pm_request_t rqst, void *data)
-{
- switch (rqst) {
- case PM_SUSPEND:
- apic_pm_suspend(data);
- break;
- case PM_RESUME:
- apic_pm_resume(data);
- break;
- }
return 0;
}
-/* perfctr driver should call this instead of pm_register() */
-struct pm_dev *apic_pm_register(pm_dev_t type,
- unsigned long id,
- pm_callback callback)
-{
- struct pm_dev *dev;
-
- if (!apic_pm_state.active)
- return pm_register(type, id, callback);
- if (apic_pm_state.perfctr_pmdev)
- return NULL; /* we're busy */
- dev = kmalloc(sizeof(struct pm_dev), GFP_KERNEL);
- if (dev) {
- memset(dev, 0, sizeof(*dev));
- dev->type = type;
- dev->id = id;
- dev->callback = callback;
- apic_pm_state.perfctr_pmdev = dev;
- }
- return dev;
-}
-
-/* perfctr driver should call this instead of pm_unregister() */
-void apic_pm_unregister(struct pm_dev *dev)
-{
- if (!apic_pm_state.active) {
- pm_unregister(dev);
- } else if (dev == apic_pm_state.perfctr_pmdev) {
- apic_pm_state.perfctr_pmdev = NULL;
- kfree(dev);
- }
-}
+static struct device_driver apic_driver = {
+ .name = "apic",
+ .bus = &system_bus_type,
+ .resume = apic_resume,
+ .suspend = apic_suspend,
+};
+
+struct sys_device device_apic = {
+ .name = "apic",
+ .id = 0,
+ .dev = {
+ .name = "APIC",
+ .driver = &apic_driver,
+ },
+};
-static void __init apic_pm_init1(void)
-{
- /* can't pm_register() at this early stage in the boot process
- (causes an immediate reboot), so just set the flag */
- apic_pm_state.active = 1;
-}
-
-static void __init apic_pm_init2(void)
+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;
}
-#else /* CONFIG_PM */
-
-static inline void apic_pm_init1(void) { }
-static inline void apic_pm_init2(void) { }
-
+device_initcall(init_apic_devicefs);
#endif /* CONFIG_PM */
/*
@@ -658,9 +627,6 @@
nmi_watchdog = NMI_LOCAL_APIC;
printk("Found and enabled local APIC!\n");
-
- apic_pm_init1();
-
return 0;
no_apic:
@@ -670,8 +636,6 @@
void __init init_apic_mappings(void)
{
- unsigned long apic_phys;
-
/*
* If no local APIC can be found then set up a fake all
* zeroes page to simulate the local APIC and another
@@ -1141,8 +1105,6 @@
phys_cpu_present_map = 1;
apic_write_around(APIC_ID, boot_cpu_physical_apicid);
- apic_pm_init2();
-
setup_local_APIC();
if (nmi_watchdog == NMI_LOCAL_APIC)
--- clean/arch/i386/kernel/apm.c 2003-01-09 22:16:11.000000000 +0100
+++ linux-swsusp/arch/i386/kernel/apm.c 2003-01-28 10:35:51.000000000 +0100
@@ -1263,6 +1263,11 @@
}
printk(KERN_CRIT "apm: suspend was vetoed, but suspending anyway.\n");
}
+
+ device_suspend(3, SUSPEND_NOTIFY);
+ device_suspend(3, SUSPEND_SAVE_STATE);
+ device_suspend(3, SUSPEND_DISABLE);
+
/* serialize with the timer interrupt */
write_lock_irq(&xtime_lock);
@@ -1283,6 +1288,8 @@
if (err != APM_SUCCESS)
apm_error("suspend", err);
err = (err == APM_SUCCESS) ? 0 : -EIO;
+ device_resume(RESUME_RESTORE_STATE);
+ device_resume(RESUME_ENABLE);
pm_send_all(PM_RESUME, (void *)0);
queue_event(APM_NORMAL_RESUME, NULL);
out:
@@ -1396,6 +1403,8 @@
write_lock_irq(&xtime_lock);
set_time();
write_unlock_irq(&xtime_lock);
+ device_resume(RESUME_RESTORE_STATE);
+ device_resume(RESUME_ENABLE);
pm_send_all(PM_RESUME, (void *)0);
queue_event(event, NULL);
}
--- clean/arch/i386/kernel/i386_ksyms.c 2003-01-17 23:09:51.000000000 +0100
+++ linux-swsusp/arch/i386/kernel/i386_ksyms.c 2003-01-19 19:58:34.000000000 +0100
@@ -161,10 +161,6 @@
EXPORT_SYMBOL(flush_tlb_page);
#endif
-#if defined(CONFIG_X86_LOCAL_APIC) && defined(CONFIG_PM)
-EXPORT_SYMBOL_GPL(set_nmi_pm_callback);
-EXPORT_SYMBOL_GPL(unset_nmi_pm_callback);
-#endif
#ifdef CONFIG_X86_IO_APIC
EXPORT_SYMBOL(IO_APIC_get_PCI_irq_vector);
#endif
--- clean/arch/i386/kernel/nmi.c 2003-01-05 22:58:19.000000000 +0100
+++ linux-swsusp/arch/i386/kernel/nmi.c 2003-02-03 16:36:51.000000000 +0100
@@ -9,6 +9,7 @@
* Mikael Pettersson : AMD K7 support for local APIC NMI watchdog.
* Mikael Pettersson : Power Management for local APIC NMI watchdog.
* Mikael Pettersson : Pentium 4 support for local APIC NMI watchdog.
+ * Pavel Machek : Driver model here, too.
*/
#include <linux/config.h>
@@ -20,6 +21,7 @@
#include <linux/interrupt.h>
#include <linux/mc146818rtc.h>
#include <linux/kernel_stat.h>
+#include <linux/device.h>
#include <asm/smp.h>
#include <asm/mtrr.h>
@@ -29,6 +31,7 @@
static unsigned int nmi_hz = HZ;
unsigned int nmi_perfctr_msr; /* the MSR to reset in NMI handler */
extern void show_registers(struct pt_regs *regs);
+static int nmi_active;
#define K7_EVNTSEL_ENABLE (1 << 22)
#define K7_EVNTSEL_INT (1 << 20)
@@ -138,12 +141,10 @@
#ifdef CONFIG_PM
-#include <linux/pm.h>
-
-struct pm_dev *nmi_pmdev;
-
-static void disable_apic_nmi_watchdog(void)
+void disable_apic_nmi_watchdog(void)
{
+ if (!nmi_active)
+ return;
switch (boot_cpu_data.x86_vendor) {
case X86_VENDOR_AMD:
wrmsr(MSR_K7_EVNTSEL0, 0, 0);
@@ -160,47 +161,56 @@
}
break;
}
+ nmi_active = 1;
}
-static int nmi_pm_callback(struct pm_dev *dev, pm_request_t rqst, void *data)
+static int nmi_suspend(struct device *dev, u32 state, u32 level)
{
- switch (rqst) {
- case PM_SUSPEND:
- disable_apic_nmi_watchdog();
- break;
- case PM_RESUME:
- setup_apic_nmi_watchdog();
- break;
- }
+ if (level != SUSPEND_DISABLE)
+ return 0;
+
+ disable_apic_nmi_watchdog();
return 0;
}
-struct pm_dev * set_nmi_pm_callback(pm_callback callback)
+static int nmi_resume(struct device *dev, u32 level)
{
- apic_pm_unregister(nmi_pmdev);
- return apic_pm_register(PM_SYS_DEV, 0, callback);
-}
+ if (level != RESUME_POWER_ON)
+ return 0;
-void unset_nmi_pm_callback(struct pm_dev * dev)
-{
- apic_pm_unregister(dev);
- nmi_pmdev = apic_pm_register(PM_SYS_DEV, 0, nmi_pm_callback);
-}
-
-static void nmi_pm_init(void)
-{
- if (!nmi_pmdev)
- nmi_pmdev = apic_pm_register(PM_SYS_DEV, 0, nmi_pm_callback);
+ if (nmi_watchdog == NMI_LOCAL_APIC)
+ setup_apic_nmi_watchdog();
+
+ return 0;
}
-#define __pminit /*empty*/
-#else /* CONFIG_PM */
+static struct device_driver nmi_driver = {
+ .name = "nmi",
+ .bus = &system_bus_type,
+ .resume = nmi_resume,
+ .suspend = nmi_suspend,
+};
-static inline void nmi_pm_init(void) { }
+static struct device device_nmi = {
+ .name = "NMI",
+ .bus_id = "NMI",
+ .driver = &nmi_driver,
+};
+
+extern struct sys_device device_apic;
+
+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);
+#define __pminit
#endif /* CONFIG_PM */
/*
@@ -314,7 +324,7 @@
default:
return;
}
- nmi_pm_init();
+ nmi_active = 1;
}
static spinlock_t nmi_print_lock = SPIN_LOCK_UNLOCKED;
--- clean/arch/i386/oprofile/nmi_int.c 2003-01-05 22:58:19.000000000 +0100
+++ linux-swsusp/arch/i386/oprofile/nmi_int.c 2003-01-31 00:30:27.000000000 +0100
@@ -11,6 +11,7 @@
#include <linux/notifier.h>
#include <linux/smp.h>
#include <linux/oprofile.h>
+#include <linux/device.h>
#include <asm/nmi.h>
#include <asm/msr.h>
#include <asm/apic.h>
@@ -24,27 +25,6 @@
static int nmi_start(void);
static void nmi_stop(void);
-
-static struct pm_dev * oprofile_pmdev;
-
-/* We're at risk of causing big trouble unless we
- * make sure to not cause any NMI interrupts when
- * suspended.
- */
-static int oprofile_pm_callback(struct pm_dev * dev,
- pm_request_t rqst, void * data)
-{
- switch (rqst) {
- case PM_SUSPEND:
- nmi_stop();
- break;
- case PM_RESUME:
- nmi_start();
- break;
- }
- return 0;
-}
-
static int nmi_callback(struct pt_regs * regs, int cpu)
{
@@ -86,7 +66,42 @@
saved_lvtpc[cpu] = apic_read(APIC_LVTPC);
apic_write(APIC_LVTPC, APIC_DM_NMI);
}
-
+
+static int nmi_enabled = -1; /* -1 == Not yet registered, 0 == registered but off, 1 == registered and on */
+
+static int nmi_suspend(struct device *dev, u32 state, u32 level)
+{
+ if (level != SUSPEND_DISABLE)
+ return 0;
+ if (nmi_enabled == 1)
+ nmi_stop();
+ return 0;
+}
+
+static int nmi_resume(struct device *dev, u32 level)
+{
+ if (level != RESUME_POWER_ON)
+ return 0;
+ if (nmi_enabled == 1)
+ nmi_start();
+ return 0;
+}
+
+
+static struct device_driver nmi_driver = {
+ .name = "oprofile",
+ .bus = &system_bus_type,
+ .resume = nmi_resume,
+ .suspend = nmi_suspend,
+};
+
+static struct device device_nmi = {
+ .name = "oprofile",
+ .bus_id = "oprofile",
+ .driver = &nmi_driver,
+};
+
+extern struct sys_device device_apic;
static int nmi_setup(void)
{
@@ -98,8 +113,13 @@
smp_call_function(nmi_cpu_setup, NULL, 0, 1);
nmi_cpu_setup(0);
set_nmi_callback(nmi_callback);
- oprofile_pmdev = set_nmi_pm_callback(oprofile_pm_callback);
- return 0;
+
+ if (nmi_enabled == -1) {
+ driver_register(&nmi_driver);
+ device_nmi.parent = &device_apic.dev;
+ }
+ nmi_enabled = 1;
+ return device_register(&device_nmi);
}
@@ -146,7 +166,7 @@
static void nmi_shutdown(void)
{
- unset_nmi_pm_callback(oprofile_pmdev);
+ nmi_enabled = 0;
unset_nmi_callback();
smp_call_function(nmi_cpu_shutdown, NULL, 0, 1);
nmi_cpu_shutdown(0);
@@ -217,9 +237,9 @@
int __init nmi_init(struct oprofile_operations ** ops, enum oprofile_cpu * cpu)
{
- __u8 vendor = current_cpu_data.x86_vendor;
- __u8 family = current_cpu_data.x86;
- __u8 cpu_model = current_cpu_data.x86_model;
+ u8 vendor = current_cpu_data.x86_vendor;
+ u8 family = current_cpu_data.x86;
+ u8 cpu_model = current_cpu_data.x86_model;
if (!cpu_has_apic)
return 0;
--
Worst form of spam? Adding advertisment signatures ala sourceforge.net.
What goes next? Inserting advertisment *into* email?
On Mon, Feb 03, 2003 at 10:18:58PM +0100, Pavel Machek wrote:
> Unfortunately my systems seems to miss bfd.h, required by libutil++.
The debian package is apparently called binutils-dev
> > I haven't seen where you added these checks. They did not exist before:
> > oprofile took responsibility for the NMI/perfctr handling off the NMI
> > watchdog entirely, then handed it back when oprofile finished.
>
> Ouch, I did not see that code. It seems not to be in nmi_int.c; where
> is that?
oprofile saves the perfctr config when it starts in nmi_int.c, then
calls the set_nmi_pm_callback. Then the suspend/resume calls oprofile's,
and we assume we have unique access to controlling the generated NMIs
(ie it becomes oprofile's job to ensure no NMIs are generated by the
perfctrs whilst suspended).
It's pretty ugly, so a correct cleanup would be appreciated.
I suppose this is quite an unusual case for the PM model ?
regards
john
Hi!
> > + set_fixmap_nocache(FIX_APIC_BASE, apic_phys); /* FIXME: this is needed for S3 resume, but why? */
>
> Intel recommends a strong uncacheable mapping otherwise it may exhibit
> undeterministic behaviour. Perhaps that could be it?
But... set_fixmp_nocache seems to only flip bits in page
tables... that should be nop after S3 because memory is preserved.
Pavel
--
Worst form of spam? Adding advertisment signatures ala sourceforge.net.
What goes next? Inserting advertisment *into* email?
Hi!
> > Unfortunately my systems seems to miss bfd.h, required by libutil++.
>
> The debian package is apparently called binutils-dev
Thanx. oprofile now seems to work for me.
> > > I haven't seen where you added these checks. They did not exist before:
> > > oprofile took responsibility for the NMI/perfctr handling off the NMI
> > > watchdog entirely, then handed it back when oprofile finished.
> >
> > Ouch, I did not see that code. It seems not to be in nmi_int.c; where
> > is that?
>
> oprofile saves the perfctr config when it starts in nmi_int.c, then
> calls the set_nmi_pm_callback. Then the suspend/resume calls oprofile's,
> and we assume we have unique access to controlling the generated NMIs
> (ie it becomes oprofile's job to ensure no NMIs are generated by the
> perfctrs whilst suspended).
I can see set_nmi_pm_callback() in nmi_setup(), but can not see where
nmi.c's pm_callback is restored in nmi_shutdown(). Perhaps that's a
bug?
> It's pretty ugly, so a correct cleanup would be appreciated.
>
> I suppose this is quite an unusual case for the PM model ?
Yes, it is pretty unusual.
Pavel
--
Worst form of spam? Adding advertisment signatures ala sourceforge.net.
What goes next? Inserting advertisment *into* email?
On Wed, Feb 05, 2003 at 10:39:19PM +0100, Pavel Machek wrote:
> I can see set_nmi_pm_callback() in nmi_setup(), but can not see where
> nmi.c's pm_callback is restored in nmi_shutdown().
It's the very first line :
149 unset_nmi_pm_callback(oprofile_pmdev);
The unset will also restore the nmi_pmdev back.
regards
john