Re: the errors about two EC patches

[Alexey Starikovskiy <aystarik@gmail.com>]

Zhao Yakui wrote:
> On Wed, 2008-09-10 at 16:37 +0400, Alexey Starikovskiy wrote:
> thanks for the so quick response. 
>
> In the update patch it seems that spin_lock is used in the function of
> ec_read_command/acpi_ec_write_cmd.
>    IMO this is not reasonable. 
>    Maybe the spin_lock is already called before OS evaluates some ACPI
> object, in which the EC internal register will be accessed. In such case
> there will give the following warning message.
>       >2 locks held by swapper/1
>
>   
I've just consulted with Greg KH about the non-sense you tell about.
spinlock will always be acquired while already holding a mutex, and
released while still holding mutex. This is perfectly fine use of mutexes
and spinlocks. You should never use reverse order, obviously.
>>  
>> Ok. Fixed, now -ETIME is returned in this case.
>>     
> Yes. The code flow seems logic and reasonable after the -Etime is added.
> If the timeout happens when EC transaction is not finished, it is
> regarded as timeout.
>
> But we should investigate why EC transaction is not finished when
> timeout happens. Is it related with EC hardware or EC driver?
> If EC can't update the status register in time after issuing
> command/address, it is reasonable that it is regarded as timeout. If
> not, maybe it is related with the EC driver.
>
>   
We should investigate that only if we receive such timeouts.
This is clearly only needed for debug purposes, as no user
of EC driver is interested in why exactly it did not made a
transaction, only that it failed temporarily (as opposed to
-EFAIL or -ENODEV), which indicate permanent failure.
Thus, there is no reason to make flowchart (your favorite)
any more complex.
> In fact when timeout happens in the above cases, OS has no opportunity
> to issue the EC command set completely.
>     For example: Read Command: 
>       a. Maybe OS has no opportunity to issue the accessed EC register
> address.
>       b. Maybe OS has no opportunity to read the returned data from EC
> controller
> In such case it is not appropriate that it is still regarded as timeout.
>
> Maybe we should use the several phases to issue the EC transaction. And
> In every phase OS should check whether the EC status is what OS expected
> in the predefined time. If in some phase EC status is not what OS
> expected, it can be regarded as timeout.
> For example: EC Read transaction:
>       a. Issuing the read command. (Write the 0x80 to EC Cmd port)
>       b. Checking whether the input buffer is empty(IBF bit) and write
> the accessed address to EC Data port.
>       c. Checking wheter the data is already(OBF bit) and read the
> returned data from EC controller
>
> Based on the above analysis IMO the flow of EC transaction is quite
> reasonable. Of course what should be improved is how to make it
> reasonable when waiting whether EC status is what OS expected.
>
> Please check whether the attached is reasonable.
>
> Of course in the attached patch there is no detect mechanism of EC GPE
> interrupt storm.
>
>   
Regards,
Alex.