public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* Re: Switch APIC to driver model (and make S3 sleep with APIC on)
@ 2003-01-28 12:19 Mikael Pettersson
  2003-01-29 20:18 ` Pavel Machek
  0 siblings, 1 reply; 17+ messages in thread
From: Mikael Pettersson @ 2003-01-28 12:19 UTC (permalink / raw)
  To: pavel; +Cc: ak, linux-kernel, torvalds

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

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: Switch APIC to driver model (and make S3 sleep with APIC on)
@ 2003-01-28  1:21 Mikael Pettersson
  2003-01-28  9:26 ` Pavel Machek
  2003-02-02 12:42 ` Pavel Machek
  0 siblings, 2 replies; 17+ messages in thread
From: Mikael Pettersson @ 2003-01-28  1:21 UTC (permalink / raw)
  To: ak, linux-kernel, pavel, torvalds

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

^ permalink raw reply	[flat|nested] 17+ messages in thread
* Switch APIC to driver model (and make S3 sleep with APIC on)
@ 2003-01-20 22:25 Pavel Machek
  0 siblings, 0 replies; 17+ messages in thread
From: Pavel Machek @ 2003-01-20 22:25 UTC (permalink / raw)
  To: ak, kernel list, torvalds

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?

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2003-02-07 11:08 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2003-01-28 12:19 Switch APIC to driver model (and make S3 sleep with APIC on) Mikael Pettersson
2003-01-29 20:18 ` Pavel Machek
2003-01-29 22:42   ` John Levon
2003-01-29 22:51     ` Pavel Machek
  -- strict thread matches above, loose matches on Subject: below --
2003-01-28  1:21 Mikael Pettersson
2003-01-28  9:26 ` Pavel Machek
2003-02-02 12:42 ` Pavel Machek
2003-02-03 10:32   ` John Levon
2003-02-03 15:40     ` Pavel Machek
2003-02-03 20:20       ` John Levon
2003-02-03 21:18         ` Pavel Machek
2003-02-03 23:43           ` John Levon
2003-02-05 21:39             ` Pavel Machek
2003-02-07 11:18               ` John Levon
2003-02-03 12:02   ` Zwane Mwaikambo
2003-02-04 22:55     ` Pavel Machek
2003-01-20 22:25 Pavel Machek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox