[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 09/16] xen/arm: IRQ: Add lock contrainst for gic_irq_{startup, shutdown}
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@xxxxxxxxxx> >> >> --- >> 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, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |