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

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

[akpm]

From: Alan Jenkins <alan-jenkins@tuffmail.co.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>
Cc: Alexey Starikovskiy <astarikovskiy@suse.de>
Cc: Maximilian Engelhardt <maxi@daemonizer.de>
Cc: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: <stable@kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/acpi/ec.c |   18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff -puN drivers/acpi/ec.c~acpi-avoid-dropping-rapid-hotkey-events-or-other-gpes-on-asus-eeepc drivers/acpi/ec.c
--- a/drivers/acpi/ec.c~acpi-avoid-dropping-rapid-hotkey-events-or-other-gpes-on-asus-eeepc
+++ a/drivers/acpi/ec.c
@@ -486,14 +486,10 @@ void acpi_ec_remove_query_handler(struct
 
 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) {
@@ -511,6 +507,18 @@ static void acpi_ec_gpe_query(void *ec_c
 	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 2/2] acpi: Avoid dropping rapid hotkey events (or other GPEs) on Asus EeePC

[Alexey Starikovskiy]

Hi Andrew,

As I understand, this patch is wrong according to comment #65 in bug #9998.

Regards,
Alex.

akpm@linux-foundation.org wrote:
> From: Alan Jenkins <alan-jenkins@tuffmail.co.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>
> Cc: Alexey Starikovskiy <astarikovskiy@suse.de>
> Cc: Maximilian Engelhardt <maxi@daemonizer.de>
> Cc: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> Cc: Andi Kleen <andi@firstfloor.org>
> Cc: <stable@kernel.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  drivers/acpi/ec.c |   18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff -puN drivers/acpi/ec.c~acpi-avoid-dropping-rapid-hotkey-events-or-other-gpes-on-asus-eeepc drivers/acpi/ec.c
> --- a/drivers/acpi/ec.c~acpi-avoid-dropping-rapid-hotkey-events-or-other-gpes-on-asus-eeepc
> +++ a/drivers/acpi/ec.c
> @@ -486,14 +486,10 @@ void acpi_ec_remove_query_handler(struct
>  
>  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) {
> @@ -511,6 +507,18 @@ static void acpi_ec_gpe_query(void *ec_c
>  	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 2/2] acpi: Avoid dropping rapid hotkey events (or other GPEs) on Asus EeePC

[Andi Kleen]

On Thu, Aug 21, 2008 at 04:12:43PM +0400, Alexey Starikovskiy wrote:
> Hi Andrew,
> 
> As I understand, this patch is wrong according to comment #65 in bug #9998.

Ok I'll skip that one for now. Andrew can you please drop it.
The bug will be handled in bugzilla.

-Andi

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

[Alan Jenkins]

Alexey Starikovskiy wrote:
> Hi Andrew,
>
> As I understand, this patch is wrong according to comment #65 in bug
> #9998.
>
> Regards,
> Alex.

That's more likely to have been a problem with my other patches, ripping
out the QUERY_PENDING flag and GPE polling, with (what were to Alex)
predictably bad results.

You're right this patch needs testing on it's own.  But the current code
is definitely wrong.

If the "GPE polling" workaround is triggered, and there is a series of
events > one every 0.5s, the EC buffer can overflow.  All it needs is an
ACPI hotkey with fast enough autorepeat.  And I bet it's not just my EC
that breaks horribly when that happens.

Thanks
Alan

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

[Alexey Starikovskiy]

Alan Jenkins wrote:
> Alexey Starikovskiy wrote:
>> Hi Andrew,
>>
>> As I understand, this patch is wrong according to comment #65 in bug
>> #9998.
>>
>> Regards,
>> Alex.
> 
> That's more likely to have been a problem with my other patches, ripping
> out the QUERY_PENDING flag and GPE polling, with (what were to Alex)
> predictably bad results.
Hi Alan,
I checked QUERY_PENDING removal, and it makes system with broken EC unusable, 
so you might stop considering it as an option.

> 
> You're right this patch needs testing on it's own.  But the current code
> is definitely wrong.
> 
> If the "GPE polling" workaround is triggered, and there is a series of
> events > one every 0.5s, the EC buffer can overflow.  All it needs is an
> ACPI hotkey with fast enough autorepeat.  And I bet it's not just my EC
> that breaks horribly when that happens.
Right now I am going to remove automatic switch to poll mode, so user could
choose, which EC mode is more tolerable.
> 
> Thanks
> Alan

Regards,
Alex.

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

[Alan Jenkins]

Alexey Starikovskiy wrote:
> Alan Jenkins wrote:
>> Alexey Starikovskiy wrote:
>>> Hi Andrew,
>>>
>>> As I understand, this patch is wrong according to comment #65 in bug
>>> #9998.
>>>
>>> Regards,
>>> Alex.
>>
>> That's more likely to have been a problem with my other patches, ripping
>> out the QUERY_PENDING flag and GPE polling, with (what were to Alex)
>> predictably bad results.
> Hi Alan,
> I checked QUERY_PENDING removal, and it makes system with broken EC
> unusable, so you might stop considering it as an option.
>
In hindsight, the bad results were predictable to me also.  I don't
advocate that any more.

>>
>> You're right this patch needs testing on it's own.  But the current code
>> is definitely wrong.
>>
>> If the "GPE polling" workaround is triggered, and there is a series of
>> events > one every 0.5s, the EC buffer can overflow.  All it needs is an
>> ACPI hotkey with fast enough autorepeat.  And I bet it's not just my EC
>> that breaks horribly when that happens.
> Right now I am going to remove automatic switch to poll mode, so user
> could
> choose, which EC mode is more tolerable.

Sounds sensible.  Hopefully we're right that most systems with these
broken EC's don't want the workaround.  I assume you will still notify
the user automatically, something like "acpi: ec: GPE storm detected,
try booting with ec=poll".

Thanks
Alan

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

[Alexey Starikovskiy]

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

Here is the patch.

Please try/review.

Regards,
Alex.
Alan Jenkins wrote:
>
> Sounds sensible.  Hopefully we're right that most systems with these
> broken EC's don't want the workaround.  I assume you will still notify
> the user automatically, something like "acpi: ec: GPE storm detected,
> try booting with ec=poll".
> 
> Thanks
> Alan


[-- Attachment #2: ec_manual_mode_switch.patch --]
[-- Type: text/x-diff, Size: 3228 bytes --]

ACPI: EC: Don't degrade to poll mode at storm automatically.

From: Alexey Starikovskiy <astarikovskiy@suse.de>

Not all users of semi-broken EC devices want to degrade to poll mode, so
give them right to choose.

Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
---

 Documentation/kernel-parameters.txt |    5 +++++
 drivers/acpi/ec.c                   |   36 +++++++++++++++++++++++++----------
 2 files changed, 31 insertions(+), 10 deletions(-)


diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index a897646..cbde9bc 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -675,6 +675,11 @@ and is between 256 and 4096 characters. It is defined in the file
 
 	eata=		[HW,SCSI]
 
+	ec_intr=	[HW,ACPI] ACPI Embedded Controller interrupt mode
+			Format: <int>
+			0: polling mode
+			non-0: interrupt mode (default)
+
 	edd=		[EDD]
 			Format: {"off" | "on" | "skip[mbr]"}
 
diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 13593f9..bdc9b6b 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -110,6 +110,8 @@ static struct acpi_ec {
 	u8 handlers_installed;
 } *boot_ec, *first_ec;
 
+int acpi_ec_intr = 1; /* Default is interrupt mode */
+
 /* 
  * Some Asus system have exchanged ECDT data/command IO addresses.
  */
@@ -516,12 +518,14 @@ static u32 acpi_ec_gpe_handler(void *data)
 	acpi_status status = AE_OK;
 	struct acpi_ec *ec = data;
 	u8 state = acpi_ec_read_status(ec);
+	static bool warn_done = 0;
 
 	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);
+	if (!warn_done && atomic_read(&ec->irq_count) > 5) {
+		pr_warning(PREFIX "GPE storm detected, try to use ec_intr=0 "
+			"kernel option, if you see problems with keyboard.\n");
+		warn_done = 1;
 		goto end;
 	}
 	clear_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
@@ -848,20 +852,21 @@ static int ec_install_handlers(struct acpi_ec *ec)
 	acpi_status status;
 	if (ec->handlers_installed)
 		return 0;
-	status = acpi_install_gpe_handler(NULL, ec->gpe,
+	if (acpi_ec_intr) {
+		status = acpi_install_gpe_handler(NULL, ec->gpe,
 					  ACPI_GPE_EDGE_TRIGGERED,
 					  &acpi_ec_gpe_handler, ec);
-	if (ACPI_FAILURE(status))
-		return -ENODEV;
-
-	acpi_set_gpe_type(NULL, ec->gpe, ACPI_GPE_TYPE_RUNTIME);
-	acpi_enable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
+		if (ACPI_FAILURE(status))
+			return -ENODEV;
 
+		acpi_set_gpe_type(NULL, ec->gpe, ACPI_GPE_TYPE_RUNTIME);
+		acpi_enable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
+	}
 	status = acpi_install_address_space_handler(ec->handle,
 						    ACPI_ADR_SPACE_EC,
 						    &acpi_ec_space_handler,
 						    NULL, ec);
-	if (ACPI_FAILURE(status)) {
+	if (ACPI_FAILURE(status) && acpi_ec_intr) {
 		acpi_remove_gpe_handler(NULL, ec->gpe, &acpi_ec_gpe_handler);
 		return -ENODEV;
 	}
@@ -1047,3 +1052,14 @@ static void __exit acpi_ec_exit(void)
 	return;
 }
 #endif	/* 0 */
+
+static int __init acpi_ec_set_intr_mode(char *str)
+{
+	if (!get_option(&str, &acpi_ec_intr)) {
+		acpi_ec_intr = 0;
+		return 0;
+	}
+	return 1;
+}
+
+__setup("ec_intr=", acpi_ec_set_intr_mode);

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

[Andrew Morton]

On Thu, 21 Aug 2008 14:22:01 +0200 Andi Kleen <andi@firstfloor.org> wrote:

> On Thu, Aug 21, 2008 at 04:12:43PM +0400, Alexey Starikovskiy wrote:
> > Hi Andrew,
> > 
> > As I understand, this patch is wrong according to comment #65 in bug #9998.
> 
> Ok I'll skip that one for now. Andrew can you please drop it.
> The bug will be handled in bugzilla.
> 

Not that I'm mistrustful or anything, but I think I'll hang onto it
until I'm told it got fixed..

http://bugzilla.kernel.org/show_bug.cgi?id=11089 is marked resolved,
and a duplicate of http://bugzilla.kernel.org/show_bug.cgi?id=10919,
which is marked resolved.  Is this all correct??

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

[Alan Jenkins]

Alexey Starikovskiy wrote:
> Here is the patch.
>
> Please try/review.
>
Sorry, I can't help with testing this any more.  The "bad state" I
complained about earlier seems to be permanent now.

I left the laptop with AC and battery disconnected last night.  This
morning both 2.6.22 and 2.6.24-19-generic (ubuntu) are locking up or
rebooting as soon as I press a hotkey.  The patch resets the default
behaviour back to 2.6.24, so it won't make any difference.

None of these kernels should have the GPE polling problem.  There are
many people using Ubuntu on the EeePC without complaint; I must have
damaged the hardware or firmware.

OTOH 2.6.21.4-eeepc (the pre-installed kernel from Xandros/Asus) is
immune.  I will work on it, but don't expect anything soon.

Thanks for your help
Alan

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

[Alan Jenkins]

Alan Jenkins wrote:
> Alexey Starikovskiy wrote:
>   
>> Here is the patch.
>>
>> Please try/review.
>>
>>     
> Sorry, I can't help with testing this any more.  The "bad state" I
> complained about earlier seems to be permanent now.
>
> I left the laptop with AC and battery disconnected last night.  This
> morning both 2.6.22 and 2.6.24-19-generic (ubuntu) are locking up or
> rebooting as soon as I press a hotkey.  The patch resets the default
> behaviour back to 2.6.24, so it won't make any difference.
>
> None of these kernels should have the GPE polling problem.  There are
> many people using Ubuntu on the EeePC without complaint; I must have
> damaged the hardware or firmware.
>
> OTOH 2.6.21.4-eeepc (the pre-installed kernel from Xandros/Asus) is
> immune.  I will work on it, but don't expect anything soon.
>   
I was able to bisect this to a config option, X86_UP_IOAPIC.  I can get
rid of it on all kernels by just booting with noapic.

I was then able to try your patch, which works fine.  I see the warning
in the kernel log, but no hotkey keypresses are lost.

Thanks
Alan

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

[Alan Jenkins]

Alexey Starikovskiy wrote:

> Here is the patch.
>
> Please try/review.
>
> Regards,
> Alex.
> Alan Jenkins wrote:
>>
>> Sounds sensible. Hopefully we're right that most systems with these
>> broken EC's don't want the workaround. I assume you will still notify
>> the user automatically, something like "acpi: ec: GPE storm detected,
>> try booting with ec=poll".

My apologies.  I seem to have tested this but neglected to reply :-(.

I've been running with this for a week or so now.  I've been bashing the
hotkeys (and holding down the ones with autorepeat), and it works fine. 
Plus I get the message in the kernel log suggesting ec_intr=0 if the
keyboard doesn't work.  It's all good.

Thanks
Alan

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

[Alexey Starikovskiy]

Hi Alan,
That patch is old news already...
There is a new shiny one appended to 9998/10724/11549...
Please give it a try. It does disable GPE, but for very small duration.

Regards,
Alex.


Alan Jenkins wrote:
> Alexey Starikovskiy wrote:
> 
>> Here is the patch.
>>
>> Please try/review.
>>
>> Regards,
>> Alex.
>> Alan Jenkins wrote:
>>> Sounds sensible. Hopefully we're right that most systems with these
>>> broken EC's don't want the workaround. I assume you will still notify
>>> the user automatically, something like "acpi: ec: GPE storm detected,
>>> try booting with ec=poll".
> 
> My apologies.  I seem to have tested this but neglected to reply :-(.
> 
> I've been running with this for a week or so now.  I've been bashing the
> hotkeys (and holding down the ones with autorepeat), and it works fine. 
> Plus I get the message in the kernel log suggesting ec_intr=0 if the
> keyboard doesn't work.  It's all good.
> 
> Thanks
> Alan

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

[Zhao Yakui]

On Sun, 2008-09-21 at 19:42 +0100, Alan Jenkins wrote:
Hi, Alan
    As I said in http://bugzilla.kernel.org/show_bug.cgi?id=11089#C27,
the bug on the Asus-EEPC can be fixed by your patch. But maybe some
other systems will be broken by your patch.
    For example: 
    On my laptop when issuing the query command, a non-zero query event
is returned but it can't be processed.(There is no corresponding ACPI
_Qxx object). At the same time the SCI_EVT bit won't be cleared. In such
case OS can't exit the function of acpi_ec_query_handler,which causes
that the acpid kernel thread can't work well.
    
    At the same time there also exist the following two issues on the
Asus-EEEPC. 
    a. EC GPE storm. (The pulse waveform given by EC is too wide).
    b. EC notification event is triggered too quickly. Before the EC
notification event is queried, another EC notification event is
triggered again. In such case some EC notification event will be lost
unless EC driver continues to query the EC notification event until zero
is returned. (As suggested in the Alan's patch).

    IMO you had better report your issues to Asus. Maybe the issue can
be fixed by upgrading BIOS.
    Thanks.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Zhao Yakui]

On Sun, 2008-09-21 at 23:17 +0400, Alexey Starikovskiy wrote:
   
    The issue on Asus-EEEPC of bug11089 is that EC notification event is
triggered too quickly. Before the EC notification event is queried,
another EC notification event is triggered again. In such case some EC
notification event will be lost unless EC driver continues to query the
EC notification event until zero is returned. (As suggested in the
Alan's patch).
   And it is different with the issue of bug 9998/10724.


> Regards,
> Alex.
> 
> 
> Alan Jenkins wrote:
> > Alexey Starikovskiy wrote:
> > 
> >> Here is the patch.
> >>
> >> Please try/review.
> >>
> >> Regards,
> >> Alex.
> >> Alan Jenkins wrote:
> >>> Sounds sensible. Hopefully we're right that most systems with these
> >>> broken EC's don't want the workaround. I assume you will still notify
> >>> the user automatically, something like "acpi: ec: GPE storm detected,
> >>> try booting with ec=poll".
> > 
> > My apologies.  I seem to have tested this but neglected to reply :-(.
> > 
> > I've been running with this for a week or so now.  I've been bashing the
> > hotkeys (and holding down the ones with autorepeat), and it works fine. 
> > Plus I get the message in the kernel log suggesting ec_intr=0 if the
> > keyboard doesn't work.  It's all good.
> > 
> > Thanks
> > Alan
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Alan Jenkins]

Alexey Starikovskiy wrote:
> Hi Alan,
> That patch is old news already...
> There is a new shiny one appended to 9998/10724/11549...
> Please give it a try. It does disable GPE, but for very small duration.

Ok.  I was put off by the noise :-).

I've just tested 2.6.27-rc6 with
<http://bugzilla.kernel.org/show_bug.cgi?id=9998#c81>.  It still "drops"
some events, but now it takes longer to happen.  I have to work much
harder bashing the keys to reproduce it.


Like before, missing an event has severe consequences.  The missed event
is buffered.  When a new event occurs, only the oldest event is removed
from the buffer.  Therefore the buffer can only grow.  Eventually,
something breaks.  Events stop being delivered altogether; presumably
the buffer overflows.  I confirmed that this does still happen.

Remember that these are the consequences of a specific EC bug.  On my
EC, querying an event always clears SCI_EVT, even if there are more
events pending.  It is only re-raised when a new event fires.


I'll try reading the patch.  I may try capturing an EC debug log to show
how the event is dropped, but that will take time.

Thanks
Alan

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

[Alan Jenkins]

Alan Jenkins wrote:
> Alexey Starikovskiy wrote:
>   
>> Hi Alan,
>> That patch is old news already...
>> There is a new shiny one appended to 9998/10724/11549...
>> Please give it a try. It does disable GPE, but for very small duration.
>>     
>
> Ok.  I was put off by the noise :-).
>
> I've just tested 2.6.27-rc6 with
> <http://bugzilla.kernel.org/show_bug.cgi?id=9998#c81>.  It still "drops"
> some events, but now it takes longer to happen.  I have to work much
> harder bashing the keys to reproduce it.
>
>
> Like before, missing an event has severe consequences.  The missed event
> is buffered.  When a new event occurs, only the oldest event is removed
> from the buffer.  Therefore the buffer can only grow.  Eventually,
> something breaks.  Events stop being delivered altogether; presumably
> the buffer overflows.  I confirmed that this does still happen.
>
> Remember that these are the consequences of a specific EC bug.  On my
> EC, querying an event always clears SCI_EVT, even if there are more
> events pending.  It is only re-raised when a new event fires.
>
>
> I'll try reading the patch.  I may try capturing an EC debug log to show
> how the event is dropped, but that will take time.
>   

Ok, I think I've isolated the problem to a specific change.  It's our
good friend EC_FLAGS_QUERY_PENDING again.

Your new patch clears the pending flag after the query transaction has
finished.  Previously, it was cleared as soon as the query command was
initiated.

I hacked it back and it works.  Was there a specific reason for the change?

Regards
Alan

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 97168d4..2078cff 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -262,6 +262,8 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec, u8 command,
 	ec->t.rlen = rdata_len;
 	/* start transaction */
 	acpi_ec_write_cmd(ec, command);
+	if (command == ACPI_EC_COMMAND_QUERY)
+		clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
 	/* if we selected poll mode or failed in GPE-mode do a poll loop */
 	if (force_poll ||
 	    !test_bit(EC_FLAGS_GPE_MODE, &ec->flags) ||
@@ -453,7 +455,6 @@ static int acpi_ec_query(struct acpi_ec *ec, u8 * data)
 	 */
 
 	result = acpi_ec_transaction(ec, ACPI_EC_COMMAND_QUERY, NULL, 0, &d, 1, 0);
-	clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
 	if (result)
 		return result;
 

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

[Alexey Starikovskiy]

Alan Jenkins wrote:
> Alan Jenkins wrote:
>> Alexey Starikovskiy wrote:
>>   
>>> Hi Alan,
>>> That patch is old news already...
>>> There is a new shiny one appended to 9998/10724/11549...
>>> Please give it a try. It does disable GPE, but for very small duration.
>>>     
>> Ok.  I was put off by the noise :-).
>>
>> I've just tested 2.6.27-rc6 with
>> <http://bugzilla.kernel.org/show_bug.cgi?id=9998#c81>.  It still "drops"
>> some events, but now it takes longer to happen.  I have to work much
>> harder bashing the keys to reproduce it.
>>
>>
>> Like before, missing an event has severe consequences.  The missed event
>> is buffered.  When a new event occurs, only the oldest event is removed
>> from the buffer.  Therefore the buffer can only grow.  Eventually,
>> something breaks.  Events stop being delivered altogether; presumably
>> the buffer overflows.  I confirmed that this does still happen.
>>
>> Remember that these are the consequences of a specific EC bug.  On my
>> EC, querying an event always clears SCI_EVT, even if there are more
>> events pending.  It is only re-raised when a new event fires.
>>
>>
>> I'll try reading the patch.  I may try capturing an EC debug log to show
>> how the event is dropped, but that will take time.
>>   
> 
> Ok, I think I've isolated the problem to a specific change.  It's our
> good friend EC_FLAGS_QUERY_PENDING again.
> 
> Your new patch clears the pending flag after the query transaction has
> finished.  Previously, it was cleared as soon as the query command was
> initiated.
> 
> I hacked it back and it works.  Was there a specific reason for the change?
Thanks! There was no particular reason to put it after transaction, other than
original place is gone.

I'll post updated patch.
Regards,
Alex.
> 
> Regards
> Alan
> 
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index 97168d4..2078cff 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -262,6 +262,8 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec, u8 command,
>  	ec->t.rlen = rdata_len;
>  	/* start transaction */
>  	acpi_ec_write_cmd(ec, command);
> +	if (command == ACPI_EC_COMMAND_QUERY)
> +		clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
>  	/* if we selected poll mode or failed in GPE-mode do a poll loop */
>  	if (force_poll ||
>  	    !test_bit(EC_FLAGS_GPE_MODE, &ec->flags) ||
> @@ -453,7 +455,6 @@ static int acpi_ec_query(struct acpi_ec *ec, u8 * data)
>  	 */
>  
>  	result = acpi_ec_transaction(ec, ACPI_EC_COMMAND_QUERY, NULL, 0, &d, 1, 0);
> -	clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
>  	if (result)
>  		return result;
>  
> 
> 

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

[Alexey Starikovskiy]

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

Hi Alan,

Here are the patches. First is updated version of patch you've tried, 
second is
even more aggressive disabling of GPEs (applies on top).

Please check them.

Thanks,
Alex.

Alexey Starikovskiy wrote:
> Alan Jenkins wrote:
>> Alan Jenkins wrote:
>>> Alexey Starikovskiy wrote:
>>>  
>>>> Hi Alan,
>>>> That patch is old news already...
>>>> There is a new shiny one appended to 9998/10724/11549...
>>>> Please give it a try. It does disable GPE, but for very small 
>>>> duration.
>>>>     
>>> Ok.  I was put off by the noise :-).
>>>
>>> I've just tested 2.6.27-rc6 with
>>> <http://bugzilla.kernel.org/show_bug.cgi?id=9998#c81>.  It still 
>>> "drops"
>>> some events, but now it takes longer to happen.  I have to work much
>>> harder bashing the keys to reproduce it.
>>>
>>>
>>> Like before, missing an event has severe consequences.  The missed 
>>> event
>>> is buffered.  When a new event occurs, only the oldest event is removed
>>> from the buffer.  Therefore the buffer can only grow.  Eventually,
>>> something breaks.  Events stop being delivered altogether; presumably
>>> the buffer overflows.  I confirmed that this does still happen.
>>>
>>> Remember that these are the consequences of a specific EC bug.  On my
>>> EC, querying an event always clears SCI_EVT, even if there are more
>>> events pending.  It is only re-raised when a new event fires.
>>>
>>>
>>> I'll try reading the patch.  I may try capturing an EC debug log to 
>>> show
>>> how the event is dropped, but that will take time.
>>>   
>>
>> Ok, I think I've isolated the problem to a specific change.  It's our
>> good friend EC_FLAGS_QUERY_PENDING again.
>>
>> Your new patch clears the pending flag after the query transaction has
>> finished.  Previously, it was cleared as soon as the query command was
>> initiated.
>>
>> I hacked it back and it works.  Was there a specific reason for the 
>> change?
> Thanks! There was no particular reason to put it after transaction, 
> other than
> original place is gone.
>
> I'll post updated patch.
> Regards,
> Alex.
>>
>> Regards
>> Alan
>>
>> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
>> index 97168d4..2078cff 100644
>> --- a/drivers/acpi/ec.c
>> +++ b/drivers/acpi/ec.c
>> @@ -262,6 +262,8 @@ static int acpi_ec_transaction_unlocked(struct 
>> acpi_ec *ec, u8 command,
>>      ec->t.rlen = rdata_len;
>>      /* start transaction */
>>      acpi_ec_write_cmd(ec, command);
>> +    if (command == ACPI_EC_COMMAND_QUERY)
>> +        clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
>>      /* if we selected poll mode or failed in GPE-mode do a poll loop */
>>      if (force_poll ||
>>          !test_bit(EC_FLAGS_GPE_MODE, &ec->flags) ||
>> @@ -453,7 +455,6 @@ static int acpi_ec_query(struct acpi_ec *ec, u8 * 
>> data)
>>       */
>>  
>>      result = acpi_ec_transaction(ec, ACPI_EC_COMMAND_QUERY, NULL, 0, 
>> &d, 1, 0);
>> -    clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
>>      if (result)
>>          return result;
>>  
>>
>>
>
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


[-- Attachment #2: fast_transaction.patch --]
[-- Type: text/x-diff, Size: 16063 bytes --]

ACPI: EC: do transaction from interrupt context

From: Alexey Starikovskiy <astarikovskiy@suse.de>

It is easier and faster to do transaction directly from interrupt context
rather than waking control thread.
Also, cleaner GPE storm avoidance is implemented.

Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
---

 drivers/acpi/ec.c |  305 +++++++++++++++++++++++++----------------------------
 1 files changed, 146 insertions(+), 159 deletions(-)


diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 13593f9..91345b2 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1,7 +1,7 @@
 /*
- *  ec.c - ACPI Embedded Controller Driver (v2.0)
+ *  ec.c - ACPI Embedded Controller Driver (v2.1)
  *
- *  Copyright (C) 2006, 2007 Alexey Starikovskiy <alexey.y.starikovskiy@intel.com>
+ *  Copyright (C) 2006-2008 Alexey Starikovskiy <astarikovskiy@suse.de>
  *  Copyright (C) 2006 Denis Sadykov <denis.m.sadykov@intel.com>
  *  Copyright (C) 2004 Luming Yu <luming.yu@intel.com>
  *  Copyright (C) 2001, 2002 Andy Grover <andrew.grover@intel.com>
@@ -26,7 +26,7 @@
  * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  */
 
-/* Uncomment next line to get verbose print outs*/
+/* Uncomment next line to get verbose printout */
 /* #define DEBUG */
 
 #include <linux/kernel.h>
@@ -38,6 +38,7 @@
 #include <linux/seq_file.h>
 #include <linux/interrupt.h>
 #include <linux/list.h>
+#include <linux/spinlock.h>
 #include <asm/io.h>
 #include <acpi/acpi_bus.h>
 #include <acpi/acpi_drivers.h>
@@ -65,22 +66,19 @@ enum ec_command {
 	ACPI_EC_COMMAND_QUERY = 0x84,
 };
 
-/* EC events */
-enum ec_event {
-	ACPI_EC_EVENT_OBF_1 = 1,	/* Output buffer full */
-	ACPI_EC_EVENT_IBF_0,		/* Input buffer empty */
-};
-
 #define ACPI_EC_DELAY		500	/* Wait 500ms max. during EC ops */
 #define ACPI_EC_UDELAY_GLK	1000	/* Wait 1ms max. to get global lock */
 #define ACPI_EC_UDELAY		100	/* Wait 100us before polling EC again */
 
+#define ACPI_EC_STORM_THRESHOLD 20	/* number of false interrupts
+					   per one transaction */
+
 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 */
+	EC_FLAGS_GPE_STORM,		/* GPE storm detected */
+	EC_FLAGS_HANDLERS_INSTALLED	/* Handlers for GPE and OpReg are installed */
 };
 
 /* If we find an EC via the ECDT, we need to keep a ptr to its context */
@@ -95,6 +93,13 @@ struct acpi_ec_query_handler {
 	u8 query_bit;
 };
 
+struct transaction_data {
+	const u8 *wdata;
+	u8 *rdata;
+	u8 wlen;
+	u8 rlen;
+};
+
 static struct acpi_ec {
 	acpi_handle handle;
 	unsigned long gpe;
@@ -102,12 +107,12 @@ static struct acpi_ec {
 	unsigned long data_addr;
 	unsigned long global_lock;
 	unsigned long flags;
+	unsigned long irq_count;
 	struct mutex lock;
 	wait_queue_head_t wait;
 	struct list_head list;
-	struct delayed_work work;
-	atomic_t irq_count;
-	u8 handlers_installed;
+	struct transaction_data *t;
+	spinlock_t spinlock;
 } *boot_ec, *first_ec;
 
 /* 
@@ -150,7 +155,7 @@ static inline u8 acpi_ec_read_data(struct acpi_ec *ec)
 {
 	u8 x = inb(ec->data_addr);
 	pr_debug(PREFIX "---> data = 0x%2.2x\n", x);
-	return inb(ec->data_addr);
+	return x;
 }
 
 static inline void acpi_ec_write_cmd(struct acpi_ec *ec, u8 command)
@@ -165,68 +170,78 @@ static inline void acpi_ec_write_data(struct acpi_ec *ec, u8 data)
 	outb(data, ec->data_addr);
 }
 
-static inline int acpi_ec_check_status(struct acpi_ec *ec, enum ec_event event)
+static int ec_transaction_done(struct acpi_ec *ec)
 {
-	if (test_bit(EC_FLAGS_WAIT_GPE, &ec->flags))
-		return 0;
-	if (event == ACPI_EC_EVENT_OBF_1) {
-		if (acpi_ec_read_status(ec) & ACPI_EC_FLAG_OBF)
-			return 1;
-	} else if (event == ACPI_EC_EVENT_IBF_0) {
-		if (!(acpi_ec_read_status(ec) & ACPI_EC_FLAG_IBF))
-			return 1;
-	}
-
-	return 0;
+	unsigned long flags;
+	int ret = 0;
+	spin_lock_irqsave(&ec->spinlock, flags);
+	if (!ec->t || (!ec->t->wlen && !ec->t->rlen))
+		ret = 1;
+	spin_unlock_irqrestore(&ec->spinlock, flags);
+	return ret;
 }
 
-static void ec_schedule_ec_poll(struct acpi_ec *ec)
+static void gpe_transaction(struct acpi_ec *ec, u8 status)
 {
-	if (test_bit(EC_FLAGS_RESCHEDULE_POLL, &ec->flags))
-		schedule_delayed_work(&ec->work,
-				      msecs_to_jiffies(ACPI_EC_DELAY));
+	unsigned long flags;
+	spin_lock_irqsave(&ec->spinlock, flags);
+	if (!ec->t)
+		goto unlock;
+	if (ec->t->wlen > 0) {
+		if ((status & ACPI_EC_FLAG_IBF) == 0) {
+			acpi_ec_write_data(ec, *(ec->t->wdata++));
+			--ec->t->wlen;
+		} else
+			/* false interrupt, state didn't change */
+			++ec->irq_count;
+
+	} else if (ec->t->rlen > 0) {
+		if ((status & ACPI_EC_FLAG_OBF) == 1) {
+			*(ec->t->rdata++) = acpi_ec_read_data(ec);
+			--ec->t->rlen;
+		} else
+			/* false interrupt, state didn't change */
+			++ec->irq_count;
+	}
+unlock:
+	spin_unlock_irqrestore(&ec->spinlock, flags);
 }
 
-static void ec_switch_to_poll_mode(struct acpi_ec *ec)
+static int acpi_ec_wait(struct acpi_ec *ec)
 {
+	if (wait_event_timeout(ec->wait, ec_transaction_done(ec),
+			       msecs_to_jiffies(ACPI_EC_DELAY)))
+		return 0;
+	/* missing GPEs, switch back to poll mode */
+	if (printk_ratelimit())
+		pr_info(PREFIX "missing confirmations, "
+				"switch off interrupt mode.\n");
 	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);
+	return 1;
 }
 
-static int acpi_ec_wait(struct acpi_ec *ec, enum ec_event event, int force_poll)
+static void acpi_ec_gpe_query(void *ec_cxt);
+
+static int ec_check_sci(struct acpi_ec *ec, u8 state)
 {
-	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),
-				       msecs_to_jiffies(ACPI_EC_DELAY)))
-			return 0;
-		clear_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
-		if (acpi_ec_check_status(ec, event)) {
-			/* missing GPEs, switch back to poll mode */
-			if (printk_ratelimit())
-				pr_info(PREFIX "missing confirmations, "
-						"switch off interrupt mode.\n");
-			ec_switch_to_poll_mode(ec);
-			ec_schedule_ec_poll(ec);
-			return 0;
-		}
-	} else {
-		unsigned long delay = jiffies + msecs_to_jiffies(ACPI_EC_DELAY);
-		clear_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
-		while (time_before(jiffies, delay)) {
-			if (acpi_ec_check_status(ec, event))
-				return 0;
-			msleep(1);
-		}
-		if (acpi_ec_check_status(ec,event))
+	if (state & ACPI_EC_FLAG_SCI) {
+		if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags))
+			return acpi_os_execute(OSL_EC_BURST_HANDLER,
+				acpi_ec_gpe_query, ec);
+	}
+	return 0;
+}
+
+static int ec_poll(struct acpi_ec *ec)
+{
+	unsigned long delay = jiffies + msecs_to_jiffies(ACPI_EC_DELAY);
+	while (time_before(jiffies, delay)) {
+		gpe_transaction(ec, acpi_ec_read_status(ec));
+		udelay(ACPI_EC_UDELAY);
+		if (ec_transaction_done(ec))
 			return 0;
 	}
-	pr_err(PREFIX "acpi_ec_wait timeout, status = 0x%2.2x, event = %s\n",
-		acpi_ec_read_status(ec),
-		(event == ACPI_EC_EVENT_OBF_1) ? "\"b0=1\"" : "\"b1=0\"");
 	return -ETIME;
 }
 
