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

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

[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 <alan-jenkins@tuffmail.co.uk>

---
diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 5622aee..2b4c5a2 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -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;

Re: [PATCH] acpi: Avoid dropping rapid hotkey events (or other GPEs) on Asus EeePC

[Alexey Starikovskiy]

Hi Alan,

Thanks for the patch, ACK.

Regards,
Alex.

Alan Jenkins wrote:
> 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 <alan-jenkins@tuffmail.co.uk>
> 
> ---
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index 5622aee..2b4c5a2 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -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;
> 
> 

Re: [PATCH] acpi: Avoid dropping rapid hotkey events (or other GPEs) on Asus EeePC

[Henrique de Moraes Holschuh]

On Thu, 17 Jul 2008, Alexey Starikovskiy wrote:
> 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?

> Alan Jenkins wrote:
>> 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 <alan-jenkins@tuffmail.co.uk>
>>
>> ---
>> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
>> index 5622aee..2b4c5a2 100644
>> --- a/drivers/acpi/ec.c
>> +++ b/drivers/acpi/ec.c
>> @@ -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;

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] acpi: Avoid dropping rapid hotkey events (or other GPEs) on Asus EeePC

[Alexey Starikovskiy]

Henrique de Moraes Holschuh wrote:
> On Thu, 17 Jul 2008, Alexey Starikovskiy wrote:
>> 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

Regards,
Alex.

Re: [PATCH] acpi: Avoid dropping rapid hotkey events (or other GPEs) on Asus EeePC

[Alexey Starikovskiy]

Hi Alan,

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

Thanks,
Alex.


Alan Jenkins wrote:
> 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 <alan-jenkins@tuffmail.co.uk>
> 
> ---
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index 5622aee..2b4c5a2 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -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;
> 
> 

Re: [PATCH] acpi: Avoid dropping rapid hotkey events (or other GPEs) on Asus EeePC

[Alan Jenkins]

Alexey Starikovskiy wrote:
> 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

Re: [PATCH] acpi: Avoid dropping rapid hotkey events (or other GPEs) on Asus EeePC

[Henrique de Moraes Holschuh]

On Thu, 17 Jul 2008, Alexey Starikovskiy wrote:
> Henrique de Moraes Holschuh wrote:
>> On Thu, 17 Jul 2008, Alexey Starikovskiy wrote:
>>> 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: stable@kernel.org 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.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] acpi: Avoid dropping rapid hotkey events (or other GPEs) on Asus EeePC

[Alan Jenkins]

Henrique de Moraes Holschuh wrote:
> On Thu, 17 Jul 2008, Alexey Starikovskiy wrote:
>   
>> Henrique de Moraes Holschuh wrote:
>>     
>>> On Thu, 17 Jul 2008, Alexey Starikovskiy wrote:
>>>       
>>>> 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: stable@kernel.org 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.

So I think I should withdraw my patch and leave this to Alexey.  I've
tested his fix on my laptop and it works fine.  It needs some more work
though - e.g. at the moment it spams the kernel log.

Thanks,
Alan

Re: [PATCH] acpi: Avoid dropping rapid hotkey events (or other GPEs) on Asus EeePC

[Alexey Starikovskiy]

Alan Jenkins wrote:
> Alexey Starikovskiy wrote:
>> 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.
>  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...
> 
> 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.
> 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
> 
> 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 :-)
> 
> Alan

Thanks again,
Alex.

Re: [PATCH] acpi: Avoid dropping rapid hotkey events (or other GPEs) on Asus EeePC

[Henrique de Moraes Holschuh]

On Thu, 17 Jul 2008, Alan Jenkins wrote:
> 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 :-)

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] acpi: Avoid dropping rapid hotkey events (or other GPEs) on Asus EeePC

[Alan Jenkins]

Alexey Starikovskiy wrote:
> Alan Jenkins wrote:
>> Alexey Starikovskiy wrote:
>>> 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.
>>  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...
>>
>> 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.
>> 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
>>
>> 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
stable@kernel.org) and grovel to akpm, etc.

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

Thanks
Alan

---

From: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
Tested-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 5622aee..2a42392 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -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;

Re: [PATCH] acpi: Avoid dropping rapid hotkey events (or other GPEs) on Asus EeePC

[Alexey Starikovskiy]

Alan Jenkins wrote:
> Alexey Starikovskiy wrote:
>> Alan Jenkins wrote:
>>> Alexey Starikovskiy wrote:
>>>> 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.
>>>  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...
>>> 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.
>>> 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
>>> 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.
Right.
> 
> 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
> stable@kernel.org) and grovel to akpm, etc.
ACK
> 
> You're latest (quieter) work still applies on top and works fine.
Good.
> 
> Thanks
> Alan
Thanks,
Alex.
> 
> ---
> 
> From: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
> Tested-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
> 
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index 5622aee..2a42392 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -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;
> 
> 
> 

Re: [PATCH] acpi: Avoid dropping rapid hotkey events (or other GPEs) on Asus EeePC

[Alan Jenkins]

Henrique de Moraes Holschuh wrote:
> On Thu, 17 Jul 2008, Alan Jenkins wrote:
>   
>> 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 :-)
>   
OK, I forgot about that. I need to do some reading on kernel
programming, workqueues in particular, and redo my patch again. Sorry,
Alexey! AFAICS there's still this (very) small window where events could
be dropped.

Alan

[PATCH 0/3] acpi: GPE fixes

[Alan Jenkins]

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).

Patch 3 would benefit from wider testing.  Fortunately there are several open
bugs about GPEs so it should be easy to find testers :-).

Alan

[PATCH 1/3] acpi: Rip out EC_FLAGS_QUERY_PENDING (prevent race condition)

[Alan Jenkins]

From: Alan Jenkins <alan-jenkins@tuffmail.co.uk>

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 <alan-jenkins@tuffmail.co.uk>
Tested-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 5622aee..8e4b1a4 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -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;
 }

[PATCH 2/3] acpi: Avoid dropping rapid GPEs on Asus EeePC and others

[Alan Jenkins]

From: Alan Jenkins <alan-jenkins@tuffmail.co.uk>

Fix regression "Asus Eee PC hotkeys stop working if pressed quickly",
<http://bugzilla.kernel.org/show_bug.cgi?id=11089>.  More important 
notifications e.g. battery level may also be affected.

It looks like this EC clears the SCI_EVT bit after every query, but there may
be more events buffered.  Therefore we should repeatedly query the EC until it
reports that no events remain.

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

Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
Tested-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 9ca71df..e764011 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -457,14 +457,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) {
@@ -482,6 +478,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) == 0)
+		acpi_ec_gpe_run_handler(ec, value);
+}
+
 static u32 acpi_ec_gpe_handler(void *data)
 {
 	acpi_status status = AE_OK;

[PATCH 3/3] acpi: remove GPE polling

[Alan Jenkins]

From: Alan Jenkins <alan-jenkins@tuffmail.co.uk>

This should fix "[regression] display dimming is slow and laggy",
<http://bugzilla.kernel.org/show_bug.cgi?id=10919>.  It has a similar effect
on my Asus EeePC.

Much of this is stolen from Alexey Starikovskiy's fix (see Bugzilla).  But
the previous two GPE fixes should have avoided the need to disable GPE
interrupts.

Signed-off-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
Tested-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index e764011..1088903 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -79,7 +79,6 @@ enum {
 	EC_FLAGS_WAIT_GPE = 0,		/* Don't check status until GPE arrives */
 	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 */
 };
 
 /* If we find an EC via the ECDT, we need to keep a ptr to its context */
@@ -105,7 +104,6 @@ static struct acpi_ec {
 	wait_queue_head_t wait;
 	struct list_head list;
 	struct delayed_work work;
-	atomic_t irq_count;
 	u8 handlers_installed;
 } *boot_ec, *first_ec;
 
@@ -154,24 +152,14 @@ static inline int acpi_ec_check_status(struct acpi_ec *ec, enum ec_event event)
 	return 0;
 }
 
