Hi,
The following series of patches removes some unused and unnecessary features
from the suspend and resume core code.
Comments welcome.
Greetings,
Rafael
--
"Premature optimization is the root of all evil." - Donald Knuth
From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
---
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
};
From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
Acked-by: David Brownell <[email protected]>
---
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);
From: Rafael J. Wysocki <[email protected]>
The suspend and resume support in struct device_type (include/linux/device.h)
is not used anywhere. It is also undocumented, so it can be removed.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/base/power/resume.c | 5 -----
drivers/base/power/suspend.c | 7 -------
include/linux/device.h | 2 --
3 files changed, 14 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 15:00:23.000000000 +0200
+++ linux-2.6.22-rc4/drivers/base/power/resume.c 2007-06-10 15:02:04.000000000 +0200
@@ -41,11 +41,6 @@ int resume_device(struct device * dev)
error = dev->bus->resume(dev);
}
- if (!error && dev->type && dev->type->resume) {
- dev_dbg(dev,"resuming\n");
- error = dev->type->resume(dev);
- }
-
if (!error && dev->class && dev->class->resume) {
dev_dbg(dev,"class resume\n");
error = dev->class->resume(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 15:00:40.000000000 +0200
+++ linux-2.6.22-rc4/drivers/base/power/suspend.c 2007-06-10 15:02:04.000000000 +0200
@@ -79,13 +79,6 @@ int suspend_device(struct device * dev,
suspend_report_result(dev->class->suspend, error);
}
- if (!error && dev->type && dev->type->suspend
- && !dev->power.power_state.event) {
- suspend_device_dbg(dev, state, "type ");
- error = dev->type->suspend(dev, state);
- suspend_report_result(dev->type->suspend, error);
- }
-
if (!error && dev->bus && dev->bus->suspend
&& !dev->power.power_state.event) {
suspend_device_dbg(dev, state, "");
Index: linux-2.6.22-rc4/include/linux/device.h
===================================================================
--- linux-2.6.22-rc4.orig/include/linux/device.h 2007-06-10 15:00:23.000000000 +0200
+++ linux-2.6.22-rc4/include/linux/device.h 2007-06-10 15:02:04.000000000 +0200
@@ -343,8 +343,6 @@ struct device_type {
int (*uevent)(struct device *dev, char **envp, int num_envp,
char *buffer, int buffer_size);
void (*release)(struct device *dev);
- int (*suspend)(struct device * dev, pm_message_t state);
- int (*resume)(struct device * dev);
};
/* interface for exporting device attributes */
From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
---
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
};
From: Rafael J. Wysocki <[email protected]>
The suspend routines should be called for every device during a system sleep
transition, regardless of the device's state, so that drivers can regard these
method calls as notifications that the system is about to go to sleep, rather
than as directives to put their devices into the 'off' state.
This is documented in Documentation/power/devices.txt and is already done in
the core resume code, so it seems reasonable to make the core suspend code
behave accordingly.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/base/power/suspend.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
Index: linux-2.6.22-rc4/drivers/base/power/suspend.c
===================================================================
--- linux-2.6.22-rc4.orig/drivers/base/power/suspend.c 2007-06-11 09:59:54.000000000 +0200
+++ linux-2.6.22-rc4/drivers/base/power/suspend.c 2007-06-11 10:18:11.000000000 +0200
@@ -71,14 +71,13 @@ int suspend_device(struct device * dev,
dev->parent->power.power_state.event);
}
- if (dev->class && dev->class->suspend && !dev->power.power_state.event) {
+ if (dev->class && dev->class->suspend) {
suspend_device_dbg(dev, state, "class ");
error = dev->class->suspend(dev, state);
suspend_report_result(dev->class->suspend, error);
}
- if (!error && dev->bus && dev->bus->suspend
- && !dev->power.power_state.event) {
+ if (!error && dev->bus && dev->bus->suspend) {
suspend_device_dbg(dev, state, "");
error = dev->bus->suspend(dev, state);
suspend_report_result(dev->bus->suspend, error);
@@ -97,8 +96,7 @@ static int suspend_device_late(struct de
{
int error = 0;
- if (dev->bus && dev->bus->suspend_late
- && !dev->power.power_state.event) {
+ if (dev->bus && dev->bus->suspend_late) {
suspend_device_dbg(dev, state, "LATE ");
error = dev->bus->suspend_late(dev, state);
suspend_report_result(dev->bus->suspend_late, error);
From: Rafael J. Wysocki <[email protected]>
Reduce code duplication in drivers/base/suspend.c by introducing a separate
function for printing diagnostic messages.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/base/power/suspend.c | 49 +++++++++++++++----------------------------
1 file changed, 18 insertions(+), 31 deletions(-)
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 13:30:45.000000000 +0200
+++ linux-2.6.22-rc4/drivers/base/power/suspend.c 2007-06-10 18:50:45.000000000 +0200
@@ -40,6 +40,14 @@ static inline char *suspend_verb(u32 eve
}
+static void
+suspend_device_dbg(struct device * dev, pm_message_t state, char *info)
+{
+ dev_dbg(dev, "%s%s%s\n", info, suspend_verb(state.event),
+ ((state.event == PM_EVENT_SUSPEND) && device_may_wakeup(dev)) ?
+ ", may wakeup" : "");
+}
+
/**
* suspend_device - Save state of one device.
* @dev: Device.
@@ -66,37 +74,21 @@ int suspend_device(struct device * dev,
dev->power.prev_state = dev->power.power_state;
if (dev->class && dev->class->suspend && !dev->power.power_state.event) {
- dev_dbg(dev, "class %s%s\n",
- suspend_verb(state.event),
- ((state.event == PM_EVENT_SUSPEND)
- && device_may_wakeup(dev))
- ? ", may wakeup"
- : ""
- );
+ suspend_device_dbg(dev, state, "class ");
error = dev->class->suspend(dev, state);
suspend_report_result(dev->class->suspend, error);
}
- if (!error && dev->type && dev->type->suspend && !dev->power.power_state.event) {
- dev_dbg(dev, "%s%s\n",
- suspend_verb(state.event),
- ((state.event == PM_EVENT_SUSPEND)
- && device_may_wakeup(dev))
- ? ", may wakeup"
- : ""
- );
+ if (!error && dev->type && dev->type->suspend
+ && !dev->power.power_state.event) {
+ suspend_device_dbg(dev, state, "type ");
error = dev->type->suspend(dev, state);
suspend_report_result(dev->type->suspend, error);
}
- if (!error && dev->bus && dev->bus->suspend && !dev->power.power_state.event) {
- dev_dbg(dev, "%s%s\n",
- suspend_verb(state.event),
- ((state.event == PM_EVENT_SUSPEND)
- && device_may_wakeup(dev))
- ? ", may wakeup"
- : ""
- );
+ if (!error && dev->bus && dev->bus->suspend
+ && !dev->power.power_state.event) {
+ suspend_device_dbg(dev, state, "");
error = dev->bus->suspend(dev, state);
suspend_report_result(dev->bus->suspend, error);
}
@@ -114,14 +106,9 @@ static int suspend_device_late(struct de
{
int error = 0;
- if (dev->bus && dev->bus->suspend_late && !dev->power.power_state.event) {
- dev_dbg(dev, "LATE %s%s\n",
- suspend_verb(state.event),
- ((state.event == PM_EVENT_SUSPEND)
- && device_may_wakeup(dev))
- ? ", may wakeup"
- : ""
- );
+ if (dev->bus && dev->bus->suspend_late
+ && !dev->power.power_state.event) {
+ suspend_device_dbg(dev, state, "LATE ");
error = dev->bus->suspend_late(dev, state);
suspend_report_result(dev->bus->suspend_late, error);
}
From: Rafael J. Wysocki <[email protected]>
The checks if the device's parent is in the right state done in
drivers/base/power/suspend.c and drivers/base/power/resume.c serve no particular
purpose, since if the parent is in a wrong power state, the device's suspend or
resume callbacks are supposed to return an error anyway. Moreover, they are
also useless from the sanity checking point of view, because they rely on the
code being checked to set dev->parent->power.power_state.event appropriately,
which need not happen if that code is buggy. For these reasons they can be
removed.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/base/power/resume.c | 7 -------
drivers/base/power/suspend.c | 7 -------
2 files changed, 14 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-11 09:59:54.000000000 +0200
+++ linux-2.6.22-rc4/drivers/base/power/resume.c 2007-06-11 10:19:37.000000000 +0200
@@ -29,13 +29,6 @@ int resume_device(struct device * dev)
down(&dev->sem);
- 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->parent->bus_id,
- dev->parent->power.power_state.event);
- }
-
if (dev->bus && dev->bus->resume) {
dev_dbg(dev,"resuming\n");
error = dev->bus->resume(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-11 10:18:11.000000000 +0200
+++ linux-2.6.22-rc4/drivers/base/power/suspend.c 2007-06-11 10:19:30.000000000 +0200
@@ -63,13 +63,6 @@ int suspend_device(struct device * dev,
dev_dbg(dev, "PM: suspend %d-->%d\n",
dev->power.power_state.event, 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->parent->bus_id,
- dev->parent->power.power_state.event);
- }
if (dev->class && dev->class->suspend) {
suspend_device_dbg(dev, state, "class ");
On Mon, 11 Jun 2007, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> 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.
> @@ -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;
> }
The error pathway here does an unbalanced get_device on dev->parent.
Anyway, I don't think you need to do this get_device at all (or the
coresponding put in device_pm_remove). As long as a device is
registered it retains a reference to its parent, and unregistration
always calls device_pm_remove. The reason it was there in the first
place was because people recognized that dev->power.pm_parent wouldn't
be one of dev's ancestors in the device hierarchy.
Alan Stern
On Monday, 11 June 2007 17:59, Alan Stern wrote:
> On Mon, 11 Jun 2007, Rafael J. Wysocki wrote:
>
> > From: Rafael J. Wysocki <[email protected]>
> >
> > 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.
>
> > @@ -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;
> > }
>
> The error pathway here does an unbalanced get_device on dev->parent.
>
> Anyway, I don't think you need to do this get_device at all (or the
> coresponding put in device_pm_remove). As long as a device is
> registered it retains a reference to its parent, and unregistration
> always calls device_pm_remove.
Yes, I've just come to the same conclusion. I'll remove the
get_device(dev->parent) and the correspondint put_device(dev->parent)
from device_pm_remove().
Greetings,
Rafael
--
"Premature optimization is the root of all evil." - Donald Knuth
On Monday, 11 June 2007 20:52, Rafael J. Wysocki wrote:
> On Monday, 11 June 2007 17:59, Alan Stern wrote:
> > On Mon, 11 Jun 2007, Rafael J. Wysocki wrote:
> >
> > > From: Rafael J. Wysocki <[email protected]>
> > >
> > > 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.
> >
> > > @@ -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;
> > > }
> >
> > The error pathway here does an unbalanced get_device on dev->parent.
> >
> > Anyway, I don't think you need to do this get_device at all (or the
> > coresponding put in device_pm_remove). As long as a device is
> > registered it retains a reference to its parent, and unregistration
> > always calls device_pm_remove.
>
> Yes, I've just come to the same conclusion. I'll remove the
> get_device(dev->parent) and the correspondint put_device(dev->parent)
> from device_pm_remove().
OK
The updated patch follows.
Greetings,
Rafael
---
From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
---
drivers/base/power/main.c | 30 ++++--------------------------
drivers/base/power/resume.c | 7 +++----
drivers/base/power/suspend.c | 7 +++----
include/linux/pm.h | 3 ---
4 files changed, 10 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-10 19:35:49.000000000 +0200
+++ linux-2.6.22-rc4/drivers/base/power/main.c 2007-06-11 21:05:10.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,20 @@ 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)))
+ 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);
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-10 19:35:49.000000000 +0200
+++ linux-2.6.22-rc4/drivers/base/power/resume.c 2007-06-11 21:04:44.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-10 19:35:49.000000000 +0200
+++ linux-2.6.22-rc4/drivers/base/power/suspend.c 2007-06-11 21:04:44.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-10 19:35:49.000000000 +0200
+++ linux-2.6.22-rc4/include/linux/pm.h 2007-06-11 21:04:44.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);
Hi!
> From: Rafael J. Wysocki <[email protected]>
>
> Reduce code duplication in drivers/base/suspend.c by introducing a separate
> function for printing diagnostic messages.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
ACK, thanks.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html