Bug: linux/acpi may execute notify handler that has been removed
Subsystems (2)
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
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.
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
@@ -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(strucdevice_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 *stif (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
@@ -338,6 +338,8 @@ int acpi_bus_receive_event(struct acpi_bstatic 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
@@ -444,6 +444,7 @@ static int acpi_bus_check_scope(struct areturn 0 ; } +#define ACPI_BATTERY_HID "PNP0C0A"/** * acpi_bus_notify * ---------------
@@ -454,8 +455,8 @@ static void acpi_bus_notify(acpi_handleint 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_handleACPI_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_handleACPI_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 ;
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_notifyfor battery hotplug support, so that the generic system notify handler will remove the battery device, including the per-device notify handleracpi_battery_notify()when receiving notificationACPI_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_notifyinvoke it...
For the non-hotplug case, adding a flush work queue just after notify handler is removed should be ok.
Thanks,
Shaohua
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_notifyfor battery hotplug support, so that the generic system notify handler will remove the battery device, including the per-device notify handleracpi_battery_notify()when receiving notificationACPI_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_notifyinvoke 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.
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_notifyfor battery hotplug support, so that the generic system notify handler will remove the battery device, including the per-device notify handleracpi_battery_notify()when receiving notificationACPI_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_notifyinvoke 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
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_notifyfor battery hotplug support, so that the generic system notify handler will remove the battery device, including the per-device notify handleracpi_battery_notify()when receiving notificationACPI_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_notifyinvoke 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
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_notifyfor battery hotplug support, so that the generic system notify handler will remove the battery device, including the per-device notifyhandler
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
acpi_battery_notify()when receiving notificationACPI_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_notifyinvoke 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