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

[Alexey Starikovskiy <astarikovskiy@suse.de>]

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