Skip to contentSkip to conversation

enums to clear suspend-state confusion

PATCH In review Pavel Machek 8 participants 23 messages Aug 12, 2004 – Aug 18, 2004

Subsystems (6)
Network drivers PCI Driver core Suspend to RAM +2 more subsystems
lore
Share & tools
lore

Pavel Machek

author
#0 lore raw

PATCH 1/1 enums to clear suspend-state confusion

Hi!

This patch should clear up some confusion between driver model and drivers people, and also prepares way to add runtime power managment later. Please apply,
Pavel

drivers/base/power/power.h +4 -5
@@ -1,5 +1,4 @@
-
-
+/* FIXME: This needs explanation... */
 enum {
 	DEVICE_PM_ON,
 	DEVICE_PM1,

@@ -66,14 +65,14 @@
 /*
  * suspend.c
  */
-extern int suspend_device(struct device *, u32);
+extern int suspend_device(struct device *, enum system_state);
 
 
 /*
  * runtime.c
  */
 
-extern int dpm_runtime_suspend(struct device *, u32);
+extern int dpm_runtime_suspend(struct device *, enum system_state);
 extern void dpm_runtime_resume(struct device *);
 
 #else /* CONFIG_PM */

@@ -88,7 +87,7 @@
 
 }
 
-static inline int dpm_runtime_suspend(struct device * dev, u32 state)
+static inline int dpm_runtime_suspend(struct device * dev, enum system_state state)
 {
 	return 0;
 }
drivers/base/power/shutdown.c +1 -0
@@ -29,6 +29,7 @@
 			dev->driver->shutdown(dev);
 		return 0;
 	}
+	/* FIXME: It probably should not be cast like this */
 	return dpm_runtime_suspend(dev, dev->detach_state);
 }
drivers/base/power/suspend.c +4 -3
@@ -28,6 +28,7 @@
  * lists. This way, the ancestors will be accessed before their descendents.
  */
 
+/* FIXME: Having both suspend_device and device_suspend is evil */
 
 /**
  *	suspend_device - Save state of one device.

@@ -35,7 +36,7 @@
  *	@state:	Power state device is entering.
  */
 
