On Friday, 21 of March 2008, Alan Stern wrote:
> On Fri, 21 Mar 2008, Rafael J. Wysocki wrote:
>
> > > One trivial problem is that your dpm_prepare and dpm_complete routines
> > > go through the list in the wrong order.
> >
> > I'm not all so sure that the order is actually wrong. What would be the
> > advantage of the forward order over the current one?
>
> The advantage is that races no longer matter.
>
> Suppose you're going through the list backwards and a race occurs, so
> that a child is registered while the parent's prepare() is running.
> That new child will of course be at the end of dpm_active, so the loop
> in dpm_prepare won't see it (assuming you adopt the approach of using
> only a single list). But if you go through the list forwards and a new
> child is added, you will naturally see the child when you reach the end
> of the list. You don't even have to go through the business about
> changing the parent's state back to DPM_ON.
>
> There are other ways of describing the situation, but they all come
> down to the same thing. For instance, this is the way Ben was talking
> about doing things.
>
> > > Since dpm_prepare is supposed to go through the list in the forward
> > > direction, logically the "root" of the device tree should be the first
> > > thing "prepared". This means you should not allow parentless devices
> > > to be registered any time after dpm_prepare has started. Is that
> > > liable to cause problems?
> >
> > I'm still not seeing the advantage of the forward direction in the first place.
> >
> > Although I don't see what particular problems that may cause, I think the
> > current approach (first, block registrations of new children for each
> > ->prepare()d device and finally block any registrations of new devices) is
> > more natural.
>
> I suppose for parentless devices it doesn't really matter. You could
> safely allow them to be registered any time up until dpm_prepare()
> finishes -- which is what your patch does. Perhaps the "all_inactive"
> flag should be renamed to "all_prepared".
>
> > > There may be similar problems with complete(). A number of drivers
> > > check during their resume method for the presence of new children
> > > plugged in while the system was asleep. All these drivers would have
> > > to be converted over to the new scheme if they weren't permitted to
> > > register the new children before complete() was called. Of course,
> > > this is easily fixed by permitting new children starting from when
> > > resume() is called rather than when complete() is called.
> >
> > Well, the problem here was the protection of the correct ordering of the
> > various lists. However, if the approach with changing 'status' is adopted
> > instead, which seems to be better, we'll be able to unblock the registering
> > of new children before ->resume().
>
> Yep. The only thing to watch out for is in device_pm_remove(); it
> would be a disaster if somehow a device was removed while it was being
> prepared/suspended/resumed/completed/whatever. I know that's not
> supposed to happen but there's nothing to prevent it, especially if
> the device in question doesn't have a driver. No doubt you can invent
> a way to allow this to happen safely.
Well, that's a separate issue that IMO should be addressed in a separate patch.
Something like the one below comes to mind.
The comment removed by the patch is wrong IMO, because it implies that
device_add() may be called with the device semaphore held and that might
deadlock in bus_attach_device(). Thus, I think we can acquire dev->sem
in device_pm_add() and in device_pm_remove().
Thanks,
Rafael
---
drivers/base/power/main.c | 21 ++++++++++++++-------
include/linux/pm.h | 1 +
2 files changed, 15 insertions(+), 7 deletions(-)
Index: linux-2.6/drivers/base/power/main.c
===================================================================
--- linux-2.6.orig/drivers/base/power/main.c
+++ linux-2.6/drivers/base/power/main.c
@@ -41,10 +41,6 @@
* from the bottom (i.e., end) up and resumed in the opposite order.
* That way no parent will be suspended while it still has an active
* child.
- *
- * Since device_pm_add() may be called with a device semaphore held,
- * we must never try to acquire a device semaphore while holding
- * dpm_list_mutex.
*/
LIST_HEAD(dpm_active);
@@ -68,6 +64,7 @@ int device_pm_add(struct device *dev)
pr_debug("PM: Adding info for %s:%s\n",
dev->bus ? dev->bus->name : "No Bus",
kobject_name(&dev->kobj));
+ down(&dev->sem);
mutex_lock(&dpm_list_mtx);
if ((dev->parent && dev->parent->power.sleeping) || all_sleeping) {
if (dev->parent->power.sleeping)
@@ -80,10 +77,13 @@ int device_pm_add(struct device *dev)
error = -EBUSY;
} else {
error = dpm_sysfs_add(dev);
- if (!error)
+ if (!error) {
+ dev->power.present = true;
list_add_tail(&dev->power.entry, &dpm_active);
+ }
}
mutex_unlock(&dpm_list_mtx);
+ up(&dev->sem);
return error;
}
@@ -98,10 +98,13 @@ 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));
+ down(&dev->sem);
mutex_lock(&dpm_list_mtx);
dpm_sysfs_remove(dev);
+ dev->power.present = false;
list_del_init(&dev->power.entry);
mutex_unlock(&dpm_list_mtx);
+ up(&dev->sem);
}
/**
@@ -196,6 +199,8 @@ static int resume_device(struct device *
TRACE_RESUME(0);
down(&dev->sem);
+ if (!dev->power.present)
+ goto End;
if (dev->bus && dev->bus->resume) {
dev_dbg(dev,"resuming\n");
@@ -211,7 +216,7 @@ static int resume_device(struct device *
dev_dbg(dev,"class resume\n");
error = dev->class->resume(dev);
}
-
+ End:
up(&dev->sem);
TRACE_RESUME(error);
@@ -366,6 +371,8 @@ static int suspend_device(struct device
int error = 0;
down(&dev->sem);
+ if (!dev->power.present)
+ goto End;
if (dev->power.power_state.event) {
dev_dbg(dev, "PM: suspend %d-->%d\n",
@@ -389,7 +396,7 @@ static int suspend_device(struct device
error = dev->bus->suspend(dev, state);
suspend_report_result(dev->bus->suspend, error);
}
-
+ End:
up(&dev->sem);
return error;
Index: linux-2.6/include/linux/pm.h
===================================================================
--- linux-2.6.orig/include/linux/pm.h
+++ linux-2.6/include/linux/pm.h
@@ -185,6 +185,7 @@ struct dev_pm_info {
unsigned can_wakeup:1;
unsigned should_wakeup:1;
bool sleeping:1; /* Owned by the PM core */
+ bool present:1; /* Owned by the PM core */
#ifdef CONFIG_PM_SLEEP
struct list_head entry;
#endif
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