@@ -235,45 +250,51 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec, u8 command,
 					u8 * rdata, unsigned rdata_len,
 					int force_poll)
 {
-	int result = 0;
-	set_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
+	unsigned long tmp;
+	struct transaction_data t = {.wdata = wdata, .rdata = rdata,
+				     .wlen = wdata_len, .rlen = rdata_len};
+	int ret = 0;
 	pr_debug(PREFIX "transaction start\n");
-	acpi_ec_write_cmd(ec, command);
-	for (; wdata_len > 0; --wdata_len) {
-		result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBF_0, force_poll);
-		if (result) {
-			pr_err(PREFIX
-			       "write_cmd timeout, command = %d\n", command);
-			goto end;
-		}
-		set_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
-		acpi_ec_write_data(ec, *(wdata++));
+	/* disable GPE during transaction if storm is detected */
+	if (test_bit(EC_FLAGS_GPE_STORM, &ec->flags)) {
+		clear_bit(EC_FLAGS_GPE_MODE, &ec->flags);
+		acpi_disable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
 	}
-
-	if (!rdata_len) {
-		result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBF_0, force_poll);
-		if (result) {
-			pr_err(PREFIX
-			       "finish-write timeout, command = %d\n", command);
-			goto end;
-		}
-	} else if (command == ACPI_EC_COMMAND_QUERY)
+	/* start transaction */
+	spin_lock_irqsave(&ec->spinlock, tmp);
+	ec->irq_count = 0;
+	/* following two actions should be kept atomic */
+	ec->t = &t;
+	acpi_ec_write_cmd(ec, command);
+	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);
-		if (result) {
-			pr_err(PREFIX "read timeout, command = %d\n", command);
-			goto end;
-		}
-		/* Don't expect GPE after last read */
-		if (rdata_len > 1)
-			set_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
-		*(rdata++) = acpi_ec_read_data(ec);
-	}
-      end:
+	spin_unlock_irqrestore(&ec->spinlock, tmp);
+	/* if we selected poll mode or failed in GPE-mode do a poll loop */
+	if (force_poll ||
+	    !test_bit(EC_FLAGS_GPE_MODE, &ec->flags) ||
+	    acpi_ec_wait(ec))
+		ret = ec_poll(ec);
 	pr_debug(PREFIX "transaction end\n");
