Skip to contentSkip to conversation

Bug: linux/acpi may execute notify handler that has been removed

DISCUSSION Zhang Rui 3 participants 8 messages Sep 27, 2007 – Sep 28, 2007

Subsystems (2)
lore
Share & tools
lore

Zhang Rui

author
#0 lore raw

Hi, all,

I found a bug that linux/acpi may execute notify handler that has been removed.

When a system notify(0~0x7f) is received, linux/acpi will first invoke the generic system notify handler (acpi_bus_notify) and then invoke the per-device notify handler if present.

In my case, I add some code in acpi_bus_notify for battery hotplug support, so that the generic system notify handler will remove the battery device, including the per-device notify handler acpi_battery_notify() when receiving notification ACPI_NOTIFY_EJECT_REQUEST.
But linux/acpi invokes the per-device notify handler soon and this breaks the system.

Further more, device hot-removal is not the only case to encounter this bug. For example, linux/acpi receives a notification and adds it in the workqueue, and then the driver(notify handler) is removed before kacpid_notify invoke it...

Attachment is the patch for battery hotplug support. Any ideas about this bug?

Thanks,
Rui

#1 lore raw
Quoted 1 line · Zhang Rui

Attachment is the patch for battery hotplug support.

There is it?

Quoted 1 line

Any ideas about this bug?

Regards,
Alex.

author
#2 lore raw
Quoted 3 lines · Alexey Starikovskiy
Quoted 1 line · Zhang Rui

Attachment is the patch for battery hotplug support.

There is it?

Oops, the battery hotplug patch is attached.

Thanks,
Rui

add battery hotplug support for ACPI battery driver.
http://bugzilla.kernel.org/show_bug.cgi?id=2884

---
 drivers/acpi/bus.c      |   30 ++++++++++-
 drivers/acpi/scan.c     |  121 +++++++++++++++++++++++++++++++++++++++++++-----
 include/acpi/acpi_bus.h |    2 
 3 files changed, 139 insertions(+), 14 deletions(-)
drivers/acpi/scan.c +110 -11
Index: linux-2.6/drivers/acpi/scan.c
===================================================================
@@ -86,22 +86,53 @@ acpi_device_modalias_show(struct device 
 }
 static DEVICE_ATTR(modalias, 0444, acpi_device_modalias_show, NULL);
 
-static int acpi_eject_operation(acpi_handle handle, int lockable)
+static acpi_status
+acpi_bus_insert_device(acpi_handle handle)
+{
+	struct acpi_object_list arg_list;
+	union acpi_object arg;
+	acpi_status status;
+
+	arg_list.count = 1;
+	arg_list.pointer = &arg;
+	arg.type = ACPI_TYPE_INTEGER;
+	arg.integer.value = 1;
+	status = acpi_evaluate_object(handle, "_LCK", &arg_list, NULL);
+	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND)
+		ACPI_DEBUG_PRINT((ACPI_DB_WARN,
+				"Locking device failed\n"));
+
+	/* power on device */
+	status = acpi_evaluate_object(handle, "_PS0", NULL, NULL);
+	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND)
+		ACPI_DEBUG_PRINT((ACPI_DB_WARN,
+				"Power-on device failed\n"));
+
+	return AE_OK;
+}
+
+static acpi_status
+acpi_bus_eject_device(acpi_handle handle, int lockable)
 {
 	struct acpi_object_list arg_list;
 	union acpi_object arg;
 	acpi_status status = AE_OK;
 
-	/*
-	 * TBD: evaluate _PS3?
-	 */
+	/* power off device */
+	status = acpi_evaluate_object(handle, "_PS3", NULL, NULL);
+	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND)
+		ACPI_DEBUG_PRINT((ACPI_DB_WARN,
+				"Power-off device failed\n"));
 
 	if (lockable) {
 		arg_list.count = 1;
 		arg_list.pointer = &arg;
 		arg.type = ACPI_TYPE_INTEGER;
 		arg.integer.value = 0;
-		acpi_evaluate_object(handle, "_LCK", &arg_list, NULL);
+		status = acpi_evaluate_object(handle, "_LCK", &arg_list, NULL);
+		if (ACPI_FAILURE(status))
+			ACPI_DEBUG_PRINT((ACPI_DB_WARN,
+					"Unlocking device failed\n"));
 	}
 
 	arg_list.count = 1;
@@ -112,13 +143,12 @@ static int acpi_eject_operation(acpi_han
 	/*
 	 * TBD: _EJD support.
 	 */
-
 	status = acpi_evaluate_object(handle, "_EJ0", &arg_list, NULL);
-	if (ACPI_FAILURE(status)) {
-		return (-ENODEV);
-	}
+	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND)
+		ACPI_DEBUG_PRINT((ACPI_DB_WARN,
+				"Ejecting device failed\n"));
 
