[RFC][PATCH 0/2] PM: Remove two fields from dev_pm_info

[RFC][PATCH 0/2] PM: Remove two fields from dev_pm_info

[Rafael J. Wysocki]

Hi,

There are two fields in dev_pm_info, pm_parent and saved_state that appear to
be unnecessary, so I think we can remove them.

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

[RFC][PATCH 1/2] PM: Remove pm_parent from dev_pm_info

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

The pm_parent member of struct dev_pm_info (defined in include/linux/pm.h) is
only used to check if the device's parent is in the right state while the
device is being suspended or resumed.  However, this can be done just as well
with the help of the parent pointer in struct device, so pm_parent can be
removed along with some code that handles it.

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

 drivers/base/power/main.c    |   36 ++++++++++--------------------------
 drivers/base/power/resume.c  |    7 +++----
 drivers/base/power/suspend.c |    7 +++----
 include/linux/pm.h           |    3 ---
 4 files changed, 16 insertions(+), 37 deletions(-)

Index: linux-2.6.22-rc4/drivers/base/power/main.c
===================================================================
--- linux-2.6.22-rc4.orig/drivers/base/power/main.c	2007-06-08 13:09:16.000000000 +0200
+++ linux-2.6.22-rc4/drivers/base/power/main.c	2007-06-09 22:19:44.000000000 +0200
@@ -31,28 +31,7 @@ DEFINE_MUTEX(dpm_list_mtx);
 
 int (*platform_enable_wakeup)(struct device *dev, int is_on);
 
-
-/**
- *	device_pm_set_parent - Specify power dependency.
- *	@dev:		Device who needs power.
- *	@parent:	Device that supplies power.
- *
- *	This function is used to manually describe a power-dependency
- *	relationship. It may be used to specify a transversal relationship
- *	(where the power supplier is not the physical (or electrical)
- *	ancestor of a specific device.
- *	The effect of this is that the supplier will not be powered down
- *	before the power dependent.
- */
-
-void device_pm_set_parent(struct device * dev, struct device * parent)
-{
-	put_device(dev->power.pm_parent);
-	dev->power.pm_parent = get_device(parent);
-}
-EXPORT_SYMBOL_GPL(device_pm_set_parent);
-
-int device_pm_add(struct device * dev)
+int device_pm_add(struct device *dev)
 {
 	int error;
 
@@ -61,21 +40,26 @@ int device_pm_add(struct device * dev)
 		 kobject_name(&dev->kobj));
 	mutex_lock(&dpm_list_mtx);
 	list_add_tail(&dev->power.entry, &dpm_active);
-	device_pm_set_parent(dev, dev->parent);
-	if ((error = dpm_sysfs_add(dev)))
+	/*
+	 * The device's parent must not be released until the device itself is
+	 * removed from the dpm_active list.
+	 */
+	get_device(dev->parent);
+	error = dpm_sysfs_add(dev);
+	if (error)
 		list_del(&dev->power.entry);
 	mutex_unlock(&dpm_list_mtx);
 	return error;
 }
 
-void device_pm_remove(struct device * dev)
+void device_pm_remove(struct device *dev)
 {
 	pr_debug("PM: Removing info for %s:%s\n",
 		 dev->bus ? dev->bus->name : "No Bus",
 		 kobject_name(&dev->kobj));
 	mutex_lock(&dpm_list_mtx);
 	dpm_sysfs_remove(dev);
-	put_device(dev->power.pm_parent);
+	put_device(dev->parent);
 	list_del_init(&dev->power.entry);
 	mutex_unlock(&dpm_list_mtx);
 }
Index: linux-2.6.22-rc4/drivers/base/power/resume.c
===================================================================
--- linux-2.6.22-rc4.orig/drivers/base/power/resume.c	2007-06-08 13:09:16.000000000 +0200
+++ linux-2.6.22-rc4/drivers/base/power/resume.c	2007-06-09 21:12:43.000000000 +0200
@@ -29,12 +29,11 @@ int resume_device(struct device * dev)
 
 	down(&dev->sem);
 