-	return result;
+	spin_lock_irqsave(&ec->spinlock, tmp);
+	ec->t = NULL;
+	spin_unlock_irqrestore(&ec->spinlock, tmp);
+	if (test_bit(EC_FLAGS_GPE_STORM, &ec->flags)) {
+		/* check if we received SCI during transaction */
+		ec_check_sci(ec, acpi_ec_read_status(ec));
+		/* it is safe to enable GPE outside of transaction */
+		acpi_enable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
+	} else if (test_bit(EC_FLAGS_GPE_MODE, &ec->flags) &&
+		   ec->irq_count > ACPI_EC_STORM_THRESHOLD) {
+		pr_debug(PREFIX "GPE storm detected\n");
+		set_bit(EC_FLAGS_GPE_STORM, &ec->flags);
+	}
+	return ret;
+}
+
+static int ec_check_ibf0(struct acpi_ec *ec)
+{
+	u8 status = acpi_ec_read_status(ec);
+	return (status & ACPI_EC_FLAG_IBF) == 0;
 }
 
 static int acpi_ec_transaction(struct acpi_ec *ec, u8 command,
@@ -283,40 +304,34 @@ static int acpi_ec_transaction(struct acpi_ec *ec, u8 command,
 {
 	int status;
 	u32 glk;
-
 	if (!ec || (wdata_len && !wdata) || (rdata_len && !rdata))
 		return -EINVAL;
-
 	if (rdata)
 		memset(rdata, 0, rdata_len);
-
 	mutex_lock(&ec->lock);
 	if (ec->global_lock) {
 		status = acpi_acquire_global_lock(ACPI_EC_UDELAY_GLK, &glk);
 		if (ACPI_FAILURE(status)) {
-			mutex_unlock(&ec->lock);
-			return -ENODEV;
+			status = -ENODEV;
+			goto unlock;
 		}
 	}
-
-	status = acpi_ec_wait(ec, ACPI_EC_EVENT_IBF_0, 0);
-	if (status) {
+	if (!wait_event_timeout(ec->wait, ec_check_ibf0(ec),
+				msecs_to_jiffies(ACPI_EC_DELAY))) {
 		pr_err(PREFIX "input buffer is not empty, "
 				"aborting transaction\n");
+		status = -ETIME;
 		goto end;
 	}
-
 	status = acpi_ec_transaction_unlocked(ec, command,
 					      wdata, wdata_len,
 					      rdata, rdata_len,
 					      force_poll);
-
-      end:
-
+end:
 	if (ec->global_lock)
 		acpi_release_global_lock(glk);
+unlock:
 	mutex_unlock(&ec->lock);
-
 	return status;
 }
 
@@ -332,7 +347,9 @@ int acpi_ec_burst_enable(struct acpi_ec *ec)
 
 int acpi_ec_burst_disable(struct acpi_ec *ec)
 {
-	return acpi_ec_transaction(ec, ACPI_EC_BURST_DISABLE, NULL, 0, NULL, 0, 0);
+	return (acpi_ec_read_status(ec) & ACPI_EC_FLAG_BURST)?
+		acpi_ec_transaction(ec, ACPI_EC_BURST_DISABLE,
+			NULL, 0, NULL, 0, 0) : 0;
 }
 
 static int acpi_ec_read(struct acpi_ec *ec, u8 address, u8 * data)
@@ -513,46 +530,26 @@ static void acpi_ec_gpe_query(void *ec_cxt)
 
 static u32 acpi_ec_gpe_handler(void *data)
 {
-	acpi_status status = AE_OK;
 	struct acpi_ec *ec = data;
-	u8 state = acpi_ec_read_status(ec);
+	u8 status;
 
 	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))
+	status = acpi_ec_read_status(ec);
+
+	gpe_transaction(ec, status);
+	if (ec_transaction_done(ec) && (status & ACPI_EC_FLAG_IBF) == 0)
 		wake_up(&ec->wait);
 
-	if (state & ACPI_EC_FLAG_SCI) {
-		if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags))
-			status = acpi_os_execute(OSL_EC_BURST_HANDLER,
-				acpi_ec_gpe_query, ec);
-	} else if (!test_bit(EC_FLAGS_GPE_MODE, &ec->flags) &&
-		   !test_bit(EC_FLAGS_NO_GPE, &ec->flags) &&
-		   in_interrupt()) {
+	ec_check_sci(ec, status);
+	if (!test_bit(EC_FLAGS_GPE_MODE, &ec->flags) &&
+	    !test_bit(EC_FLAGS_NO_GPE, &ec->flags)) {
 		/* this is non-query, must be confirmation */
 		if (printk_ratelimit())
 			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;
-}
-
-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);
+	return ACPI_INTERRUPT_HANDLED;
 }
 
 /* --------------------------------------------------------------------------
@@ -696,8 +693,8 @@ static struct acpi_ec *make_acpi_ec(void)
 	mutex_init(&ec->lock);
 	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);
+	ec->irq_count = 0;
+	spin_lock_init(&ec->spinlock);
 	return ec;
 }
 
@@ -736,22 +733,15 @@ 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");
 	if (ACPI_FAILURE(acpi_remove_gpe_handler(NULL, ec->gpe,
 				&acpi_ec_gpe_handler)))
 		pr_err(PREFIX "failed to remove gpe handler\n");
-	ec->handlers_installed = 0;
+	clear_bit(EC_FLAGS_HANDLERS_INSTALLED, &ec->flags);
 }
 
 static int acpi_ec_add(struct acpi_device *device)
@@ -846,17 +836,15 @@ ec_parse_io_ports(struct acpi_resource *resource, void *context)
 static int ec_install_handlers(struct acpi_ec *ec)
 {
 	acpi_status status;
-	if (ec->handlers_installed)
+	if (test_bit(EC_FLAGS_HANDLERS_INSTALLED, &ec->flags))
 		return 0;
 	status = acpi_install_gpe_handler(NULL, ec->gpe,
-					  ACPI_GPE_EDGE_TRIGGERED,
-					  &acpi_ec_gpe_handler, ec);
+				  ACPI_GPE_EDGE_TRIGGERED,
+				  &acpi_ec_gpe_handler, ec);
 	if (ACPI_FAILURE(status))
 		return -ENODEV;
-
 	acpi_set_gpe_type(NULL, ec->gpe, ACPI_GPE_TYPE_RUNTIME);
 	acpi_enable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
-
 	status = acpi_install_address_space_handler(ec->handle,
 						    ACPI_ADR_SPACE_EC,
 						    &acpi_ec_space_handler,
@@ -866,7 +854,7 @@ static int ec_install_handlers(struct acpi_ec *ec)
 		return -ENODEV;
 	}
 
-	ec->handlers_installed = 1;
+	set_bit(EC_FLAGS_HANDLERS_INSTALLED, &ec->flags);
 	return 0;
 }
 
@@ -887,7 +875,6 @@ static int acpi_ec_start(struct acpi_device *device)
 
 	/* EC is fully operational, allow queries */
 	clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
