Home » Mailing list archives » linux-kernel » 2010 » June » 8

[PATCH 4/5] ACPI / ACPICA: Fix GPE initialization

Previous thread: [PATCH V2 03/17] MAINTAINERS: Add STAGING - COMEDI by Joe Perches on Monday, June 7, 2010 - 2:14 pm. (1 message)

Next thread: [PATCH] x86/sfi: fix ioapic gsi range by Jacob Pan on Monday, June 7, 2010 - 4:07 pm. (26 messages)
[view original message]
From: Rafael J. Wysocki
Subject: [PATCH] ACPI / ACPICA: Fix problems related to GPE reference counting
Date: Monday, June 7, 2010 - 3:24 pm

There are a few problems with the recently introduced GPE reference
counting mechanism.

First of all, we overlooked the fact that acpi_ev_update_gpes() could
have enabled GPEs before acpi_ev_initialize_gpe_block() was called
and some GPEs are enabled twice during the initialization as a
result.

Second, acpi_set_gpe() won't enable wakeup-only GPEs, because it
attempts to do that with the help of acpi_ev_enable_gpe() which, in
turn, calls acpi_hw_write_gpe_enable_reg() that will only enable the
GPE if its bit is set in the register's enable_for_run.  Worse yet,
acpi_hw_write_gpe_enable_reg() always writes the entire
enable_for_run mask into the register, so in theory it may enable
more GPEs than just the one we want to enable (for example, it may
enable GPEs in the same register that were disabled by
acpi_hw_low_disable_gpe() previously).

Next, the enable_for_run and enable_for_wake masks of the GPE
registers only change when the GPEs' runtime_count and wakeup_count
fields change from zero to nonzero or the other way around, so it
doesn't make sense to update the enable_for_run and enable_for_wake
masks anywhere outside of acpi_enable_gpe() and acpi_disable_gpe().

Finally, the sysfs interface allowing user space to disable/enable
GPEs doesn't work correctly, because a GPE disabled this way may be
re-enabled shortly by acpi_ev_asynch_enable_gpe() if its bit is set
in its register's enable_for_run mask.

To address these problems, call acpi_ev_update_gpe_enable_masks()
directly from acpi_enable_gpe() and acpi_disable_gpe() where the
GPE's runtime_count and wakeup_count counters change in such a
way that its register's enable_for_run and enable_for_wake masks have
to be updated.  Change acpi_hw_low_disable_gpe() into
acpi_hw_low_set_gpe() that can set as well as clear the appropriate
bit in the GPE's register and use it wherever necessary.

Add a new action constant, ACPI_GPE_CHECK_AND_ENABLE, that will cause
acpi_hw_low_set_gpe() to check if the GPE's bit in its ...
[ message continues ]
[view original message]
From: Rafael J. Wysocki
Subject: [PATCH 1/5] ACPI / ACPICA: Use helper function for computing GPE masks
Date: Tuesday, June 8, 2010 - 1:48 am

From: Rafael J. Wysocki <rjw@sisk.pl>

In quite a few places ACPICA needs to compute a GPE enable mask with
only one bit, corresponding to a given GPE, set.  Currently, that
computation is always open coded which leads to unnecessary code
duplication.  Fix this by introducing a helper function for computing
one-bit GPE enable masks and using it where appropriate.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/acpica/achware.h |    3 ++
 drivers/acpi/acpica/evgpe.c   |    7 ++---
 drivers/acpi/acpica/hwgpe.c   |   52 ++++++++++++++++++++++++++++++++----------
 3 files changed, 46 insertions(+), 16 deletions(-)

