Re: Solving suspend-level confusion
From: Benjamin Herrenschmidt (benh_at_kernel.crashing.org)
Date: 07/31/04
- Previous message: Johannes Stezenbach: "Re: [PATCH] modpost warnings with external modules w/o modversions"
- In reply to: Pavel Machek: "Solving suspend-level confusion"
- Next in thread: Pavel Machek: "Re: Solving suspend-level confusion"
- Reply: Pavel Machek: "Re: Solving suspend-level confusion"
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
To: Pavel Machek <pavel@suse.cz> Date: Sat, 31 Jul 2004 08:39:30 +1000
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@kernel.crashing.org> - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
- Previous message: Johannes Stezenbach: "Re: [PATCH] modpost warnings with external modules w/o modversions"
- In reply to: Pavel Machek: "Solving suspend-level confusion"
- Next in thread: Pavel Machek: "Re: Solving suspend-level confusion"
- Reply: Pavel Machek: "Re: Solving suspend-level confusion"
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
Relevant Pages
|
|