-	return (0);
+	return 0;
 }
 
 static ssize_t
@@ -154,7 +184,7 @@ acpi_eject_store(struct device *d, struc
 	result = acpi_bus_trim(acpi_device, 1);
 
 	if (!result)
-		result = acpi_eject_operation(handle, islockable);
+		result = acpi_bus_eject_device(handle, islockable);
 
 	if (result) {
 		ret = -EBUSY;
@@ -491,6 +521,70 @@ static void acpi_device_unregister(struc
 	device_unregister(&device->dev);
 }
 
+int acpi_bus_hot_insert_device(acpi_handle handle)
+{
+	acpi_handle		phandle;
+	struct acpi_device	*pdev = NULL;
+	struct acpi_device	*device = NULL;
+	struct acpi_device_status sta;
+
+	ACPI_DEBUG_PRINT((ACPI_DB_INFO,
+			"Hot-inserting device...\n"));
+
+	if (acpi_evaluate_integer(handle, "_STA", NULL,
+				  (unsigned long *)&sta) ||
+				  !sta.present) {
+		ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
+				"Device is not present\n"));
+		return -ENODEV;
+	}
+
+	if (acpi_get_parent(handle, &phandle)) {
+		ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
+				"Can't get device's parent\n"));
+		return -ENODEV;
+	}
+
+	if (acpi_bus_get_device(phandle, &pdev)) {
+		ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
+				"Device's parrent isn't present\n"));
+		return -ENODEV;
+	}
+
+	acpi_bus_insert_device(handle);
+
+	if (acpi_bus_add(&device, pdev, handle, ACPI_BUS_TYPE_DEVICE)) {
+		ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
+				"Hot-inserting device failed\n"));
+		return -ENODEV;
+	}
+
+        return 0;
+}
+EXPORT_SYMBOL(acpi_bus_hot_insert_device);
+
+int acpi_bus_hot_remove_device(struct acpi_device *device)
+{
+        int ret;
+        acpi_status status;
+        acpi_handle handle = device->handle;
+        int lockable = device->flags.lockable;
+
+	ACPI_DEBUG_PRINT((ACPI_DB_INFO,
+			"Hot-removing device %s...\n", device->dev.bus_id));
+
+        ret = acpi_bus_trim(device, 1);
+        if (ret) {
+                ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
+                                "Removing device failed\n"));
+                return ret;
+        }
+
+        status = acpi_bus_eject_device(handle, lockable);
+
+        return 0;
+}
+EXPORT_SYMBOL(acpi_bus_hot_remove_device);
 /* --------------------------------------------------------------------------
                                  Driver Management
    -------------------------------------------------------------------------- */
@@ -1397,6 +1491,11 @@ int acpi_bus_trim(struct acpi_device *st
 		if (ACPI_FAILURE(status)) {
 			continue;
 		}
+
+		if (type != ACPI_TYPE_DEVICE && type != ACPI_TYPE_PROCESSOR
+			&& type != ACPI_TYPE_THERMAL && type != ACPI_TYPE_POWER)
+			continue;
+
 		/*
 		 * If there is a device corresponding to chandle then
 		 * parse it (depth-first).
include/acpi/acpi_bus.h +2 -0
Index: linux-2.6/include/acpi/acpi_bus.h
===================================================================
@@ -338,6 +338,8 @@ int acpi_bus_receive_event(struct acpi_b
 static inline int acpi_bus_generate_proc_event(struct acpi_device *device, u8 type, int data)
 	{ return 0; }
 #endif
+int acpi_bus_hot_insert_device(acpi_handle);
+int acpi_bus_hot_remove_device(struct acpi_device *);
 int acpi_bus_register_driver(struct acpi_driver *driver);
 void acpi_bus_unregister_driver(struct acpi_driver *driver);
 int acpi_bus_add(struct acpi_device **child, struct acpi_device *parent,
drivers/acpi/bus.c +27 -3
Index: linux-2.6/drivers/acpi/bus.c
===================================================================
@@ -444,6 +444,7 @@ static int acpi_bus_check_scope(struct a
 	return 0;
 }
 
+#define ACPI_BATTERY_HID "PNP0C0A"
 /**
  * acpi_bus_notify
  * ---------------
@@ -454,8 +455,8 @@ static void acpi_bus_notify(acpi_handle 
 	int result = 0;
 	struct acpi_device *device = NULL;
 
-
-	if (acpi_bus_get_device(handle, &device))
+	if (acpi_bus_get_device(handle, &device) &&
+		type != ACPI_NOTIFY_DEVICE_CHECK)
 		return;
 
 	switch (type) {
@@ -475,7 +476,26 @@ static void acpi_bus_notify(acpi_handle 
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 				  "Received DEVICE CHECK notification for device [%s]\n",
 				  device->pnp.bus_id));
-		result = acpi_bus_check_device(device, NULL);
+		if (device)
+			result = acpi_bus_check_device(device, NULL);
+		else {
+			/*
+			 * device hotplug support
+			 * we only support battery hotplug for now
+			 */
+			struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
+			struct acpi_device_info *info;
+			acpi_status status;
+
+			status = acpi_get_object_info(handle, &buffer);
+			if (ACPI_FAILURE(status))
+				return;
+
+			info = buffer.pointer;
+			if (info->valid & ACPI_VALID_HID)
+                        	if (!strcmp(info->hardware_id.value, ACPI_BATTERY_HID))
+					result = acpi_bus_hot_insert_device(handle);
+		}
 		/*
 		 * TBD: We'll need to outsource certain events to non-ACPI
 		 *      drivers via the device manager (device.c).
@@ -493,6 +513,10 @@ static void acpi_bus_notify(acpi_handle 
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 				  "Received EJECT REQUEST notification for device [%s]\n",
 				  device->pnp.bus_id));
+		/* hot removing battery device */
+		if (!strcmp(device->pnp.hardware_id, ACPI_BATTERY_HID))
+			result = acpi_bus_hot_remove_device(device);
+
 		/* TBD */
 		break;
 
