a problem about the two patches in bug 10724 & 11428

a problem about the two patches in bug 10724 & 11428

[Zhao Yakui]

Hi, Alexey
    You have a lot of experiences about EC driver and a lot of EC patches are from you.
    You attach two patches related with EC driver in the bug 10724 & 11428.
    In the bug 10724:
        Add a boot ption of "ec_intr= " to select the EC work mode( Interrupt/Polling mode)
        After this patch is applied, EC won't switch to polling mode automatically from interrupt mode when EC irq storm is detected.
    In the bug 11428
        In this patch the EC won't switch off GPE mode on missing interrupts

    Will the above two patches hit the upstream kernel? If the above two patches hits the upstream kernel, it 
seems that the boot option of "ec_intr=" comes back again and EC can't be switched from interrupt mode to polling 
mode if EC GPE interrupt is missing. In such case maybe the battery/AC/thermal driver can't work well if the info of
battery/AC/thermal is related with EC. Maybe there exists the regression on some laptops.
    At the same time the boot option of "ec_intr=1" indicates that EC will work in EC GPE interrupt mode. But maybe the following
phenomenon will appear on some laptops:
   EC is initialized as polling mode. After the EC GPE interrupt is triggered, it will be switched to GPE interrupt mode. But if
no EC GPE interrupt is triggered, it will still work in polling mode. Although it is harmless and EC can also work well, 
maybe the EC working mode is inconsistent with the EC boot option. At the same time after the system is resumed from S3, the EC working
mode will also be polling mode in the function of acpi_ec_resume.

    IMO the EC working mode is not very reasonable if the above two patches hit the upstream kernel.
    Are the following two methods reasonable? Which of them is better?

    a. Add the boot option of "ec_intr=" (ec_intr=auto, intr, polling). For most laptops the boot option of "ec_intr=auto" is the default option
and "ec_intr=auto" means that the EC working mode can be switched automatically between interrupt mode and polling mode.The purpose of "ec_intr=auto"
is used to avoid the regression on some laptops.(Maybe some laptops can't work well if the EC mode switch is disabled.) 
     If the boot option of "ec_intr=intr" is added, the EC will be forced to work in interrupt mode and can't be switched to polling mode even 
when the EC confirmation GPE interrupt is missing. 
     If the boot option of "ec_intr=polling" is added, the EC will be forced to work in polling mode.
     
     In such case the EC working mode is related with user input. 

    b. EC mode switch is disabled on some specific laptops. In such case the user doesn't care for the EC working mode. At the same time
EC is still started in polling mode. It will be switched to interrupt mode if the EC GPE interrupt is triggered. EC will be switched to polling
mode on most laptops in case of missing confirmation GPE interrupt. But on some specific laptops the EC mode switch is disabled, which means
that EC will still work in interrupt mode even when the EC confirmation GPE interrupt is missing. This can be realized by adding EC DMI check table.

    I am not sure whether my above suggestion is appropriate. 

Thanks for the comments.

Best regards.
   Yakui

Re: a problem about the two patches in bug 10724 & 11428

[Alexey Starikovskiy]

Hi Yakui,
Zhao Yakui wrote:
> Hi, Alexey
>     You have a lot of experiences about EC driver and a lot of EC patches are from you.
>     You attach two patches related with EC driver in the bug 10724 & 11428.
>     In the bug 10724:
>         Add a boot ption of "ec_intr= " to select the EC work mode( Interrupt/Polling mode)
>         After this patch is applied, EC won't switch to polling mode automatically from interrupt mode when EC irq storm is detected.
>     In the bug 11428
>         In this patch the EC won't switch off GPE mode on missing interrupts
> 
>     Will the above two patches hit the upstream kernel? If the above two patches hits the upstream kernel, it 
> seems that the boot option of "ec_intr=" comes back again and EC can't be switched from interrupt mode to polling 
> mode if EC GPE interrupt is missing. In such case maybe the battery/AC/thermal driver can't work well if the info of
> battery/AC/thermal is related with EC. Maybe there exists the regression on some laptops.
There are two steps here. First, you don't use GPE while waiting for status to become expected value (here you don't care if GPE is at all enabled) and second, there you can't handle GPE coming in very fast rate -- thus you need to disable it.
11428 restores first behavior, there we don't touch GPE if it does not hurt us, and provide a way to manually switch it off if it does. 

Once again, there is driver mode and there is GPE enabled/disabled. ec_intr controls the latter, and driver mode tries to follow automatically.
>     At the same time the boot option of "ec_intr=1" indicates that EC will work in EC GPE interrupt mode. But maybe the following
> phenomenon will appear on some laptops:
>    EC is initialized as polling mode. After the EC GPE interrupt is triggered, it will be switched to GPE interrupt mode. But if
> no EC GPE interrupt is triggered, it will still work in polling mode. Although it is harmless and EC can also work well, 
> maybe the EC working mode is inconsistent with the EC boot option. At the same time after the system is resumed from S3, the EC working
> mode will also be polling mode in the function of acpi_ec_resume.
Why do you care about such consistency? Code is trying to use interrupt mode if it allowed to. If it fails, it reports this failure. 
> 
>     IMO the EC working mode is not very reasonable if the above two patches hit the upstream kernel.
It is reasonable for me. If you read code more carefully, you might find it reasonable too.
>     Are the following two methods reasonable? Which of them is better?
No. Niether.
> 
>     a. Add the boot option of "ec_intr=" (ec_intr=auto, intr, polling). For most laptops the boot option of "ec_intr=auto" is the default option
> and "ec_intr=auto" means that the EC working mode can be switched automatically between interrupt mode and polling mode.The purpose of "ec_intr=auto"
> is used to avoid the regression on some laptops.(Maybe some laptops can't work well if the EC mode switch is disabled.) 
>      If the boot option of "ec_intr=intr" is added, the EC will be forced to work in interrupt mode and can't be switched to polling mode even 
> when the EC confirmation GPE interrupt is missing. 
>      If the boot option of "ec_intr=polling" is added, the EC will be forced to work in polling mode.
>      
>      In such case the EC working mode is related with user input. 
> 
>     b. EC mode switch is disabled on some specific laptops. In such case the user doesn't care for the EC working mode. At the same time
> EC is still started in polling mode. It will be switched to interrupt mode if the EC GPE interrupt is triggered. EC will be switched to polling
> mode on most laptops in case of missing confirmation GPE interrupt. But on some specific laptops the EC mode switch is disabled, which means
> that EC will still work in interrupt mode even when the EC confirmation GPE interrupt is missing. This can be realized by adding EC DMI check table.
> 
>     I am not sure whether my above suggestion is appropriate. 
> 
> Thanks for the comments.
> 
> Best regards.
>    Yakui
> 
Regards,
Alex.

Re: a problem about the two patches in bug 10724 & 11428

[Zhao Yakui]

On Mon, 2008-09-01 at 11:49 +0400, Alexey Starikovskiy wrote:
> Hi Yakui,
> Zhao Yakui wrote:
> > Hi, Alexey
> >     You have a lot of experiences about EC driver and a lot of EC patches are from you.
> >     You attach two patches related with EC driver in the bug 10724 & 11428.
> >     In the bug 10724:
> >         Add a boot ption of "ec_intr= " to select the EC work mode( Interrupt/Polling mode)
> >         After this patch is applied, EC won't switch to polling mode automatically from interrupt mode when EC irq storm is detected.
> >     In the bug 11428
> >         In this patch the EC won't switch off GPE mode on missing interrupts
> > 
> >     Will the above two patches hit the upstream kernel? If the above two patches hits the upstream kernel, it 
> > seems that the boot option of "ec_intr=" comes back again and EC can't be switched from interrupt mode to polling 
> > mode if EC GPE interrupt is missing. In such case maybe the battery/AC/thermal driver can't work well if the info of
> > battery/AC/thermal is related with EC. Maybe there exists the regression on some laptops.
> There are two steps here. First, you don't use GPE while waiting for status to become expected value (here you don't care if GPE is at all enabled) and second, there you can't handle GPE coming in very fast rate -- thus you need to disable it.
> 11428 restores first behavior, there we don't touch GPE if it does not hurt us, and provide a way to manually switch it off if it does. 
For the first step: If EC time out happens while waiting for the EC
status to become expected value(in EC GPE mode), maybe it can be
regarded as that EC can't return the expected status in the predefined
time.  In such case it will be more reasonable that EC is switched to
polling mode.(In most cases the EC will return the expected status in a
very short time while EC works in interrupt mode). 
     Of course it is appropriate that the mode switch is disabled on
some specific laptops. Maybe the EC can still return the expected status
in the predefined time after time out happens once on such laptops.

For the second step: Understand what you said. If OS can't handle the
coming GPE in a very fast rate, it should be disabled. But if the manual
switch is used, the user must reboot the system with the option of
"ec_intr=0" and EC will always be in Polling mode. What will be affected
if EC is automatically switched to polling mode and EC GPE is disabled?
> 
> Once again, there is driver mode and there is GPE enabled/disabled. ec_intr controls the latter, and driver mode tries to follow automatically.
> >     At the same time the boot option of "ec_intr=1" indicates that EC will work in EC GPE interrupt mode. But maybe the following
> > phenomenon will appear on some laptops:
> >    EC is initialized as polling mode. After the EC GPE interrupt is triggered, it will be switched to GPE interrupt mode. But if
> > no EC GPE interrupt is triggered, it will still work in polling mode. Although it is harmless and EC can also work well, 
> > maybe the EC working mode is inconsistent with the EC boot option. At the same time after the system is resumed from S3, the EC working
> > mode will also be polling mode in the function of acpi_ec_resume.
> Why do you care about such consistency? Code is trying to use interrupt mode if it allowed to. If it fails, it reports this failure.
The consistency is meaningless. In fact I don't care about it. What I
want to say is that EC working mode maybe is not what the boot option of
"ec_intr=1" expected.
>  
> > 
> >     IMO the EC working mode is not very reasonable if the above two patches hit the upstream kernel.
> It is reasonable for me. If you read code more carefully, you might find it reasonable too.
Understand what you said.
> >     Are the following two methods reasonable? Which of them is better?
> No. Niether.
> > 
> >     a. Add the boot option of "ec_intr=" (ec_intr=auto, intr, polling). For most laptops the boot option of "ec_intr=auto" is the default option
> > and "ec_intr=auto" means that the EC working mode can be switched automatically between interrupt mode and polling mode.The purpose of "ec_intr=auto"
> > is used to avoid the regression on some laptops.(Maybe some laptops can't work well if the EC mode switch is disabled.) 
> >      If the boot option of "ec_intr=intr" is added, the EC will be forced to work in interrupt mode and can't be switched to polling mode even 
> > when the EC confirmation GPE interrupt is missing. 
> >      If the boot option of "ec_intr=polling" is added, the EC will be forced to work in polling mode.
> >      
> >      In such case the EC working mode is related with user input. 
> > 
> >     b. EC mode switch is disabled on some specific laptops. In such case the user doesn't care for the EC working mode. At the same time
> > EC is still started in polling mode. It will be switched to interrupt mode if the EC GPE interrupt is triggered. EC will be switched to polling
> > mode on most laptops in case of missing confirmation GPE interrupt. But on some specific laptops the EC mode switch is disabled, which means
> > that EC will still work in interrupt mode even when the EC confirmation GPE interrupt is missing. This can be realized by adding EC DMI check table.
> > 
> >     I am not sure whether my above suggestion is appropriate. 
> > 
> > Thanks for the comments.
> > 
> > Best regards.
> >    Yakui
> > 
> Regards,
> Alex.

Re: a problem about the two patches in bug 10724 & 11428

[Alexey Starikovskiy]

Please read this, and then ask your questions again.

>>Once again, there is driver mode and there is GPE enabled/disabled. ec_intr controls the latter, and driver mode tries to follow automatically.


Zhao Yakui wrote:
> On Mon, 2008-09-01 at 11:49 +0400, Alexey Starikovskiy wrote:
>> Hi Yakui,
>> Zhao Yakui wrote:
>>> Hi, Alexey
>>>     You have a lot of experiences about EC driver and a lot of EC patches are from you.
>>>     You attach two patches related with EC driver in the bug 10724 & 11428.
>>>     In the bug 10724:
>>>         Add a boot ption of "ec_intr= " to select the EC work mode( Interrupt/Polling mode)
>>>         After this patch is applied, EC won't switch to polling mode automatically from interrupt mode when EC irq storm is detected.
>>>     In the bug 11428
>>>         In this patch the EC won't switch off GPE mode on missing interrupts
>>>
>>>     Will the above two patches hit the upstream kernel? If the above two patches hits the upstream kernel, it 
>>> seems that the boot option of "ec_intr=" comes back again and EC can't be switched from interrupt mode to polling 
>>> mode if EC GPE interrupt is missing. In such case maybe the battery/AC/thermal driver can't work well if the info of
>>> battery/AC/thermal is related with EC. Maybe there exists the regression on some laptops.
>> There are two steps here. First, you don't use GPE while waiting for status to become expected value (here you don't care if GPE is at all enabled) and second, there you can't handle GPE coming in very fast rate -- thus you need to disable it.
>> 11428 restores first behavior, there we don't touch GPE if it does not hurt us, and provide a way to manually switch it off if it does. 
> For the first step: If EC time out happens while waiting for the EC
> status to become expected value(in EC GPE mode), maybe it can be
> regarded as that EC can't return the expected status in the predefined
> time.  In such case it will be more reasonable that EC is switched to
> polling mode.(In most cases the EC will return the expected status in a
> very short time while EC works in interrupt mode). 
>      Of course it is appropriate that the mode switch is disabled on
> some specific laptops. Maybe the EC can still return the expected status
> in the predefined time after time out happens once on such laptops.
> 
> For the second step: Understand what you said. If OS can't handle the
> coming GPE in a very fast rate, it should be disabled. But if the manual
> switch is used, the user must reboot the system with the option of
> "ec_intr=0" and EC will always be in Polling mode. What will be affected
> if EC is automatically switched to polling mode and EC GPE is disabled?
>> Once again, there is driver mode and there is GPE enabled/disabled. ec_intr controls the latter, and driver mode tries to follow automatically.
>>>     At the same time the boot option of "ec_intr=1" indicates that EC will work in EC GPE interrupt mode. But maybe the following
>>> phenomenon will appear on some laptops:
>>>    EC is initialized as polling mode. After the EC GPE interrupt is triggered, it will be switched to GPE interrupt mode. But if
>>> no EC GPE interrupt is triggered, it will still work in polling mode. Although it is harmless and EC can also work well, 
>>> maybe the EC working mode is inconsistent with the EC boot option. At the same time after the system is resumed from S3, the EC working
>>> mode will also be polling mode in the function of acpi_ec_resume.
>> Why do you care about such consistency? Code is trying to use interrupt mode if it allowed to. If it fails, it reports this failure.
> The consistency is meaningless. In fact I don't care about it. What I
> want to say is that EC working mode maybe is not what the boot option of
> "ec_intr=1" expected.
>>  
>>>     IMO the EC working mode is not very reasonable if the above two patches hit the upstream kernel.
>> It is reasonable for me. If you read code more carefully, you might find it reasonable too.
> Understand what you said.
>>>     Are the following two methods reasonable? Which of them is better?
>> No. Niether.
>>>     a. Add the boot option of "ec_intr=" (ec_intr=auto, intr, polling). For most laptops the boot option of "ec_intr=auto" is the default option
>>> and "ec_intr=auto" means that the EC working mode can be switched automatically between interrupt mode and polling mode.The purpose of "ec_intr=auto"
>>> is used to avoid the regression on some laptops.(Maybe some laptops can't work well if the EC mode switch is disabled.) 
>>>      If the boot option of "ec_intr=intr" is added, the EC will be forced to work in interrupt mode and can't be switched to polling mode even 
>>> when the EC confirmation GPE interrupt is missing. 
>>>      If the boot option of "ec_intr=polling" is added, the EC will be forced to work in polling mode.
>>>      
>>>      In such case the EC working mode is related with user input. 
>>>
>>>     b. EC mode switch is disabled on some specific laptops. In such case the user doesn't care for the EC working mode. At the same time
>>> EC is still started in polling mode. It will be switched to interrupt mode if the EC GPE interrupt is triggered. EC will be switched to polling
>>> mode on most laptops in case of missing confirmation GPE interrupt. But on some specific laptops the EC mode switch is disabled, which means
>>> that EC will still work in interrupt mode even when the EC confirmation GPE interrupt is missing. This can be realized by adding EC DMI check table.
>>>
>>>     I am not sure whether my above suggestion is appropriate. 
>>>
>>> Thanks for the comments.
>>>
>>> Best regards.
>>>    Yakui
>>>
>> Regards,
>> Alex.
> 

Re: a problem about the two patches in bug 10724 & 11428

[Zhao Yakui]

On Mon, 2008-09-01 at 11:49 +0400, Alexey Starikovskiy wrote:
> Hi Yakui,
> Zhao Yakui wrote:
> > Hi, Alexey
> >     You have a lot of experiences about EC driver and a lot of EC patches are from you.
> >     You attach two patches related with EC driver in the bug 10724 & 11428.
> >     In the bug 10724:
> >         Add a boot ption of "ec_intr= " to select the EC work mode( Interrupt/Polling mode)
> >         After this patch is applied, EC won't switch to polling mode automatically from interrupt mode when EC irq storm is detected.
> >     In the bug 11428
> >         In this patch the EC won't switch off GPE mode on missing interrupts
> > 
> >     Will the above two patches hit the upstream kernel? If the above two patches hits the upstream kernel, it 
> > seems that the boot option of "ec_intr=" comes back again and EC can't be switched from interrupt mode to polling 
> > mode if EC GPE interrupt is missing. In such case maybe the battery/AC/thermal driver can't work well if the info of
> > battery/AC/thermal is related with EC. Maybe there exists the regression on some laptops.
> There are two steps here. First, you don't use GPE while waiting for status to become expected value (here you don't care if GPE is at all enabled) and second, there you can't handle GPE coming in very fast rate -- thus you need to disable it.
> 11428 restores first behavior, there we don't touch GPE if it does not hurt us, and provide a way to manually switch it off if it does. 
Maybe I don't understand what you said fully. The following is my
understanding about the patches of bug 11428 and bug 10724.

a. In the patch of bug 11428 the EC GPE won't be disabled when timeout
happens, which means that it is still possible to switch EC working mode
again from polling mode to interrupt mode. i.e. EC can still be switched
to interrupt mode again. In the current upstream kernel when timeout
happens, EC will be switched to polling mode and can't be switched to
interrupt mode again(EC GPE is disabled). Right?
    In fact when EC timeout happens in interrupt mode, it indicates that
