Re: [PATCH v2 09/16] xen/arm: IRQ: Add lock contrainst for gic_irq_{startup, shutdown}

[Julien Grall <julien.grall@linaro.org>]

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,

-- 
Julien Grall