| Message ID | 1398171530-27391-19-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
>>> On 22.04.14 at 14:58, <julien.grall@linaro.org> wrote: > On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same > interrupt. > > To be able to use multiple action, the driver has to explicitly call > {setup,request}_irq with IRQF_SHARED as 2nd parameter. > > The behavior stays the same on x86, e.g only one action is handled. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> For the non-ARM part: Acked-by: Jan Beulich <jbeulich@suse.com> > --- > Changes in v4: > - Go back to a single custom linked list. The double linked-list > doesn't fit the requirements (i.e browsing safely without look) and > the llist.h from Linux doesn't allow use to delete a node in the > middle > of the list. > > Changes in v3: > - Drop {setup,request}_irq_flags, the current function has been > extended on an earlier patch. > - Rename IRQ_SHARED into IRQF_SHARED > > Changes in v2: > - Explain design choice > - Introduce CONFIG_IRQ_HAS_MULTIPLE_ACTION > - Use list instead of custom pointer > - release_irq should not shutdown the IRQ at the beginning > - Add IRQ_SHARED flags > - Introduce request_irq_flags and setup_irq_flags > - Fix compilation on x86. Some code are creating the irqaction > via = { ... } so the "next" field should be moved > toward the end > --- > xen/arch/arm/irq.c | 79 +++++++++++++++++++++++++++++++++--------- > xen/include/asm-arm/config.h | 2 ++ > xen/include/xen/irq.h | 4 +++ > 3 files changed, 68 insertions(+), 17 deletions(-) > > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c > index 1bc960d..d05c0be 100644 > --- a/xen/arch/arm/irq.c > +++ b/xen/arch/arm/irq.c > @@ -155,7 +155,6 @@ int request_irq(unsigned int irq, unsigned int irqflags, > 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); */ > > @@ -166,7 +165,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 ( !desc->action ) > { > printk("Unknown %s %#3.3x\n", > is_fiq ? "FIQ" : "IRQ", irq); > @@ -198,12 +197,21 @@ 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; > + > desc->status &= ~IRQ_PENDING; > + action = desc->action; > + > spin_unlock_irq(&desc->lock); > - action->handler(irq, action->dev_id, regs); > + > + do > + { > + action->handler(irq, action->dev_id, regs); > + action = action->next; > + } while ( action ); > + > spin_lock_irq(&desc->lock); > } > > @@ -220,34 +228,71 @@ void release_irq(unsigned int irq, const void *dev_id) > { > struct irq_desc *desc; > unsigned long flags; > - struct irqaction *action; > + struct irqaction *action, **action_ptr; > > desc = irq_to_desc(irq); > > spin_lock_irqsave(&desc->lock,flags); > > - desc->handler->shutdown(desc); > + action_ptr = &desc->action; > + for ( ;; ) > + { > + action = *action_ptr; > + if ( !action ) > + { > + printk(XENLOG_WARNING "Trying to free already-free IRQ %u\n", > irq); > + spin_unlock_irqrestore(&desc->lock, flags); > + return; > + } > + > + if ( action->dev_id == dev_id ) > + break; > + > + action_ptr = &action->next; > + } > + > + /* Found it - remove it from the action list */ > + *action_ptr = action->next; > > - action = desc->action; > - desc->action = NULL; > - desc->status &= ~IRQ_GUEST; > + /* If this was the last action, shut down the IRQ */ > + if ( !desc->action ) > + { > + desc->handler->shutdown(desc); > + desc->status &= ~IRQ_GUEST; > + } > > spin_unlock_irqrestore(&desc->lock,flags); > > /* Wait to make sure it's not being used on another CPU */ > do { smp_mb(); } while ( desc->status & IRQ_INPROGRESS ); > > - if ( action && action->free_on_release ) > + if ( action->free_on_release ) > xfree(action); > } > > -static int __setup_irq(struct irq_desc *desc, struct irqaction *new) > +static int __setup_irq(struct irq_desc *desc, unsigned int irqflags, > + struct irqaction *new) > { > - if ( desc->action != NULL ) > - return -EBUSY; > + bool_t shared = !!(irqflags & IRQF_SHARED); > + > + ASSERT(new != NULL); > + > + /* Sanity checks: > + * - if the IRQ is marked as shared > + * - dev_id is not NULL when IRQF_SHARED is set > + */ > + if ( desc->action != NULL && (!(desc->status & IRQF_SHARED) || !shared) ) > + return -EINVAL; > + if ( shared && new->dev_id == NULL ) > + return -EINVAL; > + > + if ( shared ) > + desc->status |= IRQF_SHARED; > > - desc->action = new; > - dsb(sy); > + new->next = desc->action; > + dsb(ish); > + desc->action = new; > + dsb(ish); > > return 0; > } > @@ -275,7 +320,7 @@ int setup_irq(unsigned int irq, unsigned int irqflags, > struct irqaction *new) > > disabled = (desc->action == NULL); > > - rc = __setup_irq(desc, new); > + rc = __setup_irq(desc, irqflags, new); > if ( rc ) > goto err; > > @@ -340,7 +385,7 @@ int route_dt_irq_to_guest(struct domain *d, const struct > dt_irq *irq, > goto out; > } > > - retval = __setup_irq(desc, action); > + retval = __setup_irq(desc, 0, action); > if ( retval ) > goto out; > > diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h > index ef291ff..1c3abcf 100644 > --- a/xen/include/asm-arm/config.h > +++ b/xen/include/asm-arm/config.h > @@ -37,6 +37,8 @@ > > #define CONFIG_VIDEO 1 > > +#define CONFIG_IRQ_HAS_MULTIPLE_ACTION 1 > + > #define OPT_CONSOLE_STR "dtuart" > > #ifdef MAX_PHYS_CPUS > diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h > index f9a18d8..40c0f3f 100644 > --- a/xen/include/xen/irq.h > +++ b/xen/include/xen/irq.h > @@ -14,6 +14,9 @@ struct irqaction { > const char *name; > void *dev_id; > bool_t free_on_release; > +#ifdef CONFIG_IRQ_HAS_MULTIPLE_ACTION > + struct irqaction *next; > +#endif > }; > > /* > @@ -27,6 +30,7 @@ struct irqaction { > #define IRQ_MOVE_PENDING (1u<<5) /* IRQ is migrating to another CPUs */ > #define IRQ_PER_CPU (1u<<6) /* IRQ is per CPU */ > #define IRQ_GUEST_EOI_PENDING (1u<<7) /* IRQ was disabled, pending a guest > EOI */ > +#define IRQF_SHARED (1<<8) /* IRQ is shared */ > > /* Special IRQ numbers. */ > #define AUTO_ASSIGN_IRQ (-1) > -- > 1.7.10.4
On Tue, 2014-04-22 at 13:58 +0100, Julien Grall wrote: > On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same > interrupt. > > To be able to use multiple action, the driver has to explicitly call > {setup,request}_irq with IRQF_SHARED as 2nd parameter. > > The behavior stays the same on x86, e.g only one action is handled. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > Cc: Keir Fraser <keir@xen.org> > Cc: Jan Beulich <jbeulich@suse.com> > > --- > Changes in v4: > - Go back to a single custom linked list. The double linked-list > doesn't fit the requirements (i.e browsing safely without look) and > the llist.h from Linux doesn't allow use to delete a node in the middle > of the list. Is this variant any safer? > + do > + { > + action->handler(irq, action->dev_id, regs); > + action = action->next; > + } while ( action ); What happens if action is freed and recycled in the midst of this loop? Ian.
On 04/23/2014 03:07 PM, Ian Campbell wrote: > On Tue, 2014-04-22 at 13:58 +0100, Julien Grall wrote: >> On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same >> interrupt. >> >> To be able to use multiple action, the driver has to explicitly call >> {setup,request}_irq with IRQF_SHARED as 2nd parameter. >> >> The behavior stays the same on x86, e.g only one action is handled. >> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> >> Cc: Keir Fraser <keir@xen.org> >> Cc: Jan Beulich <jbeulich@suse.com> >> >> --- >> Changes in v4: >> - Go back to a single custom linked list. The double linked-list >> doesn't fit the requirements (i.e browsing safely without look) and >> the llist.h from Linux doesn't allow use to delete a node in the middle >> of the list. > > Is this variant any safer? Yes, everything is protected by a desc->lock, except in do_IRQ (see below why). >> + do >> + { >> + action->handler(irq, action->dev_id, regs); >> + action = action->next; >> + } while ( action ); > > What happens if action is freed and recycled in the midst of this loop? Nothing, the action won't be free until IRQ_INPROGRESS is set (see do .. while at the end of release_irq). Regards,
On Wed, 2014-04-23 at 15:56 +0100, Julien Grall wrote: > >> + do > >> + { > >> + action->handler(irq, action->dev_id, regs); > >> + action = action->next; > >> + } while ( action ); > > > > What happens if action is freed and recycled in the midst of this loop? > > Nothing, the action won't be free until IRQ_INPROGRESS is set (see do .. > while at the end of release_irq). BY that logic wasn't the version which used the list macros also safe then? Ian.
On 04/23/2014 04:16 PM, Ian Campbell wrote: > On Wed, 2014-04-23 at 15:56 +0100, Julien Grall wrote: >>>> + do >>>> + { >>>> + action->handler(irq, action->dev_id, regs); >>>> + action = action->next; >>>> + } while ( action ); >>> >>> What happens if action is freed and recycled in the midst of this loop? >> >> Nothing, the action won't be free until IRQ_INPROGRESS is set (see do .. >> while at the end of release_irq). > > BY that logic wasn't the version which used the list macros also safe > then? No because we had to modify two pointers (next and prev). Here as we modify only one pointer, do_IRQ may or may not call action for a last time.
On 04/23/2014 04:16 PM, Ian Campbell wrote: > On Wed, 2014-04-23 at 15:56 +0100, Julien Grall wrote: >>>> + do >>>> + { >>>> + action->handler(irq, action->dev_id, regs); >>>> + action = action->next; >>>> + } while ( action ); >>> >>> What happens if action is freed and recycled in the midst of this loop? >> >> Nothing, the action won't be free until IRQ_INPROGRESS is set (see do .. >> while at the end of release_irq). > > BY that logic wasn't the version which used the list macros also safe > then? No because we had to modify two pointers (next and prev). Here as we modify only one pointer, do_IRQ may or may not call action for a last time.
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c index 1bc960d..d05c0be 100644 --- a/xen/arch/arm/irq.c +++ b/xen/arch/arm/irq.c @@ -155,7 +155,6 @@ int request_irq(unsigned int irq, unsigned int irqflags, 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); */ @@ -166,7 +165,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 ( !desc->action ) { printk("Unknown %s %#3.3x\n", is_fiq ? "FIQ" : "IRQ", irq); @@ -198,12 +197,21 @@ 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; + desc->status &= ~IRQ_PENDING; + action = desc->action; + spin_unlock_irq(&desc->lock); - action->handler(irq, action->dev_id, regs); + + do + { + action->handler(irq, action->dev_id, regs); + action = action->next; + } while ( action ); + spin_lock_irq(&desc->lock); } @@ -220,34 +228,71 @@ void release_irq(unsigned int irq, const void *dev_id) { struct irq_desc *desc; unsigned long flags; - struct irqaction *action; + struct irqaction *action, **action_ptr; desc = irq_to_desc(irq); spin_lock_irqsave(&desc->lock,flags); - desc->handler->shutdown(desc); + action_ptr = &desc->action; + for ( ;; ) + { + action = *action_ptr; + if ( !action ) + { + printk(XENLOG_WARNING "Trying to free already-free IRQ %u\n", irq); + spin_unlock_irqrestore(&desc->lock, flags); + return; + } + + if ( action->dev_id == dev_id ) + break; + + action_ptr = &action->next; + } + + /* Found it - remove it from the action list */ + *action_ptr = action->next; - action = desc->action; - desc->action = NULL; - desc->status &= ~IRQ_GUEST; + /* If this was the last action, shut down the IRQ */ + if ( !desc->action ) + { + desc->handler->shutdown(desc); + desc->status &= ~IRQ_GUEST; + } spin_unlock_irqrestore(&desc->lock,flags); /* Wait to make sure it's not being used on another CPU */ do { smp_mb(); } while ( desc->status & IRQ_INPROGRESS ); - if ( action && action->free_on_release ) + if ( action->free_on_release ) xfree(action); } -static int __setup_irq(struct irq_desc *desc, struct irqaction *new) +static int __setup_irq(struct irq_desc *desc, unsigned int irqflags, + struct irqaction *new) { - if ( desc->action != NULL ) - return -EBUSY; + bool_t shared = !!(irqflags & IRQF_SHARED); + + ASSERT(new != NULL); + + /* Sanity checks: + * - if the IRQ is marked as shared + * - dev_id is not NULL when IRQF_SHARED is set + */ + if ( desc->action != NULL && (!(desc->status & IRQF_SHARED) || !shared) ) + return -EINVAL; + if ( shared && new->dev_id == NULL ) + return -EINVAL; + + if ( shared ) + desc->status |= IRQF_SHARED; - desc->action = new; - dsb(sy); + new->next = desc->action; + dsb(ish); + desc->action = new; + dsb(ish); return 0; } @@ -275,7 +320,7 @@ int setup_irq(unsigned int irq, unsigned int irqflags, struct irqaction *new) disabled = (desc->action == NULL); - rc = __setup_irq(desc, new); + rc = __setup_irq(desc, irqflags, new); if ( rc ) goto err; @@ -340,7 +385,7 @@ int route_dt_irq_to_guest(struct domain *d, const struct dt_irq *irq, goto out; } - retval = __setup_irq(desc, action); + retval = __setup_irq(desc, 0, action); if ( retval ) goto out; diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h index ef291ff..1c3abcf 100644 --- a/xen/include/asm-arm/config.h +++ b/xen/include/asm-arm/config.h @@ -37,6 +37,8 @@ #define CONFIG_VIDEO 1 +#define CONFIG_IRQ_HAS_MULTIPLE_ACTION 1 + #define OPT_CONSOLE_STR "dtuart" #ifdef MAX_PHYS_CPUS diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h index f9a18d8..40c0f3f 100644 --- a/xen/include/xen/irq.h +++ b/xen/include/xen/irq.h @@ -14,6 +14,9 @@ struct irqaction { const char *name; void *dev_id; bool_t free_on_release; +#ifdef CONFIG_IRQ_HAS_MULTIPLE_ACTION + struct irqaction *next; +#endif }; /* @@ -27,6 +30,7 @@ struct irqaction { #define IRQ_MOVE_PENDING (1u<<5) /* IRQ is migrating to another CPUs */ #define IRQ_PER_CPU (1u<<6) /* IRQ is per CPU */ #define IRQ_GUEST_EOI_PENDING (1u<<7) /* IRQ was disabled, pending a guest EOI */ +#define IRQF_SHARED (1<<8) /* IRQ is shared */ /* Special IRQ numbers. */ #define AUTO_ASSIGN_IRQ (-1)
On ARM, it may happen (eg ARM SMMU) to setup multiple handler for the same interrupt. To be able to use multiple action, the driver has to explicitly call {setup,request}_irq with IRQF_SHARED as 2nd parameter. The behavior stays the same on x86, e.g only one action is handled. Signed-off-by: Julien Grall <julien.grall@linaro.org> Cc: Keir Fraser <keir@xen.org> Cc: Jan Beulich <jbeulich@suse.com> --- Changes in v4: - Go back to a single custom linked list. The double linked-list doesn't fit the requirements (i.e browsing safely without look) and the llist.h from Linux doesn't allow use to delete a node in the middle of the list. Changes in v3: - Drop {setup,request}_irq_flags, the current function has been extended on an earlier patch. - Rename IRQ_SHARED into IRQF_SHARED Changes in v2: - Explain design choice - Introduce CONFIG_IRQ_HAS_MULTIPLE_ACTION - Use list instead of custom pointer - release_irq should not shutdown the IRQ at the beginning - Add IRQ_SHARED flags - Introduce request_irq_flags and setup_irq_flags - Fix compilation on x86. Some code are creating the irqaction via = { ... } so the "next" field should be moved toward the end --- xen/arch/arm/irq.c | 79 +++++++++++++++++++++++++++++++++--------- xen/include/asm-arm/config.h | 2 ++ xen/include/xen/irq.h | 4 +++ 3 files changed, 68 insertions(+), 17 deletions(-)