[patch] enums to clear suspend-state confusion

[patch] enums to clear suspend-state confusion

[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,
							Pavel


--- linux-mm/drivers/base/power/power.h	2004-07-28 21:29:22.000000000 +0200
+++ linux-delme/drivers/base/power/power.h	2004-08-12 13:41:12.000000000 +0200
@@ -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;
 }
--- linux-mm/drivers/base/power/shutdown.c	2004-07-28 21:29:22.000000000 +0200
+++ linux-delme/drivers/base/power/shutdown.c	2004-08-12 13:42:10.000000000 +0200
@@ -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);
 }
 
--- linux-mm/drivers/base/power/suspend.c	2004-07-28 21:29:22.000000000 +0200
+++ linux-delme/drivers/base/power/suspend.c	2004-08-12 13:43:08.000000000 +0200
@@ -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;
--- linux-mm/drivers/ide/ide-disk.c	2004-07-28 22:43:29.000000000 +0200
+++ linux-delme/drivers/ide/ide-disk.c	2004-08-12 13:41:12.000000000 +0200
@@ -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
--- linux-mm/include/linux/pci.h	2004-07-28 22:43:31.000000000 +0200
+++ linux-delme/include/linux/pci.h	2004-08-12 13:41:12.000000000 +0200
@@ -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 */
--- linux-mm/include/linux/pm.h	2004-07-28 21:29:32.000000000 +0200
+++ linux-delme/include/linux/pm.h	2004-08-12 13:41:12.000000000 +0200
@@ -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);
 

-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: [patch] enums to clear suspend-state confusion

[Andrew Morton]

Pavel Machek <pavel@ucw.cz> wrote:
>
> +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.

Re: [patch] enums to clear suspend-state confusion

[Pavel Machek]

Hi!

> > +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

--- linux-forakpm/include/linux/pci.h	2004-08-15 19:35:41.000000000 +0200
+++ linux/include/linux/pci.h	2004-08-16 08:15:59.000000000 +0200
@@ -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();
 }
 

-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: [patch] enums to clear suspend-state confusion

[Takashi Iwai]

At Thu, 12 Aug 2004 14:02:21 +0200,
Pavel Machek wrote:
> 
> 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)
> --- linux-mm/include/linux/pci.h	2004-07-28 22:43:31.000000000 +0200
> +++ linux-delme/include/linux/pci.h	2004-08-12 13:41:12.000000000 +0200
> @@ -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

Re: [patch] enums to clear suspend-state confusion

[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,

> > --- linux-mm/include/linux/pci.h	2004-07-28 22:43:31.000000000 +0200
> > +++ linux-delme/include/linux/pci.h	2004-08-12 13:41:12.000000000 +0200
> > @@ -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
-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms         

Re: [patch] enums to clear suspend-state confusion

[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,

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

--- tmp/linux/drivers/base/power/power.h	2004-08-15 19:14:55.000000000 +0200
+++ linux/drivers/base/power/power.h	2004-08-15 19:15:49.000000000 +0200
@@ -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;
 }
--- tmp/linux/drivers/base/power/shutdown.c	2004-08-15 19:14:55.000000000 +0200
+++ linux/drivers/base/power/shutdown.c	2004-08-17 12:45:39.000000000 +0200
@@ -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);
 }
 
 
--- tmp/linux/drivers/base/power/suspend.c	2004-08-15 19:14:55.000000000 +0200
+++ linux/drivers/base/power/suspend.c	2004-08-17 23:20:28.000000000 +0200
@@ -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;
--- 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
@@ -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 */
--- 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
@@ -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);
 
--- tmp/linux/kernel/power/console.c	2004-02-20 12:29:52.000000000 +0100
+++ linux/kernel/power/console.c	2004-06-03 00:27:20.000000000 +0200
@@ -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;

-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: [patch] enums to clear suspend-state confusion

[Andrew Morton]

Pavel Machek <pavel@ucw.cz> wrote:
>
> 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,
> 
> 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.

> +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;
> +
> +#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;
	...
}

Re: [patch] enums to clear suspend-state confusion

[Pavel Machek]

Hi!


> > I'd like this to be applied, so I can start fixing the drivers...
> 
> Sure, let's try to get this done.
> 
> > +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;
> > +
> > +#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 :-(.

> 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.

> 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
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: [patch] enums to clear suspend-state confusion

[Andrew Morton]

Pavel Machek <pavel@ucw.cz> wrote:
>
> > 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.

> > 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).

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 ;)

Re: [patch] enums to clear suspend-state confusion

[Pavel Machek]

Hi!

> > 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
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: [patch] enums to clear suspend-state confusion

[Benjamin Herrenschmidt]

> 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.

Re: [patch] enums to clear suspend-state confusion

[Pavel Machek]

Hi!

> > 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.

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,

-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: [patch] enums to clear suspend-state confusion

[Pavel Machek]

Hi!

> > > 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).
> 
> 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

--- tmp/linux/drivers/base/power/power.h	2004-08-15 19:14:55.000000000 +0200
+++ linux/drivers/base/power/power.h	2004-08-15 19:15:49.000000000 +0200
@@ -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;
 }
