Re: [Xen-devel] [PATCH for-4.5 6/8] xen/arm: IRQ: Add lock contrainst for gic_irq_{startup, shutdown}

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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@xxxxxxxxxx>
> ---
>  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;
>  }
>  



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