#3 lore raw
In reply to Zhang Rui
Quoted 17 lines · Zhang Rui

Hi, all,

I found a bug that linux/acpi may execute notify handler that has been removed.

When a system notify(0~0x7f) is received, linux/acpi will first invoke the generic system notify handler (acpi_bus_notify) and then invoke the per-device notify handler if present.

In my case, I add some code in acpi_bus_notify for battery hotplug support, so that the generic system notify handler will remove the battery device, including the per-device notify handler acpi_battery_notify() when receiving notification ACPI_NOTIFY_EJECT_REQUEST.
But linux/acpi invokes the per-device notify handler soon and this breaks the system.

Quoted 4 lines

Further more, device hot-removal is not the only case to encounter this bug. For example, linux/acpi receives a notification and adds it in the workqueue, and then the driver(notify handler) is removed before kacpid_notify invoke it...

For the non-hotplug case, adding a flush work queue just after notify handler is removed should be ok.

Thanks,
Shaohua

#4 lore raw
Quoted 18 lines · Shaohua Li
Quoted 17 lines · Zhang Rui

Hi, all,

I found a bug that linux/acpi may execute notify handler that has been removed.

When a system notify(0~0x7f) is received, linux/acpi will first invoke the generic system notify handler (acpi_bus_notify) and then invoke the per-device notify handler if present.

In my case, I add some code in acpi_bus_notify for battery hotplug support, so that the generic system notify handler will remove the battery device, including the per-device notify handler acpi_battery_notify() when receiving notification ACPI_NOTIFY_EJECT_REQUEST.
But linux/acpi invokes the per-device notify handler soon and this breaks the system.

Quoted 6 lines
Quoted 4 lines

Further more, device hot-removal is not the only case to encounter this bug. For example, linux/acpi receives a notification and adds it in the workqueue, and then the driver(notify handler) is removed before kacpid_notify invoke it...

For the non-hotplug case, adding a flush work queue just after notify handler is removed should be ok.

I think, the real problem with ACPI hotplug is that we know which devices might appear -- they are all enumerated in DSDT, but we don't add them until they appear.
If we do .add for all devices in DSDT, and then just .start/.stop them as they appear/disappear, we will have to do much less work, and avoid a problem described above...

Regards,
Alex.

#5 lore raw
Quoted 23 lines · Alexey Starikovskiy
Quoted 22 lines · Shaohua Li
Quoted 21 lines · Zhang Rui

Hi, all,

I found a bug that linux/acpi may execute notify handler that has been removed.

When a system notify(0~0x7f) is received, linux/acpi will first invoke the generic system notify handler (acpi_bus_notify) and then invoke the per-device notify handler if present.

In my case, I add some code in acpi_bus_notify for battery hotplug support, so that the generic system notify handler will remove the battery device, including the per-device notify handler acpi_battery_notify() when receiving notification ACPI_NOTIFY_EJECT_REQUEST.
But linux/acpi invokes the per-device notify handler soon and this breaks the system.

Further more, device hot-removal is not the only case to encounter this bug. For example, linux/acpi receives a notification and adds it in the workqueue, and then the driver(notify handler) is removed before kacpid_notify invoke it...

Quoted 6 lines
Quoted 2 lines

For the non-hotplug case, adding a flush work queue just after notify handler is removed should be ok.

