| Message ID | 5339919D.7090801@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
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.
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,
On Tue, 2014-04-01 at 14:13 +0100, Julien Grall wrote: > >> - 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... Why? If it doesn't exist we don't do anything.
On 04/01/2014 02:23 PM, Ian Campbell wrote: > On Tue, 2014-04-01 at 14:13 +0100, Julien Grall wrote: >>>> - 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... > > Why? If it doesn't exist we don't do anything. > > We can't free in the loop because the action may be used on another CPU at the same it. (see "active" loop at the end). So I still need two variables (one for the loop and one for the real action).
On Tue, 2014-04-01 at 14:52 +0100, Julien Grall wrote: > On 04/01/2014 02:23 PM, Ian Campbell wrote: > > On Tue, 2014-04-01 at 14:13 +0100, Julien Grall wrote: > >>>> - 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... > > > > Why? If it doesn't exist we don't do anything. > > > > > > We can't free in the loop because the action may be used on another CPU > at the same it. (see "active" loop at the end). So I still need two > variables (one for the loop and one for the real action). Perhaps the "goto found" pattern might help here then? Ian.
On 04/01/2014 03:31 PM, Ian Campbell wrote: > On Tue, 2014-04-01 at 14:52 +0100, Julien Grall wrote: >> On 04/01/2014 02:23 PM, Ian Campbell wrote: >>> On Tue, 2014-04-01 at 14:13 +0100, Julien Grall wrote: >>>>>> - 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... >>> >>> Why? If it doesn't exist we don't do anything. >>> >>> >> >> We can't free in the loop because the action may be used on another CPU >> at the same it. (see "active" loop at the end). So I still need two >> variables (one for the loop and one for the real action). > > Perhaps the "goto found" pattern might help here then? I'm not a big fan of "goto". But in this case the code might be cleaner.
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 7ea4da8..f4f5b71 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -545,36 +545,33 @@ void release_irq(unsigned int irq, const void *dev_id) { struct irq_desc *desc; unsigned long flags; - struct irqaction *action, **action_ptr; + struct irqaction *action, *next; desc = irq_to_desc(irq); spin_lock_irqsave(&desc->lock,flags); - desc->handler->shutdown(desc); - action = desc->action; - action_ptr = &desc->action; - for ( ;; ) + action = NULL; + list_for_each_entry(next, &desc->action, next) { - action = *action_ptr; - - if ( !action ) + if ( next->dev_id == dev_id ) { - printk(XENLOG_WARNING "Trying to free already-free IRQ %u\n", irq); - return; - } - - if ( action->dev_id == dev_id ) + action = next; break; + } + } - action_ptr = &action->next; + if ( !action ) + { + printk(XENLOG_WARNING "Trying to free already-free IRQ %u\n", irq); + return; } /* Found it - remove it from the action list */ - *action_ptr = action->next; + list_del_init(&action->next); /* If this was the list action, shut down the IRQ */ - if ( !desc->action ) + if ( list_empty(&desc->action) ) { desc->handler->shutdown(desc); desc->status &= ~IRQ_GUEST; @@ -592,12 +589,10 @@ void release_irq(unsigned int irq, const void *dev_id) static int __setup_irq(struct irq_desc *desc, struct irqaction *new, unsigned int irqflags) { - struct irqaction *action = desc->action; - ASSERT(new != NULL); /* Sanity check if the IRQ already have an action attached */ - if ( action != NULL ) + if ( !list_empty(&desc->action) ) { /* Check that IRQ is marked as shared */ if ( !(desc->status & IRQ_SHARED) || !(irqflags & IRQ_SHARED) ) @@ -610,8 +605,8 @@ static int __setup_irq(struct irq_desc *desc, struct irqaction *new, if ( irqflags & IRQ_SHARED ) desc->status |= IRQ_SHARED; - new->next = desc->action; - desc->action = new; + INIT_LIST_HEAD(&new->next); + list_add_tail(&new->next, &desc->action); dsb(sy); return 0; diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c index d6f3500..8a9ae3d 100644 --- a/xen/arch/arm/irq.c +++ b/xen/arch/arm/irq.c @@ -56,6 +56,7 @@ irq_desc_t *__irq_to_desc(int irq) int __init arch_init_one_irq_desc(struct irq_desc *desc) { desc->arch.type = DT_IRQ_TYPE_NONE; + INIT_LIST_HEAD(&desc->action); return 0; } @@ -151,7 +152,6 @@ int request_irq(unsigned int irq, void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq) { struct irq_desc *desc = irq_to_desc(irq); - struct irqaction *action = desc->action; /* TODO: perfc_incr(irqs); */ @@ -162,7 +162,7 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq) spin_lock(&desc->lock); desc->handler->ack(desc); - if ( action == NULL ) + if ( list_empty(&desc->action) ) { printk("Unknown %s %#3.3x\n", is_fiq ? "FIQ" : "IRQ", irq); @@ -171,6 +171,8 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq) if ( desc->status & IRQ_GUEST ) { + struct irqaction *action = list_entry(&desc->action, struct irqaction, + next); struct domain *d = action->dev_id; desc->handler->end(desc); @@ -194,16 +196,14 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq) desc->status |= IRQ_INPROGRESS; - action = desc->action; while ( desc->status & IRQ_PENDING ) { + struct irqaction *action, *next; + desc->status &= ~IRQ_PENDING; spin_unlock_irq(&desc->lock); - do - { + list_for_each_entry_safe(action, next, &desc->action, next) action->handler(irq, action->dev_id, regs); - action = action->next; - } while ( action ); spin_lock_irq(&desc->lock); } diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h index 20548aa..a926554 100644 --- a/xen/include/xen/irq.h +++ b/xen/include/xen/irq.h @@ -17,7 +17,9 @@ struct irqaction { const char *name; void *dev_id; bool_t free_on_release; - struct irqaction *next; +#ifdef CONFIG_ARM + struct list_head next; +#endif }; /* @@ -73,7 +75,11 @@ typedef struct irq_desc { unsigned int status; /* IRQ status */ hw_irq_controller *handler; struct msi_desc *msi_desc; +#ifdef CONFIG_ARM + struct list_head action; /* IRQ action list */ +#else struct irqaction *action; /* IRQ action list */ +#endif int irq; spinlock_t lock; struct arch_irq_desc arch;