--- tmp/linux/drivers/base/power/suspend.c	2004-08-15 19:14:55.000000000 +0200
+++ linux/drivers/base/power/suspend.c	2004-08-17 23:20:28.000000000 +0200
@@ -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;
--- tmp/linux/include/linux/pci.h	2004-08-15 19:15:05.000000000 +0200
+++ linux/include/linux/pci.h	2004-08-18 08:17:21.000000000 +0200
@@ -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 */
--- tmp/linux/include/linux/pm.h	2004-08-15 19:15:05.000000000 +0200
+++ linux/include/linux/pm.h	2004-08-18 08:14:45.000000000 +0200
@@ -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
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: [patch] enums to clear suspend-state confusion

[Andrew Morton]

Pavel Machek <pavel@ucw.cz> wrote:
>
>  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.

Re: [patch] enums to clear suspend-state confusion

[Benjamin Herrenschmidt]

> 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.

Re: [patch] enums to clear suspend-state confusion

[Takashi Iwai]

At Wed, 18 Aug 2004 08:26:01 +0200,
Pavel Machek wrote:
> 
> --- tmp/linux/include/linux/pci.h	2004-08-15 19:15:05.000000000 +0200
> +++ linux/include/linux/pci.h	2004-08-18 08:17:21.000000000 +0200
> @@ -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

Re: [patch] enums to clear suspend-state confusion

[Pavel Machek]

Hi!

> > 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
-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms         

Re: [patch] enums to clear suspend-state confusion

[Patrick Mochel]

On Wed, 18 Aug 2004, Benjamin Herrenschmidt wrote:

>
> > 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

Re: [patch] enums to clear suspend-state confusion

[David Brownell]

[-- Attachment #1: Type: text/plain, Size: 2036 bytes --]

On Tuesday 17 August 2004 11:55 pm, Benjamin Herrenschmidt wrote:
> 
> > 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


[-- Attachment #2: pmcore-0818.patch --]
[-- Type: text/x-diff, Size: 5163 bytes --]

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.


--- 1.16/include/linux/pm.h	Thu Jul  1 22:23:53 2004
+++ edited/include/linux/pm.h	Wed Aug 18 08:10:49 2004
@@ -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__ */
 
--- 1.7/drivers/base/power/power.h	Wed Jun  9 23:34:24 2004
+++ edited/drivers/base/power/power.h	Wed Aug 18 07:48:04 2004
@@ -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;
 }
--- 1.32/drivers/base/power/shutdown.c	Wed Jun  9 23:34:24 2004
+++ edited/drivers/base/power/shutdown.c	Wed Aug 18 07:48:04 2004
@@ -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);
 }
 
 
--- 1.16/drivers/base/power/suspend.c	Wed Jun  9 23:34:24 2004
+++ edited/drivers/base/power/suspend.c	Wed Aug 18 07:48:04 2004
@@ -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;
--- 1.86/kernel/power/swsusp.c	Thu Jul  1 22:23:48 2004
+++ edited/kernel/power/swsusp.c	Wed Aug 18 07:48:04 2004
@@ -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");
 

Re: [patch] enums to clear suspend-state confusion

[Alan Cox]

On Mer, 2004-08-18 at 01:27, Pavel Machek wrote:
> 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.

Re: [patch] enums to clear suspend-state confusion

[David Brownell]

On Wednesday 18 August 2004 10:31 am, Alan Cox wrote:
>    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.

Re: [patch] enums to clear suspend-state confusion

[Pavel Machek]

Hi!

> > 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.

> 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
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: [patch] enums to clear suspend-state confusion

[Pavel Machek]

Hi!

> 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.

That would not provide enough info for the driver. 

> 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:

--- clean/drivers/net/e100.c	2004-06-22 12:36:10.000000000 +0200
+++ linux/drivers/net/e100.c	2004-08-18 08:21:23.000000000 +0200
@@ -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;
 }

> +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:

> --- 1.86/kernel/power/swsusp.c	Thu Jul  1 22:23:48 2004
> +++ edited/kernel/power/swsusp.c	Wed Aug 18 07:48:04 2004
> @@ -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
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: [patch] enums to clear suspend-state confusion

[Len Brown]

Pavel,
Can you provide an example where the enum patch
causes gcc to generate a warning for incorrect code?

When I drop the wrong enum into a function,
gcc seems to eat it just as happily as when
we used u32's.  Maybe I'm missing something.

thanks,
-Len

Re: [patch] enums to clear suspend-state confusion

[Pavel Machek]

Hi!

> Can you provide an example where the enum patch
> causes gcc to generate a warning for incorrect code?
> 
> When I drop the wrong enum into a function,
> gcc seems to eat it just as happily as when
> we used u32's.  Maybe I'm missing something.

It will not provide a warning, not for now. Idea was that eventually
sparse could warn or so.

Even with that, it makes it pretty clear for user what is going on,
and hopefully people will actually think before assigning variables
from different enum types.
								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!