Re: [Xen-devel] [PATCH for-next] xen/arm: irq: Don't use _IRQ_PENDING when handling host interrupt

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 28 Jan 2019, Julien Grall wrote:
> While SPIs are shared between CPU, it is not possible to receive the
> same interrupts on a different CPU while the interrupt is in active
> state. The deactivation of the interrupt is done at the end of the
> handling.
> 
> This means the _IRQ_PENDING logic is unecessary on Arm as a same
> interrupt can never come up while in the loop. So remove it to
> simplify the interrupt handle code.
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> ---
>  xen/arch/arm/irq.c | 32 ++++++++++----------------------
>  1 file changed, 10 insertions(+), 22 deletions(-)
> 
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index c51cf333ce..3877657a52 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -199,6 +199,7 @@ int request_irq(unsigned int irq, unsigned int irqflags,
>  void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
>  {
>      struct irq_desc *desc = irq_to_desc(irq);
> +    struct irqaction *action;
>  
>      perfc_incr(irqs);
>  
> @@ -242,35 +243,22 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int 
> irq, int is_fiq)
>          goto out_no_end;
>      }
>  
> -    set_bit(_IRQ_PENDING, &desc->status);
> -
> -    /*
> -     * Since we set PENDING, if another processor is handling a different
> -     * instance of this same irq, the other processor will take care of it.
> -     */
> -    if ( test_bit(_IRQ_DISABLED, &desc->status) ||
> -         test_bit(_IRQ_INPROGRESS, &desc->status) )
> +    if ( test_bit(_IRQ_DISABLED, &desc->status) )
>          goto out;

It is a good idea to remove the IRQ_PENDING logic, that is OK.


However, are we sure that we want to remove the _IRQ_INPROGRESS check
too? IRQ handlers shouldn't be called twice in a row. Given that
_IRQ_INPROGRESS can be set manually (gicv2_set_active_state) it seems it
would be a good idea to keep the check anyway?


>      set_bit(_IRQ_INPROGRESS, &desc->status);
>  
> -    while ( test_bit(_IRQ_PENDING, &desc->status) )
> -    {
> -        struct irqaction *action;
> +    action = desc->action;
>  
> -        clear_bit(_IRQ_PENDING, &desc->status);
> -        action = desc->action;
> +    spin_unlock_irq(&desc->lock);
>  
> -        spin_unlock_irq(&desc->lock);
> -
> -        do
> -        {
> -            action->handler(irq, action->dev_id, regs);
> -            action = action->next;
> -        } while ( action );
> +    do
> +    {
> +        action->handler(irq, action->dev_id, regs);
> +        action = action->next;
> +    } while ( action );
>  
> -        spin_lock_irq(&desc->lock);
> -    }
> +    spin_lock_irq(&desc->lock);
>  
>      clear_bit(_IRQ_INPROGRESS, &desc->status);
>  
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel