Re: [RFC 2/2] PM: Lock all devices during suspend/hibernate

Rafael J. Wysocki
2007-07-31T22:20:46Z

On Tuesday, 31 July 2007 22:51, Alan Stern wrote:
> This patch adds an extra step to the device suspend/resume procedures,
> in which every device is locked/unlocked. In addition, a new global
> rwsem prevents additional devices from being registered at these times.
>
> Alan Stern
>
>
> Index: usb-2.6/drivers/base/power/main.c
> ===================================================================
> --- usb-2.6.orig/drivers/base/power/main.c
> +++ usb-2.6/drivers/base/power/main.c
> @@ -24,17 +24,38 @@
> #include <linux/mutex.h>
> #include <linux/pm.h>
> #include <linux/resume-trace.h>
> +#include <linux/rwsem.h>
>
> #include "../base.h"
> #include "power.h"
>
> +/*
> + * The entries in the dpm_active list are in a depth first order, simply
> + * because children are guaranteed to be discovered after parents, and
> + * are inserted at the back of the list on discovery.
> + *
> + * All the other lists are kept in the same order, for consistency.
> + * However the lists aren't always traversed in the same order.
> + * Semaphores must be acquired from the top (i.e., front) down
> + * and released in the opposite order. Devices must be suspended
> + * 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);
> +static LIST_HEAD(dpm_locked);
> static LIST_HEAD(dpm_off);
> static LIST_HEAD(dpm_off_irq);
>
> -static DEFINE_MUTEX(dpm_mtx);
> static DEFINE_MUTEX(dpm_list_mtx);
>
> +static DECLARE_RWSEM(device_registration_rwsem);

Is it only intended for device registration, or can it be used in some other
code paths too?

> +
> int (*platform_enable_wakeup)(struct device *dev, int is_on);
>
>
> @@ -59,29 +80,112 @@ 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));
> +
> + /* Don't remove a device while the PM core has it locked for suspend */
> + down(&dev->sem);
> mutex_lock(&dpm_list_mtx);
> dpm_sysfs_remove(dev);
> list_del_init(&dev->power.entry);
> mutex_unlock(&dpm_list_mtx);
> + up(&dev->sem);
> +}
> +
> +/**
> + * device_add_pm_lock - mutual exclusion for registration and suspend
> + *
> + * Returns 0 if no suspend is underway and device registration
> + * may proceed, otherwise -EBUSY.
> + */
> +int device_add_pm_lock(void)
> +{
> + if (down_read_trylock(&device_registration_rwsem))
> + return 0;
> + return -EBUSY;
> +}

I would do:

+ return down_read_trylock(&device_registration_rwsem) ? 0 : -EBUSY;

Apart from this, I have no comments. :-)

Greetings,
Rafael

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