Messages in this thread Patch in this message | | | Date | Wed, 18 Aug 2004 22:47:35 +0200 | From | Pavel Machek <> | Subject | Re: [patch] enums to clear suspend-state confusion |
| |
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! - 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/
| |