-	ec_schedule_ec_poll(ec);
 	return ret;
 }
 
@@ -906,7 +893,7 @@ static int acpi_ec_stop(struct acpi_device *device, int type)
 
 int __init acpi_boot_ec_enable(void)
 {
-	if (!boot_ec || boot_ec->handlers_installed)
+	if (!boot_ec || test_bit(EC_FLAGS_HANDLERS_INSTALLED, &boot_ec->flags))
 		return 0;
 	if (!ec_install_handlers(boot_ec)) {
 		first_ec = boot_ec;

[-- Attachment #3: more_storm.patch --]
[-- Type: text/x-diff, Size: 1582 bytes --]

ACPI: EC: disable GPE during QUERY handling too.

From: Alexey Starikovskiy <astarikovskiy@suse.de>


---

 drivers/acpi/ec.c |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)


diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 91345b2..a90f70c 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -226,9 +226,16 @@ static void acpi_ec_gpe_query(void *ec_cxt);
 static int ec_check_sci(struct acpi_ec *ec, u8 state)
 {
 	if (state & ACPI_EC_FLAG_SCI) {
-		if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags))
+		if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) {
+			/* disable GPE until next transaction */
+			if (in_interrupt() &&
+			    test_bit(EC_FLAGS_GPE_STORM, &ec->flags)) {
+				clear_bit(EC_FLAGS_GPE_MODE, &ec->flags);
+				acpi_disable_gpe(NULL, ec->gpe, ACPI_ISR);
+			}
 			return acpi_os_execute(OSL_EC_BURST_HANDLER,
 				acpi_ec_gpe_query, ec);
+		}
 	}
 	return 0;
 }