EC controller can't return response in time. Maybe the exception happens
in EC controller. In such case maybe it is appropriate that EC works in
polling mode. After your patch is applied,if EC can't be switched to
interrupt mode again and polling timer is not started, the EC SCI_EVT
notification will be lost.
    
    But it is noted that we should avoid the bogus EC timeout while EC
is in interrupt mode. The following source code is to identify whether
EC timeout happens while EC is in interrupt mode.
    >if (wait_event_timeout(ec->wait, acpi_ec_check_status(ec, event),
    >                                 msecs_to_jiffies(ACPI_EC_DELAY))) 
    >                    return 0;
    >clear_bit(EC_FLAGS_WAIT_GPE, &ec->flags);

   In the above source code maybe there exists the following phenomenon:
     When the EC GPE interrupt is triggered, the waiting process will be
     waked up in the GPE interrupt service routine. But as the process
     can't be scheduled immediately, maybe the wait_event_timeout will
     also return 0, which means that timeout happens and EC will be
     switched from interrupt mode to polling mode.

   IMO if bogus EC timeout happens, EC should continue its original routine. I.E. EC should
work in EC interrupt mode and it is unnecessary to clear the EC_FLAGS_GPE_MODE of ec->flags.
   If the real EC timeout happens, it will be better that EC GPE is disabled and EC is switched
to polling mode. Of course it can also be OK that EC will try GPE mode after 5 seconds.

b. In the patch of bug 10724 when EC GPE storm is detected, only warning message is printed and 
EC won't be switched to polling mode. In such case the warning message suggests that 
user restart their system with the boot option of "ec_intr=0". If the system is not restarted, 
maybe the interrupt storm still exists and the system is still unstable.
   Maybe it is more appropriate that EC is switched to polling mode in such case.
   
    
After the boot option of "ec_intr" is imported, it is used to determine whether EC GPE is enable/disabled
in the boot phase. If EC GPE is disabled, the EC will be forced to work in polling mode. 
If EC GPE is enabled, maybe it can be switched from polling mode to interrupt mode.
Right?
If so, the patch of bug 10724 had better be split into two patches. One is to add the boot option.
The other is to disable the mode switch when EC GPE interrupt storm happens. 

> Once again, there is driver mode and there is GPE enabled/disabled. ec_intr controls the latter, and driver mode tries to follow automatically.
> >     At the same time the boot option of "ec_intr=1" indicates that EC will work in EC GPE interrupt mode. But maybe the following
> > phenomenon will appear on some laptops:
> >    EC is initialized as polling mode. After the EC GPE interrupt is triggered, it will be switched to GPE interrupt mode. But if
> > no EC GPE interrupt is triggered, it will still work in polling mode. Although it is harmless and EC can also work well, 
> > maybe the EC working mode is inconsistent with the EC boot option. At the same time after the system is resumed from S3, the EC working
> > mode will also be polling mode in the function of acpi_ec_resume.
> Why do you care about such consistency? Code is trying to use interrupt mode if it allowed to. If it fails, it reports this failure. 
> > 
> >     IMO the EC working mode is not very reasonable if the above two patches hit the upstream kernel.
> It is reasonable for me. If you read code more carefully, you might find it reasonable too.
> >     Are the following two methods reasonable? Which of them is better?
> No. Niether.
> > 
> >     a. Add the boot option of "ec_intr=" (ec_intr=auto, intr, polling). For most laptops the boot option of "ec_intr=auto" is the default option
> > and "ec_intr=auto" means that the EC working mode can be switched automatically between interrupt mode and polling mode.The purpose of "ec_intr=auto"
> > is used to avoid the regression on some laptops.(Maybe some laptops can't work well if the EC mode switch is disabled.) 
> >      If the boot option of "ec_intr=intr" is added, the EC will be forced to work in interrupt mode and can't be switched to polling mode even 
> > when the EC confirmation GPE interrupt is missing. 
> >      If the boot option of "ec_intr=polling" is added, the EC will be forced to work in polling mode.
> >      
> >      In such case the EC working mode is related with user input. 
> > 
> >     b. EC mode switch is disabled on some specific laptops. In such case the user doesn't care for the EC working mode. At the same time
> > EC is still started in polling mode. It will be switched to interrupt mode if the EC GPE interrupt is triggered. EC will be switched to polling
> > mode on most laptops in case of missing confirmation GPE interrupt. But on some specific laptops the EC mode switch is disabled, which means
> > that EC will still work in interrupt mode even when the EC confirmation GPE interrupt is missing. This can be realized by adding EC DMI check table.
> > 
> >     I am not sure whether my above suggestion is appropriate. 
> > 
> > Thanks for the comments.
> > 
> > Best regards.
> >    Yakui
> > 
> Regards,
> Alex.

Re: a problem about the two patches in bug 10724 & 11428

[Alexey Starikovskiy]

Zhao Yakui wrote:
> Maybe I don't understand what you said fully. The following is my
> understanding about the patches of bug 11428 and bug 10724.
> 
> a. In the patch of bug 11428 the EC GPE won't be disabled when timeout
> happens, which means that it is still possible to switch EC working mode
> again from polling mode to interrupt mode. i.e. EC can still be switched
> to interrupt mode again. In the current upstream kernel when timeout
> happens, EC will be switched to polling mode and can't be switched to
> interrupt mode again(EC GPE is disabled). Right?
Wrong. EC GPE will stay enabled, driver will just not use it during wait for completion.

>     In fact when EC timeout happens in interrupt mode, it indicates that
> EC controller can't return response in time.
Wrong. Some EC controllers are "optimized" to not send interrupts for each confirmation.
See history of EC patches for these optimization workarounds.

>    In the above source code maybe there exists the following phenomenon:
>      When the EC GPE interrupt is triggered, the waiting process will be
>      waked up in the GPE interrupt service routine. But as the process
>      can't be scheduled immediately, maybe the wait_event_timeout will
>      also return 0, which means that timeout happens and EC will be
>      switched from interrupt mode to polling mode.
We are speaking about 500msec timeout here. it needs at least 50+ ready to run tasks of same
kernel priority to not let queue thread run. Please tell me, how could this happen?

Re: a problem about the two patches in bug 10724 & 11428

[Zhao Yakui]

On Tue, 2008-09-02 at 12:36 +0400, Alexey Starikovskiy wrote:
> Zhao Yakui wrote:
> > Maybe I don't understand what you said fully. The following is my
> > understanding about the patches of bug 11428 and bug 10724.
> > 
> > a. In the patch of bug 11428 the EC GPE won't be disabled when timeout
> > happens, which means that it is still possible to switch EC working mode
> > again from polling mode to interrupt mode. i.e. EC can still be switched
> > to interrupt mode again. In the current upstream kernel when timeout
> > happens, EC will be switched to polling mode and can't be switched to
> > interrupt mode again(EC GPE is disabled). Right?
> Wrong. EC GPE will stay enabled, driver will just not use it during wait for completion.
In current upstream kernel when EC timeout happens, EC will be switched
to polling mode by calling the function of ec_switch_to_poll, in which
EC GPE is disabled. Right?
   >ec_switch_to_poll_mode(struct acpi_ec *ec)
{
        set_bit(EC_FLAGS_NO_GPE, &ec->flags);
        clear_bit(EC_FLAGS_GPE_MODE, &ec->flags);
        acpi_disable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
        set_bit(EC_FLAGS_RESCHEDULE_POLL, &ec->flags);
}

If the patch in bug 11428 hits the upstream kernel, EC will be switched
to polling mode when EC timeout happens. But EC GPE is still enabled and
it can be switched to EC GPE interrupt mode. 
   At the same time there is a little error in the patch of bug 10428.
   When EC timeout happens, the EC_FLAGS_NO_GPE bit of ec->flags is set
and the EC_FLAGS_GPE_MODE is clear. When EC GPE interrupt is triggered,
the EC_FLAGS_GPE_MODE bit can't be set again.
   
> 
> >     In fact when EC timeout happens in interrupt mode, it indicates that
> > EC controller can't return response in time.
> Wrong. Some EC controllers are "optimized" to not send interrupts for each confirmation.
> See history of EC patches for these optimization workarounds.
Maybe what you said is right. But in fact as is defined in ACPI spec, EC
controller should issue an interrupt according to the status of IBF and
OBF.  More detailed info about EC interrupt model can be found in the
section 12.6.2 of ACPI 3.0b spec.
If some EC controller are "optimized" to not send interrupts, is it
appropriate to reject such bugs?

In fact in the bug 11428 when EC real timeout happens in case of GPE
interrupt mode, the EC status is already what we expected. But no EC GPE
interrupt is triggered. 
More detailed info can be found in the log of bug 11428, comment
#16.    
> 
> >    In the above source code maybe there exists the following phenomenon:
> >      When the EC GPE interrupt is triggered, the waiting process will be
> >      waked up in the GPE interrupt service routine. But as the process
> >      can't be scheduled immediately, maybe the wait_event_timeout will
> >      also return 0, which means that timeout happens and EC will be
> >      switched from interrupt mode to polling mode.
> We are speaking about 500msec timeout here. it needs at least 50+ ready to run tasks of same
> kernel priority to not let queue thread run. Please tell me, how could this happen?
Please refer to the bug 11141.
In the bug 11141 the laptop works in polling mode. When timeout happens,
the EC status is already what OS expected. 
>[  762.152173] ACPI: EC: acpi_ec_wait timeout, status = 0x01, event =
"b0=1"
>[  762.152173] ACPI: EC: read timeout, command = 128
>[  766.444613] ACPI Exception (evregion-0419): AE_TIME, Returned by
Handler for [EmbeddedControl] [20080609]

In fact this is related with the mechanism of Linux schedule. When a
process is waked, it will be added to running queue but it can't be
scheduled immediately. Maybe before it has an opportunity to be
scheduled, the timeout already happens.
> 

Re: a problem about the two patches in bug 10724 & 11428

[Alan Jenkins]

Zhao Yakui wrote:
> On Tue, 2008-09-02 at 12:36 +0400, Alexey Starikovskiy wrote:   
>   
>>>     In fact when EC timeout happens in interrupt mode, it indicates that
>>> EC controller can't return response in time.
>>>       
>> Wrong. Some EC controllers are "optimized" to not send interrupts for each confirmation.
>> See history of EC patches for these optimization workarounds.
>>     
> Maybe what you said is right. But in fact as is defined in ACPI spec, EC
> controller should issue an interrupt according to the status of IBF and
> OBF.  More detailed info about EC interrupt model can be found in the
> section 12.6.2 of ACPI 3.0b spec.
> If some EC controller are "optimized" to not send interrupts, is it
> appropriate to reject such bugs?
>   
Not really.  If it works on "The Other OS", it is a bug that it doesn't
work on this one.  That's a specific policy of the linux ACPI
implementation.  Given both the complexity and importance of ACPI I
think it is the right policy.

Alan

Re: a problem about the two patches in bug 10724 & 11428

[Alexey Starikovskiy]

Zhao Yakui wrote:
> On Tue, 2008-09-02 at 12:36 +0400, Alexey Starikovskiy wrote:
>> Zhao Yakui wrote:
>>> Maybe I don't understand what you said fully. The following is my
>>> understanding about the patches of bug 11428 and bug 10724.
>>>
>>> a. In the patch of bug 11428 the EC GPE won't be disabled when timeout
>>> happens, which means that it is still possible to switch EC working mode
>>> again from polling mode to interrupt mode. i.e. EC can still be switched
>>> to interrupt mode again. In the current upstream kernel when timeout
>>> happens, EC will be switched to polling mode and can't be switched to
>>> interrupt mode again(EC GPE is disabled). Right?
>> Wrong. EC GPE will stay enabled, driver will just not use it during wait for completion.
> In current upstream kernel when EC timeout happens, EC will be switched
> to polling mode by calling the function of ec_switch_to_poll, in which
> EC GPE is disabled. Right?
>    >ec_switch_to_poll_mode(struct acpi_ec *ec)
> {
>         set_bit(EC_FLAGS_NO_GPE, &ec->flags);
>         clear_bit(EC_FLAGS_GPE_MODE, &ec->flags);
>         acpi_disable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
>         set_bit(EC_FLAGS_RESCHEDULE_POLL, &ec->flags);
> }
> 
> If the patch in bug 11428 hits the upstream kernel, EC will be switched
> to polling mode when EC timeout happens. But EC GPE is still enabled and
> it can be switched to EC GPE interrupt mode. 
>    At the same time there is a little error in the patch of bug 10428.
>    When EC timeout happens, the EC_FLAGS_NO_GPE bit of ec->flags is set
> and the EC_FLAGS_GPE_MODE is clear. When EC GPE interrupt is triggered,
> the EC_FLAGS_GPE_MODE bit can't be set again.
That is intended behavior. There are interrupts from EC, and if you don't want
to use them for confirmations, you need EC_FLAGS_NO_GPE.
>    
>>>     In fact when EC timeout happens in interrupt mode, it indicates that
>>> EC controller can't return response in time.
>> Wrong. Some EC controllers are "optimized" to not send interrupts for each confirmation.
>> See history of EC patches for these optimization workarounds.
> Maybe what you said is right. But in fact as is defined in ACPI spec, EC
> controller should issue an interrupt according to the status of IBF and
> OBF.  More detailed info about EC interrupt model can be found in the
> section 12.6.2 of ACPI 3.0b spec.
> If some EC controller are "optimized" to not send interrupts, is it
> appropriate to reject such bugs?
According to Len Brown, we should not reject such bugs.
We are not doing reference implementation, we are doing _working_ implementation.
> 
> In fact in the bug 11428 when EC real timeout happens in case of GPE
> interrupt mode, the EC status is already what we expected. But no EC GPE
> interrupt is triggered. 
> More detailed info can be found in the log of bug 11428, comment
> #16.    
>>>    In the above source code maybe there exists the following phenomenon:
>>>      When the EC GPE interrupt is triggered, the waiting process will be
>>>      waked up in the GPE interrupt service routine. But as the process
>>>      can't be scheduled immediately, maybe the wait_event_timeout will
>>>      also return 0, which means that timeout happens and EC will be
>>>      switched from interrupt mode to polling mode.
>> We are speaking about 500msec timeout here. it needs at least 50+ ready to run tasks of same
>> kernel priority to not let queue thread run. Please tell me, how could this happen?
> Please refer to the bug 11141.
> In the bug 11141 the laptop works in polling mode. When timeout happens,
> the EC status is already what OS expected. 
>> [  762.152173] ACPI: EC: acpi_ec_wait timeout, status = 0x01, event =
> "b0=1"
>> [  762.152173] ACPI: EC: read timeout, command = 128
>> [  766.444613] ACPI Exception (evregion-0419): AE_TIME, Returned by
> Handler for [EmbeddedControl] [20080609]
In poll mode works this piece of code. What is wrong?
	} else {
		unsigned long delay = jiffies + msecs_to_jiffies(ACPI_EC_DELAY);
		clear_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
		while (time_before(jiffies, delay)) {
			if (acpi_ec_check_status(ec, event))
				return 0;
			msleep(1);
		}
		if (acpi_ec_check_status(ec,event))
			return 0;
	}
> 
> In fact this is related with the mechanism of Linux schedule. When a
> process is waked, it will be added to running queue but it can't be
> scheduled immediately. Maybe before it has an opportunity to be
> scheduled, the timeout already happens.
How so? What could be scheduled before it for 50+ rescheduling events?

Re: a problem about the two patches in bug 10724 & 11428

[Zhao Yakui]

On Tue, 2008-09-02 at 13:30 +0400, Alexey Starikovskiy wrote:
> Zhao Yakui wrote:
> > On Tue, 2008-09-02 at 12:36 +0400, Alexey Starikovskiy wrote:
> >> Zhao Yakui wrote:
> >>> Maybe I don't understand what you said fully. The following is my
> >>> understanding about the patches of bug 11428 and bug 10724.
> >>>
> >>> a. In the patch of bug 11428 the EC GPE won't be disabled when timeout
> >>> happens, which means that it is still possible to switch EC working mode
> >>> again from polling mode to interrupt mode. i.e. EC can still be switched
> >>> to interrupt mode again. In the current upstream kernel when timeout
> >>> happens, EC will be switched to polling mode and can't be switched to
> >>> interrupt mode again(EC GPE is disabled). Right?
> >> Wrong. EC GPE will stay enabled, driver will just not use it during wait for completion.
> > In current upstream kernel when EC timeout happens, EC will be switched
> > to polling mode by calling the function of ec_switch_to_poll, in which
> > EC GPE is disabled. Right?
> >    >ec_switch_to_poll_mode(struct acpi_ec *ec)
> > {
> >         set_bit(EC_FLAGS_NO_GPE, &ec->flags);
> >         clear_bit(EC_FLAGS_GPE_MODE, &ec->flags);
> >         acpi_disable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
> >         set_bit(EC_FLAGS_RESCHEDULE_POLL, &ec->flags);
> > }
> > 
> > If the patch in bug 11428 hits the upstream kernel, EC will be switched
> > to polling mode when EC timeout happens. But EC GPE is still enabled and
> > it can be switched to EC GPE interrupt mode. 
> >    At the same time there is a little error in the patch of bug 10428.
> >    When EC timeout happens, the EC_FLAGS_NO_GPE bit of ec->flags is set
> > and the EC_FLAGS_GPE_MODE is clear. When EC GPE interrupt is triggered,
> > the EC_FLAGS_GPE_MODE bit can't be set again.
> That is intended behavior. There are interrupts from EC, and if you don't want
> to use them for confirmations, you need EC_FLAGS_NO_GPE.
Maybe I am confusing by the two flag of EC_FLAGS_NO_GPE &
EC_FLAGS_GPE_MODE. 
> >    
> >>>     In fact when EC timeout happens in interrupt mode, it indicates that
> >>> EC controller can't return response in time.
> >> Wrong. Some EC controllers are "optimized" to not send interrupts for each confirmation.
> >> See history of EC patches for these optimization workarounds.
> > Maybe what you said is right. But in fact as is defined in ACPI spec, EC
> > controller should issue an interrupt according to the status of IBF and
> > OBF.  More detailed info about EC interrupt model can be found in the
> > section 12.6.2 of ACPI 3.0b spec.
> > If some EC controller are "optimized" to not send interrupts, is it
> > appropriate to reject such bugs?
> According to Len Brown, we should not reject such bugs.
> We are not doing reference implementation, we are doing _working_ implementation.
Agree. I will try to investigate the bugs related with the workaround
patches.
> > 
> > In fact in the bug 11428 when EC real timeout happens in case of GPE
> > interrupt mode, the EC status is already what we expected. But no EC GPE
> > interrupt is triggered. 
> > More detailed info can be found in the log of bug 11428, comment
> > #16.    
> >>>    In the above source code maybe there exists the following phenomenon:
> >>>      When the EC GPE interrupt is triggered, the waiting process will be
> >>>      waked up in the GPE interrupt service routine. But as the process
> >>>      can't be scheduled immediately, maybe the wait_event_timeout will
> >>>      also return 0, which means that timeout happens and EC will be
> >>>      switched from interrupt mode to polling mode.
> >> We are speaking about 500msec timeout here. it needs at least 50+ ready to run tasks of same
> >> kernel priority to not let queue thread run. Please tell me, how could this happen?
> > Please refer to the bug 11141.
> > In the bug 11141 the laptop works in polling mode. When timeout happens,
> > the EC status is already what OS expected. 
> >> [  762.152173] ACPI: EC: acpi_ec_wait timeout, status = 0x01, event =
> > "b0=1"
> >> [  762.152173] ACPI: EC: read timeout, command = 128
> >> [  766.444613] ACPI Exception (evregion-0419): AE_TIME, Returned by
> > Handler for [EmbeddedControl] [20080609]
> In poll mode works this piece of code. What is wrong?
> 	} else {
> 		unsigned long delay = jiffies + msecs_to_jiffies(ACPI_EC_DELAY);
> 		clear_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
> 		while (time_before(jiffies, delay)) {
> 			if (acpi_ec_check_status(ec, event))
> 				return 0;
> 			msleep(1);
> 		}
   The following source code is added by my patch. Before it is added,
it is still regarded as timeout even when EC status is already what OS
expected.
	if (acpi_ec_check_status(ec,event))
            	return 0;
   So I send the workaround patch to avoid the bogus timeout in polling
mode.
> 	}
> > 
> > In fact this is related with the mechanism of Linux schedule. When a
> > process is waked, it will be added to running queue but it can't be
> > scheduled immediately. Maybe before it has an opportunity to be
> > scheduled, the timeout already happens.
> How so? What could be scheduled before it for 50+ rescheduling events?
> 

