Re: [PATCH] driver core / ACPI: Avoid device removal locking problems

[Gu Zheng <guz.fnst@cn.fujitsu.com>]

[-- Attachment #1: Type: text/plain, Size: 1013 bytes --]

Hi Rafael,

On 08/26/2013 11:02 PM, Rafael J. Wysocki wrote:

> On Monday, August 26, 2013 04:43:26 PM Rafael J. Wysocki wrote:
>> On Monday, August 26, 2013 02:42:09 PM Rafael J. Wysocki wrote:
>>> On Monday, August 26, 2013 11:13:13 AM Gu Zheng wrote:
>>>> Hi Rafael,
> 
> [...]
> 
>>
>> OK, so the patch below is quick and dirty and overkill, but it should make the
>> splat go away at least.
> 
> And if this patch does make the splat go away for you, please also test the
> appended one (Tejun, thanks for the hint!).
> 
> I'll address the ACPI part differently later.

What about changing device_hotplug_lock and acpi_scan_lock to rwsem? like the
attached one(With a preliminary test, it also can make the splat go away).:)

Regards,
Gu

> 
[...]
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 



[-- Attachment #2: 0001-acpi-fix-removal-lock-dep.patch --]
[-- Type: text/plain, Size: 9065 bytes --]

>From f1682ceaef4105f75f4d6a0bb8e77c8a5dde365b Mon Sep 17 00:00:00 2001
From: Gu Zheng <guz.fnst@cn.fujitsu.com>
Date: Tue, 27 Aug 2013 17:59:55 +0900
Subject: [PATCH] acpi: fix removal lock dep


Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
---
 drivers/acpi/scan.c    |   43 ++++++++++++++++++++++---------------------
 drivers/acpi/sysfs.c   |    7 +++++--
 drivers/base/core.c    |   45 ++++++++++++++++++++++++++++++++++++---------
 drivers/base/memory.c  |    5 +++--
 include/linux/device.h |    8 ++++++--
 5 files changed, 72 insertions(+), 36 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 8a46c92..bb41760 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -36,7 +36,7 @@ bool acpi_force_hot_remove;
 static const char *dummy_hid = "device";
 
 static LIST_HEAD(acpi_bus_id_list);
-static DEFINE_MUTEX(acpi_scan_lock);
+static DECLARE_RWSEM(acpi_scan_rwsem);
 static LIST_HEAD(acpi_scan_handlers_list);
 DEFINE_MUTEX(acpi_device_lock);
 LIST_HEAD(acpi_wakeup_device_list);
@@ -49,13 +49,13 @@ struct acpi_device_bus_id{
 
 void acpi_scan_lock_acquire(void)
 {
-	mutex_lock(&acpi_scan_lock);
+	down_write(&acpi_scan_rwsem);
 }
 EXPORT_SYMBOL_GPL(acpi_scan_lock_acquire);
 
 void acpi_scan_lock_release(void)
 {
-	mutex_unlock(&acpi_scan_lock);
+	up_write(&acpi_scan_rwsem);
 }
 EXPORT_SYMBOL_GPL(acpi_scan_lock_release);
 
@@ -207,7 +207,7 @@ static int acpi_scan_hot_remove(struct acpi_device *device)
 		return -EINVAL;
 	}
 
-	lock_device_hotplug();
+	device_hotplug_begin();
 
 	/*
 	 * Carry out two passes here and ignore errors in the first pass,
@@ -240,7 +240,7 @@ static int acpi_scan_hot_remove(struct acpi_device *device)
 					    acpi_bus_online_companions, NULL,
 					    NULL, NULL);
 
-			unlock_device_hotplug();
+			device_hotplug_end();
 
 			put_device(&device->dev);
 			return -EBUSY;
@@ -252,7 +252,7 @@ static int acpi_scan_hot_remove(struct acpi_device *device)
 
 	acpi_bus_trim(device);
 
-	unlock_device_hotplug();
+	device_hotplug_end();
 
 	/* Device node has been unregistered. */
 	put_device(&device->dev);
@@ -308,7 +308,7 @@ static void acpi_bus_device_eject(void *context)
 	struct acpi_scan_handler *handler;
 	u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE;
 
-	mutex_lock(&acpi_scan_lock);
+	acpi_scan_lock_acquire();
 
 	acpi_bus_get_device(handle, &device);
 	if (!device)
@@ -334,7 +334,7 @@ static void acpi_bus_device_eject(void *context)
 	}
 
  out:
