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

[Pavel Machek <pavel@suse.cz>]

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!