-static void ec_schedule_ec_poll(struct acpi_ec *ec)
-{
-	if (test_bit(EC_FLAGS_RESCHEDULE_POLL, &ec->flags))
-		schedule_delayed_work(&ec->work,
-				      msecs_to_jiffies(ACPI_EC_DELAY));
-}
-
 static void ec_switch_to_poll_mode(struct acpi_ec *ec)
 {
 	set_bit(EC_FLAGS_NO_GPE, &ec->flags);
 	clear_bit(EC_FLAGS_GPE_MODE, &ec->flags);
-	acpi_disable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
-	set_bit(EC_FLAGS_RESCHEDULE_POLL, &ec->flags);
 }
 
 static int acpi_ec_wait(struct acpi_ec *ec, enum ec_event event, int force_poll)
 {
-	atomic_set(&ec->irq_count, 0);
 	if (likely(test_bit(EC_FLAGS_GPE_MODE, &ec->flags)) &&
 	    likely(!force_poll)) {
 		if (wait_event_timeout(ec->wait, acpi_ec_check_status(ec, event),
@@ -184,7 +172,6 @@ static int acpi_ec_wait(struct acpi_ec *ec, enum ec_event event, int force_poll)
 				pr_info(PREFIX "missing confirmations, "
 						"switch off interrupt mode.\n");
 			ec_switch_to_poll_mode(ec);
-			ec_schedule_ec_poll(ec);
 			return 0;
 		}
 	} else {
@@ -497,12 +484,6 @@ static u32 acpi_ec_gpe_handler(void *data)
 	u8 state = acpi_ec_read_status(ec);
 
 	pr_debug(PREFIX "~~~> interrupt\n");
-	atomic_inc(&ec->irq_count);
-	if (atomic_read(&ec->irq_count) > 5) {
-		pr_err(PREFIX "GPE storm detected, disabling EC GPE\n");
-		ec_switch_to_poll_mode(ec);
-		goto end;
-	}
 	clear_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
 	if (test_bit(EC_FLAGS_GPE_MODE, &ec->flags))
 		wake_up(&ec->wait);
@@ -519,10 +500,8 @@ static u32 acpi_ec_gpe_handler(void *data)
 			pr_info(PREFIX "non-query interrupt received,"
 				" switching to interrupt mode\n");
 		set_bit(EC_FLAGS_GPE_MODE, &ec->flags);
-		clear_bit(EC_FLAGS_RESCHEDULE_POLL, &ec->flags);
 	}
-end:
-	ec_schedule_ec_poll(ec);
+
 	return ACPI_SUCCESS(status) ?
 	    ACPI_INTERRUPT_HANDLED : ACPI_INTERRUPT_NOT_HANDLED;
 }
@@ -530,7 +509,6 @@ end:
 static void do_ec_poll(struct work_struct *work)
 {
 	struct acpi_ec *ec = container_of(work, struct acpi_ec, work.work);
-	atomic_set(&ec->irq_count, 0);
 	(void)acpi_ec_gpe_handler(ec);
 }
 
@@ -675,7 +653,6 @@ static struct acpi_ec *make_acpi_ec(void)
 	init_waitqueue_head(&ec->wait);
 	INIT_LIST_HEAD(&ec->list);
 	INIT_DELAYED_WORK_DEFERRABLE(&ec->work, do_ec_poll);
-	atomic_set(&ec->irq_count, 0);
 	return ec;
 }
 
@@ -714,15 +691,8 @@ ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval)
 	return AE_CTRL_TERMINATE;
 }
 
-static void ec_poll_stop(struct acpi_ec *ec)
-{
-	clear_bit(EC_FLAGS_RESCHEDULE_POLL, &ec->flags);
-	cancel_delayed_work(&ec->work);
-}
-
 static void ec_remove_handlers(struct acpi_ec *ec)
 {
-	ec_poll_stop(ec);
 	if (ACPI_FAILURE(acpi_remove_address_space_handler(ec->handle,
 				ACPI_ADR_SPACE_EC, &acpi_ec_space_handler)))
 		pr_err(PREFIX "failed to remove space handler\n");
@@ -863,7 +833,6 @@ static int acpi_ec_start(struct acpi_device *device)
 
 	ret = ec_install_handlers(ec);
 
-	ec_schedule_ec_poll(ec);
 	return ret;
 }
 

Re: [PATCH 0/3] acpi: GPE fixes

[Vegard Nossum]

On Sat, Jul 19, 2008 at 1:37 PM, Alan Jenkins
<alan-jenkins@tuffmail.co.uk> wrote:
> 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).

Hi,

