Date:	Thu, 17 Jul 2008 19:55:14 +0100
From:	Alan Jenkins <alan-jenkins@...fmail.co.uk>
To:	Alexey Starikovskiy <astarikovskiy@...e.de>
CC:	linux-acpi@...r.kernel.org,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Henrique de Moraes Holschuh <hmh@....eng.br>
Subject: Re: [PATCH] acpi: Avoid dropping rapid hotkey events (or other GPEs)
 on Asus EeePC

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



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists