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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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@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?

> -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.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel