acpi: Avoid dropping rapid hotkey events (or other GPEs) on Asus EeePC

It looks like this EC clears the SMI_EVT bit after every query, even if there are more events pending. The workaround is to repeatedly query the EC until it reports that no events remain.

This fixes a regression in 2.6.26 (from 2.6.25.3). Initially reported as "Asus Eee PC hotkeys stop working if pressed quickly" in bugzilla
<http://bugzilla.kernel.org/show_bug.cgi?id=11089>.

The regression was caused by a recently added check for interrupt storms. The Eee PC triggers this check and switches to polling. When multiple events arrive between polling intervals, only one is fetched from the EC. This causes erroneous behaviour; ultimately events stop being delivered altogether when the EC buffer overflows.

Signed-off-by Alan Jenkins <[email protected]>

---

drivers/acpi/ec.c +13 -5

index 5622aee..2b4c5a2 100644
@@ -459,14 +459,10 @@ void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit)
 
 EXPORT_SYMBOL_GPL(acpi_ec_remove_query_handler);
 
-static void acpi_ec_gpe_query(void *ec_cxt)
+static void acpi_ec_gpe_run_handler(struct acpi_ec *ec, u8 value)
 {
-	struct acpi_ec *ec = ec_cxt;
-	u8 value = 0;
 	struct acpi_ec_query_handler *handler, copy;
 
-	if (!ec || acpi_ec_query(ec, &value))
-		return;
 	mutex_lock(&ec->lock);
 	list_for_each_entry(handler, &ec->list, node) {
 		if (value == handler->query_bit) {

@@ -484,6 +480,18 @@ static void acpi_ec_gpe_query(void *ec_cxt)
 	mutex_unlock(&ec->lock);
 }
 
+static void acpi_ec_gpe_query(void *ec_cxt)
+{
+	struct acpi_ec *ec = ec_cxt;
+	u8 value = 0;
+
+	if (!ec)
+		return;
+
+	while (!acpi_ec_query(ec, &value))
+		acpi_ec_gpe_run_handler(ec, value);
+}
+
 static u32 acpi_ec_gpe_handler(void *data)
 {
 	acpi_status status = AE_OK;

Thanks for the patch, ACK.

Quoted 52 lines · Alan Jenkins

It looks like this EC clears the SMI_EVT bit after every query, even if there are more events pending. The workaround is to repeatedly query the EC until it reports that no events remain.

This fixes a regression in 2.6.26 (from 2.6.25.3). Initially reported as "Asus Eee PC hotkeys stop working if pressed quickly" in bugzilla
<http://bugzilla.kernel.org/show_bug.cgi?id=11089>.

The regression was caused by a recently added check for interrupt storms. The Eee PC triggers this check and switches to polling. When multiple events arrive between polling intervals, only one is fetched from the EC. This causes erroneous behaviour; ultimately events stop being delivered altogether when the EC buffer overflows.

Signed-off-by Alan Jenkins <[email protected]>

---

drivers/acpi/ec.c +12 -3

index 5622aee..2b4c5a2 100644
@@ -459,14 +459,10 @@ void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit)
  EXPORT_SYMBOL_GPL(acpi_ec_remove_query_handler);
 -static void acpi_ec_gpe_query(void *ec_cxt)
+static void acpi_ec_gpe_run_handler(struct acpi_ec *ec, u8 value)
 {
-	struct acpi_ec *ec = ec_cxt;
-	u8 value = 0;
 	struct acpi_ec_query_handler *handler, copy;
 -	if (!ec || acpi_ec_query(ec, &value))
-		return;
 	mutex_lock(&ec->lock);
 	list_for_each_entry(handler, &ec->list, node) {
 		if (value == handler->query_bit) {

@@ -484,6 +480,18 @@ static void acpi_ec_gpe_query(void *ec_cxt)
 	mutex_unlock(&ec->lock);
 }
 +static void acpi_ec_gpe_query(void *ec_cxt)
+{
+	struct acpi_ec *ec = ec_cxt;
+	u8 value = 0;
+
+	if (!ec)
+		return;
+
+	while (!acpi_ec_query(ec, &value))
+		acpi_ec_gpe_run_handler(ec, value);
+}
+
 static u32 acpi_ec_gpe_handler(void *data)
 {
 	acpi_status status = AE_OK;

Quoted 1 line · Alexey Starikovskiy

Thanks for the patch, ACK.

This one fixes a potentially bad problem, since we could ignore more than just hot key EC events by accident. Maybe it should go to -stable?

Quoted 5 lines · Henrique de Moraes Holschuh

Quoted 1 line · Alexey Starikovskiy

Thanks for the patch, ACK.

This one fixes a potentially bad problem, since we could ignore more than just hot key EC events by accident. Maybe it should go to -stable?

I vote for it

Quoted 11 lines · Alexey Starikovskiy

Quoted 7 lines · Henrique de Moraes Holschuh

Quoted 2 lines · Alexey Starikovskiy

Thanks for the patch, ACK.

This one fixes a potentially bad problem, since we could ignore more than just hot key EC events by accident. Maybe it should go to -stable?

I vote for it

Well, in that case, it would be best to tack a Cc: [email protected] git footer to it right away, I think.

IMHO, it would also be nice if the commit message made it more clear that the issue it solves can affect much more serious ACPI events than just hot key presses.

Actually Alexey has another patch in bugzilla (#10919) which resolves this issue in a better way. It avoids polling altogether, which is good because it means you get events immediately. My laptop has backlight adjustment hotkeys with hardware autorepeat, so it looks really jerky with polling.

Quoted 6 lines · Alan Jenkins

Actually Alexey has another patch in bugzilla (#10919) which resolves this issue in a better way. It avoids polling altogether, which is good because it means you get events immediately. My laptop has backlight adjustment hotkeys with hardware autorepeat, so it looks really jerky with polling.

We need to get ALL pending events from the EC, whether in polling mode or in interrupt mode

(and we must make sure that we ARE going to queue any new interrupt before we stop checking for an empty pending event queue, otherwise there is a small window where a new event might get queued, and we are still masking the GPE and don't notice it). As long as the fix does that, it is all I care :-)

Here's what I came up with -

1. I was fighting against EC_FLAGS_QUERY_PENDING. This was used to ignore multiple successive GPE interrupts and treat them as a single GPE instead. That's the exact opposite of what we want to do. Let's get rid of it.

2. Then we can apply my original patch to fix GPE polling on the Asus EeePC, by repeatedly querying for GPEs until there are none left.

3. Finally, if I'm right then we now know how to handle "GPE interrupt storms". Some EC's are raising multiple interrupts before we acknowledge them. but they're just telling us how many events are pending. There's no harm in that, so we don't ever need to disable GPE interrupts. Let's get rid of GPE polling mode. (Code mainly stolen from Alexey).

From: Alan Jenkins <[email protected]>

If several GPE interrupts arrive at a time, then perform an equal number of queries in the workqueue. It doesn't matter if there are excess interrupts; the queries will just come back with nothing.

This won't fix anything, because it still switches to polling GPEs if more than five interrupts arrive at a time. But it simplifies the code, and makes it easier to modify further without introducing race conditions.

Signed-off-by Alan Jenkins <[email protected]>

Tested-by Alan Jenkins <[email protected]>

drivers/acpi/ec.c +2 -7

index 5622aee..8e4b1a4 100644
@@ -77,7 +77,6 @@ enum ec_event {
 
 enum {
 	EC_FLAGS_WAIT_GPE = 0,		/* Don't check status until GPE arrives */
-	EC_FLAGS_QUERY_PENDING,		/* Query is pending */
 	EC_FLAGS_GPE_MODE,		/* Expect GPE to be sent for status change */
 	EC_FLAGS_NO_GPE,		/* Don't use GPE mode */
 	EC_FLAGS_RESCHEDULE_POLL	/* Re-schedule poll */

@@ -230,8 +229,7 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec, u8 command,
 			       "finish-write timeout, command = %d\n", command);
 			goto end;
 		}
-	} else if (command == ACPI_EC_COMMAND_QUERY)
-		clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
+	}
 
 	for (; rdata_len > 0; --rdata_len) {
 		result = acpi_ec_wait(ec, ACPI_EC_EVENT_OBF_1, force_poll);

@@ -502,7 +508,7 @@ static u32 acpi_ec_gpe_handler(void *data)
 		wake_up(&ec->wait);
 
 	if (state & ACPI_EC_FLAG_SCI) {
-		if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags))
+		if (ec->handlers_installed)
 			status = acpi_os_execute(OSL_EC_BURST_HANDLER,
 				acpi_ec_gpe_query, ec);
 	} else if (!test_bit(EC_FLAGS_GPE_MODE, &ec->flags) &&

@@ -665,7 +671,6 @@ static struct acpi_ec *make_acpi_ec(void)
 	struct acpi_ec *ec = kzalloc(sizeof(struct acpi_ec), GFP_KERNEL);
 	if (!ec)
 		return NULL;
-	ec->flags = 1 << EC_FLAGS_QUERY_PENDING;
 	mutex_init(&ec->lock);
 	init_waitqueue_head(&ec->wait);
 	INIT_LIST_HEAD(&ec->list);

@@ -858,8 +863,6 @@ static int acpi_ec_start(struct acpi_device *device)
 
 	ret = ec_install_handlers(ec);
 
-	/* EC is fully operational, allow queries */
-	clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
 	ec_schedule_ec_poll(ec);
 	return ret;
 }

Alan,

I don't think this is very good idea -- now every interrupt will add a new entry to workqueue,
and given the rate of this interrupt, it could be become quite long.

Your second patch is redundant if you add queue entry for each interrupt, and as it does not
require any investments into memory, I like it more.

Also, it is very cool to rip of things you don't care for, but that essentially reverts a fix done for 9998,
so at least, you should ask these people if you broke their setups.

    /* moved here from acpi_ec_transaction_unlocked() */
    clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);

    while (acpi_ec_query(ec, &value) == 0)
        acpi_ec_gpe_run_handler(ec, value);
}

The PENDING flag then reflects whether or not there's an item on the workqueue. Does that make sense?

Quoted 5 lines

Your second patch is redundant if you add queue entry for each interrupt, and as it does not
require any investments into memory, I like it more.

It's not quite redundant with the first patch. We still have GPE polling mode - patch #1 doesn't affect that. In polling mode, it's essential to query all the pending events - otherwise, if they arrive more frequently than the polling interval then you will inevitably drop events.

Patch #2 is also required to fix my buggy hardware. My laptop's EC buffers multiple events, but clears SCI_EVT after every query. This caused problems in polling mode; with multiple events between polling intervals only one gets queried - and after a while the buffer overflows and it breaks completely.

Quoted 4 lines

Also, it is very cool to rip of things you don't care for, but that essentially reverts a fix done for 9998,
so at least, you should ask these people if you broke their setups.

I assume you're referring to the "would benefit from wider testing" patch #3. Thanks for identifying the bugzilla entry - I had difficulty separating the different entries on GPEs. I'm optimistic that we can fix all these crazy buffering EC's without having to disable GPE interrupts.

The reason I like my proposed fix is that it makes the code simple enough that I can understand it, without making any assumptions about how many interrupts arrive per GPE. The EC can be broken in lots of ways, so long as:

1. We receive interrupts when one or more GPE's are pending.
2. We don't get a constant interrupt storm.

I don't think I missed anything. Is there anything else I should check before I try to get testing?

Quoted 9 lines · Alan Jenkins

    /* moved here from acpi_ec_transaction_unlocked() */
    clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);

    while (acpi_ec_query(ec, &value) == 0)
        acpi_ec_gpe_run_handler(ec, value);
}

The PENDING flag then reflects whether or not there's an item on the workqueue. Does that make sense?

Yes, this is what the flag was made for -- to guard workqueue from being filled up.

Quoted 10 lines

Quoted 5 lines

Your second patch is redundant if you add queue entry for each interrupt, and as it does not
require any investments into memory, I like it more.

It's not quite redundant with the first patch. We still have GPE polling mode - patch #1 doesn't affect that. In polling mode, it's essential to query all the pending events - otherwise, if they arrive more frequently than the polling interval then you will inevitably drop events.

I meant that if we have patch #3, which drops poll mode, we only need one of the first two, not both. And in this case I like #2 more than #1.

Quoted 6 lines

Patch #2 is also required to fix my buggy hardware. My laptop's EC buffers multiple events, but clears SCI_EVT after every query. This caused problems in polling mode; with multiple events between polling intervals only one gets queried - and after a while the buffer overflows and it breaks completely.

I fully agree here, it would be great to make sure that EC buffers are empty.

Quoted 9 lines

Quoted 4 lines

Also, it is very cool to rip of things you don't care for, but that essentially reverts a fix done for 9998,
so at least, you should ask these people if you broke their setups.

I assume you're referring to the "would benefit from wider testing" patch #3. Thanks for identifying the bugzilla entry - I had difficulty separating the different entries on GPEs. I'm optimistic that we can fix all these crazy buffering EC's without having to disable GPE interrupts.

I would say, we should test pair of #2 and #3, or your original patch
+ my patch.
I still like the option to not have any new interrupts until I'm done with current one --
much like the PENDING bit.

Quoted 11 lines

The reason I like my proposed fix is that it makes the code simple enough that I can understand it, without making any assumptions about how many interrupts arrive per GPE. The EC can be broken in lots of ways, so long as:

1. We receive interrupts when one or more GPE's are pending.
2. We don't get a constant interrupt storm.

I don't think I missed anything. Is there anything else I should check before I try to get testing?

Well, there is Keith Packard with concern that either high number of interrupts or long
spins on polling status register could push battery life down the drain...
One more argument to disable GPE until we service previous. Regards,
Alex.

It looks like this EC clears the SMI_EVT bit after every query, even if there are more events pending. The workaround is to repeatedly query the EC until it reports that no events remain.

This fixes a regression in 2.6.26 (from 2.6.25.3). Initially reported as "Asus Eee PC hotkeys stop working if pressed quickly" in bugzilla
<http://bugzilla.kernel.org/show_bug.cgi?id=11089>.

The regression was caused by a recently added check for interrupt storms. The Eee PC triggers this check and switches to polling. When multiple events arrive between polling intervals, only one is fetched from the EC. This causes erroneous behaviour; ultimately events stop being delivered altogether when the EC buffer overflows.

Signed-off-by Alan Jenkins <[email protected]>

---

drivers/acpi/ec.c +13 -5

index 5622aee..2b4c5a2 100644
@@ -459,14 +459,10 @@ void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit)
 
 EXPORT_SYMBOL_GPL(acpi_ec_remove_query_handler);
 
