Re: [PATCH v3 18/18] xen/arm: IRQ: Handle multiple action per IRQ

[Julien Grall <julien.grall@linaro.org>]

On 04/17/2014 08:07 AM, Jan Beulich wrote:
>>>> On 16.04.14 at 18:23, <julien.grall@linaro.org> wrote:
>> On 04/16/2014 05:17 PM, Ian Campbell wrote:
>>> On Wed, 2014-04-16 at 17:06 +0100, Julien Grall wrote:
>>>> On 04/16/2014 04:54 PM, Ian Campbell wrote:
>>>>> On Tue, 2014-04-08 at 15:44 +0100, Julien Grall wrote:
>>>>>>          desc->status &= ~IRQ_PENDING;
>>>>>>          spin_unlock_irq(&desc->lock);
>>>>>> -        action->handler(irq, action->dev_id, regs);
>>>>>> +        list_for_each_entry_safe(action, next, &desc->action, next)
>>>>>> +            action->handler(irq, action->dev_id, regs);
>>>>>
>>>>> You aren't removing entries from within the loop so I don't think you
>>>>> need the _safe variant.
>>>>
>>>> As we release the desc->lock here, it might be possible to have the list
>>>> changed under the CPU feet by release_irq.
>>>>
>>>> With the double-linked list, how do we make sure that it won't happen?
>>>
>>> Normally by using a lock. I don't know if even list_for_each_entry_safe
>>> is safe against concurrent changes to the list from other threads, I
>>> think it only refers to safe to changing the list within the loop in
>>> this thread.
>>>  
>>
>> Hmmmm... I'm wondering if we can keep desc->lock held while calling the
>> action handler and enable IRQ.
> 
> That would set you up for problems with the handler wanting to
> manipulate its IRQ (which might imply locking desc). I'd suggest
> looking at how Linux deals with this (synchronize_irq() in particular).

The release_irq code is borrowed from Linux. We already synchronize at
the end.

We have to delete the element in the critical section to avoid race with
adding a new element.

synchronizing in the critical section is not possible because do_IRQ
will have to take the lock to clear the IRQ_INPROGRESS flag.

In Linux case, they are safe because they are using a single linked list.

-- 
Julien Grall