| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <487F9677.4060001@suse.de>
Date: Thu, 17 Jul 2008 22:59:03 +0400
From: Alexey Starikovskiy <astarikovskiy@...e.de>
To: Alan Jenkins <alan-jenkins@...fmail.co.uk>
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
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@...nel.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@...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