@@ -281,8 +288,9 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec, u8 command,
 	if (test_bit(EC_FLAGS_GPE_STORM, &ec->flags)) {
 		/* check if we received SCI during transaction */
 		ec_check_sci(ec, acpi_ec_read_status(ec));
-		/* it is safe to enable GPE outside of transaction */
-		acpi_enable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
+		/* we expect query, enable GPE back */
+		if (!test_bit(EC_FLAGS_QUERY_PENDING, &ec->flags))
+			acpi_enable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
 	} else if (test_bit(EC_FLAGS_GPE_MODE, &ec->flags) &&
 		   ec->irq_count > ACPI_EC_STORM_THRESHOLD) {
 		pr_debug(PREFIX "GPE storm detected\n");

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

[Alan Jenkins]

Alexey Starikovskiy wrote:
> Hi Alan,
>
> Here are the patches. First is updated version of patch you've tried,
> second is
> even more aggressive disabling of GPEs (applies on top).
>
> Please check them.
>
I tested them together and they worked, yay!

That doesn't say anything about the second patch.  My EC didn't generate
enough spurious interrupts to trigger the STORM code.

Would you welcome some nitpicks on the first patch at this time?  I
didn't see errors but I have some code-style queries.

Thanks
Alan

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

[Alexey Starikovskiy]

Alan Jenkins wrote:
> Alexey Starikovskiy wrote:
>   
>> Hi Alan,
>>
>> Here are the patches. First is updated version of patch you've tried,
>> second is
>> even more aggressive disabling of GPEs (applies on top).
>>
>> Please check them.
>>
>>     
> I tested them together and they worked, yay!
>
> That doesn't say anything about the second patch.  My EC didn't generate
> enough spurious interrupts to trigger the STORM code.
>
> Would you welcome some nitpicks on the first patch at this time?  I
> didn't see errors but I have some code-style queries.
>   
Sure. Will be glad.

> Thanks
> Alan
>   

Re: [patch 2/2] 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,
>>>
>>> Here are the patches. First is updated version of patch you've tried,
>>> second is
>>> even more aggressive disabling of GPEs (applies on top).
>>>
>>> Please check them.
>>>
>>>     
>> I tested them together and they worked, yay!
>>
>> That doesn't say anything about the second patch.  My EC didn't generate
>> enough spurious interrupts to trigger the STORM code.
>>
>> Would you welcome some nitpicks on the first patch at this time?  I
>> didn't see errors but I have some code-style queries.
>>   
> Sure. Will be glad.

My main query was about acpi_ec::t being a pointer.  I thought that
meant there would be lots of kmalloc / kfrees.  But I see now it is
allocated on-stack, i.e. it points to a local variable.  Never mind.  It
is a bit clever though.  I thought the original version was clearer -
where acpi_ec::t wasn't a pointer. 

The other niggle was the spinlock.  It would be nice to indicate by
comment the resources it protects (acpi_ec::t and irq_count).

Actually, if we're being clever, why not move irq_count into
transaction_data?  After all, it's only used within the transaction.  It
fits with both the lifecycle and the locking.

Regards
Alan

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

[Alexey Starikovskiy]

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

Alan Jenkins wrote:
> Alexey Starikovskiy wrote:
>   
>> Alan Jenkins wrote:
>>     
>>> Alexey Starikovskiy wrote:
>>>  
>>>       
>>>> Hi Alan,
>>>>
>>>> Here are the patches. First is updated version of patch you've tried,
>>>> second is
>>>> even more aggressive disabling of GPEs (applies on top).
>>>>
>>>> Please check them.
>>>>
>>>>     
>>>>         
>>> I tested them together and they worked, yay!
>>>
>>> That doesn't say anything about the second patch.  My EC didn't generate
>>> enough spurious interrupts to trigger the STORM code.
>>>
>>> Would you welcome some nitpicks on the first patch at this time?  I
>>> didn't see errors but I have some code-style queries.
>>>   
>>>       
>> Sure. Will be glad.
>>     
>
> My main query was about acpi_ec::t being a pointer.  I thought that
> meant there would be lots of kmalloc / kfrees.  But I see now it is
> allocated on-stack, i.e. it points to a local variable.  Never mind.  It
> is a bit clever though.  I thought the original version was clearer -
> where acpi_ec::t wasn't a pointer. 
>
>   
I had acpi_ec::t as pointer in the very first version of the patch,
then changed it to member to make patch "cleaner", then Rafael
suggested I go with pointer, so I switched back.
> The other niggle was the spinlock.  It would be nice to indicate by
> comment the resources it protects (acpi_ec::t and irq_count).
>
>   
renamed it to t_lock. Hope it's self commenting now...
> Actually, if we're being clever, why not move irq_count into
> transaction_data?  After all, it's only used within the transaction.  It
> fits with both the lifecycle and the locking.
>   
Right, shaved couple of code lines by that :)
> Regards
> Alan
>   

Thanks,
Alex.

[-- Attachment #2: fast_transaction.patch --]
[-- Type: text/x-diff, Size: 15951 bytes --]

ACPI: EC: do transaction from interrupt context

From: Alexey Starikovskiy <astarikovskiy@suse.de>

It is easier and faster to do transaction directly from interrupt context
rather than waking control thread.
Also, cleaner GPE storm avoidance is implemented.

Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
---

 drivers/acpi/ec.c |  304 +++++++++++++++++++++++++----------------------------
 1 files changed, 145 insertions(+), 159 deletions(-)


diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 13593f9..5101e7e 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1,7 +1,7 @@
 /*
- *  ec.c - ACPI Embedded Controller Driver (v2.0)
+ *  ec.c - ACPI Embedded Controller Driver (v2.1)
  *
- *  Copyright (C) 2006, 2007 Alexey Starikovskiy <alexey.y.starikovskiy@intel.com>
+ *  Copyright (C) 2006-2008 Alexey Starikovskiy <astarikovskiy@suse.de>
  *  Copyright (C) 2006 Denis Sadykov <denis.m.sadykov@intel.com>
  *  Copyright (C) 2004 Luming Yu <luming.yu@intel.com>
  *  Copyright (C) 2001, 2002 Andy Grover <andrew.grover@intel.com>
@@ -26,7 +26,7 @@
  * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  */
 
-/* Uncomment next line to get verbose print outs*/
+/* Uncomment next line to get verbose printout */
 /* #define DEBUG */
 
 #include <linux/kernel.h>
@@ -38,6 +38,7 @@
 #include <linux/seq_file.h>
 #include <linux/interrupt.h>
 #include <linux/list.h>
+#include <linux/spinlock.h>
 #include <asm/io.h>
 #include <acpi/acpi_bus.h>
 #include <acpi/acpi_drivers.h>
@@ -65,22 +66,19 @@ enum ec_command {
 	ACPI_EC_COMMAND_QUERY = 0x84,
 };
 
-/* EC events */
-enum ec_event {
-	ACPI_EC_EVENT_OBF_1 = 1,	/* Output buffer full */
-	ACPI_EC_EVENT_IBF_0,		/* Input buffer empty */
-};
-
 #define ACPI_EC_DELAY		500	/* Wait 500ms max. during EC ops */
 #define ACPI_EC_UDELAY_GLK	1000	/* Wait 1ms max. to get global lock */
 #define ACPI_EC_UDELAY		100	/* Wait 100us before polling EC again */
 
+#define ACPI_EC_STORM_THRESHOLD 20	/* number of false interrupts
+					   per one transaction */
+
 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 */
+	EC_FLAGS_GPE_STORM,		/* GPE storm detected */
+	EC_FLAGS_HANDLERS_INSTALLED	/* Handlers for GPE and OpReg are installed */
 };
 
 /* If we find an EC via the ECDT, we need to keep a ptr to its context */
@@ -95,6 +93,14 @@ struct acpi_ec_query_handler {
 	u8 query_bit;
 };
 
+struct transaction_data {
+	const u8 *wdata;
+	u8 *rdata;
+	unsigned short irq_count;
+	u8 wlen;
+	u8 rlen;
+};
+
 static struct acpi_ec {
 	acpi_handle handle;
 	unsigned long gpe;
@@ -105,9 +111,8 @@ static struct acpi_ec {
 	struct mutex lock;
 	wait_queue_head_t wait;
 	struct list_head list;
-	struct delayed_work work;
-	atomic_t irq_count;
-	u8 handlers_installed;
+	struct transaction_data *t;
+	spinlock_t t_lock;
 } *boot_ec, *first_ec;
 
 /* 
@@ -150,7 +155,7 @@ static inline u8 acpi_ec_read_data(struct acpi_ec *ec)
 {
 	u8 x = inb(ec->data_addr);
 	pr_debug(PREFIX "---> data = 0x%2.2x\n", x);
-	return inb(ec->data_addr);
+	return x;
 }
 
 static inline void acpi_ec_write_cmd(struct acpi_ec *ec, u8 command)
@@ -165,68 +170,78 @@ static inline void acpi_ec_write_data(struct acpi_ec *ec, u8 data)
 	outb(data, ec->data_addr);
 }
 
-static inline int acpi_ec_check_status(struct acpi_ec *ec, enum ec_event event)
+static int ec_transaction_done(struct acpi_ec *ec)
 {
-	if (test_bit(EC_FLAGS_WAIT_GPE, &ec->flags))
-		return 0;
-	if (event == ACPI_EC_EVENT_OBF_1) {
-		if (acpi_ec_read_status(ec) & ACPI_EC_FLAG_OBF)
-			return 1;
-	} else if (event == ACPI_EC_EVENT_IBF_0) {
-		if (!(acpi_ec_read_status(ec) & ACPI_EC_FLAG_IBF))
-			return 1;
-	}
-
-	return 0;
+	unsigned long flags;
+	int ret = 0;
+	spin_lock_irqsave(&ec->t_lock, flags);
+	if (!ec->t || (!ec->t->wlen && !ec->t->rlen))
+		ret = 1;
+	spin_unlock_irqrestore(&ec->t_lock, flags);
+	return ret;
 }
 
-static void ec_schedule_ec_poll(struct acpi_ec *ec)
+static void gpe_transaction(struct acpi_ec *ec, u8 status)
 {
-	if (test_bit(EC_FLAGS_RESCHEDULE_POLL, &ec->flags))
-		schedule_delayed_work(&ec->work,
-				      msecs_to_jiffies(ACPI_EC_DELAY));
+	unsigned long flags;
+	spin_lock_irqsave(&ec->t_lock, flags);
+	if (!ec->t)
+		goto unlock;
+	if (ec->t->wlen > 0) {
+		if ((status & ACPI_EC_FLAG_IBF) == 0) {
+			acpi_ec_write_data(ec, *(ec->t->wdata++));
+			--ec->t->wlen;
+		} else
+			/* false interrupt, state didn't change */
+			++ec->t->irq_count;
+
+	} else if (ec->t->rlen > 0) {
+		if ((status & ACPI_EC_FLAG_OBF) == 1) {
+			*(ec->t->rdata++) = acpi_ec_read_data(ec);
+			--ec->t->rlen;
+		} else
+			/* false interrupt, state didn't change */
+			++ec->t->irq_count;
+	}
+unlock:
+	spin_unlock_irqrestore(&ec->t_lock, flags);
 }
 