-static void acpi_ec_gpe_query(void *ec_cxt)
+static void acpi_ec_gpe_run_handler(struct acpi_ec *ec, u8 value)
 {
-	struct acpi_ec *ec = ec_cxt;
-	u8 value = 0;
 	struct acpi_ec_query_handler *handler, copy;
 
-	if (!ec || acpi_ec_query(ec, &value))
-		return;
 	mutex_lock(&ec->lock);
 	list_for_each_entry(handler, &ec->list, node) {
 		if (value == handler->query_bit) {

@@ -484,6 +480,18 @@ static void acpi_ec_gpe_query(void *ec_cxt)
 	mutex_unlock(&ec->lock);
 }
 
+static void acpi_ec_gpe_query(void *ec_cxt)
+{
+	struct acpi_ec *ec = ec_cxt;
+	u8 value = 0;
+
+	if (!ec)
+		return;
+
+	while (!acpi_ec_query(ec, &value))
+		acpi_ec_gpe_run_handler(ec, value);
+}
+
 static u32 acpi_ec_gpe_handler(void *data)
 {
 	acpi_status status = AE_OK;

Hi Alan,

Could you please test if your patch works with the last patch in #10919?

Thanks,
Alex.

Quoted 6 lines · Alexey Starikovskiy

Hi Alan,

Could you please test if your patch works with the last patch in #10919?

Thanks,
Alex.

Vacuously so.

My patch still applies, but #10919 makes it obsolete.

My patch fixed a
bug that shows up in polling mode. #10919 kills polling mode.

I've tested v2.6.26 + #10919 and it works fine (except for spamming the kernel log - please read my Bugzilla comment).

It appears that interrupt mode suffered from a race which is very similar to my original problem. If two GPE interrupts arrive before the workqueue runs, then the second interrupt will be ignored because EC_FLAGS_QUERY_PENDING is still set. This will happen with any EC if interrupts are very close together, right?

I think my patch also fixes this theoretical problem.

But I'd rather
you took over on this. I was already confused by ec.c in v2.6.26, and with #10919 I understand it even less. E.g. why is ec_switch_to_poll_mode() still present; what does it do now do_ec_poll() is removed?

I'm happy to work on this with you, but I'd need to be able understand the code first :-(.

Alan

Quoted 11 lines · Alan Jenkins

Quoted 7 lines · Alexey Starikovskiy

Hi Alan,

Could you please test if your patch works with the last patch in #10919?

Thanks,
Alex.

Vacuously so.

My patch still applies, but #10919 makes it obsolete.

Not so, there are two polls in ec.c, first is poll for change in status register,
which gave the name to the mode, and still exists; the other is for event in embedded controller, which was introduced to properly solve #9998, and part of
it is removed by patch in #10919.

Quoted 8 lines

My patch fixed a
bug that shows up in polling mode. #10919 kills polling mode.

I've tested v2.6.26 + #10919 and it works fine (except for spamming the kernel log - please read my Bugzilla comment).

Quoted 5 lines

It appears that interrupt mode suffered from a race which is very similar to my original problem. If two GPE interrupts arrive before the workqueue runs, then the second interrupt will be ignored because EC_FLAGS_QUERY_PENDING is still set. This will happen with any EC if interrupts are very close together, right?

The notion of queue in embedded controller is new, it was never mentioned in
ACPI spec, so the driver was written with assumption that new query interrupt should
not arrive before we service previous one. There is even a chart in how interrupts
should occur near the EC query command...

Quoted 2 lines

I think my patch also fixes this theoretical problem.

I think, this is not a theoretical problem, but the problem we've tried to solve in
#9998, #10724, and so on.

Quoted 5 lines

But I'd rather
you took over on this. I was already confused by ec.c in v2.6.26, and with #10919 I understand it even less. E.g. why is ec_switch_to_poll_mode() still present; what does it do now do_ec_poll() is removed?

See above, I still disable EC GPE for the time than we have pending query,
so we better not wait for it to check the status register

Quoted 3 lines

I'm happy to work on this with you, but I'd need to be able understand the code first :-(.

Well, with this patch of yours, I guess, we will not have too many problems in EC left :-)

Quoted 45 lines · Alexey Starikovskiy

Quoted 11 lines · Alan Jenkins

Quoted 7 lines · Alexey Starikovskiy

Hi Alan,

Could you please test if your patch works with the last patch in #10919?

Thanks,
Alex.

Vacuously so.

My patch still applies, but #10919 makes it obsolete.

Not so, there are two polls in ec.c, first is poll for change in status register,
which gave the name to the mode, and still exists; the other is for event in embedded controller, which was introduced to properly solve #9998, and part of
it is removed by patch in #10919.

Quoted 11 lines

My patch fixed a
bug that shows up in polling mode. #10919 kills polling mode. I've tested v2.6.26 + #10919 and it works fine (except for spamming the kernel log - please read my Bugzilla comment).

It appears that interrupt mode suffered from a race which is very similar to my original problem. If two GPE interrupts arrive before the workqueue runs, then the second interrupt will be ignored because EC_FLAGS_QUERY_PENDING is still set. This will happen with any EC if interrupts are very close together, right?

The notion of queue in embedded controller is new, it was never mentioned in
ACPI spec, so the driver was written with assumption that new query interrupt should
not arrive before we service previous one. There is even a chart in how interrupts
should occur near the EC query command...

Quoted 1 line

I think my patch also fixes this theoretical problem.

I think, this is not a theoretical problem, but the problem we've tried to solve in
#9998, #10724, and so on.

Quoted 5 lines

But I'd rather
you took over on this. I was already confused by ec.c in v2.6.26, and with #10919 I understand it even less. E.g. why is ec_switch_to_poll_mode() still present; what does it do now do_ec_poll() is removed?

Quoted 7 lines

See above, I still disable EC GPE for the time than we have pending query,
so we better not wait for it to check the status register

Quoted 2 lines

I'm happy to work on this with you, but I'd need to be able understand the code first :-(.

Well, with this patch of yours, I guess, we will not have too many problems in EC left :-)

OK, I'm happy now.

However, I'm now worried that I broke the semantics for
EC_FLAGS_QUERY_PENDING. In my patch it gets cleared after the first
query, even though I'm running queries in a loop until nothing is left. It doesn't cause a problem in my patch, but it's unclean and might cause confusion later on. It'd be better to clear it after the loop has
completed.

Can I fix my patch? If you ACK the new code below, then I'll resend with a proper changelog, S-o-B, CC's from the -mm mail (including [email protected]) and grovel to akpm, etc.

You're latest (quieter) work still applies on top and works fine.

Thanks
Alan

---

From: Alan Jenkins <[email protected]>

Tested-by Alan Jenkins <[email protected]>

drivers/acpi/ec.c +16 -7

index 5622aee..2a42392 100644
@@ -230,8 +230,7 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec, u8 command,
 			       "finish-write timeout, command = %d\n", command);
 			goto end;
 		}
-	} else if (command == ACPI_EC_COMMAND_QUERY)
-		clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
+	}
 
 	for (; rdata_len > 0; --rdata_len) {
 		result = acpi_ec_wait(ec, ACPI_EC_EVENT_OBF_1, force_poll);

@@ -459,14 +458,10 @@ void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit)
 
 EXPORT_SYMBOL_GPL(acpi_ec_remove_query_handler);
 
