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@xxxxxxxxxx) 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@xxxxxxxxxxxxxx>
Tested-by: Alan Jenkins <alan-jenkins@xxxxxxxxxxxxxx>
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-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
