Hi,
The following three patches are intended to start the redesign of the suspend

and hibernation framework for devices.
The first one is the 4th revision of the patch introducing new callbacks for

suspend and hibernation.
The other two patches implement the new suspend and hibernation callbacks

for the platform and PCI bus types.
Please review.
Thanks,

Rafael
--

From: Rafael J. Wysocki <rjw@sisk.pl>
Implement new suspend and hibernation callbacks for the PCI bus type.
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

---

 drivers/pci/pci-driver.c |  387 +++++++++++++++++++++++++++++++++++++++++------

 include/linux/pci.h      |    2

 2 files changed, 342 insertions(+), 47 deletions(-)
Index: linux-2.6/drivers/pci/pci-driver.c

===================================================================

--- linux-2.6.orig/drivers/pci/pci-driver.c

+++ linux-2.6/drivers/pci/pci-driver.c

@@ -271,45 +271,57 @@ static int pci_device_remove(struct devi

 	return 0;

 }
-static int pci_device_suspend(struct device * dev, pm_message_t state)

+static void pci_device_shutdown(struct device *dev)

 {

-	struct pci_dev * pci_dev = to_pci_dev(dev);

-	struct pci_driver * drv = pci_dev->driver;

-	int i = 0;

-

-	if (drv && drv->suspend) {

-		i = drv->suspend(pci_dev, state);

-		suspend_report_result(drv->suspend, i);

-	} else {

-		pci_save_state(pci_dev);

-		/*

-		 * mark its power state as "unknown", since we don't know if

-		 * e.g. the BIOS will change its device state when we suspend.

-		 */

-		if (pci_dev->current_state == PCI_D0)

-			pci_dev->current_state = PCI_UNKNOWN;

-	}

-	return i;

+	struct pci_dev *pci_dev = to_pci_dev(dev);

+	struct pci_driver *drv = pci_dev->driver;

+

+	if (drv && drv->shutdown)

+		drv->shutdown(pci_dev);

 }
-static int pci_device_suspend_late(struct device * dev, pm_message_t state)

+#ifdef CONFIG_PM_SLEEP

+static int pci_pm_prepare(struct device *dev)

 {

-	struct pci_dev * pci_dev = to_pci_dev(dev);

-	struct pci_driver * drv = pci_dev->driver;

-	int i = 0;

+	struct pci_dev *pci_dev = to_pci_dev(dev);

+	struct pci_driver *drv = pci_dev->driver;

+	int error = 0;
-	if (drv && drv->suspend_late) {

-		i = drv->suspend_late(pci_dev, state);

-		suspend_report_result(drv->suspend_late, i);

-	}

-	return i;

+	if (drv && drv-&gt...
[ message continues ]

This is the 4th revision of the patch.
The most important changes from the previous revision are:

* The _noirq methods are executed with dpm_list_mtx held, so they are forbidden

  to unregister any device.  This behavior has been introduced to prevent the

  problem in which, on a preemptible kernel, a thread concurrent to the suspend

  thread might be caught with dpm_list_mtx locked and in the process of

  modifying dpm_list when the suspend thread turns interrupts off.

* Devices for which one of the resume methods has returned an error are

  marked as "invalid" and the PM core doesn't attempt to execute any more

  suspend/hibernation callbacks for them.

* Some comments have been clarified (hopefully).
---

From: Rafael J. Wysocki <rjw@sisk.pl>
Introduce 'struct pm_ops' and 'struct pm_noirq_ops' representing

suspend and hibernation operations for bus types, device classes and

device types.
Modify the PM core to use 'struct pm_ops' and 'struct pm_noirq_ops'

objects, if defined, instead of the ->suspend() and ->resume() or,

respectively, ->suspend_late() and ->resume_early() callbacks that

will be considered as legacy and gradually phased out.
The main purpose of doing this is to separate suspend (aka S2RAM and

standby) callbacks from hibernation callbacks in such a way that the

new callbacks won't take arguments and the semantics of each of them

will be clearly specified.  This has been requested for multiple

times by many people, including Linus himself, and the reason is that

within the current scheme if ->resume() is called, for example, it's

difficult to say why it's been called (ie. is it a resume from RAM or

from hibernation or a suspend/hibernation failure etc.?).
The second purpose is to make the suspend/hibernation callbacks more

flexible so that device drivers can handle more than they can within

the current scheme.  For example, some drivers may need to prevent

new children of the device from being registered before their

->suspend() ca...
[ message continues ]

Unfortunately, I managed to introduce two bugs into the patch.  Namely, at two

places, dev->parent->power.status is dereferenced without checking if

dev->parent is not NULL.
Corrected patch follows.
---

From: Rafael J. Wysocki <rjw@sisk.pl>
Introduce 'struct pm_ops' and 'struct pm_noirq_ops' representing

suspend and hibernation operations for bus types, device classes and

device types.
Modify the PM core to use 'struct pm_ops' and 'struct pm_noirq_ops'

objects, if defined, instead of the ->suspend() and ->resume() or,

respectively, ->suspend_late() and ->resume_early() callbacks that

will be considered as legacy and gradually phased out.
The main purpose of doing this is to separate suspend (aka S2RAM and

standby) callbacks from hibernation callbacks in such a way that the

new callbacks won't take arguments and the semantics of each of them

will be clearly specified.  This has been requested for multiple

times by many people, including Linus himself, and the reason is that

within the current scheme if ->resume() is called, for example, it's

difficult to say why it's been called (ie. is it a resume from RAM or

from hibernation or a suspend/hibernation failure etc.?).
The second purpose is to make the suspend/hibernation callbacks more

