Re: [RFC] [Patch 0/4] ACPI : several patches for EC

[Alexey Starikovskiy <aystarik@gmail.com>]

Alexey Starikovskiy wrote:
> Zhao Yakui wrote:
>> But I think that the spin_lock is overkill in the updated patch.
>> Assuming that 1000 EC transactions are done per second, the CPU
>> interrupt is disabled for 1ms. It is important that the normal laptops
>> will be affected by this.
>>   
> How do you arrive with these numbers? Where do you get this 1ms?
> Spinlock is around single inb/outb instruction plus several even simpler
> instructions. Do you claim it is going to take 1us? Do you claim that 
> it will
> add anything to interrupts-disabled time of ACPI SCI interrupt handler
> itself?
>>       At the same time the following source code looks so ugly.
>>   
> It is a matter of taste and, probably, experience.
>
>
>
>> Please look at the following source to see whether the bogus timeout
>> happens.
>>    >spin_unlock_irqrestore(&ec->spinlock, tmp);
>>    >    /* if we selected poll mode or failed in GPE-mode do a poll 
>> loop */     >    if (force_poll ||
>>    >        !test_bit(EC_FLAGS_GPE_MODE, &ec->flags) ||
>>    >        acpi_ec_wait(ec))
>>    >        ret = ec_poll(ec);
>>        
>>     When EC GPE storm happens, EC driver will work in polling mode. 
>> And it will
>> call the function of ec_poll after issuing the EC command.
>>    static int ec_poll(struct acpi_ec *ec)
>> {
>>     unsigned long delay = jiffies + msecs_to_jiffies(ACPI_EC_DELAY);
>>         //If preempt schedule happens here, the timeout happens after 
>> it is rescheduled.
>>         // In such case how to issue the following EC command sequence?
>>       while (time_before(jiffies, delay)) {
>>         gpe_transaction(ec, acpi_ec_read_status(ec));
>>         /* do a shortest sleep */
>>         msleep(1);
>>         if (ec_transaction_done(ec))
>>              return 0;
>>             //If the preempt schedule happens here, maybe the timeout 
>> happens when it is rescheduled.                 // And the EC 
>> transaction is not finished. How to explain it?
>>      }
>> -    pr_err(PREFIX "acpi_ec_wait timeout, status = 0x%2.2x, event = 
>> %s\n",
>> -        acpi_ec_read_status(ec),
>> -        (event == ACPI_EC_EVENT_OBF_1) ? "\"b0=1\"" : "\"b1=0\"");
>>      return -ETIME;
>>  }
>>        I don't care whether the above bogus timeout often happens.  
>> What I cared is
>> how to explain it or process it.        Regarded it as Timeout or 
>> ignore it? Unreasonable.      
>>   
> You seem to agree, that with working scheduler it is not possible that 
> this code will not be
> executed for half a second, right? Then, with broken scheduler (the 
> one with 120s sleeps),
> you arrive at this code 120 seconds late and still want to continue 
> transaction?
> 500msec here is _cut off_ after which we don't care about this 
> transaction. In normal
> conditions transaction should not _ever_ come close to 1/10th of this 
> value.
>>>>    b. How to deal with the laptop with "incorrect EC status before EC
>>>> GPE arrives". For example: bug 11309 (GPE storm happens and OS will
>>>> report the incorrect temperature while EC GPE is disabled.)
>>>>       
>>> This is _your guess_. None of your patches was reported to fix a 
>>> situation,
>>> and submitter is not able to compile kernel.
>>>     
>> The bug reporter doesn't give response. But please pay attention to this
>> is a regression.
>>     The detect of EC GPE storm is not shipped in 2.6.24.x kernel.
>>     The detect of EC GPE storm is shipped in 2.6.26.x kernel.
>>     On the 2.6.24.x kernel there is no detection of EC GPE storm and EC
>> GPE is enabled. In such case the temperature is reported correctly.   
>>     And after EC GPE storm happens , sometimes it will report the
>> incorrect thermal zone temperature, which causes that the system will be
>> shutdown. (In such case EC GPE is disabled and EC will work in polling
>> mode).
>>     How to explain it? The possible cause is that EC status is incorrect
>> before EC GPE arrives.
>>
>>   
> It is easy to add same msleep(1) before the while() loop to overcome
> this, right? So, as soon as the submitter will be able to test patches,
> and if he finds his hardware still not working correctly, we could
> easily fix it with 1-liner patch.
>> The laptop of bug 8110 is fixed by the following commit.
>>     >commit 9e197219605513c14d3eae41039ecf1b82d1920d
>>      > Author: Alexey Starikovskiy <alexey.y.starikovskiy@intel.com>
>>      > Date:   Wed Mar 7 18:29:35 2007 -0500
>>         >ACPI: ec: fix race in status register access
>>       If the EC GPE storm happens on this laptop, I believe that this
>> laptop will be broken by your proposed patch again.
>>  
Actually, this patch suggests, that there is _no_ storm. If storm was were,
simply waiting for an interrupt (which come in tens) does not help.
> Do you want me to insert the above msleep(1) now, or do you want
> me to drop my patch and instead go with yours?
>