Re: [PATCH] ACPI / ACPICA: Fix global lock acquisition

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Friday, January 07, 2011, Moore, Robert wrote:
> Hi Rafael,
> 
> We are integrating this change into ACPICA. So far, looks good, I like the
> solution.  I will send you the integrated functions when I'm finished, for
> your review.

OK, thanks a lot!

> As always, it is a little tricky because of various Linux<->ACPICA divergences.

Understood.

Rafael

 
> >-----Original Message-----
> >From: Rafael J. Wysocki [mailto:rjw@sisk.pl]
> >Sent: Tuesday, December 28, 2010 2:06 AM
> >To: ACPI Devel Maling List
> >Cc: Len Brown; Lin, Ming M; Moore, Robert; Thomas Renninger
> >Subject: [PATCH] ACPI / ACPICA: Fix global lock acquisition
> >
> >From: Rafael J. Wysocki <rjw@sisk.pl>
> >
> >There are two problems with the ACPICA's current implementation of
> >the global lock acquisition.  First, acpi_ev_global_lock_handler(),
> >which in fact is an interface to the outside of the kernel, doesn't
> >validate its input, so it only works correctly if the other side
> >(i.e. the ACPI firmware) is fully specification-compliant (as far
> >as the global lock is concerned).  Unfortunately, that's known not
> >to be the case on some systems (i.e. we get spurious global lock
> >signaling interrupts without the pending flag set on some systems).
> >Second, acpi_ev_global_lock_handler() attempts to acquire the global
> >lock on behalf of a thread waiting for it without checking if there
> >actually is such a thread.  Both of these shortcomings need to be
> >addressed to prevent all possible race conditions from happening.
> >
> >Rework acpi_ev_global_lock_handler() so that it doesn't try to
> >acquire the global lock and make it signal the availability of the
> >global lock to the waiting thread instead.  Make sure that the
> >availability of the global lock can only be signaled when there
> >is a thread waiting for it and that it can't be signaled more than
> >once in a row (to keep acpi_gbl_global_lock_semaphore in balance).
> >
> >Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> >---
> > drivers/acpi/acpica/evmisc.c |   94 +++++++++++++++++++++++++-------------
> >-----
> > 1 file changed, 55 insertions(+), 39 deletions(-)
> >
> >Index: linux-2.6/drivers/acpi/acpica/evmisc.c
> >===================================================================
> >--- linux-2.6.orig/drivers/acpi/acpica/evmisc.c
> >+++ linux-2.6/drivers/acpi/acpica/evmisc.c
> >@@ -284,41 +284,41 @@ static void ACPI_SYSTEM_XFACE acpi_ev_no
> >  * RETURN:      ACPI_INTERRUPT_HANDLED
> >  *
> >  * DESCRIPTION: Invoked directly from the SCI handler when a global lock
> >- *              release interrupt occurs. Attempt to acquire the global
> >lock,
> >- *              if successful, signal the thread waiting for the lock.
> >+ *              release interrupt occurs.  If there's a thread waiting for
> >+ *              the global lock, signal it.
> >  *
> >  * NOTE: Assumes that the semaphore can be signaled from interrupt level.
> >If
> >  * this is not possible for some reason, a separate thread will have to be
> >  * scheduled to do this.
> >  *
> >
> >***************************************************************************
> >***/
> >+static u8 acpi_ev_global_lock_pending;
> >+static spinlock_t _acpi_ev_global_lock_pending_lock;
> >+#define acpi_ev_global_lock_pending_lock
> >&_acpi_ev_global_lock_pending_lock
> >
> > static u32 acpi_ev_global_lock_handler(void *context)
> > {
> >-	u8 acquired = FALSE;
> >+	acpi_status status;
> >+	acpi_cpu_flags flags;
> >
> >-	/*
> >-	 * Attempt to get the lock.
> >-	 *
> >-	 * If we don't get it now, it will be marked pending and we will
> >-	 * take another interrupt when it becomes free.
> >-	 */
> >-	ACPI_ACQUIRE_GLOBAL_LOCK(acpi_gbl_FACS, acquired);
> >-	if (acquired) {
> >+	flags = acpi_os_acquire_lock(acpi_ev_global_lock_pending_lock);
> >
> >-		/* Got the lock, now wake all threads waiting for it */
> >+	if (!acpi_ev_global_lock_pending) {
> >+		goto out;
> >+	}
> >
> >-		acpi_gbl_global_lock_acquired = TRUE;
> >-		/* Send a unit to the semaphore */
> >+	/* Send a unit to the semaphore */
> >
> >-		if (ACPI_FAILURE
> >-		    (acpi_os_signal_semaphore
> >-		     (acpi_gbl_global_lock_semaphore, 1))) {
> >-			ACPI_ERROR((AE_INFO,
> >-				    "Could not signal Global Lock semaphore"));
> >-		}
> >+	status = acpi_os_signal_semaphore(acpi_gbl_global_lock_semaphore, 1);
> >+	if (ACPI_FAILURE(status)) {
> >+		ACPI_ERROR((AE_INFO, "Could not signal Global Lock
> >semaphore"));
> > 	}
> >
> >+	acpi_ev_global_lock_pending = FALSE;
> >+
> >+ out:
> >+	acpi_os_release_lock(acpi_ev_global_lock_pending_lock, flags);
> >+
> > 	return (ACPI_INTERRUPT_HANDLED);
> > }
> >
> >@@ -415,6 +415,7 @@ static int acpi_ev_global_lock_acquired;
> >
> > acpi_status acpi_ev_acquire_global_lock(u16 timeout)
> > {
> >+	acpi_cpu_flags flags;
> > 	acpi_status status = AE_OK;
> > 	u8 acquired = FALSE;
> >
> >@@ -467,32 +468,47 @@ acpi_status acpi_ev_acquire_global_lock(
> > 		return_ACPI_STATUS(AE_OK);
> > 	}
> >
> >-	/* Attempt to acquire the actual hardware lock */
> >+	flags = acpi_os_acquire_lock(acpi_ev_global_lock_pending_lock);
> >+
> >+	do {
> >+
> >+		/* Attempt to acquire the actual hardware lock */
> >
> >-	ACPI_ACQUIRE_GLOBAL_LOCK(acpi_gbl_FACS, acquired);
> >-	if (acquired) {
> >+		ACPI_ACQUIRE_GLOBAL_LOCK(acpi_gbl_FACS, acquired);
> >+		if (acquired) {
> >+			acpi_gbl_global_lock_acquired = TRUE;
> >
> >-		/* We got the lock */
> >+			ACPI_DEBUG_PRINT((ACPI_DB_EXEC,
> >+					  "Acquired hardware Global Lock\n"));
> >+			break;
> >+		}
> >+
> >+		acpi_ev_global_lock_pending = TRUE;
> >+
> >+		acpi_os_release_lock(acpi_ev_global_lock_pending_lock, flags);
> >
> >+		/*
> >+		 * Did not get the lock. The pending bit was set above, and we
> >+		 * must wait until we get the global lock released interrupt.
> >+		 */
> > 		ACPI_DEBUG_PRINT((ACPI_DB_EXEC,
> >-				  "Acquired hardware Global Lock\n"));
> >+				  "Waiting for hardware Global Lock\n"));
> >
> >-		acpi_gbl_global_lock_acquired = TRUE;
> >-		return_ACPI_STATUS(AE_OK);
> >-	}
> >+		/*
> >+		 * Wait for handshake with the global lock interrupt handler.
> >+		 * This interface releases the interpreter if we must wait.
> >+		 */
> >+		status = acpi_ex_system_wait_semaphore(
> >+						acpi_gbl_global_lock_semaphore,
> >+						ACPI_WAIT_FOREVER);
> >
> >-	/*
> >-	 * Did not get the lock. The pending bit was set above, and we must
> >now
> >-	 * wait until we get the global lock released interrupt.
> >-	 */
> >-	ACPI_DEBUG_PRINT((ACPI_DB_EXEC, "Waiting for hardware Global
> >Lock\n"));
> >+		flags = acpi_os_acquire_lock(acpi_ev_global_lock_pending_lock);
> >
> >-	/*
> >-	 * Wait for handshake with the global lock interrupt handler.
> >-	 * This interface releases the interpreter if we must wait.
> >-	 */
> >-	status =
> >acpi_ex_system_wait_semaphore(acpi_gbl_global_lock_semaphore,
> >-					       ACPI_WAIT_FOREVER);
> >+	} while (ACPI_SUCCESS(status));
> >+
> >+	acpi_ev_global_lock_pending = FALSE;
> >+
> >+	acpi_os_release_lock(acpi_ev_global_lock_pending_lock, flags);
> >
> > 	return_ACPI_STATUS(status);
> > }
> 
>