On 08/28/2013 09:48 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>
> device_hotplug_lock is held around the acpi_bus_trim() call in
> acpi_scan_hot_remove() which generally removes devices (it removes
> ACPI device objects at least, but it may also remove "physical"
> device objects through .detach() callbacks of ACPI scan handlers).
> Thus, potentially, device sysfs attributes are removed under that
> lock and to remove those attributes it is necessary to hold the
> s_active references of their directory entries for writing.
>
> On the other hand, the execution of a .show() or .store() callback
> from a sysfs attribute is carried out with that attribute's s_active
> reference held for reading. Consequently, if any device sysfs
> attribute that may be removed from within acpi_scan_hot_remove()
> through acpi_bus_trim() has a .store() or .show() callback which
> acquires device_hotplug_lock, the execution of that callback may
> deadlock with the removal of the attribute. [Unfortunately, the
> "online" device attribute of CPUs and memory blocks is one of them.]
>
> To avoid such deadlocks, make all of the sysfs attribute callbacks
> that need to lock device hotplug, for example store_online(), use
> a special function, lock_device_hotplug_sysfs(), to lock device
> hotplug and return the result of that function immediately if it is
> not zero. This will cause the s_active reference of the directory
> entry in question to be released and the syscall to be restarted
> if device_hotplug_lock cannot be acquired.
>
> [show_online() actually doesn't need to lock device hotplug, but
> it is useful to serialize it with respect to device_offline() and
> device_online() for the same device (in case user space attempts to
> run them concurrently) which can be done with the help of
> device_lock().]
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> Reported-by: Yasuaki Ishimatsu <isimatu.yasuaki@xxxxxxxxxxxxxx>
> Reported-by: Gu Zheng <guz.fnst@xxxxxxxxxxxxxx>
Tested-by: Gu Zheng <guz.fnst@xxxxxxxxxxxxxx>
> ---
> drivers/acpi/sysfs.c | 5 ++++-
> drivers/base/core.c | 43 ++++++++++++++++++++++++++++---------------
> drivers/base/memory.c | 4 +++-
> include/linux/device.h | 1 +
> 4 files changed, 36 insertions(+), 17 deletions(-)
>
> Index: linux-pm/drivers/base/core.c
> ===================================================================
> --- linux-pm.orig/drivers/base/core.c
> +++ linux-pm/drivers/base/core.c
> @@ -49,6 +49,28 @@ static struct kobject *dev_kobj;
> struct kobject *sysfs_dev_char_kobj;
> struct kobject *sysfs_dev_block_kobj;
>
> +static DEFINE_MUTEX(device_hotplug_lock);
> +
> +void lock_device_hotplug(void)
> +{
> + mutex_lock(&device_hotplug_lock);
> +}
> +
> +void unlock_device_hotplug(void)
> +{
> + mutex_unlock(&device_hotplug_lock);
> +}
> +
> +int lock_device_hotplug_sysfs(void)
> +{
> + if (mutex_trylock(&device_hotplug_lock))
> + return 0;
> +
> + /* Avoid busy looping (5 ms of sleep should do). */
> + msleep(5);
> + return restart_syscall();
> +}
> +
> #ifdef CONFIG_BLOCK
> static inline int device_is_not_partition(struct device *dev)
> {
> @@ -408,9 +430,9 @@ static ssize_t show_online(struct device
> {
> bool val;
>
> - lock_device_hotplug();
> + device_lock(dev);
> val = !dev->offline;
> - unlock_device_hotplug();
> + device_unlock(dev);
> return sprintf(buf, "%u\n", val);
> }
>
> @@ -424,7 +446,10 @@ static ssize_t store_online(struct devic
> if (ret < 0)
> return ret;
>
> - lock_device_hotplug();
> + ret = lock_device_hotplug_sysfs();
> + if (ret)
> + return ret;
> +
> ret = val ? device_online(dev) : device_offline(dev);
> unlock_device_hotplug();
> return ret < 0 ? ret : count;
> @@ -1479,18 +1504,6 @@ EXPORT_SYMBOL_GPL(put_device);
> EXPORT_SYMBOL_GPL(device_create_file);
> EXPORT_SYMBOL_GPL(device_remove_file);
>
> -static DEFINE_MUTEX(device_hotplug_lock);
> -
> -void lock_device_hotplug(void)
> -{
> - mutex_lock(&device_hotplug_lock);
> -}
> -
> -void unlock_device_hotplug(void)
> -{
> - mutex_unlock(&device_hotplug_lock);
> -}
> -
> static int device_check_offline(struct device *dev, void *not_used)
> {
> int ret;
> Index: linux-pm/drivers/base/memory.c
> ===================================================================
> --- linux-pm.orig/drivers/base/memory.c
> +++ linux-pm/drivers/base/memory.c
> @@ -351,7 +351,9 @@ store_mem_state(struct device *dev,
>
> mem = container_of(dev, struct memory_block, dev);
>
> - lock_device_hotplug();
> + ret = lock_device_hotplug_sysfs();
> + if (ret)
> + return ret;
>
> if (!strncmp(buf, "online_kernel", min_t(int, count, 13))) {
> offline = false;
> Index: linux-pm/drivers/acpi/sysfs.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/sysfs.c
> +++ linux-pm/drivers/acpi/sysfs.c
> @@ -796,7 +796,10 @@ static ssize_t force_remove_store(struct
> if (ret < 0)
> return ret;
>
> - lock_device_hotplug();
> + ret = lock_device_hotplug_sysfs();
> + if (ret)
> + return ret;
> +
> acpi_force_hot_remove = val;
> unlock_device_hotplug();
> return size;
> Index: linux-pm/include/linux/device.h
> ===================================================================
> --- linux-pm.orig/include/linux/device.h
> +++ linux-pm/include/linux/device.h
> @@ -895,6 +895,7 @@ static inline bool device_supports_offli
>
> extern void lock_device_hotplug(void);
> extern void unlock_device_hotplug(void);
> +extern int lock_device_hotplug_sysfs(void);
> extern int device_offline(struct device *dev);
> extern int device_online(struct device *dev);
> /*
>
>
--
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
