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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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.

>       - 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.

Or refactor into a find_irq_by_devid helper and use that here.

> Below an incremental patch which change next field to a list (doesn't compile
> and not finished):

It really doesn't look that bad to me.

[...]
> -    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.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel