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@xxxxxxxxxx) and grovel to akpm, etc.
You're latest (quieter) work still applies on top and works fine.
Thanks
Alan
---
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
