Alan Jenkins wrote:
> Alexey Starikovskiy wrote:
>   
>> Alan Jenkins wrote:
>>     
>>> Alexey Starikovskiy wrote:
>>>  
>>>       
>>>> Hi Alan,
>>>>
>>>> Here are the patches. First is updated version of patch you've tried,
>>>> second is
>>>> even more aggressive disabling of GPEs (applies on top).
>>>>
>>>> Please check them.
>>>>
>>>>     
>>>>         
>>> I tested them together and they worked, yay!
>>>
>>> That doesn't say anything about the second patch.  My EC didn't generate
>>> enough spurious interrupts to trigger the STORM code.
>>>
>>> Would you welcome some nitpicks on the first patch at this time?  I
>>> didn't see errors but I have some code-style queries.
>>>   
>>>       
>> Sure. Will be glad.
>>     
>
> My main query was about acpi_ec::t being a pointer.  I thought that
> meant there would be lots of kmalloc / kfrees.  But I see now it is
> allocated on-stack, i.e. it points to a local variable.  Never mind.  It
> is a bit clever though.  I thought the original version was clearer -
> where acpi_ec::t wasn't a pointer. 
>
>   
I had acpi_ec::t as pointer in the very first version of the patch,
then changed it to member to make patch "cleaner", then Rafael
suggested I go with pointer, so I switched back.
> The other niggle was the spinlock.  It would be nice to indicate by
> comment the resources it protects (acpi_ec::t and irq_count).
>
>   
renamed it to t_lock. Hope it's self commenting now...
> Actually, if we're being clever, why not move irq_count into
> transaction_data?  After all, it's only used within the transaction.  It
> fits with both the lifecycle and the locking.
>   
Right, shaved couple of code lines by that :)
> Regards
> Alan
>   

Thanks,
Alex.