-	mutex_unlock(&acpi_scan_lock);
+	acpi_scan_lock_release();
 	return;
 
  err_out:
@@ -349,8 +349,8 @@ static void acpi_scan_bus_device_check(acpi_handle handle, u32 ost_source)
 	u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE;
 	int error;
 
-	mutex_lock(&acpi_scan_lock);
-	lock_device_hotplug();
+	acpi_scan_lock_acquire();
+	device_hotplug_begin();
 
 	if (ost_source != ACPI_NOTIFY_BUS_CHECK) {
 		acpi_bus_get_device(handle, &device);
@@ -376,9 +376,9 @@ static void acpi_scan_bus_device_check(acpi_handle handle, u32 ost_source)
 		kobject_uevent(&device->dev.kobj, KOBJ_ONLINE);
 
  out:
-	unlock_device_hotplug();
+	device_hotplug_end();
 	acpi_evaluate_hotplug_ost(handle, ost_source, ost_code, NULL);
-	mutex_unlock(&acpi_scan_lock);
+	acpi_scan_lock_release();
 }
 
 static void acpi_scan_bus_check(void *context)
@@ -469,15 +469,14 @@ void acpi_bus_hot_remove_device(void *context)
 	acpi_handle handle = device->handle;
 	int error;
 
-	mutex_lock(&acpi_scan_lock);
+	acpi_scan_lock_acquire();
 
 	error = acpi_scan_hot_remove(device);
 	if (error && handle)
 		acpi_evaluate_hotplug_ost(handle, ej_event->event,
 					  ACPI_OST_SC_NON_SPECIFIC_FAILURE,
 					  NULL);
-
-	mutex_unlock(&acpi_scan_lock);
+	acpi_scan_lock_release();
 	kfree(context);
 }
 EXPORT_SYMBOL(acpi_bus_hot_remove_device);
