Zhao Yakui wrote:
>
>     >ACPI: EC: do transaction from interrupt context
>
>    In the above patch there exist the following problems:
>    a. In the following code the GPE_STORM bit will always be set regardless 
> of whether there exists the EC GPE storm.
>     >if (test_bit(EC_FLAGS_GPE_STORM, &ec->flags)) {
>     >            /* check if we received SCI during transaction */
>     >           ec_check_sci(ec, acpi_ec_read_status(ec));
>     >           /* it is safe to enable GPE outside of transaction */
>     >            acpi_enable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
>     >    } else if (test_bit(EC_FLAGS_GPE_MODE, &ec->flags) &&
>     >               atomic_read(&ec->irq_count) > ACPI_EC_STORM_THRESHOLD)
>     >           pr_debug(PREFIX "GPE storm detected\n");
>     >           set_bit(EC_FLAGS_GPE_STORM, &ec->flags);
>   
This problem is fixed two days ago, it is not completely fair to mention 
it here.
>     b. When EC GPE storm is detected, the EC GPE will be disabled and 
> EC will work in polling mode while doing EC transaction. In such case 
> maybe EC transaction is not finished. But when timeout happens, it 
> will still be regarded as finishing the EC transaction.
>   
Ok. Fixed, now -ETIME is returned in this case.
>        This error is related with the following source code:
>            > {
>            >    delay = jiffies + msecs_to_jiffies(ACPI_EC_DELAY); 
>  //Maybe the preemptible schedule happens here.If the current jiffies
>  is greater than the predefined jiffies after the process is returned 
> from preempt_schedule, OS will have no opportunity to do the EC transaction.
>  It means that EC transaction is not finished. But it is still regarded as 
> finishing EC transaction. It is incorrect.
>            >     while (time_before(jiffies, delay)) {
>             >            gpe_transaction(ec, acpi_ec_read_status(ec));
>             >            msleep(1);
> //as msleep is realized by using the function of schedule_timeout, 
> maybe the current jiffies is already greater than the predefined jiffies 
> before finishing EC transaction. In such case the EC transaction is
> not finished. But it is also regarded as finishing. It is incorrect.
>             >            if (ec_transaction_done(ec))
>             >                    goto end;
>             >    }
>   
ec_transaction_done() returns true only if there is finished transaction 
stored in
acpi_ec. It does not matter if it is late to check for this result from 
timeout point of view.
the same ideology was put into driver before this patch:
"ACPI: Avoid bogus EC timeout when EC is in Polling mode"
>      c. There is no detailed change log although a lot is changed.(
> Including the EC work mode, the detect mechanism of EC GPE storm). 
> It is not easy to understand.     
>
>      At the same time this commit can't handle the EC notification
> event in the following case:
>          EC GPE Interrupt storm is detected and EC GPE will be disabled
> when doing EC transaction. If EC notification event happens while doing
> EC transaction(EC GPE is disabled), the SCI_EVT bit of EC status
> register is cleared before OS issuing query command. In such case the EC
> notification event will be lost.
>   
As I said already, there is no way EC will clear SCI bit before driver 
issues
the Query command to it (thus reading the event).
Even more, this same behavior was in the driver for ages and it did
not broke once.
>   Based on the above analysis ,IMO the two patches are not appropriate.
>
>   
This is the best analysis I've seen so far :) Thanks, it is really 
entertaining.
Please use latest version next time, it is attached for your convenience.

Regards,
Alex.