Re: [Xen-devel] [PATCH v2 16/16] xen/arm: IRQ: Handle multiple action per IRQ

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.04.14 at 22:42, <julien.grall@xxxxxxxxxx> wrote:
> --- a/xen/include/asm-arm/irq.h
> +++ b/xen/include/asm-arm/irq.h
> @@ -45,6 +45,13 @@ int route_dt_irq_to_guest(struct domain *d, const struct 
> dt_irq *irq,
>  unsigned int platform_get_irq(const struct dt_device_node *device,
>                                int index);
>  
> +int request_irq_flags(unsigned int irq,
> +                      void (*handler)(int, void *, struct cpu_user_regs *),
> +                      unsigned int irqflags,
> +                      const char * devname, void *dev_id);
> +int setup_irq_flags(unsigned int irq, struct irqaction *new,
> +                    unsigned int irqflags);
> +

I think it is a bad choice to have separate ..._flags() versions of these,
the normal functions should take flags at least when
CONFIG_IRQ_HAS_MULTIPLE_ACTION. Perhaps, with the expectation
of wanting to add further flags in the future, it would be reasonable to
have the functions always have the flags parameters, and assert the
value passed is zero on x86.

> @@ -27,6 +30,7 @@ struct irqaction {
>  #define IRQ_MOVE_PENDING  (1u<<5) /* IRQ is migrating to another CPUs */
>  #define IRQ_PER_CPU       (1u<<6) /* IRQ is per CPU */
>  #define IRQ_GUEST_EOI_PENDING (1u<<7) /* IRQ was disabled, pending a guest 
> EOI */
> +#define IRQ_SHARED        (1<<8)  /* IRQ is shared */

This is now given two meanings (input to above functions and status),
which in the long run can become problematic. Please follow Linux by
using two distinct flag sets.

Jan


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