-	if (dev->power.pm_parent
-			&& dev->power.pm_parent->power.power_state.event) {
+	if (dev->parent && dev->parent->power.power_state.event) {
 		dev_err(dev, "PM: resume from %d, parent %s still %d\n",
 			dev->power.power_state.event,
-			dev->power.pm_parent->bus_id,
-			dev->power.pm_parent->power.power_state.event);
+			dev->parent->bus_id,
+			dev->parent->power.power_state.event);
 	}
 
 	if (dev->bus && dev->bus->resume) {
Index: linux-2.6.22-rc4/drivers/base/power/suspend.c
===================================================================
--- linux-2.6.22-rc4.orig/drivers/base/power/suspend.c	2007-06-08 13:09:16.000000000 +0200
+++ linux-2.6.22-rc4/drivers/base/power/suspend.c	2007-06-09 21:12:40.000000000 +0200
@@ -55,13 +55,12 @@ int suspend_device(struct device * dev, 
 		dev_dbg(dev, "PM: suspend %d-->%d\n",
 			dev->power.power_state.event, state.event);
 	}
-	if (dev->power.pm_parent
-			&& dev->power.pm_parent->power.power_state.event) {
+	if (dev->parent && dev->parent->power.power_state.event) {
 		dev_err(dev,
 			"PM: suspend %d->%d, parent %s already %d\n",
 			dev->power.power_state.event, state.event,
-			dev->power.pm_parent->bus_id,
-			dev->power.pm_parent->power.power_state.event);
+			dev->parent->bus_id,
+			dev->parent->power.power_state.event);
 	}
 
 	dev->power.prev_state = dev->power.power_state;
Index: linux-2.6.22-rc4/include/linux/pm.h
===================================================================
--- linux-2.6.22-rc4.orig/include/linux/pm.h	2007-06-08 13:09:17.000000000 +0200
+++ linux-2.6.22-rc4/include/linux/pm.h	2007-06-09 21:12:45.000000000 +0200
@@ -237,13 +237,10 @@ struct dev_pm_info {
 	unsigned		should_wakeup:1;
 	pm_message_t		prev_state;
 	void			* saved_state;
-	struct device		* pm_parent;
 	struct list_head	entry;
 #endif
 };
 
-extern void device_pm_set_parent(struct device * dev, struct device * parent);
-
 extern int device_power_down(pm_message_t state);
 extern void device_power_up(void);
 extern void device_resume(void);

[RFC][PATCH 2/2] PM: Remove saved_state from dev_pm_info

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

The saved_state member of struct dev_pm_info, defined in include/linux/pm.h, is
not used anywhere, so it can be removed.

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

 include/linux/pm.h |    1 -
 1 file changed, 1 deletion(-)

Index: linux-2.6.22-rc4/include/linux/pm.h
===================================================================
--- linux-2.6.22-rc4.orig/include/linux/pm.h	2007-06-09 21:12:45.000000000 +0200
+++ linux-2.6.22-rc4/include/linux/pm.h	2007-06-09 22:26:13.000000000 +0200
@@ -236,7 +236,6 @@ struct dev_pm_info {
 #ifdef	CONFIG_PM
 	unsigned		should_wakeup:1;
 	pm_message_t		prev_state;
-	void			* saved_state;
 	struct list_head	entry;
 #endif
 };

Re: [RFC][PATCH 2/2] PM: Remove saved_state from dev_pm_info

[Alan Stern]

On Sat, 9 Jun 2007, Rafael J. Wysocki wrote:

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> The saved_state member of struct dev_pm_info, defined in include/linux/pm.h, is
> not used anywhere, so it can be removed.

Along the same lines, feature_removal_schedule.txt says that 
dev->power.power_state will be removed next month.  In preparation, 
you could consider removing the prev_state member now.  As far as I 
know, it isn't used for anything other than avoiding resume method 
calls to devices that were already suspended when a system sleep 
began.

I suggest that suspend and resume always be called for every device
during a system sleep transition, regardless of the device's state.  
That will have to done anyway once dev->power.power_state is gone.  
The idea is that drivers should regard these method calls as
notifications that the system is about to go to sleep or has just woken
up, rather than as directives to put their device into a particular
state.

Thus, if a driver knows that its device was already in a low-power
state before the system sleep then it need not respond to a resume
method call by switching the device to a high-power state.  Instead the
driver should be free to take whatever action it thinks is appropriate.
This is already explained in Documentation/power/devices.txt.

Alan Stern

Re: [RFC][PATCH 2/2] PM: Remove saved_state from dev_pm_info

[Rafael J. Wysocki]

On Sunday, 10 June 2007 16:32, Alan Stern wrote:
> On Sat, 9 Jun 2007, Rafael J. Wysocki wrote:
> 
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > The saved_state member of struct dev_pm_info, defined in include/linux/pm.h, is
> > not used anywhere, so it can be removed.
> 
> Along the same lines, feature_removal_schedule.txt says that 
> dev->power.power_state will be removed next month.  In preparation, 
> you could consider removing the prev_state member now.  As far as I 
> know, it isn't used for anything other than avoiding resume method 
> calls to devices that were already suspended when a system sleep 
> began.

Yes, I considered that too, but I decided to leave if for now in case something
depended on it.

I'll take another look. ;-)

> I suggest that suspend and resume always be called for every device
> during a system sleep transition, regardless of the device's state.

That's reasonable, I think.

> That will have to done anyway once dev->power.power_state is gone.

Besides, that would fix the issue with the current code that if
dev->class->suspend() changes dev->power.power_state.event  to 'off',
then dev->bus->suspend() is not called, but dev->bus->resume() will be called
during the subsequent resume.

> The idea is that drivers should regard these method calls as
> notifications that the system is about to go to sleep or has just woken
> up, rather than as directives to put their device into a particular
> state.
> 
> Thus, if a driver knows that its device was already in a low-power
> state before the system sleep then it need not respond to a resume
> method call by switching the device to a high-power state.  Instead the
> driver should be free to take whatever action it thinks is appropriate.
> This is already explained in Documentation/power/devices.txt.

OK

I'll prepare a patch to remove the dev->power.power_state.event checks from the
suspend core code.

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

[RFC][PATCH] PM: Remove prev_state from dev_pm_info

[Rafael J. Wysocki]

On Sunday, 10 June 2007 16:32, Alan Stern wrote:
> On Sat, 9 Jun 2007, Rafael J. Wysocki wrote:
> 
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > The saved_state member of struct dev_pm_info, defined in include/linux/pm.h, is
> > not used anywhere, so it can be removed.
> 
> Along the same lines, feature_removal_schedule.txt says that 
> dev->power.power_state will be removed next month.  In preparation, 
> you could consider removing the prev_state member now.  As far as I 
> know, it isn't used for anything other than avoiding resume method 
> calls to devices that were already suspended when a system sleep 
> began.

Patch for that is appended.

Greetings,
Rafael


---
From: Rafael J. Wysocki <rjw@sisk.pl>

The prev_state member of struct dev_pm_info (defined in include/linux/pm.h) is
only used during a resume to check if the device's state before the suspend was
'off', in which case the device is not resumed.  However, in such
cases the decision whether or not to resume the device should be made on the
driver level and the resume callbacks from the device's bus and class should be
executed anyway (the may be needed for some things other than just powering on
the device).

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/base/power/resume.c  |    3 +--
 drivers/base/power/suspend.c |    2 --
 drivers/usb/core/hub.c       |    5 -----
 include/linux/pm.h           |    1 -
 4 files changed, 1 insertion(+), 10 deletions(-)

Index: linux-2.6.22-rc4/drivers/base/power/resume.c
===================================================================
--- linux-2.6.22-rc4.orig/drivers/base/power/resume.c	2007-06-10 19:36:52.000000000 +0200
+++ linux-2.6.22-rc4/drivers/base/power/resume.c	2007-06-10 19:53:57.000000000 +0200
@@ -83,8 +83,7 @@ void dpm_resume(void)
 		list_move_tail(entry, &dpm_active);
 
 		mutex_unlock(&dpm_list_mtx);
-		if (!dev->power.prev_state.event)
-			resume_device(dev);
+		resume_device(dev);
 		mutex_lock(&dpm_list_mtx);
 		put_device(dev);
 	}
