Re: [PATCH v7 12/28] xen/arm: ITS: Plumbing hw_irq_controller for LPIs

[Julien Grall <julien.grall@citrix.com>]

Hi Vijay,

Title: I would replace "Plumbing" by "Plumb"

On 18/09/15 14:08, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> Change callbacks gic_host_irq_type and gic_guest_irq_type
> to gic_get_host_irq_type and gic_get_guest_irq_type
> in gic_hw_operations, which returns hw_irq_controller based
> on irq type (SPI or LPI).
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>

With the title and 2 remarks below fixed:

Reviewed-by: Julien Grall <julien.grall@citrix.com>

[...]

> +static inline hw_irq_controller *get_host_hw_irq_controller(unsigned int irq)
> +{
> +    return gic_hw_ops->gic_get_host_irq_type(irq);
> +}
> +
> +static inline hw_irq_controller *get_guest_hw_irq_controller(unsigned int irq)
> +{
> +    return gic_hw_ops->gic_get_guest_irq_type(irq);
> +}
> +

Each of these helpers are used in a single call-site and doesn't bring
much improvement. I would prefer to see those two helpers dropped in
favor of open-coding the call to the callback.

>  /*
>   * needs to be called with a valid cpu_mask, ie each cpu in the mask has
>   * already called gic_cpu_init
> @@ -128,7 +138,7 @@ void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask,
>      ASSERT(test_bit(_IRQ_DISABLED, &desc->status));
>      ASSERT(spin_is_locked(&desc->lock));
>  
> -    desc->handler = gic_hw_ops->gic_host_irq_type;
> +    desc->handler = get_host_hw_irq_controller(desc->irq);
>      gic_set_irq_properties(desc, cpu_mask, priority);
>  }
> @@ -159,7 +169,7 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
>           test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
>          goto out;
>  
> -    desc->handler = gic_hw_ops->gic_guest_irq_type;
> +    desc->handler = get_guest_hw_irq_controller(desc->irq);
>      set_bit(_IRQ_GUEST, &desc->status);
>  
>      gic_set_irq_properties(desc, cpumask_of(v_target->processor), priority);
> diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h
> index f5fba49..4455178 100644
> --- a/xen/include/asm-arm/gic-its.h
> +++ b/xen/include/asm-arm/gic-its.h
> @@ -275,6 +275,8 @@ struct its_device {
>      struct rb_node          node;
>  };
>  
> +extern const hw_irq_controller its_host_lpi_type;
> +extern const hw_irq_controller its_guest_lpi_type;

Newline here.

>  void irqdesc_set_lpi_event(struct irq_desc *desc, unsigned id);
>  unsigned int irqdesc_get_lpi_event(struct irq_desc *desc);
>  struct its_device *irqdesc_get_its_device(struct irq_desc *desc);

Regards,

-- 
Julien Grall