| Message ID | 1396557727-19102-10-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, 2014-04-03 at 21:42 +0100, Julien Grall wrote: In the subject "constraint". A better title might be "require $FOO lock be held by callers of gic_irq...blah" > When multiple action will be supported, gic_irq_{startup,shutdown} will have s/will be/are/ > to be called in the same critical zone as setup/release. "critical section" is the more usual term I think. Or "under the same lock as". > Otherwise it could have a race condition if at the same time CPU A is calling "Otherwise there is a race condition..." > release_dt_irq and CPU B is calling setup_dt_irq. > > This could end up to the IRQ not being enabled. s/to/with/ > Signed-off-by: Julien Grall <julien.grall@linaro.org> > > --- > Changes in v2: > - Fix typoes in commit message > - Move this patch earlier in the series => move shutdown() in > release_irq and gic_route_irq > --- > xen/arch/arm/gic.c | 39 ++++++++++++++++++++++++--------------- > xen/arch/arm/irq.c | 6 ++++-- > 2 files changed, 28 insertions(+), 17 deletions(-) > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index 82e0316..8c53e52 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -123,44 +123,53 @@ void gic_restore_state(struct vcpu *v) > gic_restore_pending_irqs(v); > } > > -static void gic_irq_enable(struct irq_desc *desc) > +static unsigned int gic_irq_startup(struct irq_desc *desc) Is there code motion mixed in with this locking change? It looks a bit like the relationship between e.g. gic_irq_startup and gic_irq_enable is being turned inside out? Maybe diff has just chosen an unhelpful representation of a relatively simple change? > -static unsigned int gic_irq_startup(struct irq_desc *desc) > +static void gic_irq_enable(struct irq_desc *desc) > { > - gic_irq_enable(desc); > - return 0; > + unsigned long flags; > + > + spin_lock_irqsave(&desc->lock, flags); > + gic_irq_startup(desc); > + spin_unlock_irqrestore(&desc->lock, flags); > } > > -static void gic_irq_shutdown(struct irq_desc *desc) > +static void gic_irq_disable(struct irq_desc *desc) > { > - gic_irq_disable(desc); > + unsigned long flags; > + > + spin_lock_irqsave(&desc->lock, flags); > + gic_irq_shutdown(desc); > + spin_unlock_irqrestore(&desc->lock, flags); > } > > static void gic_irq_ack(struct irq_desc *desc) > @@ -261,11 +270,11 @@ static int gic_route_irq(unsigned int irq, bool_t level, > if ( desc->action != NULL ) > return -EBUSY; > > + spin_lock_irqsave(&desc->lock, flags); > + > /* Disable interrupt */ > desc->handler->shutdown(desc); > > - spin_lock_irqsave(&desc->lock, flags); Since desc->handler is a generic construct I think it is worth mentioning in the commit log that this is consistent with x86. After this change are arm's locking requirements wrt the hw_irq_controller callbacks now consistent with x86's? Ian.
On 04/07/2014 02:27 PM, Ian Campbell wrote: > On Thu, 2014-04-03 at 21:42 +0100, Julien Grall wrote: > > In the subject "constraint". > > A better title might be "require $FOO lock be held by callers of > gic_irq...blah" I will change the title. >> When multiple action will be supported, gic_irq_{startup,shutdown} will have > > s/will be/are/ > >> to be called in the same critical zone as setup/release. > > "critical section" is the more usual term I think. Or "under the same > lock as". > >> Otherwise it could have a race condition if at the same time CPU A is calling > > "Otherwise there is a race condition..." > >> release_dt_irq and CPU B is calling setup_dt_irq. >> >> This could end up to the IRQ not being enabled. > > s/to/with/ I will fix all the typoes. >> Signed-off-by: Julien Grall <julien.grall@linaro.org> >> >> --- >> Changes in v2: >> - Fix typoes in commit message >> - Move this patch earlier in the series => move shutdown() in >> release_irq and gic_route_irq >> --- >> xen/arch/arm/gic.c | 39 ++++++++++++++++++++++++--------------- >> xen/arch/arm/irq.c | 6 ++++-- >> 2 files changed, 28 insertions(+), 17 deletions(-) >> >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >> index 82e0316..8c53e52 100644 >> --- a/xen/arch/arm/gic.c >> +++ b/xen/arch/arm/gic.c >> @@ -123,44 +123,53 @@ void gic_restore_state(struct vcpu *v) >> gic_restore_pending_irqs(v); >> } >> >> -static void gic_irq_enable(struct irq_desc *desc) >> +static unsigned int gic_irq_startup(struct irq_desc *desc) > > Is there code motion mixed in with this locking change? > > It looks a bit like the relationship between e.g. gic_irq_startup and > gic_irq_enable is being turned inside out? Maybe diff has just chosen an > unhelpful representation of a relatively simple change? [..] > Since desc->handler is a generic construct I think it is worth > mentioning in the commit log that this is consistent with x86. > > After this change are arm's locking requirements wrt the > hw_irq_controller callbacks now consistent with x86's? Before this patch gic_irq_startup was calling gic_irq_enable, now it's flipped. After thinking, I will rework this patch. It's also possible to take the desc->lock outside for irq_enable and irq_startup. So we will be consistent with x86's that AFAIU request desc->lock to be held by the callers. Regards,
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 82e0316..8c53e52 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -123,44 +123,53 @@ void gic_restore_state(struct vcpu *v) gic_restore_pending_irqs(v); } -static void gic_irq_enable(struct irq_desc *desc) +static unsigned int gic_irq_startup(struct irq_desc *desc) { int irq = desc->irq; - unsigned long flags; - spin_lock_irqsave(&desc->lock, flags); + ASSERT(spin_is_locked(&desc->lock)); + ASSERT(!local_irq_is_enabled()); + spin_lock(&gic.lock); desc->status &= ~IRQ_DISABLED; dsb(sy); /* Enable routing */ GICD[GICD_ISENABLER + irq / 32] = (1u << (irq % 32)); spin_unlock(&gic.lock); - spin_unlock_irqrestore(&desc->lock, flags); + + return 0; } -static void gic_irq_disable(struct irq_desc *desc) +static void gic_irq_shutdown(struct irq_desc *desc) { int irq = desc->irq; - unsigned long flags; - spin_lock_irqsave(&desc->lock, flags); + ASSERT(spin_is_locked(&desc->lock)); + ASSERT(!local_irq_is_enabled()); + spin_lock(&gic.lock); /* Disable routing */ GICD[GICD_ICENABLER + irq / 32] = (1u << (irq % 32)); desc->status |= IRQ_DISABLED; spin_unlock(&gic.lock); - spin_unlock_irqrestore(&desc->lock, flags); } -static unsigned int gic_irq_startup(struct irq_desc *desc) +static void gic_irq_enable(struct irq_desc *desc) { - gic_irq_enable(desc); - return 0; + unsigned long flags; + + spin_lock_irqsave(&desc->lock, flags); + gic_irq_startup(desc); + spin_unlock_irqrestore(&desc->lock, flags); } -static void gic_irq_shutdown(struct irq_desc *desc) +static void gic_irq_disable(struct irq_desc *desc) { - gic_irq_disable(desc); + unsigned long flags; + + spin_lock_irqsave(&desc->lock, flags); + gic_irq_shutdown(desc); + spin_unlock_irqrestore(&desc->lock, flags); } static void gic_irq_ack(struct irq_desc *desc) @@ -261,11 +270,11 @@ static int gic_route_irq(unsigned int irq, bool_t level, if ( desc->action != NULL ) return -EBUSY; + spin_lock_irqsave(&desc->lock, flags); + /* Disable interrupt */ desc->handler->shutdown(desc); - spin_lock_irqsave(&desc->lock, flags); - desc->handler = &gic_host_irq_type; gic_set_irq_properties(irq, level, cpu_mask, priority); diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c index 5111b90..798353b 100644 --- a/xen/arch/arm/irq.c +++ b/xen/arch/arm/irq.c @@ -214,9 +214,10 @@ void release_irq(unsigned int irq) desc = irq_to_desc(irq); + spin_lock_irqsave(&desc->lock,flags); + desc->handler->shutdown(desc); - spin_lock_irqsave(&desc->lock,flags); action = desc->action; desc->action = NULL; desc->status &= ~IRQ_GUEST; @@ -251,11 +252,12 @@ int setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) spin_lock_irqsave(&desc->lock, flags); rc = __setup_irq(desc, new); - spin_unlock_irqrestore(&desc->lock, flags); if ( !rc ) desc->handler->startup(desc); + spin_unlock_irqrestore(&desc->lock, flags); + return rc; }
When multiple action will be supported, gic_irq_{startup,shutdown} will have to be called in the same critical zone as setup/release. Otherwise it could have a race condition if at the same time CPU A is calling release_dt_irq and CPU B is calling setup_dt_irq. This could end up to the IRQ not being enabled. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- Changes in v2: - Fix typoes in commit message - Move this patch earlier in the series => move shutdown() in release_irq and gic_route_irq --- xen/arch/arm/gic.c | 39 ++++++++++++++++++++++++--------------- xen/arch/arm/irq.c | 6 ++++-- 2 files changed, 28 insertions(+), 17 deletions(-)