Index: linux-2.6/drivers/acpi/acpica/achware.h
===================================================================
--- linux-2.6.orig/drivers/acpi/acpica/achware.h
+++ linux-2.6/drivers/acpi/acpica/achware.h
@@ -90,6 +90,9 @@ acpi_status acpi_hw_write_port(acpi_io_a
 /*
  * hwgpe - GPE support
  */
+u32 acpi_hw_gpe_register_bit(struct acpi_gpe_event_info *gpe_event_info,
+			     struct acpi_gpe_register_info *gpe_register_info);
+
 acpi_status acpi_hw_low_disable_gpe(struct acpi_gpe_event_info *gpe_event_info);
 
 acpi_status
Index: linux-2.6/drivers/acpi/acpica/hwgpe.c
===================================================================
--- linux-2.6.orig/drivers/acpi/acpica/hwgpe.c
+++ linux-2.6/drivers/acpi/acpica/hwgpe.c
@@ -57,6 +57,27 @@ acpi_hw_enable_wakeup_gpe_block(struct a
 
 /******************************************************************************
  *
+ * FUNCTION:	acpi_hw_gpe_register_bit
+ *
+ * PARAMETERS:	gpe_event_info	    - Info block for the GPE
+ *		gpe_register_info   - Info block for the GPE register
+ *
+ * RETURN:	Status
+ *
+ * DESCRIPTION:	Compute GPE enable mask with one bit corresponding to the given
+ *		GPE set.
+ *
+ ******************************************************************************/
+
+u32 acpi_hw_gpe_register_bit(struct acpi_gpe_event_info *gpe_event_info,
+			     struct ...
[ message continues ]
[view original message]
From: Rafael J. Wysocki
Subject: [PATCH 3/5] ACPI / ACPICA: Avoid writing full enable masks to GPE registers
Date: Tuesday, June 8, 2010 - 1:49 am

From: Rafael J. Wysocki <rjw@sisk.pl>

ACPICA uses acpi_hw_write_gpe_enable_reg() to re-enable a GPE after
an event signaled by it has been handled.  However, this function
writes the entire GPE enable mask to the GPE's enable register which
may not be correct.  Namely, if one of the other GPEs in the same
register was previously enabled by acpi_enable_gpe() and subsequently
disabled using acpi_set_gpe(), acpi_hw_write_gpe_enable_reg() will
re-enable it along with the target GPE.

To fix this issue rework acpi_hw_write_gpe_enable_reg() so that it
calls acpi_hw_low_set_gpe() with a special action value,
ACPI_GPE_COND_ENABLE, that will make it only enable the GPE if the
corresponding bit in its register's enable_for_run mask is set.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/acpica/hwgpe.c |   18 +++++-------------
 include/acpi/actypes.h      |    1 +
 2 files changed, 6 insertions(+), 13 deletions(-)

Index: linux-2.6/drivers/acpi/acpica/hwgpe.c
===================================================================
--- linux-2.6.orig/drivers/acpi/acpica/hwgpe.c
+++ linux-2.6/drivers/acpi/acpica/hwgpe.c
@@ -118,6 +118,10 @@ acpi_hw_low_set_gpe(struct acpi_gpe_even
 	register_bit = acpi_hw_gpe_register_bit(gpe_event_info,
 						gpe_register_info);
 	switch (action) {
+	case ACPI_GPE_COND_ENABLE:
+		if (!(register_bit & gpe_register_info->enable_for_run))
+			return (AE_BAD_PARAMETER);
+
 	case ACPI_GPE_ENABLE:
 		ACPI_SET_BIT(enable_mask, register_bit);
 		break;
@@ -154,23 +158,11 @@ acpi_hw_low_set_gpe(struct acpi_gpe_even
 acpi_status
 acpi_hw_write_gpe_enable_reg(struct acpi_gpe_event_info * gpe_event_info)
 {
-	struct acpi_gpe_register_info *gpe_register_info;
 	acpi_status status;
 
 	ACPI_FUNCTION_ENTRY();
 
-	/* Get the info block for the entire GPE register */
-
-	gpe_register_info = gpe_event_info->register_info;
-	if (!gpe_register_info) {
-		return (AE_NOT_EXIST);
-	}
-
-	/* Write the entire GPE (runtime) enable register ...
[ message continues ]
[view original message]
From: Rafael J. Wysocki
Subject: [PATCH 5/5] ACPI / ACPICA: Fix sysfs GPE interface
Date: Tuesday, June 8, 2010 - 1:50 am

From: Rafael J. Wysocki <rjw@sisk.pl>

The sysfs interface allowing user space to disable/enable GPEs
doesn't work correctly, because a GPE disabled this way will be
re-enabled shortly by acpi_ev_asynch_enable_gpe() if it was
previosuly enabled by acpi_enable_gpe() (in which case the
corresponding bit in its enable register's enable_for_run mask is
set).

To address this issue make the sysfs GPE interface use
acpi_enable_gpe() and acpi_disable_gpe() instead of acpi_set_gpe()
so that GPE reference counters are modified by it along with the
values of GPE enable registers.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/system.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Index: linux-2.6/drivers/acpi/system.c
===================================================================
--- linux-2.6.orig/drivers/acpi/system.c
+++ linux-2.6/drivers/acpi/system.c
@@ -388,10 +388,12 @@ static ssize_t counter_set(struct kobjec
 	if (index < num_gpes) {
 		if (!strcmp(buf, "disable\n") &&
 				(status & ACPI_EVENT_FLAG_ENABLED))
-			result = acpi_set_gpe(handle, index, ACPI_GPE_DISABLE);
+			result = acpi_disable_gpe(handle, index,
+						ACPI_GPE_TYPE_RUNTIME);
 		else if (!strcmp(buf, "enable\n") &&
 				!(status & ACPI_EVENT_FLAG_ENABLED))
-			result = acpi_set_gpe(handle, index, ACPI_GPE_ENABLE);
+			result = acpi_enable_gpe(handle, index,
+						ACPI_GPE_TYPE_RUNTIME);
 		else if (!strcmp(buf, "clear\n") &&
 				(status & ACPI_EVENT_FLAG_SET))
 			result = acpi_clear_gpe(handle, index);

--


[ message continues ]
[view original message]
From: Rafael J. Wysocki
Subject: [PATCH 0/5] ACPI / ACPICA: Fix problems related to GPE reference counting
Date: Tuesday, June 8, 2010 - 1:46 am

Hi,

I decided to split this big patch into a series of smaller ones addressing one
issue each.  I think all of the issues addressed by [2-5/5] may be regarded as
regressions from 2.6.33, because that kernel didn't have these problems.

[1/5] - Not really a fix, but [2/5] depends on it and I don't see a reason to
        duplicate that thing even more.
[2/5] - Low-level GPE manipulation fix
[3/5] - Fix possible issue with re-enabling a GPE after event handling
[4/5] - Fix the initialization of GPEs
[5/5] - Fix GPE sysfs interface

Please review, possibly apply.

Thanks,
Rafael

--


[ message continues ]
[view original message]
From: Rafael J. Wysocki
Subject: [PATCH 4/5] ACPI / ACPICA: Fix GPE initialization
Date: Tuesday, June 8, 2010 - 1:50 am

From: Rafael J. Wysocki <rjw@sisk.pl>

While developing the GPE reference counting code we overlooked the
fact that acpi_ev_update_gpes() could have enabled GPEs before
acpi_ev_initialize_gpe_block() was called.  As a result, some GPEs
are enabled twice during the initialization.

To fix this issue avoid calling acpi_enable_gpe() from
acpi_ev_initialize_gpe_block() for the GPEs that have nonzero
runtime reference counters.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/acpica/evgpeblk.c |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/acpi/acpica/evgpeblk.c
===================================================================
--- linux-2.6.orig/drivers/acpi/acpica/evgpeblk.c
+++ linux-2.6/drivers/acpi/acpica/evgpeblk.c
@@ -500,6 +500,19 @@ acpi_ev_initialize_gpe_block(struct acpi
 
 			gpe_index = (i * ACPI_GPE_REGISTER_WIDTH) + j;
 			gpe_event_info = &gpe_block->event_info[gpe_index];
+			gpe_number = gpe_index + gpe_block->block_base_number;
+
+			/*
+			 * If the GPE has already been enabled for runtime
+			 * signaling, make sure it remains enabled, but do not
+			 * increment its reference counter.
+			 */
+			if (gpe_event_info->runtime_count) {
+				acpi_set_gpe(gpe_device, gpe_number,
+						ACPI_GPE_ENABLE);
+				gpe_enabled_count++;
+				continue;
+			}
 
 			if (gpe_event_info->flags & ACPI_GPE_CAN_WAKE) {
 				wake_gpe_count++;
@@ -516,7 +529,6 @@ acpi_ev_initialize_gpe_block(struct acpi
 
 			/* Enable this GPE */
 
-			gpe_number = gpe_index + gpe_block->block_base_number;
 			status = acpi_enable_gpe(gpe_device, gpe_number,
 						 ACPI_GPE_TYPE_RUNTIME);
 			if (ACPI_FAILURE(status)) {

--


[ message continues ]
[view original message]
From: Rafael J. Wysocki
Subject: [PATCH 2/5] ACPI / ACPICA: Fix low-level GPE manipulation code
Date: Tuesday, June 8, 2010 - 1:49 am

From: Rafael J. Wysocki <rjw@sisk.pl>

ACPICA uses acpi_ev_enable_gpe() for enabling GPEs at the low level,
which is incorrect, because this function only enables the GPE if the
corresponding bit in its enable register's enable_for_run mask is set.
This causes acpi_set_gpe() to work incorrectly if used for enabling
GPEs that were not previously enabled with acpi_enable_gpe().  As a
result, among other things, wakeup-only GPEs are never enabled by
acpi_enable_wakeup_device(), so the devices that use them are unable
to wake up the system.

To fix this issue remove acpi_ev_enable_gpe() and its counterpart
acpi_ev_disable_gpe() and replace acpi_hw_low_disable_gpe() with
acpi_hw_low_set_gpe() that will be used instead to manipulate GPE
enable bits at the low level.  Make the users of acpi_ev_enable_gpe()
and acpi_ev_disable_gpe() call acpi_hw_low_set_gpe() instead and
make sure that GPE enable masks are only updated by acpi_enable_gpe()
and acpi_disable_gpe() when GPE reference counters change from 0
to 1 and from 1 to 0, respectively.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/acpica/acevents.h |    4 -
 drivers/acpi/acpica/achware.h  |    3 -
 drivers/acpi/acpica/evgpe.c    |  108 -----------------------------------------
 drivers/acpi/acpica/evxface.c  |    2 
 drivers/acpi/acpica/evxfevnt.c |   59 ++++++++++++++++++++--
 drivers/acpi/acpica/hwgpe.c    |   26 +++++++--
 include/acpi/actypes.h         |    2 
 7 files changed, 80 insertions(+), 124 deletions(-)

Index: linux-2.6/drivers/acpi/acpica/hwgpe.c
===================================================================
--- linux-2.6.orig/drivers/acpi/acpica/hwgpe.c
+++ linux-2.6/drivers/acpi/acpica/hwgpe.c
@@ -78,23 +78,27 @@ u32 acpi_hw_gpe_register_bit(struct acpi
 
 /******************************************************************************
  *
- * FUNCTION:	acpi_hw_low_disable_gpe
+ * FUNCTION:	acpi_hw_low_set_gpe
  *
  * PARAMETERS:	gpe_event_info	    - Info block for the GPE to be ...
[ message continues ]
[view original message]
From: Len Brown
Subject: Re: [PATCH 0/5] ACPI / ACPICA: Fix problems related to GPE reference counting
Date: Tuesday, June 8, 2010 - 9:53 pm

series applied to acpi-test

thanks,
Len Brown, Intel Open Source Technology Center

--


[ message continues ]
Previous thread: [PATCH V2 03/17] MAINTAINERS: Add STAGING - COMEDI by Joe Perches on Monday, June 7, 2010 - 2:14 pm. (1 message)

Next thread: [PATCH] x86/sfi: fix ioapic gsi range by Jacob Pan on Monday, June 7, 2010 - 4:07 pm. (26 messages)