I have seen the "GPE storm" message before but it had no apparent
side-effects. Your patches do not seem to change this, although the
message is now gone. Thanks! (I have an Acer Aspire 5720 laptop.)


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Re: [PATCH 1/3] acpi: Rip out EC_FLAGS_QUERY_PENDING (prevent race condition)

[Alexey Starikovskiy]

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.

Regards,
Alex.

Alan Jenkins wrote:
> From: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
> 
> 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 <alan-jenkins@tuffmail.co.uk>
> Tested-by: Alan Jenkins <alan-jenkins@tuffmail.co.uk>
> 
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index 5622aee..8e4b1a4 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -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;
>  }
> 
> 

Re: [PATCH 1/3] acpi: Rip out EC_FLAGS_QUERY_PENDING (prevent race condition)

[Alan Jenkins]

Alexey Starikovskiy wrote:
> Alan,

Thanks for your robust response.  Sorry for not discussing this more in
the first place.  I wanted to write the code that fixes my system first,
and post it as soon as possible.  I think I missed a flag to say "this
post is not an immediate request for submission".

> 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.
>

I confess I gave up too quickly on the synchronisation issues and
resorted to brute force.  Patch #1 is preventing a narrow race:

- acpi_ec_write_cmd() (QUERY)
- EC writes 0 (no event) to input buffer
- new event arrives, triggering a GPE interrupt which is ignored because
QUERY_PENDING is set.
- QUERY_PENDING is cleared (but too late).

Now we ignored an event, which will be delayed until the next GPE interrupt.

The original reason for patch #1 was to stop patch #2 driving me
insane.  If you apply the second patch on it's own, you break
EC_FLAGS_QUERY_PENDING.  It gets cleared as soon as the _first_ query
command is submitted.  It works, but it means the flag no longer has a
clearly defined meaning.  I tried to fix that by only clearing it once
the loop has finished - and then realised I was widening a pre-existing
race condition.

As you say, I took the easy way out and pushed it all into the
workqueue.  So the workqueue ends up as a buffer of GPE occurrences.  I
did look at reducing the memory usage while avoiding race conditions,
but I couldn't find a reasonable solution.  But I looked at it again now
and I have a better solution:

...
    /* 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?

> 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.
> 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?

Thanks
Alan

Re: [PATCH 1/3] acpi: Rip out EC_FLAGS_QUERY_PENDING (prevent race condition)

[Alexey Starikovskiy]

Alan Jenkins wrote:
>     /* 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.
> 
>> 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.
> 
> 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.
>> 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.
> 
> 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.

Re: [PATCH 1/3] acpi: Rip out EC_FLAGS_QUERY_PENDING (prevent race condition)

[Alan Jenkins]

Alexey Starikovskiy wrote:
> Alan Jenkins wrote:
>>     /* 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.
>>
>>> 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.
>>
>> 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.
>>> 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.
>>
>> 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.

Ah, the powertop argument.  Disabling interrupts is a nice safety
blanket, but it means you end up polling the status register during
queries.  I think the interrupts are better for battery life.  The
excess interrupts happen all at once, so they don't wake the CPU up more
frequently, they just keep it awake slightly longer.

I'm convinced that emptying the EC buffer will reduce the number of
interrupts, even for the reporter of #9998.  I'll ask them to test this
patchset, then respin it to fix the mess I made of
EC_FLAGS_QUERY_PENDING.  As I mentioned earlier I've worked out how to
avoid my theoretical race without causing increased memory usage.

Thanks
Alan

Re: [PATCH] acpi: Avoid dropping rapid hotkey events (or other GPEs) on Asus EeePC

[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.

Thanks.

Re: [PATCH] acpi: Avoid dropping rapid hotkey events (or other GPEs) on Asus EeePC

[Alan Jenkins]

Andrew Morton wrote:
> 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.

Thanks
Alan

Re: [PATCH] acpi: Avoid dropping rapid hotkey events (or other GPEs) on Asus EeePC

[Andrew Morton]

On Wed, 13 Aug 2008 11:21:10 +0100 Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:

> Andrew Morton wrote:
> > 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).
>

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

> 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.

Oh gad.  And there's no workaround?

Re: [PATCH] acpi: Avoid dropping rapid hotkey events (or other GPEs) on Asus EeePC

[Alan Jenkins]

[-- Attachment #1: Type: text/plain, Size: 2262 bytes --]

Andrew Morton wrote:
> On Wed, 13 Aug 2008 11:21:10 +0100 Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:
>
>   
>> Andrew Morton wrote:
>>     
>>> 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).
>>
>>     
>
> Confusing.  Please send the patch which you think we should apply.
>
>   
>> 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.
>>     
>
> 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

[-- Attachment #2: Attached Message --]
[-- Type: message/rfc822, Size: 2635 bytes --]

From: Alan Jenkins <aj504@student.cs.york.ac.uk>
To: astarikovskiy@suse.de
Cc: linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: [PATCH] acpi: Avoid dropping rapid hotkey events (or other GPEs) on Asus EeePC
Date: Tue, 15 Jul 2008 23:25:07 +0100
Message-ID: <487D23C3.5070301@student.cs.york.ac.uk>

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 <alan-jenkins@tuffmail.co.uk>

---
diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 5622aee..2b4c5a2 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -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;

Re: [PATCH] acpi: Avoid dropping rapid hotkey events (or other GPEs) on Asus EeePC

[Alan Jenkins]

[-- Attachment #1: Type: text/plain, Size: 2337 bytes --]

[Dupe apology: CC'd to stable@kernel.org, with the right address this time]

Andrew Morton wrote:
> On Wed, 13 Aug 2008 11:21:10 +0100 Alan Jenkins <alan-jenkins@tuffmail.co.uk> wrote:
>
>   
>> Andrew Morton wrote:
>>     
>>> 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).
>>
>>     
>
> Confusing.  Please send the patch which you think we should apply.
>
>   
>> 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.
>>     
>
> 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


[-- Attachment #2: Attached Message --]
[-- Type: message/rfc822, Size: 2636 bytes --]

From: Alan Jenkins <aj504@student.cs.york.ac.uk>
To: astarikovskiy@suse.de
Cc: linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: [PATCH] acpi: Avoid dropping rapid hotkey events (or other GPEs) on Asus EeePC
Date: Tue, 15 Jul 2008 23:25:07 +0100
Message-ID: <487D23C3.5070301@student.cs.york.ac.uk>

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 <alan-jenkins@tuffmail.co.uk>

---
diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 5622aee..2b4c5a2 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -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;

Re: [PATCH] acpi: Avoid dropping rapid hotkey events (or other GPEs) on Asus EeePC

[Maximilian Engelhardt]

[-- Attachment #1: Type: text/plain, Size: 3239 bytes --]

On Wednesday 13 August 2008, Alan Jenkins wrote:
> [Dupe apology: CC'd to stable@kernel.org, with the right address this time]
>
> Andrew Morton wrote:
> > On Wed, 13 Aug 2008 11:21:10 +0100 Alan Jenkins 
<alan-jenkins@tuffmail.co.uk> wrote:
> >> Andrew Morton wrote:
> >>> 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).
> >
> > Confusing.  Please send the patch which you think we should apply.
> >
> >> 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.
> >
> > 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

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] acpi: Avoid dropping rapid hotkey events (or other GPEs) on Asus EeePC

[Alan Jenkins]

Maximilian Engelhardt wrote:
> On Wednesday 13 August 2008, Alan Jenkins wrote:
>   
>> [Dupe apology: CC'd to stable@kernel.org, with the right address this time]
>>
>> Andrew Morton wrote:
>>     
>>> On Wed, 13 Aug 2008 11:21:10 +0100 Alan Jenkins 
>>>       
> <alan-jenkins@tuffmail.co.uk> wrote:
>   
>>>> Andrew Morton wrote:
>>>>         
>>>>> 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).
>>>>         
>>> Confusing.  Please send the patch which you think we should apply.
>>>
>>>       
>>>> 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.
>>>>         
>>> 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
>>     
>
> 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
>   
This is expected behaviour - I can explain it in detail if that helps. 
I see the same on my laptop, though perhaps it is more annoying on yours.

The patch I've submitted here is not intended to fix #10919.  You said
the patch at <http://bugzilla.kernel.org/show_bug.cgi?id=10919#c21>
worked for you, and I think the approach there is the way forward for
this "laggy" issue.  However I don't know if it will be ready for this
kernel release.  I pointed out a cosmetic issue with it to Alexey but
haven't heard anything since.

Alan