[RFC][PATCH] ACPI: Eliminate race conditions related to removing event handlers
Rafael J. Wysocki
The current ACPI events handling code has two design issues that
need fixing. First, there is a race condition between the event
queues and the removal of GPE handlers and notify handlers that may
cause a handler to be removed while it's being executed, which in
turn may lead to udefined behavior. Second, the GPE handler removal
routine, acpi_remove_gpe_handler(), releases ACPI_MTX_EVENTS in the
middle of the operation, which effectively renders the locking
useless. It turns out that the both of them can be fixed at the same
time.
To fix the first issue use the observation that all of the ACPI
workqueues are singlethread, so it is possible to flush the
execution of queued work by inserting a cleverly crafted work item
into the appropriate workqueue. For this purpose use a barrier work
structure containing two completion objects, 'run' and 'exit', and a
pair of interface functions, acpi_os_add_events_barrier() and
acpi_os_remove_events_barrier(), the first of which puts a barrier
work into the workqueue indicated by its argument and waits for the
'run' completion to be completed, and the second of which completes
the 'exit' completion (the first one returns a pointer to the barrier
work object to be passed as an argument to the second one). The work
function inserted into the workqueue by acpi_os_add_events_barrier()
completes the 'run' completion and waits for the 'exit' completion.
Thus, by executing acpi_os_add_events_barrier() with an appropriate
type argument the caller ensures that all events of this type queued
up earlier will be handled before it is allowed to continue running
and all events of this type queued up later will be handled until
acpi_os_remove_events_barrier() is called.
Modify acpi_remove_notify_handler() and acpi_remove_gpe_handler() to
use this interface, which also fixes the second issue, because the
new interface replaces acpi_os_wait_events_complete() that was the
reason for the releasing of ACPI_MTX_EVENTS in the middle of the
operation by acpi_remove_gpe_handler().
Signed-off-by: Rafael J. Wysocki <r...@sisk.pl>
---
drivers/acpi/acpica/evxface.c | 33 +++++++-----
drivers/acpi/osl.c | 113 ++++++++++++++++++++++++++++++++++--------
include/acpi/acpiosxf.h | 3 -
3 files changed, 115 insertions(+), 34 deletions(-)
Index: linux-2.6/drivers/acpi/osl.c
===================================================================
--- linux-2.6.orig/drivers/acpi/osl.c
+++ linux-2.6/drivers/acpi/osl.c
@@ -61,6 +61,12 @@ struct acpi_os_dpc {
int wait;
};
+struct acpi_os_barrier_work {
+ struct completion run;
+ struct completion exit;
+ struct work_struct work;
+};
+
#ifdef CONFIG_ACPI_CUSTOM_DSDT
#include CONFIG_ACPI_CUSTOM_DSDT_FILE
#endif
@@ -700,28 +706,15 @@ static void acpi_os_execute_deferred(str
{
struct acpi_os_dpc *dpc = container_of(work, struct acpi_os_dpc, work);
- if (dpc->wait)
- acpi_os_wait_events_complete(NULL);
+ if (dpc->wait) {
+ flush_workqueue(kacpid_wq);
+ flush_workqueue(kacpi_notify_wq);
+ }
dpc->function(dpc->context);
kfree(dpc);
}
-/*******************************************************************************
- *
- * FUNCTION: acpi_os_execute
- *
- * PARAMETERS: Type - Type of the callback
- * Function - Function to be executed
- * Context - Function parameters
- *
- * RETURN: Status
- *
- * DESCRIPTION: Depending on type, either queues function for deferred execution or
- * immediately executes function on a separate thread.
- *
- ******************************************************************************/
-
static acpi_status __acpi_os_execute(acpi_execute_type type,
acpi_osd_exec_callback function, void *context, int hp)
{
@@ -770,6 +763,20 @@ static acpi_status __acpi_os_execute(acp
return status;
}
+/*******************************************************************************
+ *
+ * FUNCTION: acpi_os_execute
+ *
+ * PARAMETERS: Type - Type of the callback
+ * Function - Function to be executed
+ * Context - Function parameters
+ *
+ * RETURN: Status
+ *
+ * DESCRIPTION: Depending on type, either queues function for deferred execution or
+ * immediately executes function on a separate thread.
+ *
+ ******************************************************************************/
acpi_status acpi_os_execute(acpi_execute_type type,
acpi_osd_exec_callback function, void *context)
{
@@ -783,13 +790,77 @@ acpi_status acpi_os_hotplug_execute(acpi
return __acpi_os_execute(0, function, context, 1);
}
-void acpi_os_wait_events_complete(void *context)
+static void acpi_os_barrier_fn(struct work_struct *work)
{
- flush_workqueue(kacpid_wq);
- flush_workqueue(kacpi_notify_wq);
+ struct acpi_os_barrier_work *barrier;
+
+ barrier = container_of(work, struct acpi_os_barrier_work, work);
+ complete(&barrier->run);
+ wait_for_completion(&barrier->exit);
+ kfree(barrier);
+}
+
+/*******************************************************************************
+ *
+ * FUNCTION: acpi_os_add_events_barrier
+ *
+ * PARAMETERS: type - Type of events to add the barrier for
+ *
+ * RETURN: Context to be passed to acpi_os_remove_events_barrier() or NULL
+ * on failure.
+ *
+ * DESCRIPTION: Ensure that all of the events of given type either have been
+ * handled before we continue running or will not be handled until
+ * acpi_os_remove_events_barrier() is run.
+ *
+ ******************************************************************************/
+void *acpi_os_add_events_barrier(acpi_execute_type type)
+{
+ struct acpi_os_barrier_work *barrier;
+ struct workqueue_struct *queue;
+ int ret;
+
+ barrier = kmalloc(sizeof(*barrier), GFP_KERNEL);
+ if (!barrier)
+ return NULL;
+
+ init_completion(&barrier->run);
+ init_completion(&barrier->exit);
+ INIT_WORK(&barrier->work, acpi_os_barrier_fn);
+ queue = (type == OSL_NOTIFY_HANDLER) ? kacpi_notify_wq : kacpid_wq;
+ ret = queue_work(queue, &barrier->work);
+ if (!ret) {
+ printk(KERN_ERR PREFIX "queue_work() failed!\n");
+ kfree(barrier);
+ return NULL;
+ }
+
+ wait_for_completion(&barrier->run);
+
+ return barrier;
}
+EXPORT_SYMBOL(acpi_os_add_events_barrier);
+
+/*******************************************************************************
+ *
+ * FUNCTION: acpi_os_remove_events_barrier
+ *
+ * PARAMETERS: context - Information needed to remove the barrier
+ *
+ * RETURN: None
+ *
+ * DESCRIPTION: Using the context information remove an events barrier added by
+ * acpi_os_add_events_barrier().
+ *
+ ******************************************************************************/
+void acpi_os_remove_events_barrier(void *context)
+{
+ struct acpi_os_barrier_work *barrier = context;
-EXPORT_SYMBOL(acpi_os_wait_events_complete);
+ if (barrier)
+ complete(&barrier->exit);
+}
+EXPORT_SYMBOL(acpi_os_remove_events_barrier);
/*
* Allocate the memory for a spinlock and initialize it.
Index: linux-2.6/include/acpi/acpiosxf.h
===================================================================
--- linux-2.6.orig/include/acpi/acpiosxf.h
+++ linux-2.6/include/acpi/acpiosxf.h
@@ -194,7 +194,8 @@ acpi_os_execute(acpi_execute_type type,
acpi_status
acpi_os_hotplug_execute(acpi_osd_exec_callback function, void *context);
-void acpi_os_wait_events_complete(void *context);
+void *acpi_os_add_events_barrier(acpi_execute_type type);
+void acpi_os_remove_events_barrier(void *context);
void acpi_os_sleep(acpi_integer milliseconds);
Index: linux-2.6/drivers/acpi/acpica/evxface.c
===================================================================
--- linux-2.6.orig/drivers/acpi/acpica/evxface.c
+++ linux-2.6/drivers/acpi/acpica/evxface.c
@@ -497,6 +497,7 @@ acpi_remove_notify_handler(acpi_handle d
union acpi_operand_object *notify_obj;
union acpi_operand_object *obj_desc;
struct acpi_namespace_node *node;
+ void *events_barrier;
acpi_status status;
ACPI_FUNCTION_TRACE(acpi_remove_notify_handler);
@@ -511,11 +512,16 @@ acpi_remove_notify_handler(acpi_handle d
/* Make sure all deferred tasks are completed */
- acpi_os_wait_events_complete(NULL);
+
+ events_barrier = acpi_os_add_events_barrier(OSL_NOTIFY_HANDLER);
+ if (!events_barrier) {
+ status = AE_ERROR;
+ goto exit;
+ }
status = acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE);
if (ACPI_FAILURE(status)) {
- goto exit;
+ goto release_events;
}
/* Convert and validate the device handle */
@@ -653,6 +659,8 @@ acpi_remove_notify_handler(acpi_handle d
unlock_and_exit:
(void)acpi_ut_release_mutex(ACPI_MTX_NAMESPACE);
+ release_events:
+ acpi_os_remove_events_barrier(events_barrier);
exit:
if (ACPI_FAILURE(status))
ACPI_EXCEPTION((AE_INFO, status, "Removing notify handler"));
@@ -774,6 +782,7 @@ acpi_remove_gpe_handler(acpi_handle gpe_
{
struct acpi_gpe_event_info *gpe_event_info;
struct acpi_handler_info *handler;
+ void *events_barrier;
acpi_status status;
acpi_cpu_flags flags;
@@ -785,9 +794,16 @@ acpi_remove_gpe_handler(acpi_handle gpe_
return_ACPI_STATUS(AE_BAD_PARAMETER);
}
+ /* Make sure all deferred tasks are completed */
+
+ events_barrier = acpi_os_add_events_barrier(OSL_GPE_HANDLER);
+ if (!events_barrier) {
+ return_ACPI_STATUS(AE_ERROR);
+ }
+
status = acpi_ut_acquire_mutex(ACPI_MTX_EVENTS);
if (ACPI_FAILURE(status)) {
- return_ACPI_STATUS(status);
+ goto release_events;
}
/* Ensure that we have a valid GPE number */
@@ -813,15 +829,6 @@ acpi_remove_gpe_handler(acpi_handle gpe_
goto unlock_and_exit;
}
- /* Make sure all deferred tasks are completed */
-
- (void)acpi_ut_release_mutex(ACPI_MTX_EVENTS);
- acpi_os_wait_events_complete(NULL);
- status = acpi_ut_acquire_mutex(ACPI_MTX_EVENTS);
- if (ACPI_FAILURE(status)) {
- return_ACPI_STATUS(status);
- }
-
/* Remove the handler */
flags = acpi_os_acquire_lock(acpi_gbl_gpe_lock);
@@ -842,6 +849,8 @@ acpi_remove_gpe_handler(acpi_handle gpe_
unlock_and_exit:
(void)acpi_ut_release_mutex(ACPI_MTX_EVENTS);
+ release_events:
+ acpi_os_remove_events_barrier(events_barrier);
return_ACPI_STATUS(status);
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Moore, Robert
Rafael,
I've got some issues with this design.
First of all, are the two interfaces, acpi_remove_notify_handler() and acpi_remove_gpe_handler(), really being used in such a way to require such synchronization? Is there any way to limit their use to times where it is known that no handlers will be executing?
In addition, I'm afraid that this whole "events" mechanism, with an interface to wait for event completion, and now the concept of adding "barriers" to the event queue, is much too OS-specific for the core ACPICA code. In the first place, the acpi_os_wait_events_complete is not part of the ACPICA code (it is not called by ACPICA in the code base, only in the Linux version). Such a mechanism has not been requested by any of the other users of ACPICA, so we have not attempted to add it. This is not to say that such a mechanism will not be required by other users in the future however.
Bob
Rafael J. Wysocki
>
> Rafael,
>
> I've got some issues with this design.
>
> First of all, are the two interfaces, acpi_remove_notify_handler() and
> acpi_remove_gpe_handler(), really being used in such a way to require such
> synchronization?
I had a problem with that when I was trying to implement adding more than one
system notify handler per device at the PCI level. Specifically, I was trying
to create an extra layer of notify objects to be used by the PCI layer for
installing ACPI system notify handlers such that every PCI device had one
handler that might handle wakeup notifications as well as hotplug
notifications. That didn't work, because I needed to remove a notify object
from memory after calling acpi_remove_notify_handler() for the handler
associated with it and I was unable to tell whether that was safe.
The patch at http://patchwork.kernel.org/patch/80104/ solved this issue for me
(BTW, you told me you'd have a look at this one :-)), but I think it generally
is good to know a handler is not being executed after
acpi_remove_notify_handler() or acpi_remove_gpe_handler() has been called
for it.
> Is there any way to limit their use to times where it is known that no
> handlers will be executing?
I don't think this is generally possible. Since the event handlers are called
from a workqueue, we can't really guarantee any synchronization between them
and the other threads unless we use a mechanism that will enforce it.
> In addition, I'm afraid that this whole "events" mechanism, with an interface
> to wait for event completion, and now the concept of adding "barriers" to the
> event queue, is much too OS-specific for the core ACPICA code.
OK, I'm not going to insist. However, I think that
acpi_os_wait_events_complete() should be called before we try to acquire
ACPI_MTX_EVENTS for the first time, because dropping it "temporarily" just in
order to call acpi_os_wait_events_complete() really defeats the purpose of the
locking.
Rafael