Re: a problem about the two patches in bug 10724 & 11428

[Henrique de Moraes Holschuh]

On Mon, 01 Sep 2008, Zhao Yakui wrote:
>     Will the above two patches hit the upstream kernel? If the above two patches hits the upstream kernel, it 
> seems that the boot option of "ec_intr=" comes back again and EC can't be switched from interrupt mode to polling 
> mode if EC GPE interrupt is missing. In such case maybe the battery/AC/thermal driver can't work well if the info of
> battery/AC/thermal is related with EC. Maybe there exists the regression on some laptops.

Right now, the fact that it gives up on interrupt mode too easily IS causing
regressions on ThinkPads (like the T43 I own).  Since polling mode does
work, it is just a performance regression, so you won't get many reports
about it since most people don't look for such stuff in their kernel logs.

Some ECs trigger the interrupt/poll-mode checks just on small windows
(typically during resume -- might even be a bug somewhere in ACPICA or
Linux, and not on the EC).  We should not be giving up using interrupt mode
on these so easily.  Maybe retry enabling interrupt mode after some seconds
a few times (like 3 or 5)?  If it is a transient problem, that will avoid
the permanent performance regression of polled mode.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: a problem about the two patches in bug 10724 & 11428

[Alexey Starikovskiy]

Henrique de Moraes Holschuh wrote:
> On Mon, 01 Sep 2008, Zhao Yakui wrote:
>>     Will the above two patches hit the upstream kernel? If the above two patches hits the upstream kernel, it 
>> seems that the boot option of "ec_intr=" comes back again and EC can't be switched from interrupt mode to polling 
>> mode if EC GPE interrupt is missing. In such case maybe the battery/AC/thermal driver can't work well if the info of
>> battery/AC/thermal is related with EC. Maybe there exists the regression on some laptops.
> 
> Right now, the fact that it gives up on interrupt mode too easily IS causing
> regressions on ThinkPads (like the T43 I own).  Since polling mode does
> work, it is just a performance regression, so you won't get many reports
> about it since most people don't look for such stuff in their kernel logs.
> 
> Some ECs trigger the interrupt/poll-mode checks just on small windows
> (typically during resume -- might even be a bug somewhere in ACPICA or
> Linux, and not on the EC).  We should not be giving up using interrupt mode
> on these so easily.  Maybe retry enabling interrupt mode after some seconds
> a few times (like 3 or 5)?  If it is a transient problem, that will avoid
> the permanent performance regression of polled mode.
Right, I am considering this option too.

Thanks,
Alex.

Re: a problem about the two patches in bug 10724 & 11428

[Alexey Starikovskiy]

[-- Attachment #1: Type: text/plain, Size: 1305 bytes --]

Henrique de Moraes Holschuh wrote:
> On Mon, 01 Sep 2008, Zhao Yakui wrote:
>>     Will the above two patches hit the upstream kernel? If the above two patches hits the upstream kernel, it 
>> seems that the boot option of "ec_intr=" comes back again and EC can't be switched from interrupt mode to polling 
>> mode if EC GPE interrupt is missing. In such case maybe the battery/AC/thermal driver can't work well if the info of
>> battery/AC/thermal is related with EC. Maybe there exists the regression on some laptops.
> 
> Right now, the fact that it gives up on interrupt mode too easily IS causing
> regressions on ThinkPads (like the T43 I own).  Since polling mode does
> work, it is just a performance regression, so you won't get many reports
> about it since most people don't look for such stuff in their kernel logs.
> 
> Some ECs trigger the interrupt/poll-mode checks just on small windows
> (typically during resume -- might even be a bug somewhere in ACPICA or
> Linux, and not on the EC).  We should not be giving up using interrupt mode
> on these so easily.  Maybe retry enabling interrupt mode after some seconds
> a few times (like 3 or 5)?  If it is a transient problem, that will avoid
> the permanent performance regression of polled mode.
> 
How about such patch?

Regards,
Alex.

[-- Attachment #2: never_give_up_gpe_mode.patch --]
[-- Type: text/x-diff, Size: 1780 bytes --]

ACPI: EC: retry gpe mode after 5 sec timeout

From: Alexey Starikovskiy <astarikovskiy@suse.de>

Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
---

 drivers/acpi/ec.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)


diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index f338d2b..15663c2 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -74,6 +74,7 @@ enum ec_event {
 #define ACPI_EC_DELAY		500	/* Wait 500ms max. during EC ops */
 #define ACPI_EC_UDELAY_GLK	1000	/* Wait 1ms max. to get global lock */
 #define ACPI_EC_UDELAY		100	/* Wait 100us before polling EC again */
+#define ACPI_EC_GPE_RETRY	5000	/* Wait 5s before trying to use GPE again */
 
 enum {
 	EC_FLAGS_WAIT_GPE = 0,		/* Don't check status until GPE arrives */
@@ -102,6 +103,7 @@ static struct acpi_ec {
 	unsigned long data_addr;
 	unsigned long global_lock;
 	unsigned long flags;
+	unsigned long gpe_retry;
 	struct mutex lock;
 	wait_queue_head_t wait;
 	struct list_head list;
@@ -205,6 +207,9 @@ static int acpi_ec_wait(struct acpi_ec *ec, enum ec_event event, int force_poll)
 						"switch off interrupt mode.\n");
 			set_bit(EC_FLAGS_NO_GPE, &ec->flags);
 			clear_bit(EC_FLAGS_GPE_MODE, &ec->flags);
+			 /* check again in 5 seconds */
+			ec->gpe_retry = jiffies +
+				msecs_to_jiffies(ACPI_EC_GPE_RETRY);
 			return 0;
 		}
 	} else {
@@ -530,7 +535,8 @@ static u32 acpi_ec_gpe_handler(void *data)
 				acpi_ec_gpe_query, ec);
 	} else if (!test_bit(EC_FLAGS_GPE_MODE, &ec->flags) &&
 		   !test_bit(EC_FLAGS_NO_GPE, &ec->flags) &&
-		   in_interrupt()) {
+		   in_interrupt() &&
+		   !time_before(jiffies, ec->gpe_retry)) {
 		/* this is non-query, must be confirmation */
 		if (printk_ratelimit())
 			pr_info(PREFIX "non-query interrupt received,"

Re: a problem about the two patches in bug 10724 & 11428

[Alexey Starikovskiy]

[-- Attachment #1: Type: text/plain, Size: 1433 bytes --]

Alexey Starikovskiy wrote:
> Henrique de Moraes Holschuh wrote:
>> On Mon, 01 Sep 2008, Zhao Yakui wrote:
>>>     Will the above two patches hit the upstream kernel? If the above 
>>> two patches hits the upstream kernel, it seems that the boot option 
>>> of "ec_intr=" comes back again and EC can't be switched from 
>>> interrupt mode to polling mode if EC GPE interrupt is missing. In 
>>> such case maybe the battery/AC/thermal driver can't work well if the 
>>> info of
>>> battery/AC/thermal is related with EC. Maybe there exists the 
>>> regression on some laptops.
>>
>> Right now, the fact that it gives up on interrupt mode too easily IS 
>> causing
>> regressions on ThinkPads (like the T43 I own).  Since polling mode does
>> work, it is just a performance regression, so you won't get many reports
>> about it since most people don't look for such stuff in their kernel 
>> logs.
>>
>> Some ECs trigger the interrupt/poll-mode checks just on small windows
>> (typically during resume -- might even be a bug somewhere in ACPICA or
>> Linux, and not on the EC).  We should not be giving up using interrupt 
>> mode
>> on these so easily.  Maybe retry enabling interrupt mode after some 
>> seconds
>> a few times (like 3 or 5)?  If it is a transient problem, that will avoid
>> the permanent performance regression of polled mode.
>>
> How about such patch?
Or even better (working?) patch ...
> 
> Regards,
> Alex.
> 


[-- Attachment #2: never_give_up_gpe_mode.patch --]
[-- Type: text/x-diff, Size: 1780 bytes --]

ACPI: EC: retry gpe mode after 5 sec timeout

From: Alexey Starikovskiy <astarikovskiy@suse.de>

Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
---

 drivers/acpi/ec.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)


diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index f338d2b..15663c2 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -74,6 +74,7 @@ enum ec_event {
 #define ACPI_EC_DELAY		500	/* Wait 500ms max. during EC ops */
 #define ACPI_EC_UDELAY_GLK	1000	/* Wait 1ms max. to get global lock */
 #define ACPI_EC_UDELAY		100	/* Wait 100us before polling EC again */
+#define ACPI_EC_GPE_RETRY	5000	/* Wait 5s before trying to use GPE again */
 
 enum {
 	EC_FLAGS_WAIT_GPE = 0,		/* Don't check status until GPE arrives */
@@ -102,6 +103,7 @@ static struct acpi_ec {
 	unsigned long data_addr;
 	unsigned long global_lock;
 	unsigned long flags;
+	unsigned long gpe_retry;
 	struct mutex lock;
 	wait_queue_head_t wait;
 	struct list_head list;
@@ -205,6 +207,9 @@ static int acpi_ec_wait(struct acpi_ec *ec, enum ec_event event, int force_poll)
 						"switch off interrupt mode.\n");
 			set_bit(EC_FLAGS_NO_GPE, &ec->flags);
 			clear_bit(EC_FLAGS_GPE_MODE, &ec->flags);
+			 /* check again in 5 seconds */
+			ec->gpe_retry = jiffies +
+				msecs_to_jiffies(ACPI_EC_GPE_RETRY);
 			return 0;
 		}
 	} else {
@@ -530,7 +535,8 @@ static u32 acpi_ec_gpe_handler(void *data)
 				acpi_ec_gpe_query, ec);
 	} else if (!test_bit(EC_FLAGS_GPE_MODE, &ec->flags) &&
 		   !test_bit(EC_FLAGS_NO_GPE, &ec->flags) &&
-		   in_interrupt()) {
+		   in_interrupt() &&
+		   !time_before(jiffies, ec->gpe_retry)) {
 		/* this is non-query, must be confirmation */
 		if (printk_ratelimit())
 			pr_info(PREFIX "non-query interrupt received,"

Re: a problem about the two patches in bug 10724 & 11428

[Zhao Yakui]

On Tue, 2008-09-02 at 00:59 +0400, Alexey Starikovskiy wrote:
> Alexey Starikovskiy wrote:
In this patch when timeout happens, EC will try the GPE interrupt mode
after 5 seconds.Of course EC will be in polling mode before switching to
GPE interrupt mode. It seems reasonable. 
In fact there also exists the mode switch from polling mode to interrupt
mode after EC is initialized. Is it more appropriate that ec->gpe_retry
is initialized?
Thanks.
> > Henrique de Moraes Holschuh wrote:
> >> On Mon, 01 Sep 2008, Zhao Yakui wrote:
> >>>     Will the above two patches hit the upstream kernel? If the above 
> >>> two patches hits the upstream kernel, it seems that the boot option 
> >>> of "ec_intr=" comes back again and EC can't be switched from 
> >>> interrupt mode to polling mode if EC GPE interrupt is missing. In 
> >>> such case maybe the battery/AC/thermal driver can't work well if the 
> >>> info of
> >>> battery/AC/thermal is related with EC. Maybe there exists the 
> >>> regression on some laptops.
> >>
> >> Right now, the fact that it gives up on interrupt mode too easily IS 
> >> causing
> >> regressions on ThinkPads (like the T43 I own).  Since polling mode does
> >> work, it is just a performance regression, so you won't get many reports
> >> about it since most people don't look for such stuff in their kernel 
> >> logs.
> >>
> >> Some ECs trigger the interrupt/poll-mode checks just on small windows
> >> (typically during resume -- might even be a bug somewhere in ACPICA or
> >> Linux, and not on the EC).  We should not be giving up using interrupt 
> >> mode
> >> on these so easily.  Maybe retry enabling interrupt mode after some 
> >> seconds
> >> a few times (like 3 or 5)?  If it is a transient problem, that will avoid
> >> the permanent performance regression of polled mode.
> >>
> > How about such patch?
> Or even better (working?) patch ...
> > 
> > Regards,
> > Alex.
> > 
> 

Re: a problem about the two patches in bug 10724 & 11428

[Henrique de Moraes Holschuh]

On Tue, 02 Sep 2008, Zhao Yakui wrote:
> On Tue, 2008-09-02 at 00:59 +0400, Alexey Starikovskiy wrote:
> > Alexey Starikovskiy wrote:
> In this patch when timeout happens, EC will try the GPE interrupt mode
> after 5 seconds.Of course EC will be in polling mode before switching to
> GPE interrupt mode. It seems reasonable. 

Yeah, the patch looks good.

But we probably want to limit the number of times it will try to re-enable
interrupt mode, otherwise it will keep firing on ECs that are permanently
broken re. GPE interrupt mode.  I think trying 5 times should be enough,
that gives a 25s window for the EC to get its act together.

And we should probably reset the "attempted tries to switch to GPE interrupt
mode" counter (so that the code will retry interrupt mode again) after
events that are likely to have done a lot of internal churning to the EC.
Currently, those would be when resuming from S3 or S4, I think.

> In fact there also exists the mode switch from polling mode to interrupt
> mode after EC is initialized. Is it more appropriate that ec->gpe_retry
> is initialized?

We could use the same code to do the initial switch, indeed.

The two other things that we probably want to make sure we are doing are:

1. Drain the entire EC queue at every poll cycle (this is a must, really).

2. Try to drain the entire EC queue also in interrupt mode, but be careful
about dumb ECs that will sign 10 interrupts if there are 10 bytes to read in
the queue, even when we have drained the queue when servicing the very first
interrupt.   I bet there are ECs which will give you just one interrupt and
expect the queue to be drained, ECs which will give you interrupts until you
drain the queue (but no more than necessary), and ECs which will give you
interrupts for as many bytes as it stored in the queue, regardless of when
they were read... and we mustn't think there is an interrupt storm happening
because of this.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: a problem about the two patches in bug 10724 & 11428

[Zhao Yakui]

On Mon, 2008-09-01 at 23:03 -0300, Henrique de Moraes Holschuh wrote:
> On Tue, 02 Sep 2008, Zhao Yakui wrote:
> > On Tue, 2008-09-02 at 00:59 +0400, Alexey Starikovskiy wrote:
> > > Alexey Starikovskiy wrote:
> > In this patch when timeout happens, EC will try the GPE interrupt mode
> > after 5 seconds.Of course EC will be in polling mode before switching to
> > GPE interrupt mode. It seems reasonable. 
> 
> Yeah, the patch looks good.
> 
> But we probably want to limit the number of times it will try to re-enable
> interrupt mode, otherwise it will keep firing on ECs that are permanently
> broken re. GPE interrupt mode.  I think trying 5 times should be enough,
> that gives a 25s window for the EC to get its act together.
> 
> And we should probably reset the "attempted tries to switch to GPE interrupt
> mode" counter (so that the code will retry interrupt mode again) after
> events that are likely to have done a lot of internal churning to the EC.
> Currently, those would be when resuming from S3 or S4, I think.
> 
> > In fact there also exists the mode switch from polling mode to interrupt
> > mode after EC is initialized. Is it more appropriate that ec->gpe_retry
> > is initialized?
> 
> We could use the same code to do the initial switch, indeed.
> 
> The two other things that we probably want to make sure we are doing are:
> 
> 1. Drain the entire EC queue at every poll cycle (this is a must, really).
It is difficult to drain the entire EC queue. In fact OS can't know the
length of EC entire queue. In fact there is no concept about the EC
queue.
   
> 2. Try to drain the entire EC queue also in interrupt mode, but be careful
> about dumb ECs that will sign 10 interrupts if there are 10 bytes to read in
> the queue, even when we have drained the queue when servicing the very first
> interrupt.   I bet there are ECs which will give you just one interrupt and
> expect the queue to be drained, ECs which will give you interrupts until you
> drain the queue (but no more than necessary), and ECs which will give you
> interrupts for as many bytes as it stored in the queue, regardless of when
> they were read... and we mustn't think there is an interrupt storm happening
> because of this.
Maybe what you said is right. There are too many kinds of EC controllers
available. And they come from the different OEMs. Maybe when one EC
internal event is detected, the EC GPE interrupts will always be
triggered before the notification event is processed on some ECs. But
who knows.
> 

Re: a problem about the two patches in bug 10724 & 11428

[Alan Jenkins]

Zhao Yakui wrote:
> On Mon, 2008-09-01 at 23:03 -0300, Henrique de Moraes Holschuh wrote:
>   
>> On Tue, 02 Sep 2008, Zhao Yakui wrote:
>>     
>>> On Tue, 2008-09-02 at 00:59 +0400, Alexey Starikovskiy wrote:
>>>       
>>>> Alexey Starikovskiy wrote:
>>>>         
>>> In this patch when timeout happens, EC will try the GPE interrupt mode
>>> after 5 seconds.Of course EC will be in polling mode before switching to
>>> GPE interrupt mode. It seems reasonable. 
>>>       
>> Yeah, the patch looks good.
>>
>> But we probably want to limit the number of times it will try to re-enable
>> interrupt mode, otherwise it will keep firing on ECs that are permanently
>> broken re. GPE interrupt mode.  I think trying 5 times should be enough,
>> that gives a 25s window for the EC to get its act together.
>>
>> And we should probably reset the "attempted tries to switch to GPE interrupt
>> mode" counter (so that the code will retry interrupt mode again) after
>> events that are likely to have done a lot of internal churning to the EC.
>> Currently, those would be when resuming from S3 or S4, I think.
>>
>>     
>>> In fact there also exists the mode switch from polling mode to interrupt
>>> mode after EC is initialized. Is it more appropriate that ec->gpe_retry
>>> is initialized?
>>>       
>> We could use the same code to do the initial switch, indeed.
>>
>> The two other things that we probably want to make sure we are doing are:
>>
>> 1. Drain the entire EC queue at every poll cycle (this is a must, really).
>>     
> It is difficult to drain the entire EC queue.

Why?  What was wrong with my proposed patch to do this :-P?

At least in GPE polling mode, we ought to query in a loop until the
query returns 0 (no event).

There was a problem which might have been caused by my patch, but that
was probably an excessive GPE problem.  That can't happen if we've
disabled the GPE altogether so we have to poll them.  I think Alex is
right that this is a must.

Though I am making an assumption here.  The other possibility is a buggy
EC that returns non-zero when there are no more events.  I hope not
though :-(.

>  In fact OS can't know the
> length of EC entire queue. In fact there is no concept about the EC
> queue.
>   

There is certainly the implication that more than one event could be
pending.  And there is the concept "queue is empty now", indicated by
query returning zero.

>   
>> 2. Try to drain the entire EC queue also in interrupt mode, but be careful
>> about dumb ECs that will sign 10 interrupts if there are 10 bytes to read in
>> the queue, even when we have drained the queue when servicing the very first
>> interrupt.   I bet there are ECs which will give you just one interrupt and
>> expect the queue to be drained, ECs which will give you interrupts until you
>> drain the queue (but no more than necessary), and ECs which will give you
>> interrupts for as many bytes as it stored in the queue, regardless of when
>> they were read... and we mustn't think there is an interrupt storm happening
>> because of this.
>>     
> Maybe what you said is right. There are too many kinds of EC controllers
> available. And they come from the different OEMs. Maybe when one EC
> internal event is detected, the EC GPE interrupts will always be
> triggered before the notification event is processed on some ECs. But
> who knows.
>   

Re: a problem about the two patches in bug 10724 & 11428

[Zhao Yakui]

On Tue, 2008-09-02 at 00:59 +0400, Alexey Starikovskiy wrote:
> Alexey Starikovskiy wrote:
> > Henrique de Moraes Holschuh wrote:
> >> On Mon, 01 Sep 2008, Zhao Yakui wrote:
> >>>     Will the above two patches hit the upstream kernel? If the above 
> >>> two patches hits the upstream kernel, it seems that the boot option 
> >>> of "ec_intr=" comes back again and EC can't be switched from 
> >>> interrupt mode to polling mode if EC GPE interrupt is missing. In 
> >>> such case maybe the battery/AC/thermal driver can't work well if the 
> >>> info of
> >>> battery/AC/thermal is related with EC. Maybe there exists the 
> >>> regression on some laptops.
> >>
> >> Right now, the fact that it gives up on interrupt mode too easily IS 
> >> causing
> >> regressions on ThinkPads (like the T43 I own).  Since polling mode does
> >> work, it is just a performance regression, so you won't get many reports
> >> about it since most people don't look for such stuff in their kernel 
> >> logs.
> >>
> >> Some ECs trigger the interrupt/poll-mode checks just on small windows
> >> (typically during resume -- might even be a bug somewhere in ACPICA or
> >> Linux, and not on the EC).  We should not be giving up using interrupt 
> >> mode
> >> on these so easily.  Maybe retry enabling interrupt mode after some 
> >> seconds
> >> a few times (like 3 or 5)?  If it is a transient problem, that will avoid
> >> the permanent performance regression of polled mode.
> >>
> > How about such patch?
> Or even better (working?) patch ...

The patch looks reasonable. But there is a little problem.
The EC_FLAGS_NO_GPE bit of ec->flags is set when timeout happens.(The EC_FLAGS_GPE_MODE bit is clear)
Maybe in such case the EC_FLAGS_GPE_MODE bit can't be set again when EC GPE interrupt is triggered again.

Thanks.
   Yakui

  
> > Regards,
> > Alex.
> > 
> 

Re: a problem about the two patches in bug 10724 & 11428

[Zhao Yakui]

On Tue, 2008-09-02 at 00:59 +0400, Alexey Starikovskiy wrote:
> Alexey Starikovskiy wrote:
Hi, Alexey
    In this patch after the EC timeout happens, the EC_FLAGS_GPE_MODE of
ec->flags will be clear and EC_FLAGS_NO_GPE bit will be set. But the EC
GPE won't be disabled again. Right?
    In such case when  EC is accessed, EC will work in polling mode. At
the same time EC interrupt still can be triggered. But the
EC_FLAGS_GPE_MODE can't be set again. Right?

Thanks
    Yakui.
              
> > Henrique de Moraes Holschuh wrote:
> >> On Mon, 01 Sep 2008, Zhao Yakui wrote:
> >>>     Will the above two patches hit the upstream kernel? If the above 
> >>> two patches hits the upstream kernel, it seems that the boot option 
> >>> of "ec_intr=" comes back again and EC can't be switched from 
> >>> interrupt mode to polling mode if EC GPE interrupt is missing. In 
> >>> such case maybe the battery/AC/thermal driver can't work well if the 
> >>> info of
> >>> battery/AC/thermal is related with EC. Maybe there exists the 
> >>> regression on some laptops.
> >>
> >> Right now, the fact that it gives up on interrupt mode too easily IS 
> >> causing
> >> regressions on ThinkPads (like the T43 I own).  Since polling mode does
> >> work, it is just a performance regression, so you won't get many reports
> >> about it since most people don't look for such stuff in their kernel 
> >> logs.
> >>
> >> Some ECs trigger the interrupt/poll-mode checks just on small windows
> >> (typically during resume -- might even be a bug somewhere in ACPICA or
> >> Linux, and not on the EC).  We should not be giving up using interrupt 
> >> mode
> >> on these so easily.  Maybe retry enabling interrupt mode after some 
> >> seconds
> >> a few times (like 3 or 5)?  If it is a transient problem, that will avoid
> >> the permanent performance regression of polled mode.
> >>
> > How about such patch?
> Or even better (working?) patch ...
> > 
> > Regards,
> > Alex.
> > 
> 

Re: a problem about the two patches in bug 10724 & 11428

[Alexey Starikovskiy]

Zhao Yakui wrote:
> On Tue, 2008-09-02 at 00:59 +0400, Alexey Starikovskiy wrote:
>> Alexey Starikovskiy wrote:
> Hi, Alexey
>     In this patch after the EC timeout happens, the EC_FLAGS_GPE_MODE of
> ec->flags will be clear and EC_FLAGS_NO_GPE bit will be set. But the EC
> GPE won't be disabled again. Right?
Right
>     In such case when  EC is accessed, EC will work in polling mode. At
> the same time EC interrupt still can be triggered. But the
> EC_FLAGS_GPE_MODE can't be set again. Right?
Almost. EC by itself (hardware device) still will be working in its 
"optimized gpe mode" as before, but EC driver (software) will be working in poll mode.
Regards,
Alex.
> 
> Thanks
>     Yakui.
>               
>>> Henrique de Moraes Holschuh wrote:
>>>> On Mon, 01 Sep 2008, Zhao Yakui wrote:
>>>>>     Will the above two patches hit the upstream kernel? If the above 
>>>>> two patches hits the upstream kernel, it seems that the boot option 
>>>>> of "ec_intr=" comes back again and EC can't be switched from 
>>>>> interrupt mode to polling mode if EC GPE interrupt is missing. In 
>>>>> such case maybe the battery/AC/thermal driver can't work well if the 
>>>>> info of
>>>>> battery/AC/thermal is related with EC. Maybe there exists the 
>>>>> regression on some laptops.
>>>> Right now, the fact that it gives up on interrupt mode too easily IS 
>>>> causing
>>>> regressions on ThinkPads (like the T43 I own).  Since polling mode does
>>>> work, it is just a performance regression, so you won't get many reports
>>>> about it since most people don't look for such stuff in their kernel 
>>>> logs.
>>>>
>>>> Some ECs trigger the interrupt/poll-mode checks just on small windows
>>>> (typically during resume -- might even be a bug somewhere in ACPICA or
>>>> Linux, and not on the EC).  We should not be giving up using interrupt 
>>>> mode
>>>> on these so easily.  Maybe retry enabling interrupt mode after some 
>>>> seconds
>>>> a few times (like 3 or 5)?  If it is a transient problem, that will avoid
>>>> the permanent performance regression of polled mode.
>>>>
>>> How about such patch?
>> Or even better (working?) patch ...
>>> Regards,
>>> Alex.
>>>
> 

Re: a problem about the two patches in bug 10724 & 11428

[Zhao Yakui]

On Wed, 2008-09-03 at 10:46 +0400, Alexey Starikovskiy wrote:
> Zhao Yakui wrote:
> > On Tue, 2008-09-02 at 00:59 +0400, Alexey Starikovskiy wrote:
> >> Alexey Starikovskiy wrote:
> > Hi, Alexey
> >     In this patch after the EC timeout happens, the EC_FLAGS_GPE_MODE of
> > ec->flags will be clear and EC_FLAGS_NO_GPE bit will be set. But the EC
> > GPE won't be disabled again. Right?
> Right
> >     In such case when  EC is accessed, EC will work in polling mode. At
> > the same time EC interrupt still can be triggered. But the
> > EC_FLAGS_GPE_MODE can't be set again. Right?
> Almost. EC by itself (hardware device) still will be working in its 
> "optimized gpe mode" as before, but EC driver (software) will be working in poll mode.
> Regards,
Thanks for the quick response.
Is it appropriate that EC always works in such mode? The mode switch
seems so complex.
When EC is accessed, EC will work in polling mode.
But EC GPE will be triggered. Maybe what we cared is EC notification
event(EC_SCI bit of EC status register).

thanks.
> Alex.
> > 
> > Thanks
> >     Yakui.
> >               
> >>> Henrique de Moraes Holschuh wrote:
> >>>> On Mon, 01 Sep 2008, Zhao Yakui wrote:
> >>>>>     Will the above two patches hit the upstream kernel? If the above 
> >>>>> two patches hits the upstream kernel, it seems that the boot option 
> >>>>> of "ec_intr=" comes back again and EC can't be switched from 
> >>>>> interrupt mode to polling mode if EC GPE interrupt is missing. In 
> >>>>> such case maybe the battery/AC/thermal driver can't work well if the 
> >>>>> info of
> >>>>> battery/AC/thermal is related with EC. Maybe there exists the 
> >>>>> regression on some laptops.
> >>>> Right now, the fact that it gives up on interrupt mode too easily IS 
> >>>> causing
> >>>> regressions on ThinkPads (like the T43 I own).  Since polling mode does
> >>>> work, it is just a performance regression, so you won't get many reports
> >>>> about it since most people don't look for such stuff in their kernel 
> >>>> logs.
> >>>>
> >>>> Some ECs trigger the interrupt/poll-mode checks just on small windows
> >>>> (typically during resume -- might even be a bug somewhere in ACPICA or
> >>>> Linux, and not on the EC).  We should not be giving up using interrupt 
> >>>> mode
> >>>> on these so easily.  Maybe retry enabling interrupt mode after some 
> >>>> seconds
> >>>> a few times (like 3 or 5)?  If it is a transient problem, that will avoid
> >>>> the permanent performance regression of polled mode.
> >>>>
> >>> How about such patch?
> >> Or even better (working?) patch ...
> >>> Regards,
> >>> Alex.
> >>>
> > 
> 

Re: a problem about the two patches in bug 10724 & 11428

[Zhao Yakui]

On Wed, 2008-09-03 at 10:46 +0400, Alexey Starikovskiy wrote:
> Zhao Yakui wrote:
> > On Tue, 2008-09-02 at 00:59 +0400, Alexey Starikovskiy wrote:
> >> Alexey Starikovskiy wrote:
> > Hi, Alexey
> >     In this patch after the EC timeout happens, the EC_FLAGS_GPE_MODE of
> > ec->flags will be clear and EC_FLAGS_NO_GPE bit will be set. But the EC
> > GPE won't be disabled again. Right?
> Right
> >     In such case when  EC is accessed, EC will work in polling mode. At
> > the same time EC interrupt still can be triggered. But the
> > EC_FLAGS_GPE_MODE can't be set again. Right?
> Almost. EC by itself (hardware device) still will be working in its 
> "optimized gpe mode" as before, but EC driver (software) will be working in poll mode.
> Regards,
If EC always works in such mode, it seems that we can make EC work more
simply. 
If so, a lot of source code in EC will be useless and we can do a
cleanup. After this, the EC driver will be easier to understand. When
new problems appears, we can easily identify the root cause.

Thanks.
   Yakui



> Alex.
> > 
> > Thanks
> >     Yakui.
> >               
> >>> Henrique de Moraes Holschuh wrote:
> >>>> On Mon, 01 Sep 2008, Zhao Yakui wrote:
> >>>>>     Will the above two patches hit the upstream kernel? If the above 
> >>>>> two patches hits the upstream kernel, it seems that the boot option 
> >>>>> of "ec_intr=" comes back again and EC can't be switched from 
> >>>>> interrupt mode to polling mode if EC GPE interrupt is missing. In 
> >>>>> such case maybe the battery/AC/thermal driver can't work well if the 
> >>>>> info of
> >>>>> battery/AC/thermal is related with EC. Maybe there exists the 
> >>>>> regression on some laptops.
> >>>> Right now, the fact that it gives up on interrupt mode too easily IS 
> >>>> causing
> >>>> regressions on ThinkPads (like the T43 I own).  Since polling mode does
> >>>> work, it is just a performance regression, so you won't get many reports
> >>>> about it since most people don't look for such stuff in their kernel 
> >>>> logs.
> >>>>
> >>>> Some ECs trigger the interrupt/poll-mode checks just on small windows
> >>>> (typically during resume -- might even be a bug somewhere in ACPICA or
> >>>> Linux, and not on the EC).  We should not be giving up using interrupt 
> >>>> mode
> >>>> on these so easily.  Maybe retry enabling interrupt mode after some 
> >>>> seconds
> >>>> a few times (like 3 or 5)?  If it is a transient problem, that will avoid
> >>>> the permanent performance regression of polled mode.
> >>>>
> >>> How about such patch?
> >> Or even better (working?) patch ...
> >>> Regards,
> >>> Alex.
> >>>
> > 
> 

Re: a problem about the two patches in bug 10724 & 11428

[Alexey Starikovskiy]

Zhao Yakui wrote:
> On Wed, 2008-09-03 at 10:46 +0400, Alexey Starikovskiy wrote:
>> Zhao Yakui wrote:
>>> On Tue, 2008-09-02 at 00:59 +0400, Alexey Starikovskiy wrote:
>>>> Alexey Starikovskiy wrote:
>>> Hi, Alexey
>>>     In this patch after the EC timeout happens, the EC_FLAGS_GPE_MODE of
>>> ec->flags will be clear and EC_FLAGS_NO_GPE bit will be set. But the EC
>>> GPE won't be disabled again. Right?
>> Right
>>>     In such case when  EC is accessed, EC will work in polling mode. At
>>> the same time EC interrupt still can be triggered. But the
>>> EC_FLAGS_GPE_MODE can't be set again. Right?
>> Almost. EC by itself (hardware device) still will be working in its 
>> "optimized gpe mode" as before, but EC driver (software) will be working in poll mode.
>> Regards,
> If EC always works in such mode, it seems that we can make EC work more
> simply. 
If you _disable_ EC GPE or not enable it in first place, EC(hardware) will be working in
poll mode. So, there is no spare code to clean up, sorry. 
> If so, a lot of source code in EC will be useless and we can do a
> cleanup. After this, the EC driver will be easier to understand. When
> new problems appears, we can easily identify the root cause.
> 
> Thanks.
>    Yakui

Re: a problem about the two patches in bug 10724 & 11428

[Zhao Yakui]

On Wed, 2008-09-03 at 11:53 +0400, Alexey Starikovskiy wrote:
> Zhao Yakui wrote:
> > On Wed, 2008-09-03 at 10:46 +0400, Alexey Starikovskiy wrote:
> >> Zhao Yakui wrote:
> >>> On Tue, 2008-09-02 at 00:59 +0400, Alexey Starikovskiy wrote:
> >>>> Alexey Starikovskiy wrote:
> >>> Hi, Alexey
> >>>     In this patch after the EC timeout happens, the EC_FLAGS_GPE_MODE of
> >>> ec->flags will be clear and EC_FLAGS_NO_GPE bit will be set. But the EC
> >>> GPE won't be disabled again. Right?
> >> Right
> >>>     In such case when  EC is accessed, EC will work in polling mode. At
> >>> the same time EC interrupt still can be triggered. But the
> >>> EC_FLAGS_GPE_MODE can't be set again. Right?
> >> Almost. EC by itself (hardware device) still will be working in its 
> >> "optimized gpe mode" as before, but EC driver (software) will be working in poll mode.
> >> Regards,
> > If EC always works in such mode, it seems that we can make EC work more
> > simply. 
> If you _disable_ EC GPE or not enable it in first place, EC(hardware) will be working in
> poll mode. So, there is no spare code to clean up, sorry. 
Not understand what you said.
After your three patches hit the upstream kernel, the EC GPE is still
enabled when EC timeout happens. In such case when EC is accessed, EC
will work in polling mode. At the same time EC GPE interrupt still can
be triggered. 
If so, maybe it is also reasonable that EC always works in such mode
after the ACPI EC driver is loaded fully.(ACPI EC device is bound with
the ACPI EC driver). 
    a. EC GPE interrupt can be triggered 
    b. When EC internal register is accessed, EC driver works in polling
mode.

If EC always works in above mode, the working flowchart in EC driver can
be simplified.
   a. The mode switch from polling mode to interrupt mode is useless
   b. Some flags are also useless. (For example:
WAIT_GPE/RESCHEDULE_POLL/FLAGS_NO_GPE)
   c. polling timer is useless
   d. In EC GPE interrupt handler we can only care the EC notification
event.(The SCI_EVT bit of EC status register).

   



> > If so, a lot of source code in EC will be useless and we can do a
> > cleanup. After this, the EC driver will be easier to understand. When
> > new problems appears, we can easily identify the root cause.
> > 
> > Thanks.
> >    Yakui

RFC: fast transactions in EC [was: a problem about the two patches in bug 10724 & 11428]

[Alexey Starikovskiy]

[-- Attachment #1: Type: text/plain, Size: 7108 bytes --]

Hi,
Here is the patch, which moves almost all transaction functionality into interrupt handler, which is IMHO good.

with the enabled DEBUG, the interrupt picture looks like this (single transaction):
[  228.823886] ACPI: EC: ---> status = 0x08 -- this is a check for IBF=0 before starting transaction
[  228.823888] ACPI: EC: transaction start
[  228.823890] ACPI: EC: <--- command = 0x80 -- here we write command
[  228.824123] ACPI: EC: ~~~> interrupt -- this interrupt is generated with the same content in status register as before writing a command...
[  228.824127] ACPI: EC: ---> status = 0x08
[  228.824129] ACPI: EC: <--- data = 0x58
[  228.824152] ACPI: EC: ~~~> interrupt -- and now EC start sending us interrupts like crazy...
[  228.824155] ACPI: EC: ---> status = 0x02
[  228.824169] ACPI: EC: ~~~> interrupt
[  228.824172] ACPI: EC: ---> status = 0x02
[  228.824186] ACPI: EC: ~~~> interrupt
[  228.824189] ACPI: EC: ---> status = 0x02
[  228.824203] ACPI: EC: ~~~> interrupt
[  228.824206] ACPI: EC: ---> status = 0x02
[  228.824219] ACPI: EC: ~~~> interrupt
[  228.824222] ACPI: EC: ---> status = 0x02
[  228.824236] ACPI: EC: ~~~> interrupt
[  228.824239] ACPI: EC: ---> status = 0x02
[  228.824253] ACPI: EC: ~~~> interrupt
[  228.824256] ACPI: EC: ---> status = 0x02
[  228.824270] ACPI: EC: ~~~> interrupt
[  228.824273] ACPI: EC: ---> status = 0x02
[  228.824287] ACPI: EC: ~~~> interrupt
[  228.824290] ACPI: EC: ---> status = 0x02
[  228.824304] ACPI: EC: ~~~> interrupt
[  228.824307] ACPI: EC: ---> status = 0x02
[  228.824321] ACPI: EC: ~~~> interrupt
[  228.824324] ACPI: EC: ---> status = 0x02
[  228.824337] ACPI: EC: ~~~> interrupt
[  228.824340] ACPI: EC: ---> status = 0x02
[  228.824354] ACPI: EC: ~~~> interrupt
[  228.824357] ACPI: EC: ---> status = 0x02
[  228.824371] ACPI: EC: ~~~> interrupt
[  228.824374] ACPI: EC: ---> status = 0x02
[  228.824388] ACPI: EC: ~~~> interrupt
[  228.824391] ACPI: EC: ---> status = 0x02
[  228.824405] ACPI: EC: ~~~> interrupt
[  228.824408] ACPI: EC: ---> status = 0x02
[  228.824422] ACPI: EC: ~~~> interrupt
[  228.824425] ACPI: EC: ---> status = 0x02
[  228.824439] ACPI: EC: ~~~> interrupt
[  228.824442] ACPI: EC: ---> status = 0x02
[  228.824455] ACPI: EC: ~~~> interrupt
[  228.824458] ACPI: EC: ---> status = 0x02
[  228.824472] ACPI: EC: ~~~> interrupt
[  228.824475] ACPI: EC: ---> status = 0x02
[  228.824489] ACPI: EC: ~~~> interrupt
[  228.824492] ACPI: EC: ---> status = 0x02
[  228.824506] ACPI: EC: ~~~> interrupt
[  228.824509] ACPI: EC: ---> status = 0x02
[  228.824523] ACPI: EC: ~~~> interrupt
[  228.824526] ACPI: EC: ---> status = 0x02
[  228.824540] ACPI: EC: ~~~> interrupt
[  228.824543] ACPI: EC: ---> status = 0x02
[  228.824557] ACPI: EC: ~~~> interrupt
[  228.824560] ACPI: EC: ---> status = 0x02
[  228.824573] ACPI: EC: ~~~> interrupt
[  228.824576] ACPI: EC: ---> status = 0x02
[  228.824590] ACPI: EC: ~~~> interrupt
[  228.824593] ACPI: EC: ---> status = 0x02
[  228.824607] ACPI: EC: ~~~> interrupt
[  228.824610] ACPI: EC: ---> status = 0x02
[  228.824624] ACPI: EC: ~~~> interrupt
[  228.824627] ACPI: EC: ---> status = 0x02
[  228.824641] ACPI: EC: ~~~> interrupt
[  228.824644] ACPI: EC: ---> status = 0x02
[  228.824658] ACPI: EC: ~~~> interrupt
[  228.824661] ACPI: EC: ---> status = 0x02
[  228.824675] ACPI: EC: ~~~> interrupt
[  228.824678] ACPI: EC: ---> status = 0x02
[  228.824691] ACPI: EC: ~~~> interrupt
[  228.824694] ACPI: EC: ---> status = 0x02
[  228.824708] ACPI: EC: ~~~> interrupt
[  228.824711] ACPI: EC: ---> status = 0x02
[  228.824785] ACPI: EC: ~~~> interrupt    --- one more "real" interrupt. Others are just garbage.
[  228.824788] ACPI: EC: ---> status = 0x01
[  228.824791] ACPI: EC: ---> data = 0x35
[  228.824805] ACPI: EC: ~~~> interrupt
[  228.824808] ACPI: EC: ---> status = 0x00
[  228.824822] ACPI: EC: ~~~> interrupt
[  228.824825] ACPI: EC: ---> status = 0x00
[  228.824839] ACPI: EC: ~~~> interrupt
[  228.824842] ACPI: EC: ---> status = 0x00
[  228.824856] ACPI: EC: ~~~> interrupt
[  228.824859] ACPI: EC: ---> status = 0x00
[  228.824873] ACPI: EC: ~~~> interrupt
[  228.824876] ACPI: EC: ---> status = 0x00
[  228.824890] ACPI: EC: ~~~> interrupt
[  228.824893] ACPI: EC: ---> status = 0x00
[  228.824907] ACPI: EC: ~~~> interrupt
[  228.824910] ACPI: EC: ---> status = 0x00
[  228.824924] ACPI: EC: ~~~> interrupt
[  228.824926] ACPI: EC: ---> status = 0x00
[  228.824940] ACPI: EC: ~~~> interrupt
[  228.824943] ACPI: EC: ---> status = 0x00
[  228.824957] ACPI: EC: ~~~> interrupt
[  228.824960] ACPI: EC: ---> status = 0x00
[  228.824974] ACPI: EC: ~~~> interrupt
[  228.824977] ACPI: EC: ---> status = 0x00
[  228.824991] ACPI: EC: ~~~> interrupt
[  228.824994] ACPI: EC: ---> status = 0x00
[  228.825008] ACPI: EC: ~~~> interrupt
[  228.825011] ACPI: EC: ---> status = 0x00
[  228.825025] ACPI: EC: ~~~> interrupt
[  228.825028] ACPI: EC: ---> status = 0x00
[  228.825042] ACPI: EC: ~~~> interrupt
[  228.825044] ACPI: EC: ---> status = 0x00
[  228.825058] ACPI: EC: ~~~> interrupt
[  228.825061] ACPI: EC: ---> status = 0x00
[  228.825075] ACPI: EC: ~~~> interrupt
[  228.825078] ACPI: EC: ---> status = 0x00
[  228.825092] ACPI: EC: ~~~> interrupt
[  228.825095] ACPI: EC: ---> status = 0x00
[  228.825109] ACPI: EC: ~~~> interrupt
[  228.825112] ACPI: EC: ---> status = 0x00
[  228.825126] ACPI: EC: ~~~> interrupt
[  228.825129] ACPI: EC: ---> status = 0x00
[  228.825143] ACPI: EC: ~~~> interrupt
[  228.825146] ACPI: EC: ---> status = 0x00
[  228.825160] ACPI: EC: ~~~> interrupt
[  228.825162] ACPI: EC: ---> status = 0x00
[  228.825176] ACPI: EC: ~~~> interrupt
[  228.825179] ACPI: EC: ---> status = 0x00
[  228.825193] ACPI: EC: ~~~> interrupt
[  228.825196] ACPI: EC: ---> status = 0x00
[  228.825210] ACPI: EC: ~~~> interrupt
[  228.825213] ACPI: EC: ---> status = 0x00
[  228.825227] ACPI: EC: ~~~> interrupt
[  228.825230] ACPI: EC: ---> status = 0x00
[  228.825244] ACPI: EC: ~~~> interrupt
[  228.825247] ACPI: EC: ---> status = 0x00
[  228.825261] ACPI: EC: ~~~> interrupt
[  228.825264] ACPI: EC: ---> status = 0x00
[  228.825277] ACPI: EC: ~~~> interrupt
[  228.825280] ACPI: EC: ---> status = 0x00
[  228.825294] ACPI: EC: ~~~> interrupt
[  228.825297] ACPI: EC: ---> status = 0x00
[  228.825311] ACPI: EC: ~~~> interrupt
[  228.825314] ACPI: EC: ---> status = 0x00
[  228.825328] ACPI: EC: ~~~> interrupt
[  228.825331] ACPI: EC: ---> status = 0x00
[  228.825345] ACPI: EC: ~~~> interrupt
[  228.825348] ACPI: EC: ---> status = 0x00
[  228.825362] ACPI: EC: ~~~> interrupt
[  228.825365] ACPI: EC: ---> status = 0x00
[  228.825379] ACPI: EC: ~~~> interrupt
[  228.825381] ACPI: EC: ---> status = 0x00
[  228.825387] ACPI: EC: transaction end

"transaction end" happens to be printed than EC stops sending interrupts. It looks like we get 100% interrupt load for 1ms or so...
This piece does not have 0x20 (SCI) bit set, so it is not a query issue.

I'm out of ideas, so if you have some, please speak up.
Also, Intel wants back this problematic notebook in 2 weeks, so I will not be able to debug it afterwards.

Regards,
Alex.

[-- Attachment #2: fast_transaction.patch --]
[-- Type: text/x-diff, Size: 8862 bytes --]

ACPI: EC: do transaction from interrupt context

From:  <>

It is easier and faster to do transaction directly from interrupt context
rather than waking control thread.

Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
---

 drivers/acpi/ec.c |  181 ++++++++++++++++++++++-------------------------------
 1 files changed, 74 insertions(+), 107 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index bdc9b6b..0167b93 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -65,18 +65,11 @@ enum ec_command {
 	ACPI_EC_COMMAND_QUERY = 0x84,
 };
 
-/* EC events */
-enum ec_event {
-	ACPI_EC_EVENT_OBF_1 = 1,	/* Output buffer full */
-	ACPI_EC_EVENT_IBF_0,		/* Input buffer empty */
-};
-
 #define ACPI_EC_DELAY		500	/* Wait 500ms max. during EC ops */
 #define ACPI_EC_UDELAY_GLK	1000	/* Wait 1ms max. to get global lock */
 #define ACPI_EC_UDELAY		100	/* Wait 100us before polling EC again */
 
 enum {
-	EC_FLAGS_WAIT_GPE = 0,		/* Don't check status until GPE arrives */
 	EC_FLAGS_QUERY_PENDING,		/* Query is pending */
 	EC_FLAGS_GPE_MODE,		/* Expect GPE to be sent for status change */
 	EC_FLAGS_NO_GPE,		/* Don't use GPE mode */
@@ -95,6 +88,14 @@ struct acpi_ec_query_handler {
 	u8 query_bit;
 };
 
+struct transaction_data {
+	const u8 *wdata;
+	u8 *rdata;
+	u8 command;
+	u8 wlen;
+	u8 rlen;
+};
+
 static struct acpi_ec {
 	acpi_handle handle;
 	unsigned long gpe;
@@ -106,6 +107,7 @@ static struct acpi_ec {
 	wait_queue_head_t wait;
 	struct list_head list;
 	struct delayed_work work;
+	struct transaction_data t;
 	atomic_t irq_count;
 	u8 handlers_installed;
 } *boot_ec, *first_ec;
@@ -152,13 +154,13 @@ static inline u8 acpi_ec_read_data(struct acpi_ec *ec)
 {
 	u8 x = inb(ec->data_addr);
 	pr_debug(PREFIX "---> data = 0x%2.2x\n", x);
-	return inb(ec->data_addr);
+	return x;
 }
 
 static inline void acpi_ec_write_cmd(struct acpi_ec *ec, u8 command)
 {
 	pr_debug(PREFIX "<--- command = 0x%2.2x\n", command);
-	outb(command, ec->command_addr);
+	outb((ec->t.command = command), ec->command_addr);
 }
 
 static inline void acpi_ec_write_data(struct acpi_ec *ec, u8 data)
@@ -167,21 +169,6 @@ static inline void acpi_ec_write_data(struct acpi_ec *ec, u8 data)
 	outb(data, ec->data_addr);
 }
 
-static inline int acpi_ec_check_status(struct acpi_ec *ec, enum ec_event event)
-{
-	if (test_bit(EC_FLAGS_WAIT_GPE, &ec->flags))
-		return 0;
-	if (event == ACPI_EC_EVENT_OBF_1) {
-		if (acpi_ec_read_status(ec) & ACPI_EC_FLAG_OBF)
-			return 1;
-	} else if (event == ACPI_EC_EVENT_IBF_0) {
-		if (!(acpi_ec_read_status(ec) & ACPI_EC_FLAG_IBF))
-			return 1;
-	}
-
-	return 0;
-}
-
 static void ec_schedule_ec_poll(struct acpi_ec *ec)
 {
 	if (test_bit(EC_FLAGS_RESCHEDULE_POLL, &ec->flags))
@@ -189,47 +176,45 @@ static void ec_schedule_ec_poll(struct acpi_ec *ec)
 				      msecs_to_jiffies(ACPI_EC_DELAY));
 }
 
-static void ec_switch_to_poll_mode(struct acpi_ec *ec)
-{
-	set_bit(EC_FLAGS_NO_GPE, &ec->flags);
-	clear_bit(EC_FLAGS_GPE_MODE, &ec->flags);
-	acpi_disable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
-	set_bit(EC_FLAGS_RESCHEDULE_POLL, &ec->flags);
+static int ec_transaction_done(struct acpi_ec *ec) {
+	if (!ec->t.command)
+		return 1;
+	if (!ec->t.wlen && !ec->t.rlen)
+		return 1;
+	return 0;
 }
 
-static int acpi_ec_wait(struct acpi_ec *ec, enum ec_event event, int force_poll)
+static void gpe_transaction(struct acpi_ec *ec, u8 status)
 {
-	atomic_set(&ec->irq_count, 0);
-	if (likely(test_bit(EC_FLAGS_GPE_MODE, &ec->flags)) &&
-	    likely(!force_poll)) {
-		if (wait_event_timeout(ec->wait, acpi_ec_check_status(ec, event),
-				       msecs_to_jiffies(ACPI_EC_DELAY)))
-			return 0;
-		clear_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
-		if (acpi_ec_check_status(ec, event)) {
-			/* missing GPEs, switch back to poll mode */
-			if (printk_ratelimit())
-				pr_info(PREFIX "missing confirmations, "
-						"switch off interrupt mode.\n");
-			ec_switch_to_poll_mode(ec);
-			ec_schedule_ec_poll(ec);
-			return 0;
+	if (!ec->t.command)
+		return;
+	if (ec->t.wlen > 0) {
+		if ((status & ACPI_EC_FLAG_IBF) == 0) {
+			acpi_ec_write_data(ec, *(ec->t.wdata++));
+			--ec->t.wlen;
 		}
-	} else {
-		unsigned long delay = jiffies + msecs_to_jiffies(ACPI_EC_DELAY);
-		clear_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
-		while (time_before(jiffies, delay)) {
-			if (acpi_ec_check_status(ec, event))
-				return 0;
-			msleep(1);
+	} else if (ec->t.rlen > 0) {
+		if ((status & ACPI_EC_FLAG_OBF) == 1) {
+			*(ec->t.rdata++) = acpi_ec_read_data(ec);
+			--ec->t.rlen;
 		}
-		if (acpi_ec_check_status(ec,event))
-			return 0;
 	}
-	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;
+}
+
+static void acpi_ec_wait(struct acpi_ec *ec)
+{
+	if (wait_event_timeout(ec->wait, ec_transaction_done(ec),
+			       msecs_to_jiffies(ACPI_EC_DELAY)))
+		return;
+	/* missing GPEs, switch back to poll mode */
+	if (printk_ratelimit())
+		pr_info(PREFIX "missing confirmations, "
+				"switch off interrupt mode.\n");
+#if 0
+	set_bit(EC_FLAGS_NO_GPE, &ec->flags);
+	clear_bit(EC_FLAGS_GPE_MODE, &ec->flags);
+#endif
+	return;
 }
 
 static int acpi_ec_transaction_unlocked(struct acpi_ec *ec, u8 command,
@@ -237,45 +222,33 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec, u8 command,
 					u8 * rdata, unsigned rdata_len,
 					int force_poll)
 {
-	int result = 0;
-	set_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
 	pr_debug(PREFIX "transaction start\n");
+	ec->t.wdata = wdata;
+	ec->t.wlen = wdata_len;
+	ec->t.rdata = rdata;
+	ec->t.rlen = rdata_len;
 	acpi_ec_write_cmd(ec, command);
-	for (; wdata_len > 0; --wdata_len) {
-		result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBF_0, force_poll);
-		if (result) {
-			pr_err(PREFIX
-			       "write_cmd timeout, command = %d\n", command);
-			goto end;
-		}
-		set_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
-		acpi_ec_write_data(ec, *(wdata++));
-	}
-
-	if (!rdata_len) {
-		result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBF_0, force_poll);
-		if (result) {
-			pr_err(PREFIX
-			       "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);
-		if (result) {
-			pr_err(PREFIX "read timeout, command = %d\n", command);
-			goto end;
+	if (!force_poll && test_bit(EC_FLAGS_GPE_MODE, &ec->flags))
+		acpi_ec_wait(ec);
+	else {
+		unsigned long delay = jiffies + msecs_to_jiffies(ACPI_EC_DELAY);
+		while (time_before(jiffies, delay)) {
+			gpe_transaction(ec, acpi_ec_read_status(ec));
+			if (ec_transaction_done(ec))
+				goto end;
+			udelay(5);
 		}
-		/* Don't expect GPE after last read */
-		if (rdata_len > 1)
-			set_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
-		*(rdata++) = acpi_ec_read_data(ec);
 	}
-      end:
+end:
 	pr_debug(PREFIX "transaction end\n");
-	return result;
+	ec->t.command = 0;
+	return 0;
+}
+
+static int ec_check_ibf0(struct acpi_ec *ec)
+{
+	u8 status = acpi_ec_read_status(ec);
+	return (status & ACPI_EC_FLAG_IBF) == 0;
 }
 
 static int acpi_ec_transaction(struct acpi_ec *ec, u8 command,
@@ -301,8 +274,8 @@ static int acpi_ec_transaction(struct acpi_ec *ec, u8 command,
 		}
 	}
 
-	status = acpi_ec_wait(ec, ACPI_EC_EVENT_IBF_0, 0);
-	if (status) {
+	if (!wait_event_timeout(ec->wait, ec_check_ibf0(ec),
+				msecs_to_jiffies(ACPI_EC_DELAY))) {
 		pr_err(PREFIX "input buffer is not empty, "
 				"aborting transaction\n");
 		goto end;
@@ -439,6 +412,7 @@ static int acpi_ec_query(struct acpi_ec *ec, u8 * data)
 	 */
 
 	result = acpi_ec_transaction(ec, ACPI_EC_COMMAND_QUERY, NULL, 0, &d, 1, 0);
+	clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
 	if (result)
 		return result;
 
@@ -517,19 +491,13 @@ static u32 acpi_ec_gpe_handler(void *data)
 {
 	acpi_status status = AE_OK;
 	struct acpi_ec *ec = data;
-	u8 state = acpi_ec_read_status(ec);
-	static bool warn_done = 0;
+	u8 state;
 
 	pr_debug(PREFIX "~~~> interrupt\n");
-	atomic_inc(&ec->irq_count);
-	if (!warn_done && atomic_read(&ec->irq_count) > 5) {
-		pr_warning(PREFIX "GPE storm detected, try to use ec_intr=0 "
-			"kernel option, if you see problems with keyboard.\n");
-		warn_done = 1;
-		goto end;
-	}
-	clear_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
-	if (test_bit(EC_FLAGS_GPE_MODE, &ec->flags))
+	state = acpi_ec_read_status(ec);
+
+	gpe_transaction(ec, state);
+	if ((status & ACPI_EC_FLAG_IBF) == 0)
 		wake_up(&ec->wait);
 
 	if (state & ACPI_EC_FLAG_SCI) {
@@ -546,7 +514,6 @@ static u32 acpi_ec_gpe_handler(void *data)
 		set_bit(EC_FLAGS_GPE_MODE, &ec->flags);
 		clear_bit(EC_FLAGS_RESCHEDULE_POLL, &ec->flags);
 	}
-end:
 	ec_schedule_ec_poll(ec);
 	return ACPI_SUCCESS(status) ?
 	    ACPI_INTERRUPT_HANDLED : ACPI_INTERRUPT_NOT_HANDLED;

Re: RFC: fast transactions in EC [was: a problem about the two patches in bug 10724 & 11428]

[Zhao Yakui]

On Thu, 2008-09-04 at 01:55 +0400, Alexey Starikovskiy wrote:
> Hi,
> Here is the patch, which moves almost all transaction functionality into interrupt handler, which is IMHO good.
> 
> with the enabled DEBUG, the interrupt picture looks like this (single transaction):
Thanks for your work and efforts. Maybe you have tested the patch on
your laptop. But IMO this is not reasonable. In the following cases
maybe the patch can't work well.
   a. EC GPE storm.  According to ACPI spec the EC uses the pulse
interrupt and interrupt is firmware generated using an EC GPIO output,
which is connected with chipset GPIO input. If the pulse waveform is
very wide, maybe several EC GPE interrupts will be triggered although EC
firmware generates one pulse waveform. How can we read the corresponding
data from EC in the GPE interrupt service handler? Maybe the read/write
data is completely incorrect.
   b. If there is no EC interrupt although OBF_1 bit is valid.(In theory
when OBF_1 is valid, EC should trigger GPE interrupt). In such case the
read/write flowchart will be wrong.

  At the same time it seems that EC transaction flowchart will become
more complex in this patch.


> [  228.823886] ACPI: EC: ---> status = 0x08 -- this is a check for IBF=0 before starting transaction
> [  228.823888] ACPI: EC: transaction start
> [  228.823890] ACPI: EC: <--- command = 0x80 -- here we write command
> [  228.824123] ACPI: EC: ~~~> interrupt -- this interrupt is generated with the same content in status register as before writing a command...
> [  228.824127] ACPI: EC: ---> status = 0x08
> [  228.824129] ACPI: EC: <--- data = 0x58
> [  228.824152] ACPI: EC: ~~~> interrupt -- and now EC start sending us interrupts like crazy...
> [  228.824155] ACPI: EC: ---> status = 0x02
> [  228.824169] ACPI: EC: ~~~> interrupt
> [  228.824172] ACPI: EC: ---> status = 0x02
> [  228.824186] ACPI: EC: ~~~> interrupt
> [  228.824189] ACPI: EC: ---> status = 0x02
> [  228.824203] ACPI: EC: ~~~> interrupt
> [  228.824206] ACPI: EC: ---> status = 0x02
> [  228.824219] ACPI: EC: ~~~> interrupt
> [  228.824222] ACPI: EC: ---> status = 0x02
> [  228.824236] ACPI: EC: ~~~> interrupt
> [  228.824239] ACPI: EC: ---> status = 0x02
> [  228.824253] ACPI: EC: ~~~> interrupt
> [  228.824256] ACPI: EC: ---> status = 0x02
> [  228.824270] ACPI: EC: ~~~> interrupt
> [  228.824273] ACPI: EC: ---> status = 0x02
> [  228.824287] ACPI: EC: ~~~> interrupt
> [  228.824290] ACPI: EC: ---> status = 0x02
> [  228.824304] ACPI: EC: ~~~> interrupt
> [  228.824307] ACPI: EC: ---> status = 0x02
> [  228.824321] ACPI: EC: ~~~> interrupt
> [  228.824324] ACPI: EC: ---> status = 0x02
> [  228.824337] ACPI: EC: ~~~> interrupt
> [  228.824340] ACPI: EC: ---> status = 0x02
> [  228.824354] ACPI: EC: ~~~> interrupt
> [  228.824357] ACPI: EC: ---> status = 0x02
> [  228.824371] ACPI: EC: ~~~> interrupt
> [  228.824374] ACPI: EC: ---> status = 0x02
> [  228.824388] ACPI: EC: ~~~> interrupt
> [  228.824391] ACPI: EC: ---> status = 0x02
> [  228.824405] ACPI: EC: ~~~> interrupt
> [  228.824408] ACPI: EC: ---> status = 0x02
> [  228.824422] ACPI: EC: ~~~> interrupt
> [  228.824425] ACPI: EC: ---> status = 0x02
> [  228.824439] ACPI: EC: ~~~> interrupt
> [  228.824442] ACPI: EC: ---> status = 0x02
> [  228.824455] ACPI: EC: ~~~> interrupt
> [  228.824458] ACPI: EC: ---> status = 0x02
> [  228.824472] ACPI: EC: ~~~> interrupt
> [  228.824475] ACPI: EC: ---> status = 0x02
> [  228.824489] ACPI: EC: ~~~> interrupt
> [  228.824492] ACPI: EC: ---> status = 0x02
> [  228.824506] ACPI: EC: ~~~> interrupt
> [  228.824509] ACPI: EC: ---> status = 0x02
> [  228.824523] ACPI: EC: ~~~> interrupt
> [  228.824526] ACPI: EC: ---> status = 0x02
> [  228.824540] ACPI: EC: ~~~> interrupt
> [  228.824543] ACPI: EC: ---> status = 0x02
> [  228.824557] ACPI: EC: ~~~> interrupt
> [  228.824560] ACPI: EC: ---> status = 0x02
> [  228.824573] ACPI: EC: ~~~> interrupt
> [  228.824576] ACPI: EC: ---> status = 0x02
> [  228.824590] ACPI: EC: ~~~> interrupt
> [  228.824593] ACPI: EC: ---> status = 0x02
> [  228.824607] ACPI: EC: ~~~> interrupt
> [  228.824610] ACPI: EC: ---> status = 0x02
> [  228.824624] ACPI: EC: ~~~> interrupt
> [  228.824627] ACPI: EC: ---> status = 0x02
> [  228.824641] ACPI: EC: ~~~> interrupt
> [  228.824644] ACPI: EC: ---> status = 0x02
> [  228.824658] ACPI: EC: ~~~> interrupt
> [  228.824661] ACPI: EC: ---> status = 0x02
> [  228.824675] ACPI: EC: ~~~> interrupt
> [  228.824678] ACPI: EC: ---> status = 0x02
> [  228.824691] ACPI: EC: ~~~> interrupt
> [  228.824694] ACPI: EC: ---> status = 0x02
> [  228.824708] ACPI: EC: ~~~> interrupt
> [  228.824711] ACPI: EC: ---> status = 0x02
> [  228.824785] ACPI: EC: ~~~> interrupt    --- one more "real" interrupt. Others are just garbage.
> [  228.824788] ACPI: EC: ---> status = 0x01
> [  228.824791] ACPI: EC: ---> data = 0x35
> [  228.824805] ACPI: EC: ~~~> interrupt
> [  228.824808] ACPI: EC: ---> status = 0x00
> [  228.824822] ACPI: EC: ~~~> interrupt
> [  228.824825] ACPI: EC: ---> status = 0x00
> [  228.824839] ACPI: EC: ~~~> interrupt
> [  228.824842] ACPI: EC: ---> status = 0x00
> [  228.824856] ACPI: EC: ~~~> interrupt
> [  228.824859] ACPI: EC: ---> status = 0x00
> [  228.824873] ACPI: EC: ~~~> interrupt
> [  228.824876] ACPI: EC: ---> status = 0x00
> [  228.824890] ACPI: EC: ~~~> interrupt
> [  228.824893] ACPI: EC: ---> status = 0x00
> [  228.824907] ACPI: EC: ~~~> interrupt
> [  228.824910] ACPI: EC: ---> status = 0x00
> [  228.824924] ACPI: EC: ~~~> interrupt
> [  228.824926] ACPI: EC: ---> status = 0x00
> [  228.824940] ACPI: EC: ~~~> interrupt
> [  228.824943] ACPI: EC: ---> status = 0x00
> [  228.824957] ACPI: EC: ~~~> interrupt
> [  228.824960] ACPI: EC: ---> status = 0x00
> [  228.824974] ACPI: EC: ~~~> interrupt
> [  228.824977] ACPI: EC: ---> status = 0x00
> [  228.824991] ACPI: EC: ~~~> interrupt
> [  228.824994] ACPI: EC: ---> status = 0x00
> [  228.825008] ACPI: EC: ~~~> interrupt
> [  228.825011] ACPI: EC: ---> status = 0x00
> [  228.825025] ACPI: EC: ~~~> interrupt
> [  228.825028] ACPI: EC: ---> status = 0x00
> [  228.825042] ACPI: EC: ~~~> interrupt
> [  228.825044] ACPI: EC: ---> status = 0x00
> [  228.825058] ACPI: EC: ~~~> interrupt
> [  228.825061] ACPI: EC: ---> status = 0x00
> [  228.825075] ACPI: EC: ~~~> interrupt
> [  228.825078] ACPI: EC: ---> status = 0x00
> [  228.825092] ACPI: EC: ~~~> interrupt
> [  228.825095] ACPI: EC: ---> status = 0x00
> [  228.825109] ACPI: EC: ~~~> interrupt
> [  228.825112] ACPI: EC: ---> status = 0x00
> [  228.825126] ACPI: EC: ~~~> interrupt
> [  228.825129] ACPI: EC: ---> status = 0x00
> [  228.825143] ACPI: EC: ~~~> interrupt
> [  228.825146] ACPI: EC: ---> status = 0x00
> [  228.825160] ACPI: EC: ~~~> interrupt
> [  228.825162] ACPI: EC: ---> status = 0x00
> [  228.825176] ACPI: EC: ~~~> interrupt
> [  228.825179] ACPI: EC: ---> status = 0x00
> [  228.825193] ACPI: EC: ~~~> interrupt
> [  228.825196] ACPI: EC: ---> status = 0x00
> [  228.825210] ACPI: EC: ~~~> interrupt
> [  228.825213] ACPI: EC: ---> status = 0x00
> [  228.825227] ACPI: EC: ~~~> interrupt
> [  228.825230] ACPI: EC: ---> status = 0x00
> [  228.825244] ACPI: EC: ~~~> interrupt
> [  228.825247] ACPI: EC: ---> status = 0x00
> [  228.825261] ACPI: EC: ~~~> interrupt
> [  228.825264] ACPI: EC: ---> status = 0x00
> [  228.825277] ACPI: EC: ~~~> interrupt
> [  228.825280] ACPI: EC: ---> status = 0x00
> [  228.825294] ACPI: EC: ~~~> interrupt
> [  228.825297] ACPI: EC: ---> status = 0x00
> [  228.825311] ACPI: EC: ~~~> interrupt
> [  228.825314] ACPI: EC: ---> status = 0x00
> [  228.825328] ACPI: EC: ~~~> interrupt
> [  228.825331] ACPI: EC: ---> status = 0x00
> [  228.825345] ACPI: EC: ~~~> interrupt
> [  228.825348] ACPI: EC: ---> status = 0x00
> [  228.825362] ACPI: EC: ~~~> interrupt
> [  228.825365] ACPI: EC: ---> status = 0x00
> [  228.825379] ACPI: EC: ~~~> interrupt
> [  228.825381] ACPI: EC: ---> status = 0x00
> [  228.825387] ACPI: EC: transaction end
> 
> "transaction end" happens to be printed than EC stops sending interrupts. It looks like we get 100% interrupt load for 1ms or so...
> This piece does not have 0x20 (SCI) bit set, so it is not a query issue.
> 
> I'm out of ideas, so if you have some, please speak up.
> Also, Intel wants back this problematic notebook in 2 weeks, so I will not be able to debug it afterwards.
> 
> Regards,
> Alex.

Re: RFC: fast transactions in EC [was: a problem about the two patches in bug 10724 & 11428]

[Alexey Starikovskiy]

Zhao Yakui wrote:
> On Thu, 2008-09-04 at 01:55 +0400, Alexey Starikovskiy wrote:
>> Hi,
>> Here is the patch, which moves almost all transaction functionality into interrupt handler, which is IMHO good.
>>
>> with the enabled DEBUG, the interrupt picture looks like this (single transaction):
> Thanks for your work and efforts. Maybe you have tested the patch on
> your laptop. But IMO this is not reasonable. In the following cases
> maybe the patch can't work well.
>    a. EC GPE storm.  According to ACPI spec the EC uses the pulse
This _is_ the machine with the EC GPE storm. Acer TM 2300. And the patch works...
And you may see it in the quote I gave.
> interrupt and interrupt is firmware generated using an EC GPIO output,
> which is connected with chipset GPIO input. If the pulse waveform is
> very wide, maybe several EC GPE interrupts will be triggered although EC
> firmware generates one pulse waveform. How can we read the corresponding
> data from EC in the GPE interrupt service handler? Maybe the read/write
> data is completely incorrect.
Well, it is not any different from the data I get with unpatched ec.c.
>    b. If there is no EC interrupt although OBF_1 bit is valid.(In theory
> when OBF_1 is valid, EC should trigger GPE interrupt). In such case the
> read/write flowchart will be wrong.
Oh, you mean it does not cover all the error paths yet?
>   At the same time it seems that EC transaction flowchart will become
> more complex in this patch.
How so? It even does not involve second thread any longer :)

Re: RFC: fast transactions in EC [was: a problem about the two patches in bug 10724 & 11428]

[Alexey Starikovskiy]

Zhao Yakui wrote:
>    a. EC GPE storm.  According to ACPI spec the EC uses the pulse
> interrupt and interrupt is firmware generated using an EC GPIO output,
> which is connected with chipset GPIO input. If the pulse waveform is
> very wide, maybe several EC GPE interrupts will be triggered although EC
> firmware generates one pulse waveform. How can we read the corresponding
> data from EC in the GPE interrupt service handler? Maybe the read/write
> data is completely incorrect.
Sorry, I missed the whole idea on first reading :)
You've started talking hardware terms :)
EC in our case exposes two I/O ports 0x62 (status/command) and 0x66 (data r/w).
It always exposes two, but numbers sometime differ... We always did status read
in interrupt context, which is inb(0x62), and always relayed on that being working.
Now you are telling that there is a huge difference between that and inb(0x66), which
will accomplish EC_READ command.
So, could you please tell, how reading of port 0x62 might be all good from interrupt context,
and reading of port 0x66 is completely wrong idea?

Regards,
Alex.

Re: RFC: fast transactions in EC [was: a problem about the two patches in bug 10724 & 11428]

[Alexey Starikovskiy]

[-- Attachment #1: Type: text/plain, Size: 1227 bytes --]

Zhao Yakui wrote:
> On Thu, 2008-09-04 at 01:55 +0400, Alexey Starikovskiy wrote:
>> Hi,
>> Here is the patch, which moves almost all transaction functionality into interrupt handler, which is IMHO good.
>>
>> with the enabled DEBUG, the interrupt picture looks like this (single transaction):
> Thanks for your work and efforts. Maybe you have tested the patch on
> your laptop. But IMO this is not reasonable. In the following cases
> maybe the patch can't work well.
>    a. EC GPE storm.  According to ACPI spec the EC uses the pulse
> interrupt and interrupt is firmware generated using an EC GPIO output,
> which is connected with chipset GPIO input. If the pulse waveform is
> very wide, maybe several EC GPE interrupts will be triggered although EC
> firmware generates one pulse waveform. How can we read the corresponding
> data from EC in the GPE interrupt service handler? Maybe the read/write
> data is completely incorrect.
>    b. If there is no EC interrupt although OBF_1 bit is valid.(In theory
> when OBF_1 is valid, EC should trigger GPE interrupt). In such case the
> read/write flowchart will be wrong.
Ok, both issues are solved by following patch. Interrupts are down to ~500 from 70000.
Regards,
Alex.

[-- Attachment #2: fast_transaction.patch --]
[-- Type: text/x-diff, Size: 10138 bytes --]

ACPI: EC: do transaction from interrupt context

From:  <>

It is easier and faster to do transaction directly from interrupt context
rather than waking control thread.

Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
---

 drivers/acpi/ec.c |  203 ++++++++++++++++++++++++-----------------------------
 1 files changed, 91 insertions(+), 112 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index bdc9b6b..5a344e6 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -26,8 +26,8 @@
  * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  */
 
-/* Uncomment next line to get verbose print outs*/
-/* #define DEBUG */
+/* Uncomment next line to get verbose printout */
+#define DEBUG
 
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -65,18 +65,11 @@ enum ec_command {
 	ACPI_EC_COMMAND_QUERY = 0x84,
 };
 
-/* EC events */
-enum ec_event {
-	ACPI_EC_EVENT_OBF_1 = 1,	/* Output buffer full */
-	ACPI_EC_EVENT_IBF_0,		/* Input buffer empty */
-};
-
 #define ACPI_EC_DELAY		500	/* Wait 500ms max. during EC ops */
 #define ACPI_EC_UDELAY_GLK	1000	/* Wait 1ms max. to get global lock */
 #define ACPI_EC_UDELAY		100	/* Wait 100us before polling EC again */
 
 enum {
-	EC_FLAGS_WAIT_GPE = 0,		/* Don't check status until GPE arrives */
 	EC_FLAGS_QUERY_PENDING,		/* Query is pending */
 	EC_FLAGS_GPE_MODE,		/* Expect GPE to be sent for status change */
 	EC_FLAGS_NO_GPE,		/* Don't use GPE mode */
@@ -95,6 +88,14 @@ struct acpi_ec_query_handler {
 	u8 query_bit;
 };
 
+struct transaction_data {
+	const u8 *wdata;
+	u8 *rdata;
+	u8 command;
+	u8 wlen;
+	u8 rlen;
+};
+
 static struct acpi_ec {
 	acpi_handle handle;
 	unsigned long gpe;
@@ -106,6 +107,7 @@ static struct acpi_ec {
 	wait_queue_head_t wait;
 	struct list_head list;
 	struct delayed_work work;
+	struct transaction_data t;
 	atomic_t irq_count;
 	u8 handlers_installed;
 } *boot_ec, *first_ec;
@@ -152,13 +154,13 @@ static inline u8 acpi_ec_read_data(struct acpi_ec *ec)
 {
 	u8 x = inb(ec->data_addr);
 	pr_debug(PREFIX "---> data = 0x%2.2x\n", x);
-	return inb(ec->data_addr);
+	return x;
 }
 
 static inline void acpi_ec_write_cmd(struct acpi_ec *ec, u8 command)
 {
 	pr_debug(PREFIX "<--- command = 0x%2.2x\n", command);
-	outb(command, ec->command_addr);
+	outb((ec->t.command = command), ec->command_addr);
 }
 
 static inline void acpi_ec_write_data(struct acpi_ec *ec, u8 data)
@@ -167,69 +169,62 @@ static inline void acpi_ec_write_data(struct acpi_ec *ec, u8 data)
 	outb(data, ec->data_addr);
 }
 
-static inline int acpi_ec_check_status(struct acpi_ec *ec, enum ec_event event)
+static void ec_schedule_ec_poll(struct acpi_ec *ec)
 {
-	if (test_bit(EC_FLAGS_WAIT_GPE, &ec->flags))
-		return 0;
-	if (event == ACPI_EC_EVENT_OBF_1) {
-		if (acpi_ec_read_status(ec) & ACPI_EC_FLAG_OBF)
-			return 1;
-	} else if (event == ACPI_EC_EVENT_IBF_0) {
-		if (!(acpi_ec_read_status(ec) & ACPI_EC_FLAG_IBF))
-			return 1;
-	}
+	if (test_bit(EC_FLAGS_RESCHEDULE_POLL, &ec->flags))
+		schedule_delayed_work(&ec->work,
+				      msecs_to_jiffies(ACPI_EC_DELAY));
+}
 
+static int ec_transaction_done(struct acpi_ec *ec) {
+	if (!ec->t.command)
+		return 1;
+	if (!ec->t.wlen && !ec->t.rlen)
+		return 1;
 	return 0;
 }
 
-static void ec_schedule_ec_poll(struct acpi_ec *ec)
+static void gpe_transaction(struct acpi_ec *ec, u8 status)
 {
-	if (test_bit(EC_FLAGS_RESCHEDULE_POLL, &ec->flags))
-		schedule_delayed_work(&ec->work,
-				      msecs_to_jiffies(ACPI_EC_DELAY));
+	if (!ec->t.command)
+		return;
+	if (ec->t.wlen > 0) {
+		if ((status & ACPI_EC_FLAG_IBF) == 0) {
+			acpi_ec_write_data(ec, *(ec->t.wdata++));
+			--ec->t.wlen;
+		}
+	} else if (ec->t.rlen > 0) {
+		if ((status & ACPI_EC_FLAG_OBF) == 1) {
+			*(ec->t.rdata++) = acpi_ec_read_data(ec);
+			--ec->t.rlen;
+		}
+	}
 }
 
-static void ec_switch_to_poll_mode(struct acpi_ec *ec)
+static int acpi_ec_wait(struct acpi_ec *ec)
 {
+	if (wait_event_timeout(ec->wait, ec_transaction_done(ec),
+			       msecs_to_jiffies(ACPI_EC_DELAY)))
+		return 0;
+	/* missing GPEs, switch back to poll mode */
+	if (printk_ratelimit())
+		pr_info(PREFIX "missing confirmations, "
+				"switch off interrupt mode.\n");
 	set_bit(EC_FLAGS_NO_GPE, &ec->flags);
 	clear_bit(EC_FLAGS_GPE_MODE, &ec->flags);
-	acpi_disable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
-	set_bit(EC_FLAGS_RESCHEDULE_POLL, &ec->flags);
+	return 1;
 }
 
-static int acpi_ec_wait(struct acpi_ec *ec, enum ec_event event, int force_poll)
+static void acpi_ec_gpe_query(void *ec_cxt);
+
+static int ec_check_sci(struct acpi_ec *ec, u8 state)
 {
-	atomic_set(&ec->irq_count, 0);
-	if (likely(test_bit(EC_FLAGS_GPE_MODE, &ec->flags)) &&
-	    likely(!force_poll)) {
-		if (wait_event_timeout(ec->wait, acpi_ec_check_status(ec, event),
-				       msecs_to_jiffies(ACPI_EC_DELAY)))
-			return 0;
-		clear_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
-		if (acpi_ec_check_status(ec, event)) {
-			/* missing GPEs, switch back to poll mode */
-			if (printk_ratelimit())
-				pr_info(PREFIX "missing confirmations, "
-						"switch off interrupt mode.\n");
-			ec_switch_to_poll_mode(ec);
-			ec_schedule_ec_poll(ec);
-			return 0;
-		}
-	} else {
-		unsigned long delay = jiffies + msecs_to_jiffies(ACPI_EC_DELAY);
-		clear_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
-		while (time_before(jiffies, delay)) {
-			if (acpi_ec_check_status(ec, event))
-				return 0;
-			msleep(1);
-		}
-		if (acpi_ec_check_status(ec,event))
-			return 0;
+	if (state & ACPI_EC_FLAG_SCI) {
+		if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags))
+			return acpi_os_execute(OSL_EC_BURST_HANDLER,
+				acpi_ec_gpe_query, ec);
 	}
-	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;
+	return 0;
 }
 
 static int acpi_ec_transaction_unlocked(struct acpi_ec *ec, u8 command,
@@ -237,45 +232,38 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec, u8 command,
 					u8 * rdata, unsigned rdata_len,
 					int force_poll)
 {
-	int result = 0;
-	set_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
 	pr_debug(PREFIX "transaction start\n");
+	set_bit(EC_FLAGS_NO_GPE, &ec->flags);
+	clear_bit(EC_FLAGS_GPE_MODE, &ec->flags);
+	acpi_disable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
+	ec->t.wdata = wdata;
+	ec->t.wlen = wdata_len;
+	ec->t.rdata = rdata;
+	ec->t.rlen = rdata_len;
 	acpi_ec_write_cmd(ec, command);
-	for (; wdata_len > 0; --wdata_len) {
-		result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBF_0, force_poll);
-		if (result) {
-			pr_err(PREFIX
-			       "write_cmd timeout, command = %d\n", command);
-			goto end;
-		}
-		set_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
-		acpi_ec_write_data(ec, *(wdata++));
-	}
-
-	if (!rdata_len) {
-		result = acpi_ec_wait(ec, ACPI_EC_EVENT_IBF_0, force_poll);
-		if (result) {
-			pr_err(PREFIX
-			       "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);
-		if (result) {
-			pr_err(PREFIX "read timeout, command = %d\n", command);
-			goto end;
+	if (force_poll ||
+	    !test_bit(EC_FLAGS_GPE_MODE, &ec->flags) ||
+	    acpi_ec_wait(ec)) {
+		unsigned long delay = jiffies + msecs_to_jiffies(ACPI_EC_DELAY);
+		while (time_before(jiffies, delay)) {
+			gpe_transaction(ec, acpi_ec_read_status(ec));
+			msleep(1);
+			if (ec_transaction_done(ec))
+				goto end;
 		}
-		/* Don't expect GPE after last read */
-		if (rdata_len > 1)
-			set_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
-		*(rdata++) = acpi_ec_read_data(ec);
 	}
-      end:
+end:
 	pr_debug(PREFIX "transaction end\n");
-	return result;
+	ec->t.command = 0;
+	ec_check_sci(ec, acpi_ec_read_status(ec));
+	acpi_enable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
+	return 0;
+}
+
+static int ec_check_ibf0(struct acpi_ec *ec)
+{
+	u8 status = acpi_ec_read_status(ec);
+	return (status & ACPI_EC_FLAG_IBF) == 0;
 }
 
 static int acpi_ec_transaction(struct acpi_ec *ec, u8 command,
@@ -301,8 +289,8 @@ static int acpi_ec_transaction(struct acpi_ec *ec, u8 command,
 		}
 	}
 
-	status = acpi_ec_wait(ec, ACPI_EC_EVENT_IBF_0, 0);
-	if (status) {
+	if (!wait_event_timeout(ec->wait, ec_check_ibf0(ec),
+				msecs_to_jiffies(ACPI_EC_DELAY))) {
 		pr_err(PREFIX "input buffer is not empty, "
 				"aborting transaction\n");
 		goto end;
@@ -439,6 +427,7 @@ static int acpi_ec_query(struct acpi_ec *ec, u8 * data)
 	 */
 
 	result = acpi_ec_transaction(ec, ACPI_EC_COMMAND_QUERY, NULL, 0, &d, 1, 0);
+	clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags);
 	if (result)
 		return result;
 
@@ -517,26 +506,17 @@ static u32 acpi_ec_gpe_handler(void *data)
 {
 	acpi_status status = AE_OK;
 	struct acpi_ec *ec = data;
-	u8 state = acpi_ec_read_status(ec);
-	static bool warn_done = 0;
+	u8 state;
 
 	pr_debug(PREFIX "~~~> interrupt\n");
-	atomic_inc(&ec->irq_count);
-	if (!warn_done && atomic_read(&ec->irq_count) > 5) {
-		pr_warning(PREFIX "GPE storm detected, try to use ec_intr=0 "
-			"kernel option, if you see problems with keyboard.\n");
-		warn_done = 1;
-		goto end;
-	}
-	clear_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
-	if (test_bit(EC_FLAGS_GPE_MODE, &ec->flags))
+	state = acpi_ec_read_status(ec);
+
+	gpe_transaction(ec, state);
+	if ((status & ACPI_EC_FLAG_IBF) == 0)
 		wake_up(&ec->wait);
 
-	if (state & ACPI_EC_FLAG_SCI) {
-		if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags))
-			status = acpi_os_execute(OSL_EC_BURST_HANDLER,
-				acpi_ec_gpe_query, ec);
-	} else if (!test_bit(EC_FLAGS_GPE_MODE, &ec->flags) &&
+	status = ec_check_sci(ec, state);
+	if (!test_bit(EC_FLAGS_GPE_MODE, &ec->flags) &&
 		   !test_bit(EC_FLAGS_NO_GPE, &ec->flags) &&
 		   in_interrupt()) {
 		/* this is non-query, must be confirmation */
@@ -546,7 +526,6 @@ static u32 acpi_ec_gpe_handler(void *data)
 		set_bit(EC_FLAGS_GPE_MODE, &ec->flags);
 		clear_bit(EC_FLAGS_RESCHEDULE_POLL, &ec->flags);
 	}
-end:
 	ec_schedule_ec_poll(ec);
 	return ACPI_SUCCESS(status) ?
 	    ACPI_INTERRUPT_HANDLED : ACPI_INTERRUPT_NOT_HANDLED;

Re: RFC: fast transactions in EC [was: a problem about the two patches in bug 10724 & 11428]

[Andrew Morton]

On Thu, 04 Sep 2008 08:51:53 +0400
Alexey Starikovskiy <astarikovskiy@suse.de> wrote:

> [fast_transaction.patch  text/x-diff (9.9KB)]
> ACPI: EC: do transaction from interrupt context
> 
> From:  <>
> 
> It is easier and faster to do transaction directly from interrupt context
> rather than waking control thread.

This change broke
acpi-ec-dont-degrade-to-poll-mode-at-storm-automatically.patch, below.

Is acpi-ec-dont-degrade-to-poll-mode-at-storm-automatically.patch now
obsolete?  Is the regression which
acpi-ec-dont-degrade-to-poll-mode-at-storm-automatically.patch fixed
now fixed?

Thanks.

From: Alexey Starikovskiy <astarikovskiy@suse.de>

Not all users of semi-broken EC devices want to degrade to poll mode, so
give them right to choose.

This fixes a regression in 2.6.26 (from 2.6.25.3).  Initially reported as
"Asus Eee PC hotkeys stop working if pressed quickly" in bugzilla
<http://bugzilla.kernel.org/show_bug.cgi?id=11089>.

The regression was caused by a recently added check for interrupt storms. 
The Eee PC triggers this check and switches to polling.  When multiple
events arrive between polling intervals, only one is fetched from the EC. 
This causes erroneous behaviour; ultimately events stop being delivered
altogether when the EC buffer overflows.

Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
Cc: Alexey Starikovskiy <astarikovskiy@suse.de>
Cc: Maximilian Engelhardt <maxi@daemonizer.de>
Cc: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: <stable@kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 Documentation/kernel-parameters.txt |    5 +++
 drivers/acpi/ec.c                   |   36 ++++++++++++++++++--------
 2 files changed, 31 insertions(+), 10 deletions(-)

diff -puN Documentation/kernel-parameters.txt~acpi-ec-dont-degrade-to-poll-mode-at-storm-automatically Documentation/kernel-parameters.txt
--- a/Documentation/kernel-parameters.txt~acpi-ec-dont-degrade-to-poll-mode-at-storm-automatically
+++ a/Documentation/kernel-parameters.txt
@@ -741,6 +741,11 @@ and is between 256 and 4096 characters. 
 
 	eata=		[HW,SCSI]
 
+	ec_intr=	[HW,ACPI] ACPI Embedded Controller interrupt mode
+			Format: <int>
+			0: polling mode
+			non-0: interrupt mode (default)
+
 	edd=		[EDD]
 			Format: {"off" | "on" | "skip[mbr]"}
 
diff -puN drivers/acpi/ec.c~acpi-ec-dont-degrade-to-poll-mode-at-storm-automatically drivers/acpi/ec.c
--- a/drivers/acpi/ec.c~acpi-ec-dont-degrade-to-poll-mode-at-storm-automatically
+++ a/drivers/acpi/ec.c
@@ -110,6 +110,8 @@ static struct acpi_ec {
 	u8 handlers_installed;
 } *boot_ec, *first_ec;
 
+int acpi_ec_intr = 1; /* Default is interrupt mode */
+
 /* 
  * Some Asus system have exchanged ECDT data/command IO addresses.
  */
@@ -516,12 +518,14 @@ static u32 acpi_ec_gpe_handler(void *dat
 	acpi_status status = AE_OK;
 	struct acpi_ec *ec = data;
 	u8 state = acpi_ec_read_status(ec);
+	static bool warn_done = 0;
 
 	pr_debug(PREFIX "~~~> interrupt\n");
 	atomic_inc(&ec->irq_count);
-	if (atomic_read(&ec->irq_count) > 5) {
-		pr_err(PREFIX "GPE storm detected, disabling EC GPE\n");
-		ec_switch_to_poll_mode(ec);
+	if (!warn_done && atomic_read(&ec->irq_count) > 5) {
+		pr_warning(PREFIX "GPE storm detected, try to use ec_intr=0 "
+			"kernel option, if you see problems with keyboard.\n");
+		warn_done = 1;
 		goto end;
 	}
 	clear_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
@@ -848,20 +852,21 @@ static int ec_install_handlers(struct ac
 	acpi_status status;
 	if (ec->handlers_installed)
 		return 0;
-	status = acpi_install_gpe_handler(NULL, ec->gpe,
+	if (acpi_ec_intr) {
+		status = acpi_install_gpe_handler(NULL, ec->gpe,
 					  ACPI_GPE_EDGE_TRIGGERED,
 					  &acpi_ec_gpe_handler, ec);
-	if (ACPI_FAILURE(status))
-		return -ENODEV;
-
-	acpi_set_gpe_type(NULL, ec->gpe, ACPI_GPE_TYPE_RUNTIME);
-	acpi_enable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
+		if (ACPI_FAILURE(status))
+			return -ENODEV;
 
+		acpi_set_gpe_type(NULL, ec->gpe, ACPI_GPE_TYPE_RUNTIME);
+		acpi_enable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
+	}
 	status = acpi_install_address_space_handler(ec->handle,
 						    ACPI_ADR_SPACE_EC,
 						    &acpi_ec_space_handler,
 						    NULL, ec);
-	if (ACPI_FAILURE(status)) {
+	if (ACPI_FAILURE(status) && acpi_ec_intr) {
 		acpi_remove_gpe_handler(NULL, ec->gpe, &acpi_ec_gpe_handler);
 		return -ENODEV;
 	}
@@ -1047,3 +1052,14 @@ static void __exit acpi_ec_exit(void)
 	return;
 }
 #endif	/* 0 */
+
+static int __init acpi_ec_set_intr_mode(char *str)
+{
+	if (!get_option(&str, &acpi_ec_intr)) {
+		acpi_ec_intr = 0;
+		return 0;
+	}
+	return 1;
+}
+
+__setup("ec_intr=", acpi_ec_set_intr_mode);
_

Re: RFC: fast transactions in EC [was: a problem about the two patches in bug 10724 & 11428]

[Alexey Starikovskiy]

Hi Andrew,

yes to both questions. This patch hopefully solves the problem better.

Thanks,
Alex.

Andrew Morton wrote:
> On Thu, 04 Sep 2008 08:51:53 +0400
> Alexey Starikovskiy <astarikovskiy@suse.de> wrote:
> 
>> [fast_transaction.patch  text/x-diff (9.9KB)]
>> ACPI: EC: do transaction from interrupt context
>>
>> From:  <>
>>
>> It is easier and faster to do transaction directly from interrupt context
>> rather than waking control thread.
> 
> This change broke
> acpi-ec-dont-degrade-to-poll-mode-at-storm-automatically.patch, below.
> 
> Is acpi-ec-dont-degrade-to-poll-mode-at-storm-automatically.patch now
> obsolete?  Is the regression which
> acpi-ec-dont-degrade-to-poll-mode-at-storm-automatically.patch fixed
> now fixed?
> 
> Thanks.
> 
> From: Alexey Starikovskiy <astarikovskiy@suse.de>
> 
> Not all users of semi-broken EC devices want to degrade to poll mode, so
> give them right to choose.
> 
> This fixes a regression in 2.6.26 (from 2.6.25.3).  Initially reported as
> "Asus Eee PC hotkeys stop working if pressed quickly" in bugzilla
> <http://bugzilla.kernel.org/show_bug.cgi?id=11089>.
> 
> The regression was caused by a recently added check for interrupt storms. 
> The Eee PC triggers this check and switches to polling.  When multiple
> events arrive between polling intervals, only one is fetched from the EC. 
> This causes erroneous behaviour; ultimately events stop being delivered
> altogether when the EC buffer overflows.
> 
> Signed-off-by: Alexey Starikovskiy <astarikovskiy@suse.de>
> Cc: Alexey Starikovskiy <astarikovskiy@suse.de>
> Cc: Maximilian Engelhardt <maxi@daemonizer.de>
> Cc: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> Cc: Andi Kleen <andi@firstfloor.org>
> Cc: <stable@kernel.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  Documentation/kernel-parameters.txt |    5 +++
>  drivers/acpi/ec.c                   |   36 ++++++++++++++++++--------
>  2 files changed, 31 insertions(+), 10 deletions(-)
> 
> diff -puN Documentation/kernel-parameters.txt~acpi-ec-dont-degrade-to-poll-mode-at-storm-automatically Documentation/kernel-parameters.txt
> --- a/Documentation/kernel-parameters.txt~acpi-ec-dont-degrade-to-poll-mode-at-storm-automatically
> +++ a/Documentation/kernel-parameters.txt
> @@ -741,6 +741,11 @@ and is between 256 and 4096 characters. 
>  
>  	eata=		[HW,SCSI]
>  
> +	ec_intr=	[HW,ACPI] ACPI Embedded Controller interrupt mode
> +			Format: <int>
> +			0: polling mode
> +			non-0: interrupt mode (default)
> +
>  	edd=		[EDD]
>  			Format: {"off" | "on" | "skip[mbr]"}
>  
> diff -puN drivers/acpi/ec.c~acpi-ec-dont-degrade-to-poll-mode-at-storm-automatically drivers/acpi/ec.c
> --- a/drivers/acpi/ec.c~acpi-ec-dont-degrade-to-poll-mode-at-storm-automatically
> +++ a/drivers/acpi/ec.c
> @@ -110,6 +110,8 @@ static struct acpi_ec {
>  	u8 handlers_installed;
>  } *boot_ec, *first_ec;
>  
> +int acpi_ec_intr = 1; /* Default is interrupt mode */
> +
>  /* 
>   * Some Asus system have exchanged ECDT data/command IO addresses.
>   */
> @@ -516,12 +518,14 @@ static u32 acpi_ec_gpe_handler(void *dat
>  	acpi_status status = AE_OK;
>  	struct acpi_ec *ec = data;
>  	u8 state = acpi_ec_read_status(ec);
> +	static bool warn_done = 0;
>  
>  	pr_debug(PREFIX "~~~> interrupt\n");
>  	atomic_inc(&ec->irq_count);
> -	if (atomic_read(&ec->irq_count) > 5) {
> -		pr_err(PREFIX "GPE storm detected, disabling EC GPE\n");
> -		ec_switch_to_poll_mode(ec);
> +	if (!warn_done && atomic_read(&ec->irq_count) > 5) {
> +		pr_warning(PREFIX "GPE storm detected, try to use ec_intr=0 "
> +			"kernel option, if you see problems with keyboard.\n");
> +		warn_done = 1;
>  		goto end;
>  	}
>  	clear_bit(EC_FLAGS_WAIT_GPE, &ec->flags);
> @@ -848,20 +852,21 @@ static int ec_install_handlers(struct ac
>  	acpi_status status;
>  	if (ec->handlers_installed)
>  		return 0;
> -	status = acpi_install_gpe_handler(NULL, ec->gpe,
> +	if (acpi_ec_intr) {
> +		status = acpi_install_gpe_handler(NULL, ec->gpe,
>  					  ACPI_GPE_EDGE_TRIGGERED,
>  					  &acpi_ec_gpe_handler, ec);
> -	if (ACPI_FAILURE(status))
> -		return -ENODEV;
> -
> -	acpi_set_gpe_type(NULL, ec->gpe, ACPI_GPE_TYPE_RUNTIME);
> -	acpi_enable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
> +		if (ACPI_FAILURE(status))
> +			return -ENODEV;
>  
> +		acpi_set_gpe_type(NULL, ec->gpe, ACPI_GPE_TYPE_RUNTIME);
> +		acpi_enable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
> +	}
>  	status = acpi_install_address_space_handler(ec->handle,
>  						    ACPI_ADR_SPACE_EC,
>  						    &acpi_ec_space_handler,
>  						    NULL, ec);
> -	if (ACPI_FAILURE(status)) {
> +	if (ACPI_FAILURE(status) && acpi_ec_intr) {
>  		acpi_remove_gpe_handler(NULL, ec->gpe, &acpi_ec_gpe_handler);
>  		return -ENODEV;
>  	}
> @@ -1047,3 +1052,14 @@ static void __exit acpi_ec_exit(void)
>  	return;
>  }
>  #endif	/* 0 */
> +
> +static int __init acpi_ec_set_intr_mode(char *str)
> +{
> +	if (!get_option(&str, &acpi_ec_intr)) {
> +		acpi_ec_intr = 0;
> +		return 0;
> +	}
> +	return 1;
> +}
> +
> +__setup("ec_intr=", acpi_ec_set_intr_mode);
> _
> 

Re: RFC: fast transactions in EC [was: a problem about the two patches in bug 10724 & 11428]

[Andrew Morton]

On Mon, 08 Sep 2008 12:19:32 +0400 Alexey Starikovskiy <astarikovskiy@suse.de> wrote:

> yes to both questions. This patch hopefully solves the problem better.
> 

I locally reverted "ACPI: EC: do transaction from interrupt context"
because of the regression which Bartlomiej Zolnierkiewicz
<bzolnier@gmail.com> reported :(

Re: RFC: fast transactions in EC [was: a problem about the two patches in bug 10724 & 11428]

[Alexey Starikovskiy]

Andrew Morton wrote:
> On Mon, 08 Sep 2008 12:19:32 +0400 Alexey Starikovskiy <astarikovskiy@suse.de> wrote:
> 
>> yes to both questions. This patch hopefully solves the problem better.
>>
> 
> I locally reverted "ACPI: EC: do transaction from interrupt context"
> because of the regression which Bartlomiej Zolnierkiewicz
> <bzolnier@gmail.com> reported :(
Could you please forward his message to me?
It seems I don't have it...

Thanks,
Alex.

Re: RFC: fast transactions in EC [was: a problem about the two patches in bug 10724 & 11428]

[Andrew Morton]

On Mon, 08 Sep 2008 12:30:38 +0400 Alexey Starikovskiy <astarikovskiy@suse.de> wrote:

> Andrew Morton wrote:
> > On Mon, 08 Sep 2008 12:19:32 +0400 Alexey Starikovskiy <astarikovskiy@suse.de> wrote:
> > 
> >> yes to both questions. This patch hopefully solves the problem better.
> >>
> > 
> > I locally reverted "ACPI: EC: do transaction from interrupt context"
> > because of the regression which Bartlomiej Zolnierkiewicz
> > <bzolnier@gmail.com> reported :(
> Could you please forward his message to me?
> It seems I don't have it...
> 

hm, you were cc'ed.  Subject: Re: linux-next: Tree for September 5

I'll bounce it over to you.

Re: a problem about the two patches in bug 10724 & 11428

[Alexey Starikovskiy]

Hi Yakui,

Zhao Yakui wrote:
>> If you _disable_ EC GPE or not enable it in first place, EC(hardware) will be working in
>> poll mode. So, there is no spare code to clean up, sorry. 
> Not understand what you said.
It seems to be not my problem...
> After your three patches hit the upstream kernel, the EC GPE is still
> enabled when EC timeout happens. In such case when EC is accessed, EC
> will work in polling mode. At the same time EC GPE interrupt still can
> be triggered.
One of my patches adds option to not enable EC GPE (ec_intr=0), so it is
possible to run EC driver in hardware poll mode, and all the code for such
operation is needed. I do not know how to say it in simpler terms, sorry.
If it is still unclear, please consult with Len... 
Regards,
Alex.

Re: a problem about the two patches in bug 10724 & 11428

[Zhao Yakui]

On Thu, 2008-09-04 at 02:28 +0400, Alexey Starikovskiy wrote:
> Hi Yakui,
> 
> Zhao Yakui wrote:
> >> If you _disable_ EC GPE or not enable it in first place, EC(hardware) will be working in
> >> poll mode. So, there is no spare code to clean up, sorry. 
> > Not understand what you said.
> It seems to be not my problem...
> > After your three patches hit the upstream kernel, the EC GPE is still
> > enabled when EC timeout happens. In such case when EC is accessed, EC
> > will work in polling mode. At the same time EC GPE interrupt still can
> > be triggered.
> One of my patches adds option to not enable EC GPE (ec_intr=0), so it is
> possible to run EC driver in hardware poll mode, and all the code for such
> operation is needed. I do not know how to say it in simpler terms, sorry.
> If it is still unclear, please consult with Len... 
Hi, Alexey
   After investigation I found that the laptop in bug 11428 is the same
as that in bug 8459. Is the EC on such laptop that you mean "optimized"
EC? But in fact the OBF and IBF can reflect the EC status correctly
although sometimes there is no GPE interrupt confirmation.
   
  Accorind to ACPI spec 3.0b, the embedded controller must generate SCIs for commands as follows:
• Read Command (3 Bytes)
   Byte #1     (Command byte Header)          Interrupt on IBF=0 
   Byte #2     (Address byte to read)         No Interrupt
   Byte #3     (Data read to host)            Interrupt on OBF=1
• Write Command (3 Bytes)
  Byte #1     (Command byte Header)          Interrupt on IBF=0
  Byte #2     (Address byte to write)        Interrupt on IBF=0
  Byte #3     (Data to read )                Interrupt on IBF=0
• Query Command (2 Bytes)
  Byte #1     (Command byte Header)          No Interrupt
  Byte #2     (Query value to host)          Interrupt on OBF=1
• Burst Enable Command (2 Bytes)
  Byte #1     (Command byte Header)          No Interrupt
  Byte #2     (Burst acknowledge byte)       Interrupt on OBF=1
• Burst Disable Command (1 Byte)
  Byte #1     (Command byte Header)          Interrupt on IBF=0

But from the log in comment #46 of bug8459 and comment #26 of bug 11428
we can get the following info:
   a. In the course of read command, EC GPE interrupt is triggered only
after read command is issued. Sometimes there is no interrupt after OBF
becomes 1(data ready and Host can read it). 
   b. In the course of query command, sometimes there is no interrupt
after OBF becomes 1(data ready).
   c. In the course of burst disable command, sometimes there is no
interrupt after burst disable command is issued although the IBF is
already zero.(Input buffer is empty)

>From the above log we can know that the EC status is already what OS
expected although there is no interrupt confirmation in some cases. For
example: When issuing read/query command, the OBF already becomes
one(Data ready). When issuing the burst disable command, the IBF already
becomes zero.(EC input buffer is empty).  It means that EC still can
work well by polling EC status to check whether it is what OS expected.
And I believe that is valid for all laptops. If the OBF and IBF can't
reflect the EC status correctly, the windows also can't work well. 

At the same time maybe EC will send a notification event requiring OS's
attention. In such case OS can detect whether the notification event is
sent  by checking the SCI_EVT bit in the EC GPE interrupt service
routine.(I.E . acpi_ec_gpe_handler). In fact only checking notification
event in ec gpe handler is enough to make EC work.

If so, the EC work flowchart will become very simple. I will try to
write this patch and consult with Len.

There is another issue. From the log in  comment #26 of bug 11428 I find
that OS still issues the burst disable command although EC already exits
the burst mode. Maybe we should check whether the EC is in burst mode
before issuing EC burst disable command.
    >ACPI: EC: transaction start
    >[  124.540016] ACPI: EC: <--- command = 0x83
    >[  124.540016] ACPI: EC: ---> status = 0x0a
    >[  124.542009] ACPI: EC: ---> status = 0x08
    >[  124.542009] ACPI: EC: transaction end

Thanks.
  
 
   

> Alex.

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: a problem about the two patches in bug 10724 & 11428

[Alexey Starikovskiy]

Zhao Yakui wrote:
> Hi, Alexey
>    After investigation I found that the laptop in bug 11428 is the same
> as that in bug 8459. Is the EC on such laptop that you mean "optimized"
> EC? But in fact the OBF and IBF can reflect the EC status correctly
> although sometimes there is no GPE interrupt confirmation.
Why you use "but in fact" here? There is no reason to repeat to me my own
findings as a "new truth"...
> At the same time maybe EC will send a notification event requiring OS's
> attention. In such case OS can detect whether the notification event is
> sent  by checking the SCI_EVT bit in the EC GPE interrupt service
> routine.(I.E . acpi_ec_gpe_handler). In fact only checking notification
> event in ec gpe handler is enough to make EC work.
> 
> If so, the EC work flowchart will become very simple. I will try to
> write this patch and consult with Len.
You just described software poll mode of EC driver,
"missing confirmations" are the switch from pure interrupt mode to "the 
poll for IBF/OBF changes, but expect interrupt for SCI change".
Do you plan to drop pure interrupt mode now?
> 
> There is another issue. From the log in  comment #26 of bug 11428 I find
> that OS still issues the burst disable command although EC already exits
> the burst mode. Maybe we should check whether the EC is in burst mode
> before issuing EC burst disable command.
Does it hurt? 
>     >ACPI: EC: transaction start
>     >[  124.540016] ACPI: EC: <--- command = 0x83
>     >[  124.540016] ACPI: EC: ---> status = 0x0a
>     >[  124.542009] ACPI: EC: ---> status = 0x08
>     >[  124.542009] ACPI: EC: transaction end
> 
> Thanks.
>   
>  
>    
> 
>> Alex.
> 

Re: a problem about the two patches in bug 10724 & 11428

[Zhao Yakui]

On Thu, 2008-09-04 at 07:47 +0400, Alexey Starikovskiy wrote:
> Zhao Yakui wrote:
> > Hi, Alexey
> >    After investigation I found that the laptop in bug 11428 is the same
> > as that in bug 8459. Is the EC on such laptop that you mean "optimized"
> > EC? But in fact the OBF and IBF can reflect the EC status correctly
> > although sometimes there is no GPE interrupt confirmation.
> Why you use "but in fact" here? There is no reason to repeat to me my own
> findings as a "new truth"...
> > At the same time maybe EC will send a notification event requiring OS's
> > attention. In such case OS can detect whether the notification event is
> > sent  by checking the SCI_EVT bit in the EC GPE interrupt service
> > routine.(I.E . acpi_ec_gpe_handler). In fact only checking notification
> > event in ec gpe handler is enough to make EC work.
> > 
> > If so, the EC work flowchart will become very simple. I will try to
> > write this patch and consult with Len.
> You just described software poll mode of EC driver,
> "missing confirmations" are the switch from pure interrupt mode to "the 
> poll for IBF/OBF changes, but expect interrupt for SCI change".
> Do you plan to drop pure interrupt mode now?
> > 
> > There is another issue. From the log in  comment #26 of bug 11428 I find
> > that OS still issues the burst disable command although EC already exits
> > the burst mode. Maybe we should check whether the EC is in burst mode
> > before issuing EC burst disable command.
> Does it hurt? 
It is harmless. But it is meaningless that OS still issue burst disable
command after EC exits the burst mode. In theory after burst disable
command is issued, EC GPE interrupt will be triggered. In such case when
obtaining the info of the battery related with EC, there will exist some
meaningless ACPI interrupt.
> >     >ACPI: EC: transaction start
> >     >[  124.540016] ACPI: EC: <--- command = 0x83
> >     >[  124.540016] ACPI: EC: ---> status = 0x0a
> >     >[  124.542009] ACPI: EC: ---> status = 0x08
> >     >[  124.542009] ACPI: EC: transaction end
> > 
> > Thanks.
> >   
> >  
> >    
> > 
> >> Alex.
> > 
>