-int suspend_device(struct device * dev, u32 state)
+int suspend_device(struct device * dev, enum system_state state)
 {
 	int error = 0;
 

@@ -70,7 +71,7 @@
  *
  */
 
-int device_suspend(u32 state)
+int device_suspend(enum system_state state)
 {
 	int error = 0;
 

@@ -112,7 +113,7 @@
  *	done, power down system devices.
  */
 
-int device_power_down(u32 state)
+int device_power_down(enum system_state state)
 {
 	int error = 0;
 	struct device * dev;
drivers/ide/ide-disk.c +1 -0
@@ -1406,6 +1406,7 @@
 {
 	switch (rq->pm->pm_step) {
 	case idedisk_pm_flush_cache:	/* Suspend step 1 (flush cache) complete */
+		/* FIXME: This is buggy. */
 		if (rq->pm->pm_state == 4)
 			rq->pm->pm_step = ide_pm_state_completed;
 		else
include/linux/pci.h +22 -1
@@ -637,7 +637,7 @@
 	const struct pci_device_id *id_table;	/* must be non-NULL for probe to be called */
 	int  (*probe)  (struct pci_dev *dev, const struct pci_device_id *id);	/* New device inserted */
 	void (*remove) (struct pci_dev *dev);	/* Device removed (NULL if not a hot-plug capable driver) */
-	int  (*suspend) (struct pci_dev *dev, u32 state);	/* Device suspended */
+	int  (*suspend) (struct pci_dev *dev, suspend_state_t reason);	/* Device suspended */
 	int  (*resume) (struct pci_dev *dev);	                /* Device woken up */
 	int  (*enable_wake) (struct pci_dev *dev, u32 state, int enable);   /* Enable wake event */
 

@@ -1021,5 +1021,26 @@
 #define PCIPCI_VSFX		16
 #define PCIPCI_ALIMAGIK		32
 
+enum pci_state {
+	D0 = 0,
+	D1 = 1,
+	D2 = 2,
+	D3hot = 3,
+	D3cold = 4
+};
+
+static inline enum pci_state to_pci_state(suspend_state_t state)
+{
+	if (SUSPEND_EQ(state, PM_SUSPEND_ON))
+		return D0;
+	if (SUSPEND_EQ(state, PM_SUSPEND_STANDBY))
+		return D1;
+	if (SUSPEND_EQ(state, PM_SUSPEND_MEM))
+		return D3hot;
+	if (SUSPEND_EQ(state, PM_SUSPEND_DISK))
+		return D3cold;
+	BUG();
+}
+
 #endif /* __KERNEL__ */
 #endif /* LINUX_PCI_H */
include/linux/pm.h +18 -7
@@ -193,14 +193,22 @@
 extern void (*pm_idle)(void);
 extern void (*pm_power_off)(void);
 
-enum {
-	PM_SUSPEND_ON,
-	PM_SUSPEND_STANDBY,
-	PM_SUSPEND_MEM,
-	PM_SUSPEND_DISK,
+enum system_state {
+	PM_SUSPEND_ON = 0,
+	PM_SUSPEND_STANDBY = 1,
+	PM_SUSPEND_MEM = 2,
+	PM_SUSPEND_DISK = 3,
 	PM_SUSPEND_MAX,
 };
 
+/*
+ * For now, drivers only get system state. Later, this is going to become
+ * structure or something to enable runtime power managment.
+ */
+typedef enum system_state suspend_state_t;
+
+#define SUSPEND_EQ(a, b) (a == b)
+
 enum {
 	PM_DISK_FIRMWARE = 1,
 	PM_DISK_PLATFORM,

@@ -241,8 +249,11 @@
 
 extern void device_pm_set_parent(struct device * dev, struct device * parent);
 
-extern int device_suspend(u32 state);
-extern int device_power_down(u32 state);
+/*
+ * apply system suspend policy to all devices
+ */
+extern int device_suspend(enum system_state reason);
+extern int device_power_down(enum system_state reason);
 extern void device_power_up(void);
 extern void device_resume(void);
Signature · 2 lines
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!
#1 lore raw
Quoted 5 lines · Pavel Machek

+enum pci_state {
+ D0 = 0,
+ D1 = 1,
+ D2 = 2,

These symbols are too generic. They don't appear to currently clash with anything else, but they could.

author
#2 lore raw

Hi!

Quoted 7 lines
Quoted 4 lines

+enum pci_state {
+ D0 = 0,
+ D1 = 1,
+ D2 = 2,

These symbols are too generic. They don't appear to currently clash with anything else, but they could.

Actually jffs is using macros called D1, D2 and D3. Ouch. This one should fix it.
Pavel

include/linux/pci.h +9 -9
@@ -1023,23 +1023,23 @@
 #define PCIPCI_ALIMAGIK		32
 
 enum pci_state {
-	D0 = 0,
-	D1 = 1,
-	D2 = 2,
-	D3hot = 3,
-	D3cold = 4
+	PCI_D0 = 0,
+	PCI_D1 = 1,
+	PCI_D2 = 2,
+	PCI_D3hot = 3,
+	PCI_D3cold = 4
 };
 
 static inline enum pci_state to_pci_state(suspend_state_t state)
 {
 	if (SUSPEND_EQ(state, PM_SUSPEND_ON))
-		return D0;
+		return PCI_D0;
 	if (SUSPEND_EQ(state, PM_SUSPEND_STANDBY))
-		return D1;
+		return PCI_D1;
 	if (SUSPEND_EQ(state, PM_SUSPEND_MEM))
-		return D3hot;
+		return PCI_D3hot;
 	if (SUSPEND_EQ(state, PM_SUSPEND_DISK))
-		return D3cold;
+		return PCI_D3cold;
 	BUG();
 }
Signature · 2 lines
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!
#3 lore raw
In reply to Pavel Machek

At Thu, 12 Aug 2004 14:02:21 +0200,

Quoted 6 lines · Pavel Machek

Hi!

This patch should clear up some confusion between driver model and drivers people, and also prepares way to add runtime power managment later. Please apply,

(snip)

include/linux/pci.h line 640

Quoted 8 lines
include/linux/pci.h +1 -1
@@ -637,7 +637,7 @@
 	const struct pci_device_id *id_table;	/* must be non-NULL for probe to be called */
 	int  (*probe)  (struct pci_dev *dev, const struct pci_device_id *id);	/* New device inserted */
 	void (*remove) (struct pci_dev *dev);	/* Device removed (NULL if not a hot-plug capable driver) */
-	int  (*suspend) (struct pci_dev *dev, u32 state);	/* Device suspended */
+	int  (*suspend) (struct pci_dev *dev, suspend_state_t reason);	/* Device suspended */

Does this mean that each driver needs rewrite of suspend callback?

Takashi

#4 lore raw

Hi!

Quoted 4 lines
Quoted 3 lines

This patch should clear up some confusion between driver model and drivers people, and also prepares way to add runtime power managment later. Please apply,

Quoted 10 lines

include/linux/pci.h line 640

Quoted 8 lines
include/linux/pci.h +1 -1
@@ -637,7 +637,7 @@
 	const struct pci_device_id *id_table;	/* must be non-NULL for probe to be called */
 	int  (*probe)  (struct pci_dev *dev, const struct pci_device_id *id);	/* New device inserted */
 	void (*remove) (struct pci_dev *dev);	/* Device removed (NULL if not a hot-plug capable driver) */
-	int  (*suspend) (struct pci_dev *dev, u32 state);	/* Device suspended */
+	int  (*suspend) (struct pci_dev *dev, suspend_state_t reason);	/* Device suspended */

Does this mean that each driver needs rewrite of suspend callback?

Yes, all drivers will eventually have to be touched. Thats unavoidable. Fortunately most drivers ignore state, and those that don't are saved by suspend_state_t being u32 for now... so we
are not breaking them just now.

Pavel

Signature · 1 line
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms
author
#5 lore raw
In reply to Pavel Machek

Hi!

Quoted 3 lines

This patch should clear up some confusion between driver model and drivers people, and also prepares way to add runtime power managment later. Please apply,

PCI states are now marked with PCI_D? so that confusion is not possible. Andrew complained about warning in to_pci_state(); I do not see it, but I added catch-all return anyway.

I'd like this to be applied, so I can start fixing the drivers...

Pavel

drivers/base/power/power.h renamed from linux/drivers/base/power/power.h +4 -5
@@ -1,5 +1,4 @@
-
-
+/* FIXME: This needs explanation... */
 enum {
 	DEVICE_PM_ON,
 	DEVICE_PM1,

@@ -66,14 +65,14 @@
 /*
  * suspend.c
  */
-extern int suspend_device(struct device *, u32);
+extern int suspend_device(struct device *, enum system_state);
 
 
 /*
  * runtime.c
  */
 
-extern int dpm_runtime_suspend(struct device *, u32);
+extern int dpm_runtime_suspend(struct device *, enum system_state);
 extern void dpm_runtime_resume(struct device *);
 
 #else /* CONFIG_PM */

@@ -88,7 +87,7 @@
 
 }
 
-static inline int dpm_runtime_suspend(struct device * dev, u32 state)
+static inline int dpm_runtime_suspend(struct device * dev, enum system_state state)
 {
 	return 0;
 }
drivers/base/power/shutdown.c renamed from linux/drivers/base/power/shutdown.c +2 -1
@@ -29,7 +29,8 @@
 			dev->driver->shutdown(dev);
 		return 0;
 	}
-	return dpm_runtime_suspend(dev, dev->detach_state);
+	/* FIXME: It probably should not be cast like this */
+	return dpm_runtime_suspend(dev, (enum system_state) dev->detach_state);
 }
drivers/base/power/suspend.c renamed from linux/drivers/base/power/suspend.c +4 -3
@@ -28,6 +28,7 @@
  * lists. This way, the ancestors will be accessed before their descendents.
  */
 
+/* FIXME: Having both suspend_device and device_suspend is evil */
 
 /**
  *	suspend_device - Save state of one device.

@@ -35,7 +36,7 @@
  *	@state:	Power state device is entering.
  */
 
-int suspend_device(struct device * dev, u32 state)
+int suspend_device(struct device * dev, enum system_state state)
 {
 	int error = 0;
 

@@ -70,7 +71,7 @@
  *
  */
 
-int device_suspend(u32 state)
+int device_suspend(enum system_state state)
 {
 	int error = 0;
 

@@ -112,7 +113,7 @@
  *	done, power down system devices.
  */
 
-int device_power_down(u32 state)
+int device_power_down(enum system_state state)
 {
 	int error = 0;
 	struct device * dev;
include/linux/pci.h renamed from linux/include/linux/pci.h +24 -1
@@ -18,6 +18,7 @@
 #define LINUX_PCI_H
 
 #include <linux/mod_devicetable.h>
+#include <linux/pci.h>
 
 /*
  * Under PCI, each device has 256 bytes of configuration address space,

@@ -637,7 +638,7 @@
 	const struct pci_device_id *id_table;	/* must be non-NULL for probe to be called */
 	int  (*probe)  (struct pci_dev *dev, const struct pci_device_id *id);	/* New device inserted */
 	void (*remove) (struct pci_dev *dev);	/* Device removed (NULL if not a hot-plug capable driver) */
-	int  (*suspend) (struct pci_dev *dev, u32 state);	/* Device suspended */
+	int  (*suspend) (struct pci_dev *dev, suspend_state_t reason);	/* Device suspended */
 	int  (*resume) (struct pci_dev *dev);	                /* Device woken up */
 	int  (*enable_wake) (struct pci_dev *dev, u32 state, int enable);   /* Enable wake event */
 

@@ -1021,5 +1022,27 @@
 #define PCIPCI_VSFX		16
 #define PCIPCI_ALIMAGIK		32
 
+enum pci_state {
+	PCI_D0 = 0,
+	PCI_D1 = 1,
+	PCI_D2 = 2,
+	PCI_D3hot = 3,
+	PCI_D3cold = 4
+};
+
+static inline enum pci_state to_pci_state(suspend_state_t state)
+{
+	if (SUSPEND_EQ(state, PM_SUSPEND_ON))
+		return PCI_D0;
+	if (SUSPEND_EQ(state, PM_SUSPEND_STANDBY))
+		return PCI_D1;
+	if (SUSPEND_EQ(state, PM_SUSPEND_MEM))
+		return PCI_D3hot;
+	if (SUSPEND_EQ(state, PM_SUSPEND_DISK))
+		return PCI_D3cold;
+	BUG();
+	return PCI_D0;	/* akpm complained about warnings? */
+}
+
 #endif /* __KERNEL__ */
 #endif /* LINUX_PCI_H */
include/linux/pm.h renamed from linux/include/linux/pm.h +18 -7
@@ -193,14 +193,22 @@
 extern void (*pm_idle)(void);
 extern void (*pm_power_off)(void);
 
-enum {
-	PM_SUSPEND_ON,
-	PM_SUSPEND_STANDBY,
-	PM_SUSPEND_MEM,
-	PM_SUSPEND_DISK,
+enum system_state {
+	PM_SUSPEND_ON = 0,
+	PM_SUSPEND_STANDBY = 1,
+	PM_SUSPEND_MEM = 2,
+	PM_SUSPEND_DISK = 3,
 	PM_SUSPEND_MAX,
 };
 
+/*
+ * For now, drivers only get system state. Later, this is going to become
+ * structure or something to enable runtime power managment.
+ */
+typedef enum system_state suspend_state_t;
+
+#define SUSPEND_EQ(a, b) (a == b)
+
 enum {
 	PM_DISK_FIRMWARE = 1,
 	PM_DISK_PLATFORM,

@@ -241,8 +248,11 @@
 
 extern void device_pm_set_parent(struct device * dev, struct device * parent);
 
-extern int device_suspend(u32 state);
-extern int device_power_down(u32 state);
+/*
+ * apply system suspend policy to all devices
+ */
+extern int device_suspend(enum system_state reason);
+extern int device_power_down(enum system_state reason);
 extern void device_power_up(void);
 extern void device_resume(void);
kernel/power/console.c renamed from linux/kernel/power/console.c +1 -0
@@ -7,6 +7,7 @@
 #include <linux/vt_kern.h>
 #include <linux/kbd_kern.h>
 #include <linux/console.h>
+#include <linux/delay.h>
 #include "power.h"
 
 static int new_loglevel = 10;
Signature · 2 lines
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!
#6 lore raw
Quoted 7 lines · Pavel Machek

Hi!

Quoted 3 lines

This patch should clear up some confusion between driver model and drivers people, and also prepares way to add runtime power managment later. Please apply,

Quoted 6 lines

PCI states are now marked with PCI_D? so that confusion is not possible. Andrew complained about warning in to_pci_state(); I do not see it, but I added catch-all return anyway.

I'd like this to be applied, so I can start fixing the drivers...

Sure, let's try to get this done.

Quoted 16 lines
include/linux/pci.h renamed from linux/include/linux/pci.h +15 -0
+static inline enum pci_state to_pci_state(suspend_state_t state)
+{
+	if (SUSPEND_EQ(state, PM_SUSPEND_ON))
+		return PCI_D0;
+	if (SUSPEND_EQ(state, PM_SUSPEND_STANDBY))
+		return PCI_D1;
+	if (SUSPEND_EQ(state, PM_SUSPEND_MEM))
+		return PCI_D3hot;
+	if (SUSPEND_EQ(state, PM_SUSPEND_DISK))
+		return PCI_D3cold;
+	BUG();
+	return PCI_D0;	/* akpm complained about warnings? */
+}
+
...
+/*

include/linux/pci.h

Quoted 10 lines
include/linux/pci.h renamed from linux/include/linux/pci.h +7 -0
+ * For now, drivers only get system state. Later, this is going to become
+ * structure or something to enable runtime power managment.
+ */
+typedef enum system_state suspend_state_t;
+
+#define SUSPEND_EQ(a, b) (a == b)
+
 enum {
 	PM_DISK_FIRMWARE = 1,
 	PM_DISK_PLATFORM,

This is a bit ugly, and I don't think it actually works.

If, at some time in the future you change the suspend state to a struct then you will want to pass that thing around by reference, not by value. Hence your new suspend_state_t will need to be typecast to a pointer to struct, and not a struct. And that's not a thing which we do in-kernel much at all. (There's nothing wrong with the practice per-se, but in the kernel it does violate the principle of least surprise).

So if you really do intend to add more things to the suspend state I'd suggest that you set the final framework in place immediately. Do:

struct suspend_state {
	enum system_state state;
}

static inline enum pci_state to_pci_state(struct suspend_state *state)

{
	if (state->state == PM_SUSPEND_ON)
		return PCI_D0;
	if (state->state == PM_SUSPEND_STANDBY)
		return PCI_D1;
	...
}
author
#7 lore raw

Hi!

Quoted 24 lines
Quoted 1 line

I'd like this to be applied, so I can start fixing the drivers...

Sure, let's try to get this done.

Quoted 20 lines
include/linux/pci.h renamed from linux/include/linux/pci.h +19 -0
+static inline enum pci_state to_pci_state(suspend_state_t state)
+{
+	if (SUSPEND_EQ(state, PM_SUSPEND_ON))
+		return PCI_D0;
+	if (SUSPEND_EQ(state, PM_SUSPEND_STANDBY))
+		return PCI_D1;
+	if (SUSPEND_EQ(state, PM_SUSPEND_MEM))
+		return PCI_D3hot;
+	if (SUSPEND_EQ(state, PM_SUSPEND_DISK))
+		return PCI_D3cold;
+	BUG();
+	return PCI_D0;	/* akpm complained about warnings? */
+}
+
...
+/*
+ * For now, drivers only get system state. Later, this is going to become
+ * structure or something to enable runtime power managment.
+ */
+typedef enum system_state suspend_state_t;
Quoted 8 lines

include/linux/pm.h line 214

Quoted 6 lines
include/linux/pm.h renamed from linux/include/linux/pm.h +3 -0
+
+#define SUSPEND_EQ(a, b) (a == b)
+
 enum {
 	PM_DISK_FIRMWARE = 1,
 	PM_DISK_PLATFORM,

This is a bit ugly, and I don't think it actually works.

I agree about the ugly bit :-(.

Quoted 3 lines

If, at some time in the future you change the suspend state to a struct then you will want to pass that thing around by reference, not by value.

Actually I expect it to become struct of two members, system-state and bus-specific state. That seems small enough to pass by value.

Quoted 11 lines

Hence your new suspend_state_t will need to be typecast to a pointer to struct, and not a struct. And that's not a thing which we do in-kernel much at all. (There's nothing wrong with the practice per-se, but in the kernel it does violate the principle of least surprise).

So if you really do intend to add more things to the suspend state I'd suggest that you set the final framework in place immediately. Do:

struct suspend_state {
	enum system_state state;
}

I can do that... but it will break compilation of every driver in the tree. I can fix drivers I use and try to fix some more will sed, but it will be painfull (and pretty big diff, and I'll probably miss some).

Should I do that?
Pavel

Signature · 2 lines
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!
#8 lore raw
Quoted 7 lines · Pavel Machek
Quoted 4 lines

If, at some time in the future you change the suspend state to a struct then you will want to pass that thing around by reference, not by value.

Actually I expect it to become struct of two members, system-state and bus-specific state. That seems small enough to pass by value.

hm. Yes, it's hard to see how this structure can grow much larger.

But we'll be stuck for all time with a weird interface, just because once back in August of '04 we didn't want to patch 129 files.

Quoted 8 lines
Quoted 8 lines

Hence your new suspend_state_t will need to be typecast to a pointer to struct, and not a struct. And that's not a thing which we do in-kernel much at all. (There's nothing wrong with the practice per-se, but in the kernel it does violate the principle of least surprise).

So if you really do intend to add more things to the suspend state I'd suggest that you set the final framework in place immediately. Do:

Quoted 7 lines
Quoted 3 lines
struct suspend_state {
	enum system_state state;
}

I can do that... but it will break compilation of every driver in the tree. I can fix drivers I use and try to fix some more will sed, but it will be painfull (and pretty big diff, and I'll probably miss some).

That's OK - it's just an hour's work. I'd be more concerned about irritating people who are maintaining and using out-of-tree drivers.

Can you remind me why we need _any_ of this? "enums to clear suspend-state confusion" sounds like something which is very optional. I'd be opting to go do something else instead ;)

author
#9 lore raw

Hi!

Quoted 10 lines
Quoted 3 lines

I can do that... but it will break compilation of every driver in the tree. I can fix drivers I use and try to fix some more will sed, but it will be painfull (and pretty big diff, and I'll probably miss some).

That's OK - it's just an hour's work. I'd be more concerned about irritating people who are maintaining and using out-of-tree drivers.

Can you remind me why we need _any_ of this? "enums to clear suspend-state confusion" sounds like something which is very optional. I'd be opting to go do something else instead ;)

Okay... currently, we are passing u32 down the drivers. Some pieces interpret it as a PCI state, and some pieces interpret it as a system state. We really do want system state to go down to the drivers, so they can do different thing on reboot vs. just-before-suspend-to-disk etc.

Now, Patrick has some plans with device power managment and they included something bigger being passed down to the drivers. I wanted to prepare for those plans.

I can replace suspend_state_t with enum system_state, but it might mean that enum system_state will have to be extended with things like RUNTIME_PM_PCI_D0 in future... I guess that's easiest thing to do. It solves all the problems we have *now*.

Pavel

Signature · 2 lines
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!
#10 lore raw
Quoted 9 lines

Now, Patrick has some plans with device power managment and they included something bigger being passed down to the drivers. I wanted to prepare for those plans.

I can replace suspend_state_t with enum system_state, but it might mean that enum system_state will have to be extended with things like RUNTIME_PM_PCI_D0 in future... I guess that's easiest thing to do. It solves all the problems we have *now*.

Better is to use a typedef then, so that enum can be turned into a pointer to a structure later on, and drivers using the "helper" function to_pci_state() would not need any change when that transition happen.

Ben.

author
#11 lore raw

Hi!

Quoted 9 lines
Quoted 8 lines

Now, Patrick has some plans with device power managment and they included something bigger being passed down to the drivers. I wanted to prepare for those plans.

I can replace suspend_state_t with enum system_state, but it might mean that enum system_state will have to be extended with things like RUNTIME_PM_PCI_D0 in future... I guess that's easiest thing to do. It solves all the problems we have *now*.

Quoted 4 lines

Better is to use a typedef then, so that enum can be turned into a pointer to a structure later on, and drivers using the "helper" function to_pci_state() would not need any change when that transition happen.

Yes, that's exactly what I did... Unfortunately typedef means ugly code. So I'll just switch it back to enum system_state, and lets care about device power managment when it hits us, okay?

--- tmp/linux/include/linux/pci.h 2004-08-15 19:15:05.000000000 +0200
+++ linux/include/linux/pci.h 2004-08-17 23:16:41.000000000 +0200
...
const struct pci_device_id *id_table; /* must be non-NULL for probe to be called */
int (*probe) (struct pci_dev *dev, const struct pci_device_id *id); /* New device inserted */
void (*remove) (struct pci_dev *dev); /* Device removed (NULL if not a hot-plug capable driver) */

-	int  (*suspend) (struct pci_dev *dev, u32 state);	/* Device suspended */
+	int  (*suspend) (struct pci_dev *dev, suspend_state_t reason);	/* Device suspended */
 	int  (*resume) (struct pci_dev *dev);	                /* Device woken up */
 	int  (*enable_wake) (struct pci_dev *dev, u32 state, int enable);   /* Enable wake event */

--- tmp/linux/include/linux/pm.h 2004-08-15 19:15:05.000000000 +0200
+++ linux/include/linux/pm.h 2004-08-15 19:35:41.000000000 +0200
...
+/*
+ * For now, drivers only get system state. Later, this is going to become
+ * structure or something to enable runtime power managment.
+ */
+typedef enum system_state suspend_state_t;
+
+#define SUSPEND_EQ(a, b) (a == b)
+
enum {
PM_DISK_FIRMWARE = 1,
PM_DISK_PLATFORM,

Signature · 2 lines
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!
#12 lore raw
Quoted 4 lines

Yes, that's exactly what I did... Unfortunately typedef means ugly code. So I'll just switch it back to enum system_state, and lets care about device power managment when it hits us, okay?

I don't agree... with this approach, we'll have to change all drivers _twice_ when we move to something more descriptive than a scalar

Ben.

author
#13 lore raw

Hi!

Quoted 7 lines
Quoted 3 lines

Yes, that's exactly what I did... Unfortunately typedef means ugly code. So I'll just switch it back to enum system_state, and lets care about device power managment when it hits us, okay?

I don't agree... with this approach, we'll have to change all drivers _twice_ when we move to something more descriptive than a scalar

I'm not entirely concinced we need anything more than a scalar.
I think we could simply create PM_RUNTIME_0 .. PM_RUNTIME_9, and that will work as long as all devices have <= 10 power managment states.

That can well mean "forever".

Besides, when enums are in place, you can just searcg&replace. Search part is way harder with u32s.

Pavel

Signature · 1 line
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms
#14 lore raw
In reply to Benjamin Herrenschmidt
Quoted 7 lines · Benjamin Herrenschmidt
Quoted 4 lines

Yes, that's exactly what I did... Unfortunately typedef means ugly code. So I'll just switch it back to enum system_state, and lets care about device power managment when it hits us, okay?

I don't agree... with this approach, we'll have to change all drivers _twice_ when we move to something more descriptive than a scalar

I totally agree. Why be hasty? We need to do it right and do it once. If that means a couple of more weeks and several more emails, than so be it. Otherwise, we'll be stuck with a sub-par solution for who knows how long.

Pat

#15 lore raw
In reply to Benjamin Herrenschmidt
Quoted 7 lines · Benjamin Herrenschmidt
Quoted 4 lines

Yes, that's exactly what I did... Unfortunately typedef means ugly code. So I'll just switch it back to enum system_state, and lets care about device power managment when it hits us, okay?

I don't agree... with this approach, we'll have to change all drivers _twice_ when we move to something more descriptive than a scalar

The advantage of switching _now_ to enums is that it can be done without actually changing drivers ... however, there are actually two different suspend-state confusions. That patch just affects the first of them, whereas it's the second which actively breaks things today:

- Confusion #1 ... "generic" PM framework uses a u32, and the meaning of that u32 has never been formally defined.

In practice, most kernel code (except swsusp) passes an enum there already ... PM_SUSPEND_* values, which are unfortunately in an anonymous enum so there's no way even a smartened "sparse" could report errors.

- Confusion #2 ... PCI suspend calls use a u32, which has since
at least 2.4 meant a PCI power state. Those values are not
the same as PM_SUSPEND_* values.

PCI drivers that use that u32 are currently getting burnt by the way PM_SUSPEND_* values get passed in when the drivers expect a PCI power state. Example, passing PM_SUSPEND_MEM when the intention is PCI_D3hot (2 rather than 3).

I happen to think the _right_ fix involves switching from a scalar to something that recognizes {bus,device,driver}-specific PM states.

Such a switch would be extremely disruptive; it'd mean changing
every driver. So it's something I'd not rush into ... it'd be worth
doing as part of fixing multiple holes in the PM framework.

Which leaves me thinking that the desirable near-term fix involves cleanup for #1, and minor sleight-of-hand to fix #2. Something like the attached patch (untested) ... which would let us answer Andrew's "why?" question by pointing to some bugtraq IDs.

- Dave

Cleanup of some of the "new 2.6" suspend call path:

- Documents existing practice for non-PCI drivers by naming an enum that was previously anonymous (as "enum system_state"), and by passing it where an undefined "u32" previously appeared. GCC won't complain if a constant is passed, but other tools could ... and before this, it's been unclear what that "u32" meant.

- Changes the PM_SUSPEND_MEM (and PM_SUSPEND_DISK) enum values so that they make sense as PCI power states. This is a bit ugly, but it

(a) fixes bugs whereby PCI drivers are being given bogus values, without changing the PCI PM calls (unchanged since 2.4)

(b) doesn't break the (IMO unwise) assumptions in the "new 2.6" PM code that dev->power.power_state and dev->detach_state and /sys/power/state (and /sys/bus/*/devices/power/state) files all use the same numeric codes.

Really there ought to be distinct "struct" types for device power states (accomodating bus-specific and device-specific power states) and for system power policies, but such changes would be rather disruptive.

include/linux/pm.h +11 -8
@@ -193,11 +193,12 @@
 extern void (*pm_idle)(void);
 extern void (*pm_power_off)(void);
 
-enum {
-	PM_SUSPEND_ON,
-	PM_SUSPEND_STANDBY,
-	PM_SUSPEND_MEM,
-	PM_SUSPEND_DISK,
+enum system_state {
+	PM_SUSPEND_ON = 0,
+	PM_SUSPEND_STANDBY = 1,
+	/* HACK ALERT:  PM_SUSPEND_MEM == PCI_D3cold */
+	PM_SUSPEND_MEM = 3,
+	PM_SUSPEND_DISK = 4,
 	PM_SUSPEND_MAX,
 };
 

@@ -241,11 +242,13 @@
 
 extern void device_pm_set_parent(struct device * dev, struct device * parent);
 
-extern int device_suspend(u32 state);
-extern int device_power_down(u32 state);
+/*
+ * apply system suspend policy to all devices
+ */
+extern int device_suspend(enum system_state reason);
+extern int device_power_down(enum system_state reason);
 extern void device_power_up(void);
 extern void device_resume(void);
-
 
 #endif /* __KERNEL__ */
drivers/base/power/power.h +3 -3
@@ -66,14 +66,14 @@
 /*
  * suspend.c
  */
-extern int suspend_device(struct device *, u32);
+extern int suspend_device(struct device *, enum system_state);
 
 
 /*
  * runtime.c
  */
 
-extern int dpm_runtime_suspend(struct device *, u32);
+extern int dpm_runtime_suspend(struct device *, enum system_state);
 extern void dpm_runtime_resume(struct device *);
 
 #else /* CONFIG_PM */

@@ -88,7 +88,7 @@
 
 }
 
-static inline int dpm_runtime_suspend(struct device * dev, u32 state)
+static inline int dpm_runtime_suspend(struct device * dev, enum system_state state)
 {
 	return 0;
 }
drivers/base/power/shutdown.c +1 -1
@@ -29,7 +29,7 @@
 			dev->driver->shutdown(dev);
 		return 0;
 	}
-	return dpm_runtime_suspend(dev, dev->detach_state);
+	return dpm_runtime_suspend(dev, (enum system_state) dev->detach_state);
 }
drivers/base/power/suspend.c +3 -3
@@ -35,7 +35,7 @@
  *	@state:	Power state device is entering.
  */
 
-int suspend_device(struct device * dev, u32 state)
+int suspend_device(struct device * dev, enum system_state state)
 {
 	int error = 0;
 

@@ -70,7 +70,7 @@
  *
  */
 
-int device_suspend(u32 state)
+int device_suspend(enum system_state state)
 {
 	int error = 0;
 

@@ -112,7 +112,7 @@
  *	done, power down system devices.
  */
 
-int device_power_down(u32 state)
+int device_power_down(enum system_state state)
 {
 	int error = 0;
 	struct device * dev;
kernel/power/swsusp.c +5 -5
@@ -699,7 +699,7 @@
 	else
 #endif
 	{
-		device_suspend(3);
+		device_suspend(PM_SUSPEND_DISK);
 		device_shutdown();
 		machine_power_off();
 	}

@@ -720,7 +720,7 @@
 	mb();
 	spin_lock_irq(&suspend_pagedir_lock);	/* Done to disable interrupts */ 
 
-	device_power_down(3);
+	device_power_down(PM_SUSPEND_DISK);
 	PRINTK( "Waiting for DMAs to settle down...\n");
 	mdelay(1000);	/* We do not want some readahead with DMA to corrupt our memory, right?
 			   Do it with disabled interrupts for best effect. That way, if some

@@ -789,7 +789,7 @@
 {
 	int is_problem;
 	read_swapfiles();
-	device_power_down(3);
+	device_power_down(PM_SUSPEND_DISK);
 	is_problem = suspend_prepare_image();
 	device_power_up();
 	spin_unlock_irq(&suspend_pagedir_lock);

@@ -845,7 +845,7 @@
 		disable_nonboot_cpus();
 		/* Save state of all device drivers, and stop them. */
 		printk("Suspending devices... ");
-		if ((res = device_suspend(3))==0) {
+		if ((res = device_suspend(PM_SUSPEND_DISK))==0) {
 			/* If stopping device drivers worked, we proceed basically into
 			 * suspend_save_image.
 			 *

@@ -1200,7 +1200,7 @@
 		goto read_failure;
 	/* FIXME: Should we stop processes here, just to be safer? */
 	disable_nonboot_cpus();
-	device_suspend(3);
+	device_suspend(PM_SUSPEND_DISK);
 	do_magic(1);
 	panic("This never returns");
#16 lore raw

Hi!

Quoted 18 lines

The advantage of switching _now_ to enums is that it can be done without actually changing drivers ... however, there are actually two different suspend-state confusions. That patch just affects the first of them, whereas it's the second which actively breaks things today:

- Confusion #1 ... "generic" PM framework uses a u32, and the meaning of that u32 has never been formally defined.

In practice, most kernel code (except swsusp) passes an enum there already ... PM_SUSPEND_* values, which are unfortunately in an anonymous enum so there's no way even a smartened "sparse" could report errors.

- Confusion #2 ... PCI suspend calls use a u32, which has since
at least 2.4 meant a PCI power state. Those values are not
the same as PM_SUSPEND_* values.

Quoted 7 lines

PCI drivers that use that u32 are currently getting burnt by the way PM_SUSPEND_* values get passed in when the drivers expect a PCI power state. Example, passing PM_SUSPEND_MEM when the intention is PCI_D3hot (2 rather than 3).

I happen to think the _right_ fix involves switching from a scalar to something that recognizes {bus,device,driver}-specific PM states.

That would not provide enough info for the driver.

Quoted 8 lines

Such a switch would be extremely disruptive; it'd mean changing
every driver. So it's something I'd not rush into ... it'd be worth
doing as part of fixing multiple holes in the PM framework.

Which leaves me thinking that the desirable near-term fix involves cleanup for #1, and minor sleight-of-hand to fix #2. Something like the attached patch (untested) ... which would let us answer Andrew's "why?" question by pointing to some bugtraq IDs.

I believe correct fix for #2 is what I've done. Pass enum system_state down to driver, and make them use to_pci_state() helper. Fix of driver then looks like:

drivers/net/e100.c +2 -2
@@ -2269,7 +2269,7 @@
 }
 
 #ifdef CONFIG_PM
-static int e100_suspend(struct pci_dev *pdev, u32 state)
+static int e100_suspend(struct pci_dev *pdev, enum system_state state)
 {
 	struct net_device *netdev = pci_get_drvdata(pdev);
 	struct nic *nic = netdev_priv(netdev);

@@ -2282,7 +2282,7 @@
 	pci_save_state(pdev, nic->pm_state);
 	pci_enable_wake(pdev, state, nic->flags & (wol_magic | e100_asf(nic)));
 	pci_disable_device(pdev);
-	pci_set_power_state(pdev, state);
+	pci_set_power_state(pdev, to_pci_state(state));
 
 	return 0;
 }

Review

Quoted 8 lines
(quoted diff) +6 -0
+enum system_state {
+	PM_SUSPEND_ON = 0,
+	PM_SUSPEND_STANDBY = 1,
+	/* HACK ALERT:  PM_SUSPEND_MEM == PCI_D3cold */
+	PM_SUSPEND_MEM = 3,
+	PM_SUSPEND_DISK = 4,
 	PM_SUSPEND_MAX,
 };

Okay, I did not do this one. I'll probably just fix those drivers that care, instead. The rest of patch was pretty much same as mine patch, except:

kernel/power/swsusp.c line 705

Quoted 11 lines
kernel/power/swsusp.c +1 -1
@@ -699,7 +699,7 @@
 	else
 #endif
 	{
-		device_suspend(3);
+		device_suspend(PM_SUSPEND_DISK);
 		device_shutdown();
 		machine_power_off();
 	}

this is no longer neccessary. -mm swsusp already uses right constants.

Pavel

Signature · 2 lines
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!
#17 lore raw
In reply to Pavel Machek
Quoted 4 lines · Pavel Machek

I can replace suspend_state_t with enum system_state, but it might mean that enum system_state will have to be extended with things like RUNTIME_PM_PCI_D0 in future... I guess that's easiest thing to do. It solves all the problems we have *now*.

Can you not also provide a function

pci_state_for(system_state)

then most of the drivers don't have to care. It would also be nice to have a driver flag to indicate which devices can simply be hotunplug/hotreplugged over a suspend and don't need extra duplicate code.

#18 lore raw
Quoted 4 lines · Alan Cox

It would also be nice
to have a driver flag to indicate which devices can simply be hotunplug/hotreplugged over a suspend and don't need extra duplicate code.

In fact, that should be the default: if there's no suspend(), use drivers' unplug/replug logic. Bugs in that code need to be fixed regardless of PM being used (or not).

Right now that won't work because of a self-deadlock in the PM core, but that's a fixable problem.

#19 lore raw
In reply to Alan Cox

Hi!

Quoted 10 lines
Quoted 4 lines

I can replace suspend_state_t with enum system_state, but it might mean that enum system_state will have to be extended with things like RUNTIME_PM_PCI_D0 in future... I guess that's easiest thing to do. It solves all the problems we have *now*.

Can you not also provide a function

pci_state_for(system_state)

then most of the drivers don't have to care. It would also be nice

I actually named that function to_pci_state(), but you had same idea.

Quoted 3 lines

to have a driver flag to indicate which devices can simply be hotunplug/hotreplugged over a suspend and don't need extra duplicate code.

Hmm, I do not think it is that easy

suspend is:
stop hardware

hotunplug is:
stop hardware
tell user/system it is gone

So it is more like suspend is subset of hotunplug. If coded properly, hotunplug should probably call suspend code, resulting in no duplication.

Pavel

Signature · 2 lines
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!
author
#20 lore raw
In reply to Andrew Morton

Hi!

Quoted 16 lines
Quoted 8 lines
Quoted 8 lines

Hence your new suspend_state_t will need to be typecast to a pointer to struct, and not a struct. And that's not a thing which we do in-kernel much at all. (There's nothing wrong with the practice per-se, but in the kernel it does violate the principle of least surprise).

So if you really do intend to add more things to the suspend state I'd suggest that you set the final framework in place immediately. Do:

Quoted 7 lines
Quoted 3 lines
struct suspend_state {
	enum system_state state;
}

I can do that... but it will break compilation of every driver in the tree. I can fix drivers I use and try to fix some more will sed, but it will be painfull (and pretty big diff, and I'll probably miss some).

Quoted 6 lines

That's OK - it's just an hour's work. I'd be more concerned about irritating people who are maintaining and using out-of-tree drivers.

Can you remind me why we need _any_ of this? "enums to clear suspend-state confusion" sounds like something which is very optional. I'd be opting to go do something else instead ;)

Ok, here's non-ugly patch. It may mean that ugly patch is comming in future (BenH would argue that), but it is probably best solution for now. Please apply,

[changelog for you:]

Switch from plain u32 to enums in power managment, so that drivers are not confused what the parameter means, and introduce helper to convert between system state and PCI state.

Pavel

drivers/base/power/power.h renamed from linux/drivers/base/power/power.h +4 -5
@@ -1,5 +1,4 @@
-
-
+/* FIXME: This needs explanation... */
 enum {
 	DEVICE_PM_ON,
 	DEVICE_PM1,

@@ -66,14 +65,14 @@
 /*
  * suspend.c
  */
-extern int suspend_device(struct device *, u32);
+extern int suspend_device(struct device *, enum system_state);
 
 
 /*
  * runtime.c
  */
 
-extern int dpm_runtime_suspend(struct device *, u32);
+extern int dpm_runtime_suspend(struct device *, enum system_state);
 extern void dpm_runtime_resume(struct device *);
 
 #else /* CONFIG_PM */

@@ -88,7 +87,7 @@
 
 }
 
-static inline int dpm_runtime_suspend(struct device * dev, u32 state)
+static inline int dpm_runtime_suspend(struct device * dev, enum system_state state)
 {
 	return 0;
 }
drivers/base/power/suspend.c renamed from linux/drivers/base/power/suspend.c +4 -3
@@ -28,6 +28,7 @@
  * lists. This way, the ancestors will be accessed before their descendents.
  */
 
+/* FIXME: Having both suspend_device and device_suspend is evil */
 
 /**
  *	suspend_device - Save state of one device.

@@ -35,7 +36,7 @@
  *	@state:	Power state device is entering.
  */
 
-int suspend_device(struct device * dev, u32 state)
+int suspend_device(struct device * dev, enum system_state state)
 {
 	int error = 0;
 

@@ -70,7 +71,7 @@
  *
  */
 
-int device_suspend(u32 state)
+int device_suspend(enum system_state state)
 {
 	int error = 0;
 

@@ -112,7 +113,7 @@
  *	done, power down system devices.
  */
 
-int device_power_down(u32 state)
+int device_power_down(enum system_state state)
 {
 	int error = 0;
 	struct device * dev;
include/linux/pci.h renamed from linux/include/linux/pci.h +27 -1
@@ -18,6 +18,7 @@
 #define LINUX_PCI_H
 
 #include <linux/mod_devicetable.h>
+#include <linux/pci.h>
 
 /*
  * Under PCI, each device has 256 bytes of configuration address space,

@@ -637,7 +638,7 @@
 	const struct pci_device_id *id_table;	/* must be non-NULL for probe to be called */
 	int  (*probe)  (struct pci_dev *dev, const struct pci_device_id *id);	/* New device inserted */
 	void (*remove) (struct pci_dev *dev);	/* Device removed (NULL if not a hot-plug capable driver) */
-	int  (*suspend) (struct pci_dev *dev, u32 state);	/* Device suspended */
+	int  (*suspend) (struct pci_dev *dev, enum system_state reason);	/* Device suspended */
 	int  (*resume) (struct pci_dev *dev);	                /* Device woken up */
 	int  (*enable_wake) (struct pci_dev *dev, u32 state, int enable);   /* Enable wake event */
 

@@ -1021,5 +1022,30 @@
 #define PCIPCI_VSFX		16
 #define PCIPCI_ALIMAGIK		32
 
+enum pci_state {
+	PCI_D0 = 0,
+	PCI_D1 = 1,
+	PCI_D2 = 2,
+	PCI_D3hot = 3,
+	PCI_D3cold = 4
+};
+
+static inline enum pci_state to_pci_state(enum system_state state)
+{
+	switch (state) {
+	case PM_SUSPEND_ON:
+		return PCI_D0;
+	case PM_SUSPEND_STANDBY:
+		return PCI_D1;
+	case PM_SUSPEND_MEM:
+		return PCI_D3hot;
+	case PM_SUSPEND_DISK:
+		return PCI_D3cold;
+	default:
+		BUG();
+		return PCI_D0;
+	}
+}
+
 #endif /* __KERNEL__ */
 #endif /* LINUX_PCI_H */
include/linux/pm.h renamed from linux/include/linux/pm.h +10 -7
@@ -193,11 +193,11 @@
 extern void (*pm_idle)(void);
 extern void (*pm_power_off)(void);
 
-enum {
-	PM_SUSPEND_ON,
-	PM_SUSPEND_STANDBY,
-	PM_SUSPEND_MEM,
-	PM_SUSPEND_DISK,
+enum system_state {
+	PM_SUSPEND_ON = 0,
+	PM_SUSPEND_STANDBY = 1,
+	PM_SUSPEND_MEM = 2,
+	PM_SUSPEND_DISK = 3,
 	PM_SUSPEND_MAX,
 };
 

@@ -241,8 +240,11 @@
 
 extern void device_pm_set_parent(struct device * dev, struct device * parent);
 
-extern int device_suspend(u32 state);
-extern int device_power_down(u32 state);
+/*
+ * apply system suspend policy to all devices
+ */
+extern int device_suspend(enum system_state reason);
+extern int device_power_down(enum system_state reason);
 extern void device_power_up(void);
 extern void device_resume(void);

Pavel

Signature · 2 lines
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!
#21 lore raw
Quoted 4 lines · Pavel Machek

Ok, here's non-ugly patch. It may mean that ugly patch is comming in future (BenH would argue that), but it is probably best solution for now. Please apply,

ok ;) Let's run with that.

#22 lore raw
In reply to Pavel Machek

At Wed, 18 Aug 2004 08:26:01 +0200,

include/linux/pci.h line 22

Quoted 9 lines · Pavel Machek
include/linux/pci.h renamed from linux/include/linux/pci.h +1 -0
@@ -18,6 +18,7 @@
 #define LINUX_PCI_H
 
 #include <linux/mod_devicetable.h>
+#include <linux/pci.h>

I don't think it's necessary ;)

Takashi