-static void acpi_ec_gpe_query(void *ec_cxt)
+static void acpi_ec_gpe_run_handler(struct acpi_ec *ec, u8 value)
 {
-	struct acpi_ec *ec = ec_cxt;
-	u8 value = 0;
 	struct acpi_ec_query_handler *handler, copy;
 
-	if (!ec || acpi_ec_query(ec, &value))
-		return;
 	mutex_lock(&ec->lock);
 	list_for_each_entry(handler, &ec->list, node) {
 		if (value == handler->query_bit) {

@@ -484,6 +479,20 @@ static void acpi_ec_gpe_query(void *ec_cxt)
 	mutex_unlock(&ec->lock);
 }
 
+static void acpi_ec_gpe_query(void *ec_cxt)
+{
+	struct acpi_ec *ec = ec_cxt;
+	u8 value = 0;
+
+	if (!ec)
+		return;
+
+	while (acpi_ec_query(ec, &value) != 0)
+		acpi_ec_gpe_run_handler(ec, value);
+
+	clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
+}
+
 static u32 acpi_ec_gpe_handler(void *data)
 {
 	acpi_status status = AE_OK;

Did this get fixed yet?

I have an patch in -mm which I just restored (I had to tempdrop it because the acpi tree was busted for some time). But it seems to be old.

http://bugzilla.kernel.org/show_bug.cgi?id=10919 is marked "resolved" but the reporter (Maximilian) seems to think otherwise. 2.6.26.x is, afaik, still unfixed, as is 2.6.27-rc.

Quoted 10 lines · Andrew Morton

Did this get fixed yet?

I have an patch in -mm which I just restored (I had to tempdrop it because the acpi tree was busted for some time). But it seems to be old.

http://bugzilla.kernel.org/show_bug.cgi?id=10919 is marked "resolved" but the reporter (Maximilian) seems to think otherwise. 2.6.26.x is, afaik, still unfixed, as is 2.6.27-rc.

That's correct. I think this specific patch should go in 2.6.27 and 2.6.26-stable. No objections have been raised so far.

I still need this patch to make my brightness and volume control keys usable in 2.6.27-rc3. (They auto-repeat fast enough to trigger the bug). This is true even after applying the latest patches from bug 10919 (#25 + #27).

I think the 10919 fix makes it harder to reproduce, but it definitely still happens. I guess this is because the polling-driven EC transactions add 1ms delays between each byte. The slower timings leave a window where the buggy behaviour of my EC can make a difference. (It has been seen to clear the "pending event" bit after a single event is read, despite having more events pending).

There are more serious consequences of this bug. After a while it can confuse the EC enough to cause lockups or reboots during boot, or after pressing a single hotkey. This bad state is preserved over reboots, even into known good kernels. Fortunately the badness clears when power is removed for a long enough period. For a while I was worried that something had physically burnt out.

Quoted 13 lines · On Wed, 13 Aug 2008 11:21:10 +0100 Alan Jenkins <[email protected]>…

Quoted 11 lines · Andrew Morton

Did this get fixed yet?

I have an patch in -mm which I just restored (I had to tempdrop it because the acpi tree was busted for some time). But it seems to be old.

http://bugzilla.kernel.org/show_bug.cgi?id=10919 is marked "resolved" but the reporter (Maximilian) seems to think otherwise. 2.6.26.x is, afaik, still unfixed, as is 2.6.27-rc.

Quoted 9 lines

That's correct. I think this specific patch should go in 2.6.27 and 2.6.26-stable. No objections have been raised so far.

I still need this patch to make my brightness and volume control keys usable in 2.6.27-rc3. (They auto-repeat fast enough to trigger the bug). This is true even after applying the latest patches from bug 10919 (#25 + #27).

Confusing. Please send the patch which you think we should apply.

Quoted 7 lines

I think the 10919 fix makes it harder to reproduce, but it definitely still happens. I guess this is because the polling-driven EC transactions add 1ms delays between each byte. The slower timings leave a window where the buggy behaviour of my EC can make a difference. (It has been seen to clear the "pending event" bit after a single event is read, despite having more events pending).

Quoted 7 lines

There are more serious consequences of this bug. After a while it can confuse the EC enough to cause lockups or reboots during boot, or after pressing a single hotkey. This bad state is preserved over reboots, even into known good kernels. Fortunately the badness clears when power is removed for a long enough period. For a while I was worried that something had physically burnt out.

Oh gad. And there's no workaround?

Quoted 13 lines · On Wed, 13 Aug 2008 11:21:10 +0100 Alan Jenkins <[email protected]>…

Quoted 11 lines · Andrew Morton

Did this get fixed yet?

I have an patch in -mm which I just restored (I had to tempdrop it because the acpi tree was busted for some time). But it seems to be old.

http://bugzilla.kernel.org/show_bug.cgi?id=10919 is marked "resolved" but the reporter (Maximilian) seems to think otherwise. 2.6.26.x is, afaik, still unfixed, as is 2.6.27-rc.

Quoted 9 lines

That's correct. I think this specific patch should go in 2.6.27 and 2.6.26-stable. No objections have been raised so far.

I still need this patch to make my brightness and volume control keys usable in 2.6.27-rc3. (They auto-repeat fast enough to trigger the bug). This is true even after applying the latest patches from bug 10919 (#25 + #27).

Confusing. Please send the patch which you think we should apply.

Quoted 7 lines

I think the 10919 fix makes it harder to reproduce, but it definitely still happens. I guess this is because the polling-driven EC transactions add 1ms delays between each byte. The slower timings leave a window where the buggy behaviour of my EC can make a difference. (It has been seen to clear the "pending event" bit after a single event is read, despite having more events pending).

Quoted 7 lines

There are more serious consequences of this bug. After a while it can confuse the EC enough to cause lockups or reboots during boot, or after pressing a single hotkey. This bad state is preserved over reboots, even into known good kernels. Fortunately the badness clears when power is removed for a long enough period. For a while I was worried that something had physically burnt out.

Oh gad. And there's no workaround?

[Dupe apology: CC'd to [email protected], with the right address this time]

Quoted 1 line · Andrew Morton

On Wed, 13 Aug 2008 11:21:10 +0100 Alan Jenkins

Quoted 28 lines

Quoted 11 lines

Quoted 9 lines · Andrew Morton

Did this get fixed yet?

I have an patch in -mm which I just restored (I had to tempdrop it because the acpi tree was busted for some time). But it seems to be old.

http://bugzilla.kernel.org/show_bug.cgi?id=10919 is marked "resolved" but the reporter (Maximilian) seems to think otherwise. 2.6.26.x is, afaik, still unfixed, as is 2.6.27-rc.

Quoted 7 lines

That's correct. I think this specific patch should go in 2.6.27 and 2.6.26-stable. No objections have been raised so far.

I still need this patch to make my brightness and volume control keys usable in 2.6.27-rc3. (They auto-repeat fast enough to trigger the bug). This is true even after applying the latest patches from bug 10919 (#25 + #27).

Confusing. Please send the patch which you think we should apply.

Quoted 7 lines

I think the 10919 fix makes it harder to reproduce, but it definitely still happens. I guess this is because the polling-driven EC transactions add 1ms delays between each byte. The slower timings leave a window where the buggy behaviour of my EC can make a difference. (It has been seen to clear the "pending event" bit after a single event is read, despite having more events pending).

Quoted 8 lines

Quoted 6 lines

There are more serious consequences of this bug. After a while it can confuse the EC enough to cause lockups or reboots during boot, or after pressing a single hotkey. This bad state is preserved over reboots, even into known good kernels. Fortunately the badness clears when power is removed for a long enough period. For a while I was worried that something had physically burnt out.

Oh gad. And there's no workaround?

Sorry, that was confusing.

The patch in currently in -mm _is_ the workaround for this damage. It was not initially obvious just how important it was :-). I've re-attached it as requested.

10919, "laggy hotkeys" is just what it says; ACPI EC events are slower because of polling. It appears to be a more cosmetic issue which is orthogonal to the _dropping_ of events.

Thanks
Alan

Quoted 6 lines · Alan Jenkins

[Dupe apology: CC'd to [email protected], with the right address this time]

Quoted 2 lines · Andrew Morton

On Wed, 13 Aug 2008 11:21:10 +0100 Alan Jenkins

Quoted 45 lines · <[email protected]> wrote:

Quoted 30 lines

Quoted 12 lines

Quoted 10 lines · Andrew Morton

Did this get fixed yet?

I have an patch in -mm which I just restored (I had to tempdrop it because the acpi tree was busted for some time). But it seems to be old.

http://bugzilla.kernel.org/show_bug.cgi?id=10919 is marked "resolved" but the reporter (Maximilian) seems to think otherwise. 2.6.26.x is, afaik, still unfixed, as is 2.6.27-rc.

Quoted 8 lines

That's correct. I think this specific patch should go in 2.6.27 and 2.6.26-stable. No objections have been raised so far.

I still need this patch to make my brightness and volume control keys usable in 2.6.27-rc3. (They auto-repeat fast enough to trigger the bug). This is true even after applying the latest patches from bug 10919 (#25 + #27).

Confusing. Please send the patch which you think we should apply.

Quoted 7 lines

I think the 10919 fix makes it harder to reproduce, but it definitely still happens. I guess this is because the polling-driven EC transactions add 1ms delays between each byte. The slower timings leave a window where the buggy behaviour of my EC can make a difference. (It has been seen to clear the "pending event" bit after a single event is read, despite having more events pending).

Quoted 9 lines

Quoted 7 lines

There are more serious consequences of this bug. After a while it can confuse the EC enough to cause lockups or reboots during boot, or after pressing a single hotkey. This bad state is preserved over reboots, even into known good kernels. Fortunately the badness clears when power is removed for a long enough period. For a while I was worried that something had physically burnt out.

Oh gad. And there's no workaround?

Sorry, that was confusing.

The patch in currently in -mm _is_ the workaround for this damage. It was not initially obvious just how important it was :-). I've re-attached it as requested.

Quoted 7 lines

10919, "laggy hotkeys" is just what it says; ACPI EC events are slower because of polling. It appears to be a more cosmetic issue which is orthogonal to the _dropping_ of events.

Thanks
Alan

This patch doesn't fix my problem (bug 10919), it only changes it a bit. When I press the dimming key and hold it pressed the display should dim up/down step by step as long as I hold the key pressed (that's how it has always been).
I'm now running a 2.6.27-rc3 kernel with your patch applied and the display does dim as described below.
When I press the dimming key first the display brightness doesn't change at all, then it jumps multiple steps, stays there for a short while and again jumps some steps till it reaches the end of the dimming interval. It looks like the key presses (assuming holding down the key does generate multiple key presses) get queued up, then all processed all at once, then again queued up...

Maxi