Re: [PATCH for-4.5 7/8] xen/irq: Handle multiple action per IRQ

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

On 04/01/2014 01:29 PM, Ian Campbell wrote:
> On Mon, 2014-03-31 at 17:02 +0100, Julien Grall wrote:
>> On 03/31/2014 04:53 PM, Ian Campbell wrote:
>>> On Mon, 2014-03-31 at 16:45 +0100, Julien Grall wrote:
>>>> On 02/19/2014 11:55 AM, Ian Campbell wrote:
>>>>> On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote:
>>>>>> On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same
>>>>>> interrupt.
>>>>>
>>>>> Mention here that you are therefore creating a linked list of actions
>>>>> for each interrupt.
>>>>>
>>>>> If you use xen/list.h for this then you get a load of helpers and
>>>>> iterators which would save you open coding them.
>>>>
>>>> I've tried to use xen/list.h. The amount of code it's basically the same
>>>> and we I have to write open code to get the first element of the list
>>>
>>> Why? Can you post your WIP patch please for comparison.
>>
>> Because:
>> 	- there is no helper to get the first element (__setup_irq)
> 
> Wrong function? I don't see any problem in __setup_irq.

__setup_irq has to check if every action has a dev_id. After thinking, I
don't need to get the first action as now we have IRQ_SHARED flags set.

> 
>> 	- I need to use 2 variables to search for an element in a list as there is
>> 	no way to know after the end of the loop if we found or not an element.
> 
> You've written that a bit weirdly IMHO.
> 
> list_for_each(...)
>    if (not the one we want)
> 	continue
>    free the one we wanted
>    break;
> 
> don't worry about warning on a non-existent IRQ, or set a simple
> boolean.

We have to worry about non-existent action otherwise Xen may segfault...

> It really doesn't look that bad to me.

Ok. I will continue on this way then.

> [...]
>> -    struct irqaction *next;
>> +#ifdef CONFIG_ARM
>> +    struct list_head next;
>> +#endif
> [...]
>> +#ifdef CONFIG_ARM
>> +    struct list_head action;    /* IRQ action list */
>> +#else
>>      struct irqaction *action;   /* IRQ action list */
>> +#endif
> 
> Now these might be a legitimate problem with this approach. At the very
> least this should be CONFIG_IRQ_HAS_IRQ_ACTION_LIST or some more
> suitable name.

Ok. I will use it.

Regards,

-- 
Julien Grall