@@ -530,7 +529,8 @@ acpi_eject_store(struct device *d, struct device_attribute *attr,
 	if (ACPI_FAILURE(status) || !acpi_device->flags.ejectable)
 		return -ENODEV;
 
-	mutex_lock(&acpi_scan_lock);
+	if (!down_write_trylock(&acpi_scan_rwsem))
+		return -EBUSY;
 
 	if (acpi_device->flags.eject_pending) {
 		/* ACPI eject notification event. */
@@ -560,7 +560,7 @@ acpi_eject_store(struct device *d, struct device_attribute *attr,
 	ret = count;
 
  out:
-	mutex_unlock(&acpi_scan_lock);
+	up_write(&acpi_scan_rwsem);
 	return ret;
 
  err_out:
@@ -1858,11 +1858,12 @@ void acpi_scan_hotplug_enabled(struct acpi_hotplug_profile *hotplug, bool val)
 	if (!!hotplug->enabled == !!val)
 		return;
 
-	mutex_lock(&acpi_scan_lock);
+	acpi_scan_lock_acquire();
 
 	hotplug->enabled = val;
 
-	mutex_unlock(&acpi_scan_lock);
+	acpi_scan_lock_release();
+
 }
 
 static void acpi_scan_init_hotplug(acpi_handle handle, int type)
@@ -2141,7 +2142,7 @@ int __init acpi_scan_init(void)
 	acpi_memory_hotplug_init();
 	acpi_dock_init();
 
-	mutex_lock(&acpi_scan_lock);
+	acpi_scan_lock_acquire();
 	/*
 	 * Enumerate devices in the ACPI namespace.
 	 */
@@ -2164,6 +2165,6 @@ int __init acpi_scan_init(void)
 	acpi_pci_root_hp_init();
 
  out:
-	mutex_unlock(&acpi_scan_lock);
+	acpi_scan_lock_release();
 	return result;
 }
diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
index 05306a5..6d8b54f 100644
--- a/drivers/acpi/sysfs.c
+++ b/drivers/acpi/sysfs.c
@@ -796,9 +796,12 @@ static ssize_t force_remove_store(struct kobject *kobj,
 	if (ret < 0)
 		return ret;
 
-	lock_device_hotplug();
+	if (!write_lock_device_hotplug())
+		return -EBUSY;
+
 	acpi_force_hot_remove = val;
-	unlock_device_hotplug();
+
+	write_unlock_device_hotplug();
 	return size;
 }
 
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 8856d74..83c0f46 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -408,9 +408,13 @@ static ssize_t show_online(struct device *dev, struct device_attribute *attr,
 {
 	bool val;
 
-	lock_device_hotplug();
+	if (!read_lock_device_hotplug()) {
+		msleep(10);
+		return restart_syscall();
+	}
+
 	val = !dev->offline;
-	unlock_device_hotplug();
+	read_unlock_device_hotplug();
 	return sprintf(buf, "%u\n", val);
 }
 
@@ -424,9 +428,12 @@ static ssize_t store_online(struct device *dev, struct device_attribute *attr,
 	if (ret < 0)
 		return ret;
 
-	lock_device_hotplug();
+	if (!write_lock_device_hotplug()) {
+		msleep(10);
+		return restart_syscall();
+	}
 	ret = val ? device_online(dev) : device_offline(dev);
-	unlock_device_hotplug();
+	write_unlock_device_hotplug();
 	return ret < 0 ? ret : count;
 }
 
@@ -1479,16 +1486,36 @@ EXPORT_SYMBOL_GPL(put_device);
 EXPORT_SYMBOL_GPL(device_create_file);
 EXPORT_SYMBOL_GPL(device_remove_file);
 
-static DEFINE_MUTEX(device_hotplug_lock);
+static DECLARE_RWSEM(device_hotplug_rwsem);
+
+bool __must_check read_lock_device_hotplug(void)
+{
+	return down_read_trylock(&device_hotplug_rwsem);
+}
+
+void read_unlock_device_hotplug(void)
+{
+	up_read(&device_hotplug_rwsem);
+}
+
+bool __must_check write_lock_device_hotplug(void)
+{
+	return down_write_trylock(&device_hotplug_rwsem);
+}
+
+void write_unlock_device_hotplug(void)
+{
+	up_write(&device_hotplug_rwsem);
+}
 
-void lock_device_hotplug(void)
+void device_hotplug_begin(void)
 {
-	mutex_lock(&device_hotplug_lock);
+	down_write(&device_hotplug_rwsem);
 }
 
-void unlock_device_hotplug(void)
+void device_hotplug_end(void)
 {
-	mutex_unlock(&device_hotplug_lock);
+	up_write(&device_hotplug_rwsem);
 }
 
 static int device_check_offline(struct device *dev, void *not_used)
diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index 2b7813e..71991b9 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -351,7 +351,8 @@ store_mem_state(struct device *dev,
 
 	mem = container_of(dev, struct memory_block, dev);
 
-	lock_device_hotplug();
+	if (!write_lock_device_hotplug())
+		return -EBUSY;
 
 	if (!strncmp(buf, "online_kernel", min_t(int, count, 13))) {
 		offline = false;
@@ -373,7 +374,7 @@ store_mem_state(struct device *dev,
 	if (!ret)
 		dev->offline = offline;
 
-	unlock_device_hotplug();
+	write_unlock_device_hotplug();
 
 	if (ret)
 		return ret;
diff --git a/include/linux/device.h b/include/linux/device.h
index 22b546a..08581f4 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -893,8 +893,12 @@ static inline bool device_supports_offline(struct device *dev)
 	return dev->bus && dev->bus->offline && dev->bus->online;
 }
 
-extern void lock_device_hotplug(void);
-extern void unlock_device_hotplug(void);
+extern bool read_lock_device_hotplug(void);
+extern void read_unlock_device_hotplug(void);
+extern bool write_lock_device_hotplug(void);
+extern void write_unlock_device_hotplug(void);
+extern void device_hotplug_begin(void);
+extern void device_hotplug_end(void);
 extern int device_offline(struct device *dev);
 extern int device_online(struct device *dev);
 /*
-- 
1.7.1