-static void ec_switch_to_poll_mode(struct acpi_ec *ec)
+static int acpi_ec_wait(struct acpi_ec *ec)
 {
+	if (wait_event_timeout(ec->wait, ec_transaction_done(ec),
+			       msecs_to_jiffies(ACPI_EC_DELAY)))
+		return 0;
+	/* missing GPEs, switch back to poll mode */
+	if (printk_ratelimit())
+		pr_info(PREFIX "missing confirmations, "
+				"switch off interrupt mode.\n");
 	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);
+	return 1;
 }
 
-static int acpi_ec_wait(struct acpi_ec *ec, enum ec_event event, int force_poll)
+static void acpi_ec_gpe_query(void *ec_cxt);
+
+static int ec_check_sci(struct acpi_ec *ec, u8 state)
 {
-	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),
-				       msecs_to_jiffies(ACPI_EC_DELAY)))
-			return 0;
-		clear_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
-		if (acpi_ec_check_status(ec, event)) {
-			/* missing GPEs, switch back to poll mode */
-			if (printk_ratelimit())
-				pr_info(PREFIX "missing confirmations, "
-						"switch off interrupt mode.\n");
-			ec_switch_to_poll_mode(ec);
-			ec_schedule_ec_poll(ec);
-			return 0;
-		}
-	} else {
-		unsigned long delay = jiffies + msecs_to_jiffies(ACPI_EC_DELAY);
-		clear_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
-		while (time_before(jiffies, delay)) {
-			if (acpi_ec_check_status(ec, event))
-				return 0;
-			msleep(1);
-		}
-		if (acpi_ec_check_status(ec,event))
+	if (state & ACPI_EC_FLAG_SCI) {
+		if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags))
+			return acpi_os_execute(OSL_EC_BURST_HANDLER,
+				acpi_ec_gpe_query, ec);
+	}
+	return 0;
+}
+
+static int ec_poll(struct acpi_ec *ec)
+{
+	unsigned long delay = jiffies + msecs_to_jiffies(ACPI_EC_DELAY);
+	while (time_before(jiffies, delay)) {
+		gpe_transaction(ec, acpi_ec_read_status(ec));
+		udelay(ACPI_EC_UDELAY);
+		if (ec_transaction_done(ec))
 			return 0;
 	}
-	pr_err(PREFIX "acpi_ec_wait timeout, status = 0x%2.2x, event = %s\n",
-		acpi_ec_read_status(ec),
-		(event == ACPI_EC_EVENT_OBF_1) ? "\"b0=1\"" : "\"b1=0\"");
 	return -ETIME;
 }
 
@@ -235,45 +250,51 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec, u8 command,
 					u8 * rdata, unsigned rdata_len,
 					int force_poll)
 {
-	int result = 0;
-	set_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
+	unsigned long tmp;
+	struct transaction_data t = {.wdata = wdata, .rdata = rdata,
+				     .wlen = wdata_len, .rlen = rdata_len,
+				     .irq_count = 0};
+	int ret = 0;
 	pr_debug(PREFIX "transaction start\n");
-	acpi_ec_write_cmd(ec, command);
-	for (; wdata_len > 0; --wdata_len) {
-		result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBF_0, force_poll);
-		if (result) {
-			pr_err(PREFIX
-			       "write_cmd timeout, command = %d\n", command);
-			goto end;
-		}
-		set_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
-		acpi_ec_write_data(ec, *(wdata++));
+	/* disable GPE during transaction if storm is detected */
+	if (test_bit(EC_FLAGS_GPE_STORM, &ec->flags)) {
+		clear_bit(EC_FLAGS_GPE_MODE, &ec->flags);
+		acpi_disable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
 	}
-
-	if (!rdata_len) {
-		result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBF_0, force_poll);
-		if (result) {
-			pr_err(PREFIX
-			       "finish-write timeout, command = %d\n", command);
-			goto end;
-		}
-	} else if (command == ACPI_EC_COMMAND_QUERY)
+	/* start transaction */
+	spin_lock_irqsave(&ec->t_lock, tmp);
+	/* following two actions should be kept atomic */
+	ec->t = &t;
+	acpi_ec_write_cmd(ec, command);
+	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);
-		if (result) {
-			pr_err(PREFIX "read timeout, command = %d\n", command);
-			goto end;
-		}
-		/* Don't expect GPE after last read */
-		if (rdata_len > 1)
-			set_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
-		*(rdata++) = acpi_ec_read_data(ec);
-	}
-      end:
+	spin_unlock_irqrestore(&ec->t_lock, tmp);
+	/* if we selected poll mode or failed in GPE-mode do a poll loop */
+	if (force_poll ||
+	    !test_bit(EC_FLAGS_GPE_MODE, &ec->flags) ||
+	    acpi_ec_wait(ec))
+		ret = ec_poll(ec);
 	pr_debug(PREFIX "transaction end\n");