I think, the real problem with ACPI hotplug is that we know which devices might appear -- they are all enumerated in DSDT, but we don't add them until they appear.
If we do .add for all devices in DSDT, and then just .start/.stop them as they appear/disappear, we will have to do much less work, and avoid a problem described above...

If a device is not present, you can't access any method of it.

The real problem here is ACPICA will call some drivers routines (notification handler), but doesn't get a reference count of the driver module.

Thanks,
Shaohua

#6 lore raw
Quoted 24 lines · Shaohua Li
Quoted 23 lines · Alexey Starikovskiy
Quoted 22 lines · Shaohua Li
Quoted 21 lines · Zhang Rui

Hi, all,

I found a bug that linux/acpi may execute notify handler that has been removed.

When a system notify(0~0x7f) is received, linux/acpi will first invoke the generic system notify handler (acpi_bus_notify) and then invoke the per-device notify handler if present.

In my case, I add some code in acpi_bus_notify for battery hotplug support, so that the generic system notify handler will remove the battery device, including the per-device notify handler acpi_battery_notify() when receiving notification ACPI_NOTIFY_EJECT_REQUEST.
But linux/acpi invokes the per-device notify handler soon and this breaks the system.

Further more, device hot-removal is not the only case to encounter this bug. For example, linux/acpi receives a notification and adds it in the workqueue, and then the driver(notify handler) is removed before kacpid_notify invoke it...

Quoted 7 lines
Quoted 6 lines
Quoted 2 lines

For the non-hotplug case, adding a flush work queue just after notify handler is removed should be ok.

I think, the real problem with ACPI hotplug is that we know which devices might appear -- they are all enumerated in DSDT, but we don't add them until they appear.
If we do .add for all devices in DSDT, and then just .start/.stop them as they appear/disappear, we will have to do much less work, and avoid a problem described above...

If a device is not present, you can't access any method of it.

What is because you didn't call .add() yet, or already called .remove(), yes?

Quoted 4 lines

The real problem here is ACPICA will call some drivers routines (notification handler), but doesn't get a reference count of the driver module.

We just should be able to load drivers for declared, but physically absent devices.

Quoted 3 lines

Thanks,
Shaohua

author
#7 lore raw
Quoted 34 lines · Alexey Starikovskiy
Quoted 8 lines · Shaohua Li
Quoted 7 lines · Alexey Starikovskiy
Quoted 6 lines · Shaohua Li
Quoted 5 lines · Zhang Rui

Hi, all,

I found a bug that linux/acpi may execute notify handler that has been removed.

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines

When a system notify(0~0x7f) is received, linux/acpi will first invoke the generic system notify handler (acpi_bus_notify) and then invoke the per-device notify handler if present.

In my case, I add some code in acpi_bus_notify for battery hotplug support, so that the generic system notify handler will remove the battery device, including the per-device notify

handler

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines

acpi_battery_notify() when receiving notification ACPI_NOTIFY_EJECT_REQUEST.
But linux/acpi invokes the per-device notify handler soon and this breaks the system.

Further more, device hot-removal is not the only case to

encounter

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

this bug. For example, linux/acpi receives a notification and

adds it

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 2 lines

in the workqueue, and then the driver(notify handler) is removed before kacpid_notify invoke it...

For the non-hotplug case, adding a flush work queue just after

notify

Quoted 2 lines
Quoted 2 lines
Quoted 1 line

handler is removed should be ok.

I think, the real problem with ACPI hotplug is that we know which

devices might appear --

Quoted 1 line
Quoted 1 line

they are all enumerated in DSDT, but we don't add them until they

Quoted 9 lines

appear.

Quoted 1 line
Quoted 1 line

If we do .add for all devices in DSDT, and then just .start/.stop

them as they appear/disappear,

Quoted 1 line
Quoted 1 line

we will have to do much less work, and avoid a problem described

above...

Quoted 1 line

If a device is not present, you can't access any method of it.

What is because you didn't call .add() yet, or already called .remove(), yes?

Quoted 1 line

Hi, Alexey, sorry that I'm not quite clear about what you mean above. IMO, many methods are evaluated to set all kinds of flags during the acpi_scan_init(), which may be invalid if the device isn't physically present.

Quoted 7 lines
Quoted 2 lines

The real problem here is ACPICA will call some drivers routines (notification handler), but doesn't get a reference count of the

driver

Quoted 1 line

module.

We just should be able to load drivers for declared, but physically absent devices.

Quoted 1 line

For the hot-plug case, we can move all the code to the per-device notify handler, which is called in the last minute.
Then in the generic notify handler, we need to send a uevent to load the driver dynamically when a new device is inserted.
Or we can add the hot_add code in generic notify handler and move the hot_removal code to the per-device notify handler, this is much easier while it's a kind of ugly.

Thanks,
Rui