| Message ID | 1390581822-32624-7-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote: > When multiple action will be supported, gic_irq_{startup,shutdown} will have > to be called in the same critical zone has setup/release. s/has/as/? > 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 is not enabled. "This could end up with the IRQ not being enabled." > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > --- > xen/arch/arm/gic.c | 40 +++++++++++++++++++++++++--------------- > 1 file changed, 25 insertions(+), 15 deletions(-) > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index 2643b46..ebd2b5f 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -129,43 +129,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) unsigned? What are the error codes here going to be? > { > 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(); > /* 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; > > - spin_lock(&desc->lock); > + 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(&desc->lock); > } > > -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) > @@ -528,9 +538,8 @@ void release_dt_irq(const struct dt_irq *irq, const void *dev_id) > > desc = irq_to_desc(irq->irq); > > - desc->handler->shutdown(desc); > - > spin_lock_irqsave(&desc->lock,flags); > + desc->handler->shutdown(desc); > action = desc->action; > desc->action = NULL; > desc->status &= ~IRQ_GUEST; > @@ -600,11 +609,12 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) > } > > rc = __setup_irq(desc, irq->irq, new); > - spin_unlock_irqrestore(&desc->lock, flags); > > if ( !rc ) > desc->handler->startup(desc); > > + spin_unlock_irqrestore(&desc->lock, flags); > + > return rc; > } >
Hi Ian, On 02/19/2014 11:51 AM, Ian Campbell wrote: > On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote: >> When multiple action will be supported, gic_irq_{startup,shutdown} will have >> to be called in the same critical zone has setup/release. > > s/has/as/? Yes. >> 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 is not enabled. > > "This could end up with the IRQ not being enabled." > >> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> >> --- >> xen/arch/arm/gic.c | 40 +++++++++++++++++++++++++--------------- >> 1 file changed, 25 insertions(+), 15 deletions(-) >> >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >> index 2643b46..ebd2b5f 100644 >> --- a/xen/arch/arm/gic.c >> +++ b/xen/arch/arm/gic.c >> @@ -129,43 +129,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) > > unsigned? What are the error codes here going to be? This is the return type requested by hw_interrupt_type.startup. It seems that the return is never checked (even in x86 code). Maybe we should change the prototype of hw_interrupt_type.startup. Cheers,
On Wed, 2014-02-19 at 14:35 +0000, Julien Grall wrote: > >> -static void gic_irq_enable(struct irq_desc *desc) > >> +static unsigned int gic_irq_startup(struct irq_desc *desc) > > > > unsigned? What are the error codes here going to be? > > This is the return type requested by hw_interrupt_type.startup. > > It seems that the return is never checked (even in x86 code). Maybe we > should change the prototype of hw_interrupt_type.startup. Worth investigating. I wonder if someone thought this might return the resulting interrupt number (those are normally unsigned int I think) or if it actually did used to etc. Ian.
Adding Keir and Jan. On 02/19/2014 02:38 PM, Ian Campbell wrote: > On Wed, 2014-02-19 at 14:35 +0000, Julien Grall wrote: > >>>> -static void gic_irq_enable(struct irq_desc *desc) >>>> +static unsigned int gic_irq_startup(struct irq_desc *desc) >>> >>> unsigned? What are the error codes here going to be? >> >> This is the return type requested by hw_interrupt_type.startup. >> >> It seems that the return is never checked (even in x86 code). Maybe we >> should change the prototype of hw_interrupt_type.startup. > > Worth investigating. I wonder if someone thought this might return the > resulting interrupt number (those are normally unsigned int I think) or > if it actually did used to etc. I think it was copied from Linux which also have unsigned int. I gave a quick look to the code and this callback is only used in 2 places which always return 0. Surprisingly, the wrapper irq_startup (kernel/irq/manage.c) is returning an int... I can create a patch to return void instead of unsigned if everyone is happy with this solution.
On 02/19/2014 03:07 PM, Jan Beulich wrote: >>>> On 19.02.14 at 15:51, Julien Grall <julien.grall@linaro.org> wrote: >> Adding Keir and Jan. >> >> On 02/19/2014 02:38 PM, Ian Campbell wrote: >>> On Wed, 2014-02-19 at 14:35 +0000, Julien Grall wrote: >>> >>>>>> -static void gic_irq_enable(struct irq_desc *desc) >>>>>> +static unsigned int gic_irq_startup(struct irq_desc *desc) >>>>> >>>>> unsigned? What are the error codes here going to be? >>>> >>>> This is the return type requested by hw_interrupt_type.startup. >>>> >>>> It seems that the return is never checked (even in x86 code). Maybe we >>>> should change the prototype of hw_interrupt_type.startup. >>> >>> Worth investigating. I wonder if someone thought this might return the >>> resulting interrupt number (those are normally unsigned int I think) or >>> if it actually did used to etc. >> >> I think it was copied from Linux which also have unsigned int. I gave a >> quick look to the code and this callback is only used in 2 places which >> always return 0. >> >> Surprisingly, the wrapper irq_startup (kernel/irq/manage.c) is returning >> an int... >> >> I can create a patch to return void instead of unsigned if everyone is >> happy with this solution. > > I'd be fine with such a change; I'd like to ask though that if you > do this, you at the same time do the resulting possible cleanup: > As an example, xen/arch/x86/msi.c:startup_msi_irq() becomes > unnecessary then. It will in fact be interesting to see how many > distinct startup routines actually remain. Sure, I will give a look at it.
Hi Jan, On 02/19/2014 03:07 PM, Jan Beulich wrote: >>>> On 19.02.14 at 15:51, Julien Grall <julien.grall@linaro.org> wrote: >> Adding Keir and Jan. >> >> On 02/19/2014 02:38 PM, Ian Campbell wrote: >>> On Wed, 2014-02-19 at 14:35 +0000, Julien Grall wrote: >>> >>>>>> -static void gic_irq_enable(struct irq_desc *desc) >>>>>> +static unsigned int gic_irq_startup(struct irq_desc *desc) >>>>> >>>>> unsigned? What are the error codes here going to be? >>>> >>>> This is the return type requested by hw_interrupt_type.startup. >>>> >>>> It seems that the return is never checked (even in x86 code). Maybe we >>>> should change the prototype of hw_interrupt_type.startup. >>> >>> Worth investigating. I wonder if someone thought this might return the >>> resulting interrupt number (those are normally unsigned int I think) or >>> if it actually did used to etc. >> >> I think it was copied from Linux which also have unsigned int. I gave a >> quick look to the code and this callback is only used in 2 places which >> always return 0. >> >> Surprisingly, the wrapper irq_startup (kernel/irq/manage.c) is returning >> an int... >> >> I can create a patch to return void instead of unsigned if everyone is >> happy with this solution. > > I'd be fine with such a change; I'd like to ask though that if you > do this, you at the same time do the resulting possible cleanup: > As an example, xen/arch/x86/msi.c:startup_msi_irq() becomes > unnecessary then. It will in fact be interesting to see how many > distinct startup routines actually remain. Before the clean up there was 8 distinct startup routines for x86. No there is only 2: - drivers/passthrough/amd/iommu_init.c: iommu_maskable_msi_startup - arch/x86/ioapic.c: startup_edge_ioapic_irq For a latter one, I'm a bit surprised that the function can return 1, but the result is never used. Cheers,
On 02/21/2014 08:55 AM, Jan Beulich wrote: >>>> On 20.02.14 at 21:48, Julien Grall <julien.grall@linaro.org> wrote: >> Before the clean up there was 8 distinct startup routines for x86. No >> there is only 2: >> - drivers/passthrough/amd/iommu_init.c: iommu_maskable_msi_startup >> - arch/x86/ioapic.c: startup_edge_ioapic_irq >> >> For a latter one, I'm a bit surprised that the function can return 1, >> but the result is never used. > > Which means consumption of the return value was intended, but > never implemented (or lost _very_ long ago). Looking at the Linux > code, the intention apparently would be for the non-zero return > value to propagate into IRQ_PENDING in one very special case we > didn't ever support (auto-probing). Re-sending of an already > pending interrupt is being handled differently there anyway. So if > needed something like the setting of IRQ_PENDING at some point, > I guess we could as well have the startup routine do this itself. I.e. > I think converting the return value to void is still fine, as long as > you leave some commentary in > arch/x86/ioapic.c:startup_edge_ioapic_irq(). I will send the patch to change startup prototype separately later.
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 2643b46..ebd2b5f 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -129,43 +129,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(); /* 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; - spin_lock(&desc->lock); + 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(&desc->lock); } -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) @@ -528,9 +538,8 @@ void release_dt_irq(const struct dt_irq *irq, const void *dev_id) desc = irq_to_desc(irq->irq); - desc->handler->shutdown(desc); - spin_lock_irqsave(&desc->lock,flags); + desc->handler->shutdown(desc); action = desc->action; desc->action = NULL; desc->status &= ~IRQ_GUEST; @@ -600,11 +609,12 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new) } rc = __setup_irq(desc, irq->irq, 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 has 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 is not enabled. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- xen/arch/arm/gic.c | 40 +++++++++++++++++++++++++--------------- 1 file changed, 25 insertions(+), 15 deletions(-)