Re: [PATCH] ACPI: EC: do transaction from interrupt context

Alexey Starikovskiy
2008-09-26T10:42:39Z

Alan,

Could you please uncomment DEBUG and send dmesg after application of the patch.
I put the "storm detected" message under debug, so it is not visible in normal situation.
I think, that interrupt storm is still detected on your machine, and workaround works.

Regards,
Alex.

Alan Jenkins wrote:
> Alexey Starikovskiy wrote:
>> Sitsofe Wheeler wrote:
>>> Zhao Yakui wrote:
>>>> I think that the problem on the asus-EEEPC can't be resolved really
>>>> by the Alexey's patch. IMO it is only lucky.
>>> How lucky? It really does go from excruciatingly slow to quite fast
>>> on that laptop. What will happen should my luck fail? What are the
>>> odds of my luck failing?
>>>
>>>> In fact the main problem on Asus-EEEPC is related with the broken
>>>> EC.
>>>> Before an EC notification event is processed, another EC notification
>>>> event arrives again.
>>>> If EC driver check whether the SCI_EVT bit is set after processing
>>>> one EC notification event, the problem will be resolved.
>>>> http://bugzilla.kernel.org/show_bug.cgi?id=11089
>>>> Alan Jenkins already sent a patch about how to fix the issue on the
>>>> Asus-EEEPC.
>
> I submitted alternative patches in the past, but they were not up to
> scratch. The first patch stopped the EeePC dropping events, but left
> them arriving in bursts every 0.5 seconds - which is still a UI
> regression; it looks even more weird when you hold down a brightness
> key. My other patches fixed the EeePC, but caused severe regressions on
> other hardware.
>
> So I have stopped pushing any patches of my own. In the interest of
> removing this regression as soon as reasonably possible, I will not
> object to changes that actually work.
>
>>> I'll cc Alan and see what he makes of this patch.
>>>
>
>> Alan already tested my patch and it works for him.

>
> Yup. I agree with Zhao, in that it is not crystal clear why this new
> patch helps.
>
> The new patch includes a fallback which requires polling-driven
> transactions. That's a different type of polling fallback to the
> previous one. However, from the experience of testing a range of
> patches, I believe the new polling fallback would also drop events. At
> face value, the trigger for the polling fallback is the same - 20
> "false" (surplus to requirements) interrupts, so it shouldn't work :-).
>
> I can think of two possibilities
>
> 1. Doing more in the interrupt handler leaves the interrupt disabled for
> longer, hiding some of the false interrupts (which arrive in bursts).
>
> 2. Maybe when the device is serviced (e.g. a data byte is read), it
> stops the current burst of false interrupts.
>
>>>> But if the above patch is merged , maybe it will break some laptops.
>>> Fair enough but can those people with such laptops test the patch too
>>> and report back? Can you say in what form the breakages will take?
>>> From what you are saying it sounds like this patch shouldn't have had
>>> any affect for me but it does. What happens when they try it? What
>>> happens when you try it?
>>>
>
> Alan