flexible so that device drivers can handle more than they can within

the current scheme.  For example, some drivers may need to prevent

new children of the device from being registered before their

->suspend() callbacks are executed or they may want to carry out some

operations requiring the availability of some other devices, not

directly bound via the parent-child relationship, in order to prepare

for the execution of ->suspend(), etc.
Ultimately, we'd like to stop using the freezing of tasks for suspend

and therefore the drivers' suspend/hibernation code will have to take

care of the handling of the user space during suspend/hibernation.

That, in turn, would be difficult within the current scheme,...
[ message continues ]

You forgot to check for dev->parent->power.status == DPM_INVALID.
Alan Stern
--

Right, will fix.
Thanks,

Rafael

--

IMHO, no need to add _noirq in both struct and struct members.

pm_noirq->suspend_noirq does not look good...
--

I added the _noirq part to the names of the callbacks, since otherwise the

second struct looks confusingly similar to the first one.
I also could have put the _noirq callbacks into the first struct, but that

would have been wasteful, since device types and device classes don't

use them.  Also, the majority of bus types won't use them and the vast majority

of drivers won't use them as well.
Thanks,

Rafael

--

There is absolutely no point getting a second struct anymore.
BTW. I haven't had a chance to review the rest of the discussion on that

thread yet, been busy with other things, I'll try to go back to it today

or tomorrow.
Cheers,

Ben.
--

OK
Thanks,

Rafael

--

Well, what does it bring you ? Why can't it be one struct ? To save

space in the data area ? I don't think it makes things much cleaner. But

I won't fight a war for it, now that they are clearly named differently

and things like prepare/complete are no longer in "noirq", it's

semantically the same thing as having the fields in one structure, so

it's mostly cosmetic.
Ben.
--

Mostly, but not only that.
There are users of 'struct pm_ops' that aren't even supposed to define the

_noirq callbacks (device types and device classes), so I thought it would be
Well, I'm not going to fight for having the two separate stuctures either.

Also, it wouldn't be difficult to rearrange thigs so that all of the callbacks

are in one structure, so if other people think it's better to do it this way,

I'll go for it.
Thanks,

Rafael

--

Make sense... USB has no use of noirq for example.
Ben.
--

Well, FWIW, we can also do something like this:
struct pm_ops {

	int (*prepare)(struct device *dev);

	void (*complete)(struct device *dev);

	int (*suspend)(struct device *dev);

	int (*resume)(struct device *dev);

	int (*freeze)(struct device *dev);

	int (*thaw)(struct device *dev);

	int (*poweroff)(struct device *dev);

	int (*restore)(struct device *dev);

};
struct pm_ext_ops {

	struct pm_ops base;

	int (*suspend_noirq)(struct device *dev);

	int (*resume_noirq)(struct device *dev);

	int (*freeze_noirq)(struct device *dev);

	int (*thaw_noirq)(struct device *dev);

	int (*poweroff_noirq)(struct device *dev);

	int (*restore_noirq)(struct device *dev);

};
and use 'struct pm_ext_ops' for the entities that may need to implement the

_noirq callbacks.  This way we'll avoid the duplication of "_noirq" in the code

pointed to by Alex and there will be one "pm" pointer per bus type, device

type, device class, etc.
Thoughts?
Thanks,

Rafael

--

From: Rafael J. Wysocki <rjw@sisk.pl>
Implement new suspend and hibernation callbacks for the platform bus

type.
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

---

 drivers/base/platform.c         |  321 +++++++++++++++++++++++++++++++++++++---

 include/linux/platform_device.h |    2

 2 files changed, 302 insertions(+), 21 deletions(-)
Index: linux-2.6/include/linux/platform_device.h

===================================================================

--- linux-2.6.orig/include/linux/platform_device.h

+++ linux-2.6/include/linux/platform_device.h

@@ -53,6 +53,8 @@ struct platform_driver {

 	int (*suspend_late)(struct platform_device *, pm_message_t state);

 	int (*resume_early)(struct platform_device *);

 	int (*resume)(struct platform_device *);

+	struct pm_ops *pm;

+	struct pm_noirq_ops *pm_noirq;

 	struct device_driver driver;

 };
Index: linux-2.6/drivers/base/platform.c

===================================================================

--- linux-2.6.orig/drivers/base/platform.c

+++ linux-2.6/drivers/base/platform.c

@@ -560,61 +560,340 @@ static int platform_match(struct device

 	return (strncmp(pdev->name, drv->name, BUS_ID_SIZE) == 0);

 }
-static int platform_suspend(struct device *dev, pm_message_t mesg)

+#ifdef CONFIG_PM_SLEEP

+static int platform_pm_prepare(struct device *dev)

 {

+	struct platform_driver *drv;

 	int ret = 0;
-	if (dev->driver && dev->driver->suspend)

-		ret = dev->driver->suspend(dev, mesg);

+	if (!dev->driver)

+		return 0;

+

+	drv = to_platform_driver(dev->driver);

+	if (drv->pm && drv->pm->prepare)

+		ret = drv->pm->prepare(dev);

+

+	return ret;

+}

+

+static void platform_pm_complete(struct device *dev)

+{

+	struct platform_driver *drv;

+

+	if (!dev->driver)

+		return;

+

+	drv = to_platform_driver(dev->driver);

+	if (drv->pm && drv->pm->complete)

+		drv->pm->complete(dev);

+}

+

+#ifdef CONFIG_SUSPEND

+static int pla...
[ message continues ]