Index: linux-2.6.22-rc4/drivers/base/power/suspend.c
===================================================================
--- linux-2.6.22-rc4.orig/drivers/base/power/suspend.c	2007-06-10 19:36:52.000000000 +0200
+++ linux-2.6.22-rc4/drivers/base/power/suspend.c	2007-06-10 19:53:09.000000000 +0200
@@ -71,8 +71,6 @@ int suspend_device(struct device * dev, 
 			dev->parent->power.power_state.event);
 	}
 
-	dev->power.prev_state = dev->power.power_state;
-
 	if (dev->class && dev->class->suspend && !dev->power.power_state.event) {
 		suspend_device_dbg(dev, state, "class ");
 		error = dev->class->suspend(dev, state);
Index: linux-2.6.22-rc4/drivers/usb/core/hub.c
===================================================================
--- linux-2.6.22-rc4.orig/drivers/usb/core/hub.c	2007-06-08 13:09:18.000000000 +0200
+++ linux-2.6.22-rc4/drivers/usb/core/hub.c	2007-06-10 19:54:08.000000000 +0200
@@ -1110,11 +1110,6 @@ void usb_root_hub_lost_power(struct usb_
 
 	dev_warn(&rhdev->dev, "root hub lost power or was reset\n");
 
-	/* Make sure no potential wakeup events get lost,
-	 * by forcing the root hub to be resumed.
-	 */
-	rhdev->dev.power.prev_state.event = PM_EVENT_ON;
-
 	spin_lock_irqsave(&device_state_lock, flags);
 	hub = hdev_to_hub(rhdev);
 	for (port1 = 1; port1 <= rhdev->maxchild; ++port1) {
Index: linux-2.6.22-rc4/include/linux/pm.h
===================================================================
--- linux-2.6.22-rc4.orig/include/linux/pm.h	2007-06-10 19:36:52.000000000 +0200
+++ linux-2.6.22-rc4/include/linux/pm.h	2007-06-10 19:52:56.000000000 +0200
@@ -235,7 +235,6 @@ struct dev_pm_info {
 	unsigned		can_wakeup:1;
 #ifdef	CONFIG_PM
 	unsigned		should_wakeup:1;
-	pm_message_t		prev_state;
 	struct list_head	entry;
 #endif
 };

Re: [RFC][PATCH] PM: Remove prev_state from dev_pm_info

[Alan Stern]

On Mon, 11 Jun 2007, Rafael J. Wysocki wrote:

> > Along the same lines, feature_removal_schedule.txt says that 
> > dev->power.power_state will be removed next month.  In preparation, 
> > you could consider removing the prev_state member now.  As far as I 
> > know, it isn't used for anything other than avoiding resume method 
> > calls to devices that were already suspended when a system sleep 
> > began.
> 
> Patch for that is appended.

It looks good.  The USB portion might generate a merge conflict because 
of other ongoing changes, but they will be easy to fix.

Alan Stern