-	return result;
+	spin_lock_irqsave(&ec->t_lock, tmp);
+	ec->t = NULL;
+	spin_unlock_irqrestore(&ec->t_lock, tmp);
+	if (test_bit(EC_FLAGS_GPE_STORM, &ec->flags)) {
+		/* check if we received SCI during transaction */
+		ec_check_sci(ec, acpi_ec_read_status(ec));
+		/* it is safe to enable GPE outside of transaction */
+		acpi_enable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
+	} else if (test_bit(EC_FLAGS_GPE_MODE, &ec->flags) &&
+		   t.irq_count > ACPI_EC_STORM_THRESHOLD) {
+		pr_debug(PREFIX "GPE storm detected\n");
+		set_bit(EC_FLAGS_GPE_STORM, &ec->flags);
+	}
+	return ret;
+}
+
+static int ec_check_ibf0(struct acpi_ec *ec)
+{
+	u8 status = acpi_ec_read_status(ec);
+	return (status & ACPI_EC_FLAG_IBF) == 0;
 }
 
 static int acpi_ec_transaction(struct acpi_ec *ec, u8 command,
@@ -283,40 +304,34 @@ static int acpi_ec_transaction(struct acpi_ec *ec, u8 command,
 {
 	int status;
 	u32 glk;
-
 	if (!ec || (wdata_len && !wdata) || (rdata_len && !rdata))
 		return -EINVAL;
-
 	if (rdata)
 		memset(rdata, 0, rdata_len);
-
 	mutex_lock(&ec->lock);
 	if (ec->global_lock) {
 		status = acpi_acquire_global_lock(ACPI_EC_UDELAY_GLK, &glk);
 		if (ACPI_FAILURE(status)) {
-			mutex_unlock(&ec->lock);
-			return -ENODEV;
+			status = -ENODEV;
+			goto unlock;
 		}
 	}
-
-	status = acpi_ec_wait(ec, ACPI_EC_EVENT_IBF_0, 0);
-	if (status) {
+	if (!wait_event_timeout(ec->wait, ec_check_ibf0(ec),
+				msecs_to_jiffies(ACPI_EC_DELAY))) {
 		pr_err(PREFIX "input buffer is not empty, "
 				"aborting transaction\n");
+		status = -ETIME;
 		goto end;
 	}
-
 	status = acpi_ec_transaction_unlocked(ec, command,
 					      wdata, wdata_len,
 					      rdata, rdata_len,
 					      force_poll);
-
-      end:
-
+end:
 	if (ec->global_lock)
 		acpi_release_global_lock(glk);
+unlock:
 	mutex_unlock(&ec->lock);
-
 	return status;
 }
 
@@ -332,7 +347,9 @@ int acpi_ec_burst_enable(struct acpi_ec *ec)
 
 int acpi_ec_burst_disable(struct acpi_ec *ec)
 {
-	return acpi_ec_transaction(ec, ACPI_EC_BURST_DISABLE, NULL, 0, NULL, 0, 0);
+	return (acpi_ec_read_status(ec) & ACPI_EC_FLAG_BURST)?
+		acpi_ec_transaction(ec, ACPI_EC_BURST_DISABLE,
+			NULL, 0, NULL, 0, 0) : 0;
 }
 
 static int acpi_ec_read(struct acpi_ec *ec, u8 address, u8 * data)
@@ -513,46 +530,26 @@ static void acpi_ec_gpe_query(void *ec_cxt)
 
 static u32 acpi_ec_gpe_handler(void *data)
 {
-	acpi_status status = AE_OK;
 	struct acpi_ec *ec = data;
-	u8 state = acpi_ec_read_status(ec);
+	u8 status;
 
 	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))
+	status = acpi_ec_read_status(ec);
+
+	gpe_transaction(ec, status);
+	if (ec_transaction_done(ec) && (status & ACPI_EC_FLAG_IBF) == 0)
 		wake_up(&ec->wait);
 
-	if (state & ACPI_EC_FLAG_SCI) {
-		if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags))
-			status = acpi_os_execute(OSL_EC_BURST_HANDLER,
-				acpi_ec_gpe_query, ec);
-	} else if (!test_bit(EC_FLAGS_GPE_MODE, &ec->flags) &&
-		   !test_bit(EC_FLAGS_NO_GPE, &ec->flags) &&
-		   in_interrupt()) {
+	ec_check_sci(ec, status);
+	if (!test_bit(EC_FLAGS_GPE_MODE, &ec->flags) &&
+	    !test_bit(EC_FLAGS_NO_GPE, &ec->flags)) {
 		/* this is non-query, must be confirmation */
 		if (printk_ratelimit())
 			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;
-}
-
-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);
+	return ACPI_INTERRUPT_HANDLED;
 }
 
 /* --------------------------------------------------------------------------
@@ -696,8 +693,7 @@ static struct acpi_ec *make_acpi_ec(void)
 	mutex_init(&ec->lock);
 	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);
+	spin_lock_init(&ec->t_lock);
 	return ec;
 }
 
@@ -736,22 +732,15 @@ 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");
 	if (ACPI_FAILURE(acpi_remove_gpe_handler(NULL, ec->gpe,
 				&acpi_ec_gpe_handler)))
 		pr_err(PREFIX "failed to remove gpe handler\n");
-	ec->handlers_installed = 0;
+	clear_bit(EC_FLAGS_HANDLERS_INSTALLED, &ec->flags);
 }
 
 static int acpi_ec_add(struct acpi_device *device)
@@ -846,17 +835,15 @@ ec_parse_io_ports(struct acpi_resource *resource, void *context)
 static int ec_install_handlers(struct acpi_ec *ec)
 {
 	acpi_status status;
-	if (ec->handlers_installed)
+	if (test_bit(EC_FLAGS_HANDLERS_INSTALLED, &ec->flags))
 		return 0;
 	status = acpi_install_gpe_handler(NULL, ec->gpe,
-					  ACPI_GPE_EDGE_TRIGGERED,
-					  &acpi_ec_gpe_handler, ec);
+				  ACPI_GPE_EDGE_TRIGGERED,
+				  &acpi_ec_gpe_handler, ec);
 	if (ACPI_FAILURE(status))
 		return -ENODEV;
-
 	acpi_set_gpe_type(NULL, ec->gpe, ACPI_GPE_TYPE_RUNTIME);
 	acpi_enable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
-
 	status = acpi_install_address_space_handler(ec->handle,
 						    ACPI_ADR_SPACE_EC,
 						    &acpi_ec_space_handler,
@@ -866,7 +853,7 @@ static int ec_install_handlers(struct acpi_ec *ec)
 		return -ENODEV;
 	}
 
-	ec->handlers_installed = 1;
+	set_bit(EC_FLAGS_HANDLERS_INSTALLED, &ec->flags);
 	return 0;
 }
 
@@ -887,7 +874,6 @@ static int acpi_ec_start(struct acpi_device *device)
 
 	/* EC is fully operational, allow queries */
 	clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
-	ec_schedule_ec_poll(ec);
 	return ret;
 }
 
@@ -906,7 +892,7 @@ static int acpi_ec_stop(struct acpi_device *device, int type)
 
 int __init acpi_boot_ec_enable(void)
 {
-	if (!boot_ec || boot_ec->handlers_installed)
+	if (!boot_ec || test_bit(EC_FLAGS_HANDLERS_INSTALLED, &boot_ec->flags))
 		return 0;
 	if (!ec_install_handlers(boot_ec)) {
 		first_ec = boot_ec;

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

[Zhao Yakui]

On Mon, 2008-09-22 at 15:49 +0400, Alexey Starikovskiy wrote:
> Hi Alan,
Can you give some explanation why the EC GPE is also disabled during the
EC querying?

   Rui and I raised several issues about the fast-transaction.patch. But
there is no improvement or explanation about it.
   a. bogus timeout 
   b. incorrect EC status before EC GPE arrives on some laptops. 

   ec->t will be accessed by acpi_ec_gpe_handler and ec_poll at the same
time when the force_poll is non-zero or the GPE mode is cleared.
Although there is no problem after adding the spin_lock, IMO it is
overkill. This will also affect the laptops without any problems. If
there are about 1000 EC GPE interrupts per second, it will disable local
CPU interrupt for so long time(About 1ms).

   At the same time this is pointed several times about the following
ugly code.
    The address of local variable is assigned to the global pointer
variable. Although it won't break the system, it looks very ugly. 
      >struct transaction_data t = {.wdata = wdata, .rdata = rdata,
                                     .wlen = wdata_len, .rlen =
rdata_len};
      >ec->t = &t;
      And the ec->t will be accessed in the function of
acpi_ec_gpe_handler(Interrupt context).
    

   
   
> 
> Here are the patches. First is updated version of patch you've tried, 
> second is
> even more aggressive disabling of GPEs (applies on top).
> 
> Please check them.
> 
> Thanks,
> Alex.
> 
> Alexey Starikovskiy wrote:
> > Alan Jenkins wrote:
> >> Alan Jenkins wrote:
> >>> Alexey Starikovskiy wrote:
> >>>  
> >>>> Hi Alan,
> >>>> That patch is old news already...
> >>>> There is a new shiny one appended to 9998/10724/11549...
> >>>> Please give it a try. It does disable GPE, but for very small 
> >>>> duration.
> >>>>     
> >>> Ok.  I was put off by the noise :-).
> >>>
> >>> I've just tested 2.6.27-rc6 with
> >>> <http://bugzilla.kernel.org/show_bug.cgi?id=9998#c81>.  It still 
> >>> "drops"
> >>> some events, but now it takes longer to happen.  I have to work much
> >>> harder bashing the keys to reproduce it.
> >>>
> >>>
> >>> Like before, missing an event has severe consequences.  The missed 
> >>> event
> >>> is buffered.  When a new event occurs, only the oldest event is removed
> >>> from the buffer.  Therefore the buffer can only grow.  Eventually,
> >>> something breaks.  Events stop being delivered altogether; presumably
> >>> the buffer overflows.  I confirmed that this does still happen.
> >>>
> >>> Remember that these are the consequences of a specific EC bug.  On my
> >>> EC, querying an event always clears SCI_EVT, even if there are more
> >>> events pending.  It is only re-raised when a new event fires.
> >>>
> >>>
> >>> I'll try reading the patch.  I may try capturing an EC debug log to 
> >>> show
> >>> how the event is dropped, but that will take time.
> >>>   
> >>
> >> Ok, I think I've isolated the problem to a specific change.  It's our
> >> good friend EC_FLAGS_QUERY_PENDING again.
> >>
> >> Your new patch clears the pending flag after the query transaction has
> >> finished.  Previously, it was cleared as soon as the query command was
> >> initiated.
> >>
> >> I hacked it back and it works.  Was there a specific reason for the 
> >> change?
> > Thanks! There was no particular reason to put it after transaction, 
> > other than
> > original place is gone.
> >
> > I'll post updated patch.
> > Regards,
> > Alex.
> >>
> >> Regards
> >> Alan
> >>
> >> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> >> index 97168d4..2078cff 100644
> >> --- a/drivers/acpi/ec.c
> >> +++ b/drivers/acpi/ec.c
> >> @@ -262,6 +262,8 @@ static int acpi_ec_transaction_unlocked(struct 
> >> acpi_ec *ec, u8 command,
> >>      ec->t.rlen = rdata_len;
> >>      /* start transaction */
> >>      acpi_ec_write_cmd(ec, command);
> >> +    if (command == ACPI_EC_COMMAND_QUERY)
> >> +        clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
> >>      /* if we selected poll mode or failed in GPE-mode do a poll loop */
> >>      if (force_poll ||
> >>          !test_bit(EC_FLAGS_GPE_MODE, &ec->flags) ||
> >> @@ -453,7 +455,6 @@ static int acpi_ec_query(struct acpi_ec *ec, u8 * 
> >> data)
> >>       */
> >>  
> >>      result = acpi_ec_transaction(ec, ACPI_EC_COMMAND_QUERY, NULL, 0, 
> >> &d, 1, 0);
> >> -    clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
> >>      if (result)
> >>          return result;
> >>  
> >>
> >>
> >
> > -- 
> > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>