Messages in this thread Patch in this message | | | Date | Thu, 9 Jun 2005 02:04:02 +0200 | From | Pavel Machek <> | Subject | Re: [PATCH] fix tulip suspend/resume |
| |
Hi!
> > > I think we should also use the pm_message_t defines. We will need to > > > add PMSG_FREEZE eventually. I decided to default to the current state > > > rather than panic. Does this patch look ok? > > > > No. > > Hrm... I don't follow you anymore here ... > > > case PM_EVENT_ON: > > return PCI_D0; > > case PM_EVENT_FREEZE: > > case PM_EVENT_SUSPEND: > > return PCI_D3hot; > > What are these new PM_EVENT_* things ? I though we defined PMSG_* ?
PMSG_* are for struct; you can't case on struct.
> > You passed invalid argument; I see no reason why you should paper over > > it and risk continuing. This happens during system suspend; it is > > quite possible that user will not see your printk when machine powers > > off just after that; and remember that it will not be in syslog after > > resume. > > Crap. I don't think a BUG() makes any useful help neither in this place, > and when I locally turn PMSG_FREEZE to something sane I suddenly blow up > in there (and I wonder in how many other places).
At least you can see & report that error... That would not be a case for simple printk.
This is the patch I'd like to go in. I hope it makes it clear... Please base your development on top of this one... Pavel
---
Turn pm_message_t into struct, so that it is typechecked properly (and so that we can add flags field in future). This should not go in before 2.6.12.
Note: this rejects against -mm tree. Some rejects are caused by state_store getting more arguments, and some by added hook in pci_choose_state. I still do not like that pci_choose_state hook, it really should return pci_power_t, not int.
Signed-off-by: Pavel Machek <pavel@suse.cz>
--- commit a249072c4e0ef136c27c9e59d664e5be0d677ddc tree 24a21ff5302734d40e08c400b14c0c1624cceded parent f4bed68f59d9f32a4460288f40bb7f2af463babd author <pavel@amd.(none)> Tue, 31 May 2005 11:57:50 +0200 committer <pavel@amd.(none)> Tue, 31 May 2005 11:57:50 +0200
drivers/base/power/resume.c | 8 ++++---- drivers/base/power/runtime.c | 8 ++++---- drivers/base/power/suspend.c | 12 ++++++------ drivers/base/power/sysfs.c | 8 ++++---- drivers/ide/ide.c | 4 ++-- drivers/pci/pci.c | 14 +++++++------- drivers/serial/pmac_zilog.c | 2 +- drivers/usb/core/hub.c | 18 +++++++++--------- drivers/usb/core/usb.c | 2 +- drivers/usb/host/ehci-dbg.c | 2 +- drivers/usb/host/ohci-dbg.c | 2 +- drivers/usb/host/sl811-hcd.c | 6 +++--- drivers/video/aty/atyfb_base.c | 4 ++-- drivers/video/aty/radeon_pm.c | 12 ++++++------ drivers/video/i810/i810_main.c | 6 +++--- include/linux/pm.h | 14 ++++++++++---- 16 files changed, 64 insertions(+), 58 deletions(-)
Index: drivers/base/power/resume.c =================================================================== --- cdda23a10f60ce0fce85bd8b8667e7c7cf022118/drivers/base/power/resume.c (mode:100644) +++ 24a21ff5302734d40e08c400b14c0c1624cceded/drivers/base/power/resume.c (mode:100644) @@ -23,11 +23,11 @@ int resume_device(struct device * dev) { if (dev->power.pm_parent - && dev->power.pm_parent->power.power_state) { + && dev->power.pm_parent->power.power_state.event) { dev_err(dev, "PM: resume from %d, parent %s still %d\n", - dev->power.power_state, + dev->power.power_state.event, dev->power.pm_parent->bus_id, - dev->power.pm_parent->power.power_state); + dev->power.pm_parent->power.power_state.event); } if (dev->bus && dev->bus->resume) { dev_dbg(dev,"resuming\n"); @@ -50,7 +50,7 @@ list_add_tail(entry, &dpm_active); up(&dpm_list_sem); - if (!dev->power.prev_state) + if (!dev->power.prev_state.event) resume_device(dev); down(&dpm_list_sem); put_device(dev); Index: drivers/base/power/runtime.c =================================================================== --- cdda23a10f60ce0fce85bd8b8667e7c7cf022118/drivers/base/power/runtime.c (mode:100644) +++ 24a21ff5302734d40e08c400b14c0c1624cceded/drivers/base/power/runtime.c (mode:100644) @@ -13,10 +13,10 @@ static void runtime_resume(struct device * dev) { dev_dbg(dev, "resuming\n"); - if (!dev->power.power_state) + if (!dev->power.power_state.event) return; if (!resume_device(dev)) - dev->power.power_state = 0; + dev->power.power_state = PMSG_ON; } @@ -49,10 +49,10 @@ int error = 0; down(&dpm_sem); - if (dev->power.power_state == state) + if (dev->power.power_state.event == state.event) goto Done; - if (dev->power.power_state) + if (dev->power.power_state.event) runtime_resume(dev); if (!(error = suspend_device(dev, state))) Index: drivers/base/power/suspend.c =================================================================== --- cdda23a10f60ce0fce85bd8b8667e7c7cf022118/drivers/base/power/suspend.c (mode:100644) +++ 24a21ff5302734d40e08c400b14c0c1624cceded/drivers/base/power/suspend.c (mode:100644) @@ -39,22 +39,22 @@ { int error = 0; - if (dev->power.power_state) { + if (dev->power.power_state.event) { dev_dbg(dev, "PM: suspend %d-->%d\n", - dev->power.power_state, state); + dev->power.power_state.event, state.event); } if (dev->power.pm_parent - && dev->power.pm_parent->power.power_state) { + && dev->power.pm_parent->power.power_state.event) { dev_err(dev, "PM: suspend %d->%d, parent %s already %d\n", - dev->power.power_state, state, + dev->power.power_state.event, state.event, dev->power.pm_parent->bus_id, - dev->power.pm_parent->power.power_state); + dev->power.pm_parent->power.power_state.event); } dev->power.prev_state = dev->power.power_state; - if (dev->bus && dev->bus->suspend && !dev->power.power_state) { + if (dev->bus && dev->bus->suspend && !dev->power.power_state.event) { dev_dbg(dev, "suspending\n"); error = dev->bus->suspend(dev, state); } Index: drivers/base/power/sysfs.c =================================================================== --- cdda23a10f60ce0fce85bd8b8667e7c7cf022118/drivers/base/power/sysfs.c (mode:100644) +++ 24a21ff5302734d40e08c400b14c0c1624cceded/drivers/base/power/sysfs.c (mode:100644) @@ -26,19 +26,19 @@ static ssize_t state_show(struct device * dev, char * buf) { - return sprintf(buf, "%u\n", dev->power.power_state); + return sprintf(buf, "%u\n", dev->power.power_state.event); } static ssize_t state_store(struct device * dev, const char * buf, size_t n) { - u32 state; + pm_message_t state; char * rest; int error = 0; - state = simple_strtoul(buf, &rest, 10); + state.event = simple_strtoul(buf, &rest, 10); if (*rest) return -EINVAL; - if (state) + if (state.event) error = dpm_runtime_suspend(dev, state); else dpm_runtime_resume(dev); Index: drivers/ide/ide.c =================================================================== --- cdda23a10f60ce0fce85bd8b8667e7c7cf022118/drivers/ide/ide.c (mode:100644) +++ 24a21ff5302734d40e08c400b14c0c1624cceded/drivers/ide/ide.c (mode:100644) @@ -1385,7 +1385,7 @@ rq.special = &args; rq.pm = &rqpm; rqpm.pm_step = ide_pm_state_start_suspend; - rqpm.pm_state = state; + rqpm.pm_state = state.event; return ide_do_drive_cmd(drive, &rq, ide_wait); } @@ -1404,7 +1404,7 @@ rq.special = &args; rq.pm = &rqpm; rqpm.pm_step = ide_pm_state_start_resume; - rqpm.pm_state = 0; + rqpm.pm_state = PM_EVENT_ON; return ide_do_drive_cmd(drive, &rq, ide_head_wait); } Index: drivers/pci/pci.c =================================================================== --- cdda23a10f60ce0fce85bd8b8667e7c7cf022118/drivers/pci/pci.c (mode:100644) +++ 24a21ff5302734d40e08c400b14c0c1624cceded/drivers/pci/pci.c (mode:100644) @@ -316,14 +316,14 @@ pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t state) { - if (!pci_find_capability(dev, PCI_CAP_ID_PM)) + switch (state.event) { + case PM_EVENT_ON: return PCI_D0; - - switch (state) { - case 0: return PCI_D0; - case 3: return PCI_D3hot; - default: - printk("They asked me for state %d\n", state); + case PM_EVENT_FREEZE: + case PM_EVENT_SUSPEND: + return PCI_D3hot; + default: + printk("They asked me for state %d\n", state.event); BUG(); } return PCI_D0; Index: drivers/serial/pmac_zilog.c =================================================================== --- cdda23a10f60ce0fce85bd8b8667e7c7cf022118/drivers/serial/pmac_zilog.c (mode:100644) +++ 24a21ff5302734d40e08c400b14c0c1624cceded/drivers/serial/pmac_zilog.c (mode:100644) @@ -1601,7 +1601,7 @@ return 0; } - if (pm_state == mdev->ofdev.dev.power.power_state || pm_state < 2) + if (pm_state.event == mdev->ofdev.dev.power.power_state.event) return 0; pmz_debug("suspend, switching to state %d\n", pm_state); Index: drivers/usb/core/hub.c =================================================================== --- cdda23a10f60ce0fce85bd8b8667e7c7cf022118/drivers/usb/core/hub.c (mode:100644) +++ 24a21ff5302734d40e08c400b14c0c1624cceded/drivers/usb/core/hub.c (mode:100644) @@ -1564,7 +1564,7 @@ struct usb_driver *driver; intf = udev->actconfig->interface[i]; - if (state <= intf->dev.power.power_state) + if (state.event <= intf->dev.power.power_state.event) continue; if (!intf->dev.driver) continue; @@ -1572,11 +1572,11 @@ if (driver->suspend) { status = driver->suspend(intf, state); - if (intf->dev.power.power_state != state + if (intf->dev.power.power_state.event != state.event || status) dev_err(&intf->dev, "suspend %d fail, code %d\n", - state, status); + state.event, status); } /* only drivers with suspend() can ever resume(); @@ -1589,7 +1589,7 @@ * since we know every driver's probe/disconnect works * even for drivers that can't suspend. */ - if (!driver->suspend || state > PM_SUSPEND_MEM) { + if (!driver->suspend || state.event > PM_EVENT_FREEZE) { #if 1 dev_warn(&intf->dev, "resume is unsafe!\n"); #else @@ -1610,7 +1610,7 @@ * policies (when HNP doesn't apply) once we have mechanisms to * turn power back on! (Likely not before 2.7...) */ - if (state > PM_SUSPEND_MEM) { + if (state.event > PM_EVENT_FREEZE) { dev_warn(&udev->dev, "no poweroff yet, suspending instead\n"); } @@ -1727,7 +1727,7 @@ struct usb_driver *driver; intf = udev->actconfig->interface[i]; - if (intf->dev.power.power_state == PMSG_SUSPEND) + if (intf->dev.power.power_state.event == PM_EVENT_ON) continue; if (!intf->dev.driver) { /* FIXME maybe force to alt 0 */ @@ -1741,11 +1741,11 @@ /* can we do better than just logging errors? */ status = driver->resume(intf); - if (intf->dev.power.power_state != PMSG_ON + if (intf->dev.power.power_state.event != PM_EVENT_ON || status) dev_dbg(&intf->dev, "resume fail, state %d code %d\n", - intf->dev.power.power_state, status); + intf->dev.power.power_state.event, status); } status = 0; @@ -1928,7 +1928,7 @@ unsigned port1; int status; - if (intf->dev.power.power_state == PM_SUSPEND_ON) + if (intf->dev.power.power_state.event == PM_EVENT_ON) return 0; for (port1 = 1; port1 <= hdev->maxchild; port1++) { Index: drivers/usb/core/usb.c =================================================================== --- cdda23a10f60ce0fce85bd8b8667e7c7cf022118/drivers/usb/core/usb.c (mode:100644) +++ 24a21ff5302734d40e08c400b14c0c1624cceded/drivers/usb/core/usb.c (mode:100644) @@ -1389,7 +1389,7 @@ driver = to_usb_driver(dev->driver); /* there's only one USB suspend state */ - if (intf->dev.power.power_state) + if (intf->dev.power.power_state.event) return 0; if (driver->suspend) Index: drivers/usb/host/ehci-dbg.c =================================================================== --- cdda23a10f60ce0fce85bd8b8667e7c7cf022118/drivers/usb/host/ehci-dbg.c (mode:100644) +++ 24a21ff5302734d40e08c400b14c0c1624cceded/drivers/usb/host/ehci-dbg.c (mode:100644) @@ -641,7 +641,7 @@ spin_lock_irqsave (&ehci->lock, flags); - if (bus->controller->power.power_state) { + if (bus->controller->power.power_state.event) { size = scnprintf (next, size, "bus %s, device %s (driver " DRIVER_VERSION ")\n" "SUSPENDED (no register access)\n", Index: drivers/usb/host/ohci-dbg.c =================================================================== --- cdda23a10f60ce0fce85bd8b8667e7c7cf022118/drivers/usb/host/ohci-dbg.c (mode:100644) +++ 24a21ff5302734d40e08c400b14c0c1624cceded/drivers/usb/host/ohci-dbg.c (mode:100644) @@ -631,7 +631,7 @@ hcd->product_desc, hcd_name); - if (bus->controller->power.power_state) { + if (bus->controller->power.power_state.event) { size -= scnprintf (next, size, "SUSPENDED (no register access)\n"); goto done; Index: drivers/usb/host/sl811-hcd.c =================================================================== --- cdda23a10f60ce0fce85bd8b8667e7c7cf022118/drivers/usb/host/sl811-hcd.c (mode:100644) +++ 24a21ff5302734d40e08c400b14c0c1624cceded/drivers/usb/host/sl811-hcd.c (mode:100644) @@ -1781,9 +1781,9 @@ if (phase != SUSPEND_POWER_DOWN) return retval; - if (state <= PM_SUSPEND_MEM) + if (state.event == PM_EVENT_FREEZE) retval = sl811h_hub_suspend(hcd); - else + else if (state.event == PM_EVENT_SUSPEND) port_power(sl811, 0); if (retval == 0) dev->power.power_state = state; @@ -1802,7 +1802,7 @@ /* with no "check to see if VBUS is still powered" board hook, * let's assume it'd only be powered to enable remote wakeup. */ - if (dev->power.power_state > PM_SUSPEND_MEM + if (dev->power.power_state.event == PM_EVENT_SUSPEND || !hcd->can_wakeup) { sl811->port1 = 0; port_power(sl811, 1); Index: drivers/video/aty/atyfb_base.c =================================================================== --- cdda23a10f60ce0fce85bd8b8667e7c7cf022118/drivers/video/aty/atyfb_base.c (mode:100644) +++ 24a21ff5302734d40e08c400b14c0c1624cceded/drivers/video/aty/atyfb_base.c (mode:100644) @@ -2071,12 +2071,12 @@ struct fb_info *info = pci_get_drvdata(pdev); struct atyfb_par *par = (struct atyfb_par *) info->par; - if (pdev->dev.power.power_state == 0) + if (pdev->dev.power.power_state.event == PM_EVENT_ON) return 0; acquire_console_sem(); - if (pdev->dev.power.power_state == 2) + if (pdev->dev.power.power_state.event == 2) aty_power_mgmt(0, par); par->asleep = 0; Index: drivers/video/aty/radeon_pm.c =================================================================== --- cdda23a10f60ce0fce85bd8b8667e7c7cf022118/drivers/video/aty/radeon_pm.c (mode:100644) +++ 24a21ff5302734d40e08c400b14c0c1624cceded/drivers/video/aty/radeon_pm.c (mode:100644) @@ -2526,18 +2526,18 @@ struct radeonfb_info *rinfo = info->par; int i; - if (state == pdev->dev.power.power_state) + if (state.event == pdev->dev.power.power_state.event) return 0; printk(KERN_DEBUG "radeonfb (%s): suspending to state: %d...\n", - pci_name(pdev), state); + pci_name(pdev), state.event); /* For suspend-to-disk, we cheat here. We don't suspend anything and * let fbcon continue drawing until we are all set. That shouldn't * really cause any problem at this point, provided that the wakeup * code knows that any state in memory may not match the HW */ - if (state != PM_SUSPEND_MEM) + if (state.event == PM_EVENT_FREEZE) goto done; acquire_console_sem(); @@ -2616,7 +2616,7 @@ struct radeonfb_info *rinfo = info->par; int rc = 0; - if (pdev->dev.power.power_state == 0) + if (pdev->dev.power.power_state.event == PM_EVENT_ON) return 0; if (rinfo->no_schedule) { @@ -2626,7 +2626,7 @@ acquire_console_sem(); printk(KERN_DEBUG "radeonfb (%s): resuming from state: %d...\n", - pci_name(pdev), pdev->dev.power.power_state); + pci_name(pdev), pdev->dev.power.power_state.event); if (pci_enable_device(pdev)) { @@ -2637,7 +2637,7 @@ } pci_set_master(pdev); - if (pdev->dev.power.power_state == PM_SUSPEND_MEM) { + if (pdev->dev.power.power_state.event == PM_EVENT_SUSPEND) { /* Wakeup chip. Check from config space if we were powered off * (todo: additionally, check CLK_PIN_CNTL too) */ Index: drivers/video/i810/i810_main.c =================================================================== --- cdda23a10f60ce0fce85bd8b8667e7c7cf022118/drivers/video/i810/i810_main.c (mode:100644) +++ 24a21ff5302734d40e08c400b14c0c1624cceded/drivers/video/i810/i810_main.c (mode:100644) @@ -1506,12 +1506,12 @@ struct i810fb_par *par = (struct i810fb_par *) info->par; int blank = 0, prev_state = par->cur_state; - if (state == prev_state) + if (state.event == prev_state) return 0; - par->cur_state = state; + par->cur_state = state.event; - switch (state) { + switch (state.event) { case 1: blank = VESA_VSYNC_SUSPEND; break; Index: include/linux/pm.h =================================================================== --- cdda23a10f60ce0fce85bd8b8667e7c7cf022118/include/linux/pm.h (mode:100644) +++ 24a21ff5302734d40e08c400b14c0c1624cceded/include/linux/pm.h (mode:100644) @@ -185,7 +185,9 @@ struct device; -typedef u32 __bitwise pm_message_t; +typedef struct pm_message { + int event; +} pm_message_t; /* * There are 4 important states driver can be in: @@ -205,9 +207,13 @@ * or something similar soon. */ -#define PMSG_FREEZE ((__force pm_message_t) 3) -#define PMSG_SUSPEND ((__force pm_message_t) 3) -#define PMSG_ON ((__force pm_message_t) 0) +#define PM_EVENT_ON 0 +#define PM_EVENT_FREEZE 1 +#define PM_EVENT_SUSPEND 2 + +#define PMSG_FREEZE ({struct pm_message m; m.event = PM_EVENT_FREEZE; m; }) +#define PMSG_SUSPEND ({struct pm_message m; m.event = PM_EVENT_SUSPEND; m; }) +#define PMSG_ON ({struct pm_message m; m.event = PM_EVENT_ON; m; }) struct dev_pm_info { pm_message_t power_state; - 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/
| |