Re: Solving suspend-level confusion

[Benjamin Herrenschmidt]

On Sat, 2004-07-31 at 02:44, Pavel Machek wrote:
> Hi!
> 
> There's quite big confusion what 'u32 state' in suspend callbacks
> mean. Half code interprets it as a system-wide suspend level, and
> second half thinks it is PCI Dx state. Bad.
> 
> What about this solution:
> 
> * system-wide suspend level is always passed down (it is more
> detailed, and for example IDE driver cares)
> 
> * if you want to derive Dx state, just use provided inline function to
> convert.
> 
> That should be acceptable to everyone. I plan on explicitly using
> enums to prevent further confusion. Patch is likely to be pretty big:
> ideally all prototypes of suspend function would be fixed so noone can
> be confused.
> 
> What do you think?

Your constants are ugly ;) But the whole idea makes sense, I've started
implementing something on my side, though I didn't change the u32 to an
enum to avoid having to "fix" bazillions of drivers. Proper
documentation may just be enough...
								Pavel
> PS: I'll be on holidays for a week, feel free to implement this or
> something similar.. it is going to be lot of search/replace in drivers
> :-(.

I'll have some patches soon along with the PPC stuff.


> --- tmp/linux/include/linux/pci.h	2004-06-22 12:36:45.000000000 +0200
> +++ linux/include/linux/pci.h	2004-07-30 18:24:02.000000000 +0200
> @@ -593,7 +593,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 suspend_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 */
>  
> --- tmp/linux/include/linux/pm.h	2004-06-22 12:36:45.000000000 +0200
> +++ linux/include/linux/pm.h	2004-07-30 18:23:11.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,11 +240,47 @@
>  
>  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);
> +enum suspend_state {
> +	SNAPSHOT = 10,		/* For debugging, symbolic constants should be everywhere */
> +	POWERDOWN,
> +	RESTART,
> +	RUNTIME_D1,
> +	RUNTIME_D2,
> +	RUNTIME_D3hot,
> +	RUNTIME_D3cold
> +};
> +
> +extern int device_suspend(enum suspend_state reason);
> +extern int device_power_down(enum suspend_state reason);
>  extern void device_power_up(void);
>  extern void device_resume(void);
>  
> +enum pci_state {
> +	D0 = 20,		/* For debugging, symbolic constants should be everywhere */
> +	D1,
> +	D2,
> +	D3hot,
> +	D3cold
> +};
> +
> +static inline enum pci_state to_pci_state(enum suspend_state state)
> +{
> +	switch(state) {
> +	case RUNTIME_D1:
> +		return D1;
> +	case RUNTIME_D2:
> +		return D2;
> +	case RUNTIME_D3hot:
> +		return D3hot;
> +	case SNAPSHOT:
> +	case POWERDOWN:
> +	case RESTART:
> +	case RUNTIME_D3cold:
> +		return D3cold;
> +	default:
> +		BUG();
> +	}
> +}
>  
>  #endif /* __KERNEL__ */
>  
> --- tmp/linux/kernel/power/disk.c	2004-05-20 23:08:36.000000000 +0200
> +++ linux/kernel/power/disk.c	2004-07-30 18:18:04.000000000 +0200
> @@ -46,20 +46,25 @@
>  	int error = 0;
>  
>  	local_irq_save(flags);
> -	device_power_down(PM_SUSPEND_DISK);
>  	switch(mode) {
>  	case PM_DISK_PLATFORM:
> +		device_power_down(POWERDOWN);
>  		error = pm_ops->enter(PM_SUSPEND_DISK);
>  		break;
>  	case PM_DISK_SHUTDOWN:
> +		device_power_down(POWERDOWN);
>  		printk("Powering off system\n");
>  		machine_power_off();
>  		break;
>  	case PM_DISK_REBOOT:
> +		device_power_down(RESTART);
>  		machine_restart(NULL);
>  		break;
>  	}
>  	machine_halt();
> +	/* Valid image is on the disk, if we continue we risk serious data corruption
> +	   after resume. */
> +	while(1);
>  	device_power_up();
>  	local_irq_restore(flags);
>  	return 0;
-- 
Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx>

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/