[XEN PATCH 0/3] IRQ: address violations of MISRA C:2012 Rules 8.2 and 8.3

[XEN PATCH 0/3] IRQ: address violations of MISRA C:2012 Rules 8.2 and 8.3

[Federico Serafini]

This patch serie addresses violations of rules 8.2 and 8.3 related to the IRQ
module.
Changes are divided by architecture.
No functional changes.

Federico Serafini (3):
  xen/irq: add missing parameter names
  xen/arm: irq: address violations of MISRA C: 2012 Rules 8.2 and 8.3
  x86/irq: address violations of MISRA C:2012 Rules 8.2 and 8.3

 xen/arch/arm/irq.c             |  7 ++--
 xen/arch/x86/include/asm/irq.h | 32 +++++++++---------
 xen/arch/x86/irq.c             | 28 ++++++++--------
 xen/include/xen/irq.h          | 59 +++++++++++++++++-----------------
 xen/include/xen/softirq.h      |  2 +-
 5 files changed, 65 insertions(+), 63 deletions(-)

-- 
2.34.1

[XEN PATCH 1/3] xen/irq: add missing parameter names

[Federico Serafini]

Add missing parameter names thus addressing violations of MISRA C:2012
Rule 8.2: "Function types shall be in prototype form with named
parameters".

No functional changes.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/include/xen/irq.h     | 59 ++++++++++++++++++++-------------------
 xen/include/xen/softirq.h |  2 +-
 2 files changed, 31 insertions(+), 30 deletions(-)

diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
index 300625e56d..7cc6917513 100644
--- a/xen/include/xen/irq.h
+++ b/xen/include/xen/irq.h
@@ -18,7 +18,7 @@
     ASSERT(!in_irq() && (local_irq_is_enabled() || num_online_cpus() <= 1))
 
 struct irqaction {
-    void (*handler)(int, void *, struct cpu_user_regs *);
+    void (*handler)(int irq, void *data, struct cpu_user_regs *regs);
     const char *name;
     void *dev_id;
     bool_t free_on_release;
@@ -58,21 +58,21 @@ struct irq_desc;
 
 /*
  * Interrupt controller descriptor. This is all we need
- * to describe about the low-level hardware. 
+ * to describe about the low-level hardware.
  */
 struct hw_interrupt_type {
     const char *typename;
-    unsigned int (*startup)(struct irq_desc *);
-    void (*shutdown)(struct irq_desc *);
-    void (*enable)(struct irq_desc *);
-    void (*disable)(struct irq_desc *);
-    void (*ack)(struct irq_desc *);
+    unsigned int (*startup)(struct irq_desc *desc);
+    void (*shutdown)(struct irq_desc *desc);
+    void (*enable)(struct irq_desc *desc);
+    void (*disable)(struct irq_desc *desc);
+    void (*ack)(struct irq_desc *desc);
 #ifdef CONFIG_X86
-    void (*end)(struct irq_desc *, u8 vector);
+    void (*end)(struct irq_desc *desc, u8 vector);
 #else
-    void (*end)(struct irq_desc *);
+    void (*end)(struct irq_desc *desc);
 #endif
-    void (*set_affinity)(struct irq_desc *, const cpumask_t *);
+    void (*set_affinity)(struct irq_desc *desc, const cpumask_t *mask);
 };
 
 typedef const struct hw_interrupt_type hw_irq_controller;
@@ -110,22 +110,23 @@ typedef struct irq_desc {
 #define irq_to_desc(irq)    (&irq_desc[irq])
 #endif
 
-int init_one_irq_desc(struct irq_desc *);
-int arch_init_one_irq_desc(struct irq_desc *);
+int init_one_irq_desc(struct irq_desc *desc);
+int arch_init_one_irq_desc(struct irq_desc *desc);
 
 #define irq_desc_initialized(desc) ((desc)->handler != NULL)
 
 extern int setup_irq(unsigned int irq, unsigned int irqflags,
-                     struct irqaction *);
+                     struct irqaction *new);
 extern void release_irq(unsigned int irq, const void *dev_id);
 extern int request_irq(unsigned int irq, unsigned int irqflags,
-               void (*handler)(int, void *, struct cpu_user_regs *),
+               void (*handler)(int irq, void *dev_id,
+                     struct cpu_user_regs *regs),
                const char * devname, void *dev_id);
 
 extern hw_irq_controller no_irq_type;
 void cf_check no_action(int cpl, void *dev_id, struct cpu_user_regs *regs);
-unsigned int cf_check irq_startup_none(struct irq_desc *);
-void cf_check irq_actor_none(struct irq_desc *);
+unsigned int cf_check irq_startup_none(struct irq_desc *desc);
+void cf_check irq_actor_none(struct irq_desc *desc);
 #define irq_shutdown_none irq_actor_none
 #define irq_disable_none irq_actor_none
 #define irq_enable_none irq_actor_none
@@ -146,7 +147,7 @@ struct pirq {
 #define pirq_info(d, p) ((struct pirq *)radix_tree_lookup(&(d)->pirq_tree, p))
 
 /* Use this instead of pirq_info() if the structure may need allocating. */
-extern struct pirq *pirq_get_info(struct domain *, int pirq);
+extern struct pirq *pirq_get_info(struct domain *d, int pirq);
 
 #define pirq_field(d, p, f, def) ({ \
     const struct pirq *__pi = pirq_info(d, p); \
@@ -155,30 +156,30 @@ extern struct pirq *pirq_get_info(struct domain *, int pirq);
 #define pirq_to_evtchn(d, pirq) pirq_field(d, pirq, evtchn, 0)
 #define pirq_masked(d, pirq) pirq_field(d, pirq, masked, 0)
 
-void pirq_cleanup_check(struct pirq *, struct domain *);
+void pirq_cleanup_check(struct pirq *pirq, struct domain *d);
 
 #define pirq_cleanup_check(pirq, d) \
     ((pirq)->evtchn ? pirq_cleanup_check(pirq, d) : (void)0)
 
-extern void pirq_guest_eoi(struct pirq *);
-extern void desc_guest_eoi(struct irq_desc *, struct pirq *);
+extern void pirq_guest_eoi(struct pirq *pirq);
+extern void desc_guest_eoi(struct irq_desc *desc, struct pirq *pirq);
 extern int pirq_guest_unmask(struct domain *d);
-extern int pirq_guest_bind(struct vcpu *, struct pirq *, int will_share);
-extern void pirq_guest_unbind(struct domain *d, struct pirq *);
-extern void pirq_set_affinity(struct domain *d, int irq, const cpumask_t *);
-extern irq_desc_t *domain_spin_lock_irq_desc(
+extern int pirq_guest_bind(struct vcpu *v, struct pirq *pirq, int will_share);
+extern void pirq_guest_unbind(struct domain *d, struct pirq *pirq);
+extern void pirq_set_affinity(struct domain *d, int irq, const cpumask_t *mask);
+extern struct irq_desc *domain_spin_lock_irq_desc(
     struct domain *d, int irq, unsigned long *pflags);
-extern irq_desc_t *pirq_spin_lock_irq_desc(
-    const struct pirq *, unsigned long *pflags);
+extern struct irq_desc *pirq_spin_lock_irq_desc(
+    const struct pirq *pirq, unsigned long *pflags);
 
-unsigned int set_desc_affinity(struct irq_desc *, const cpumask_t *);
+unsigned int set_desc_affinity(struct irq_desc *desc, const cpumask_t *mask);
 
 #ifndef arch_hwdom_irqs
-unsigned int arch_hwdom_irqs(domid_t);
+unsigned int arch_hwdom_irqs(domid_t domid);
 #endif
 
 #ifndef arch_evtchn_bind_pirq
-void arch_evtchn_bind_pirq(struct domain *, int pirq);
+void arch_evtchn_bind_pirq(struct domain *d, int pirq);
 #endif
 
 #endif /* __XEN_IRQ_H__ */
diff --git a/xen/include/xen/softirq.h b/xen/include/xen/softirq.h
index 1f6c4783da..33d6f2ecd2 100644
--- a/xen/include/xen/softirq.h
+++ b/xen/include/xen/softirq.h
@@ -26,7 +26,7 @@ typedef void (*softirq_handler)(void);
 void do_softirq(void);
 void open_softirq(int nr, softirq_handler handler);
 
-void cpumask_raise_softirq(const cpumask_t *, unsigned int nr);
+void cpumask_raise_softirq(const cpumask_t *mask, unsigned int nr);
 void cpu_raise_softirq(unsigned int cpu, unsigned int nr);
 void raise_softirq(unsigned int nr);
 
-- 
2.34.1

[XEN PATCH 2/3] xen/arm: irq: address violations of MISRA C: 2012 Rules 8.2 and 8.3

[Federico Serafini]

Give a name to unnamed parameters thus addressing violations of
MISRA C:2012 Rule 8.2 ("Function types shall be in prototype form with
named parameters").
Keep consistency between parameter names and types used in function
declarations and the ones used in the corresponding function
definitions, thus addressing violations of MISRA C:2012 Rule 8.3
("All declarations of an object or function shall use the same names
and type qualifiers").

No functional changes.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/arch/arm/irq.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 16e56f8945..335e06a2a7 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -58,7 +58,7 @@ hw_irq_controller no_irq_type = {
 static irq_desc_t irq_desc[NR_IRQS];
 static DEFINE_PER_CPU(irq_desc_t[NR_LOCAL_IRQS], local_irq_desc);
 
-irq_desc_t *__irq_to_desc(int irq)
+struct irq_desc *__irq_to_desc(int irq)
 {
     if ( irq < NR_LOCAL_IRQS )
         return &this_cpu(local_irq_desc)[irq];
@@ -182,7 +182,8 @@ void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask)
 }
 
 int request_irq(unsigned int irq, unsigned int irqflags,
-                void (*handler)(int, void *, struct cpu_user_regs *),
+                void (*handler)(int irq, void *dev_id,
+                                struct cpu_user_regs *regs),
                 const char *devname, void *dev_id)
 {
     struct irqaction *action;
@@ -617,7 +618,7 @@ void pirq_guest_unbind(struct domain *d, struct pirq *pirq)
     BUG();
 }
 
-void pirq_set_affinity(struct domain *d, int pirq, const cpumask_t *mask)
+void pirq_set_affinity(struct domain *d, int irq, const cpumask_t *mask)
 {
     BUG();
 }
-- 
2.34.1

[XEN PATCH 3/3] x86/irq: address violations of MISRA C:2012 Rules 8.2 and 8.3

[Federico Serafini]

Give a name to unnamed parameters thus addressing violations of
MISRA C:2012 Rule 8.2 ("Function types shall be in prototype form with
named parameters").
Keep consistency between parameter names and types used in function
declarations and the ones used in the corresponding function
definitions, thus addressing violations of MISRA C:2012 Rule 8.3
("All declarations of an object or function shall use the same names
and type qualifiers").

No functional changes.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/arch/x86/include/asm/irq.h | 32 ++++++++++++++++----------------
 xen/arch/x86/irq.c             | 28 ++++++++++++++--------------
 2 files changed, 30 insertions(+), 30 deletions(-)

diff --git a/xen/arch/x86/include/asm/irq.h b/xen/arch/x86/include/asm/irq.h
index 3f95dd39b7..bb8b7ff2cc 100644
--- a/xen/arch/x86/include/asm/irq.h
+++ b/xen/arch/x86/include/asm/irq.h
@@ -101,18 +101,18 @@ void cf_check irq_move_cleanup_interrupt(struct cpu_user_regs *regs);
 uint8_t alloc_hipriority_vector(void);
 
 void set_direct_apic_vector(
-    uint8_t vector, void (*handler)(struct cpu_user_regs *));
+    uint8_t vector, void (*handler)(struct cpu_user_regs *regs));
 void alloc_direct_apic_vector(
-    uint8_t *vector, void (*handler)(struct cpu_user_regs *));
+    uint8_t *vector, void (*handler)(struct cpu_user_regs *regs));
 
 void do_IRQ(struct cpu_user_regs *regs);
 
-void cf_check disable_8259A_irq(struct irq_desc *);
-void cf_check enable_8259A_irq(struct irq_desc *);
+void cf_check disable_8259A_irq(struct irq_desc *desc);
+void cf_check enable_8259A_irq(struct irq_desc *desc);
 int i8259A_irq_pending(unsigned int irq);
 void mask_8259A(void);
 void unmask_8259A(void);
-void init_8259A(int aeoi);
+void init_8259A(int auto_eoi);
 void make_8259A_irq(unsigned int irq);
 bool bogus_8259A_irq(unsigned int irq);
 int i8259A_suspend(void);
@@ -148,9 +148,9 @@ int map_domain_pirq(struct domain *d, int pirq, int irq, int type,
                            void *data);
 int unmap_domain_pirq(struct domain *d, int pirq);
 int get_free_pirq(struct domain *d, int type);
-int get_free_pirqs(struct domain *, unsigned int nr);
+int get_free_pirqs(struct domain *d, unsigned int nr);
 void free_domain_pirqs(struct domain *d);
-int map_domain_emuirq_pirq(struct domain *d, int pirq, int irq);
+int map_domain_emuirq_pirq(struct domain *d, int pirq, int emuirq);
 int unmap_domain_pirq_emuirq(struct domain *d, int pirq);
 
 /* Reset irq affinities to match the given CPU mask. */
@@ -168,9 +168,9 @@ int irq_to_vector(int irq);
  */
 int create_irq(nodeid_t node, bool grant_access);
 void destroy_irq(unsigned int irq);
-int assign_irq_vector(int irq, const cpumask_t *);
+int assign_irq_vector(int irq, const cpumask_t *mask);
 
-void cf_check irq_complete_move(struct irq_desc *);
+void cf_check irq_complete_move(struct irq_desc *desc);
 
 extern struct irq_desc *irq_desc;
 
@@ -179,16 +179,16 @@ void unlock_vector_lock(void);
 
 void setup_vector_irq(unsigned int cpu);
 
-void move_native_irq(struct irq_desc *);
-void move_masked_irq(struct irq_desc *);
+void move_native_irq(struct irq_desc *desc);
+void move_masked_irq(struct irq_desc *desc);
 
-int bind_irq_vector(int irq, int vector, const cpumask_t *);
+int bind_irq_vector(int irq, int vector, const cpumask_t *mask);
 
-void cf_check end_nonmaskable_irq(struct irq_desc *, uint8_t vector);
-void irq_set_affinity(struct irq_desc *, const cpumask_t *mask);
+void cf_check end_nonmaskable_irq(struct irq_desc *desc, uint8_t vector);
+void irq_set_affinity(struct irq_desc *desc, const cpumask_t *mask);
 
-int init_domain_irq_mapping(struct domain *);
-void cleanup_domain_irq_mapping(struct domain *);
+int init_domain_irq_mapping(struct domain *d);
+void cleanup_domain_irq_mapping(struct domain *d);
 
 #define domain_pirq_to_irq(d, pirq) pirq_field(d, pirq, arch.irq, 0)
 #define domain_irq_to_pirq(d, irq) ({                           \
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 20150b1c7f..c2ec1182f1 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -148,14 +148,14 @@ static void trace_irq_mask(uint32_t event, int irq, int vector,
 }
 
 static int __init _bind_irq_vector(struct irq_desc *desc, int vector,
-                                   const cpumask_t *cpu_mask)
+                                   const cpumask_t *mask)
 {
     cpumask_t online_mask;
     int cpu;
 
     BUG_ON((unsigned)vector >= X86_NR_VECTORS);
 
-    cpumask_and(&online_mask, cpu_mask, &cpu_online_map);
+    cpumask_and(&online_mask, mask, &cpu_online_map);
     if (cpumask_empty(&online_mask))
         return -EINVAL;
     if ( (desc->arch.vector == vector) &&
@@ -177,7 +177,7 @@ static int __init _bind_irq_vector(struct irq_desc *desc, int vector,
     return 0;
 }
 
-int __init bind_irq_vector(int irq, int vector, const cpumask_t *cpu_mask)
+int __init bind_irq_vector(int irq, int vector, const cpumask_t *mask)
 {
     struct irq_desc *desc = irq_to_desc(irq);
     unsigned long flags;
@@ -187,7 +187,7 @@ int __init bind_irq_vector(int irq, int vector, const cpumask_t *cpu_mask)
 
     spin_lock_irqsave(&desc->lock, flags);
     spin_lock(&vector_lock);
-    ret = _bind_irq_vector(desc, vector, cpu_mask);
+    ret = _bind_irq_vector(desc, vector, mask);
     spin_unlock(&vector_lock);
     spin_unlock_irqrestore(&desc->lock, flags);
 
@@ -893,10 +893,10 @@ void irq_set_affinity(struct irq_desc *desc, const cpumask_t *mask)
     desc->status |= IRQ_MOVE_PENDING;
 }
 
-void pirq_set_affinity(struct domain *d, int pirq, const cpumask_t *mask)
+void pirq_set_affinity(struct domain *d, int irq, const cpumask_t *mask)
 {
     unsigned long flags;
-    struct irq_desc *desc = domain_spin_lock_irq_desc(d, pirq, &flags);
+    struct irq_desc *desc = domain_spin_lock_irq_desc(d, irq, &flags);
 
     if ( !desc )
         return;
@@ -915,16 +915,16 @@ uint8_t alloc_hipriority_vector(void)
     return next++;
 }
 
-static void (*direct_apic_vector[X86_NR_VECTORS])(struct cpu_user_regs *);
+static void (*direct_apic_vector[X86_NR_VECTORS])(struct cpu_user_regs *regs);
 void set_direct_apic_vector(
-    uint8_t vector, void (*handler)(struct cpu_user_regs *))
+    uint8_t vector, void (*handler)(struct cpu_user_regs *regs))
 {
     BUG_ON(direct_apic_vector[vector] != NULL);
     direct_apic_vector[vector] = handler;
 }
 
 void alloc_direct_apic_vector(
-    uint8_t *vector, void (*handler)(struct cpu_user_regs *))
+    uint8_t *vector, void (*handler)(struct cpu_user_regs *regs))
 {
     static DEFINE_SPINLOCK(lock);
 
@@ -964,7 +964,7 @@ static int __init cf_check irq_ratelimit_init(void)
 __initcall(irq_ratelimit_init);
 
 int __init request_irq(unsigned int irq, unsigned int irqflags,
-        void (*handler)(int, void *, struct cpu_user_regs *),
+        void (*handler)(int irq, void *dev_id, struct cpu_user_regs *regs),
         const char * devname, void *dev_id)
 {
     struct irqaction * action;
@@ -1194,9 +1194,9 @@ static void cf_check irq_guest_eoi_timer_fn(void *data)
  * to the per-domain irq-to-vector mapping.
  */
 struct irq_desc *domain_spin_lock_irq_desc(
-    struct domain *d, int pirq, unsigned long *pflags)
+    struct domain *d, int irq, unsigned long *pflags)
 {
-    const struct pirq *info = pirq_info(d, pirq);
+    const struct pirq *info = pirq_info(d, irq);
 
     return info ? pirq_spin_lock_irq_desc(info, pflags) : NULL;
 }
@@ -1525,14 +1525,14 @@ static int irq_acktype(const struct irq_desc *desc)
     return 0;
 }
 
-int pirq_shared(struct domain *d, int pirq)
+int pirq_shared(struct domain *d, int irq)
 {
     struct irq_desc    *desc;
     const irq_guest_action_t *action;
     unsigned long       flags;
     int                 shared;
 
-    desc = domain_spin_lock_irq_desc(d, pirq, &flags);
+    desc = domain_spin_lock_irq_desc(d, irq, &flags);
     if ( desc == NULL )
         return 0;
 
-- 
2.34.1

Re: [XEN PATCH 2/3] xen/arm: irq: address violations of MISRA C: 2012 Rules 8.2 and 8.3

[Julien Grall]

Hi,

On 24/07/2023 18:50, Federico Serafini wrote:
> Give a name to unnamed parameters thus addressing violations of
> MISRA C:2012 Rule 8.2 ("Function types shall be in prototype form with
> named parameters").
> Keep consistency between parameter names and types used in function
> declarations and the ones used in the corresponding function
> definitions, thus addressing violations of MISRA C:2012 Rule 8.3
> ("All declarations of an object or function shall use the same names
> and type qualifiers").
> 
> No functional changes.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

I think it would be better if this is folded in patch #1 where you 
modify the prototype.

Cheers,

-- 
Julien Grall

Re: [XEN PATCH 1/3] xen/irq: add missing parameter names

[Stefano Stabellini]

On Mon, 24 Jul 2023, Federico Serafini wrote:
> -extern int pirq_guest_bind(struct vcpu *, struct pirq *, int will_share);
> -extern void pirq_guest_unbind(struct domain *d, struct pirq *);
> -extern void pirq_set_affinity(struct domain *d, int irq, const cpumask_t *);
> -extern irq_desc_t *domain_spin_lock_irq_desc(
> +extern int pirq_guest_bind(struct vcpu *v, struct pirq *pirq, int will_share);
> +extern void pirq_guest_unbind(struct domain *d, struct pirq *pirq);
> +extern void pirq_set_affinity(struct domain *d, int irq, const cpumask_t *mask);

This should be:
void pirq_set_affinity(struct domain *d, int pirq, const cpumask_t *mask)


Everything else looks good to me

Re: [XEN PATCH 2/3] xen/arm: irq: address violations of MISRA C: 2012 Rules 8.2 and 8.3

[Stefano Stabellini]

On Mon, 24 Jul 2023, Federico Serafini wrote:
> Give a name to unnamed parameters thus addressing violations of
> MISRA C:2012 Rule 8.2 ("Function types shall be in prototype form with
> named parameters").
> Keep consistency between parameter names and types used in function
> declarations and the ones used in the corresponding function
> definitions, thus addressing violations of MISRA C:2012 Rule 8.3
> ("All declarations of an object or function shall use the same names
> and type qualifiers").
> 
> No functional changes.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
> ---
>  xen/arch/arm/irq.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 16e56f8945..335e06a2a7 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -58,7 +58,7 @@ hw_irq_controller no_irq_type = {
>  static irq_desc_t irq_desc[NR_IRQS];
>  static DEFINE_PER_CPU(irq_desc_t[NR_LOCAL_IRQS], local_irq_desc);
>  
> -irq_desc_t *__irq_to_desc(int irq)
> +struct irq_desc *__irq_to_desc(int irq)
>  {
>      if ( irq < NR_LOCAL_IRQS )
>          return &this_cpu(local_irq_desc)[irq];
> @@ -182,7 +182,8 @@ void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask)
>  }
>  
>  int request_irq(unsigned int irq, unsigned int irqflags,
> -                void (*handler)(int, void *, struct cpu_user_regs *),
> +                void (*handler)(int irq, void *dev_id,
> +                                struct cpu_user_regs *regs),

We have an inconsistency where the handler functions on x86 typically
call it void *data, while on arm they typically use void *dev_id
(see xen/arch/x86/irq.c:request_irq and
xen/arch/x86/hpet.c:hpet_interrupt_handler). I think we should be
consistent. Or, if this is not a MISRA requirement because this is just
a function pointer rather than a proper function, then I would leave it
alone.


>                  const char *devname, void *dev_id)
>  {
>      struct irqaction *action;
> @@ -617,7 +618,7 @@ void pirq_guest_unbind(struct domain *d, struct pirq *pirq)
>      BUG();
>  }
>  
> -void pirq_set_affinity(struct domain *d, int pirq, const cpumask_t *mask)
> +void pirq_set_affinity(struct domain *d, int irq, const cpumask_t *mask)

I think we should leave it as is because there is also the x86
implementation of pirq_set_affinity that uses int pirq as parameter. It
is not a good idea to introduce inconsistencies between the x86 and the
ARM versions of the same function.

Re: [XEN PATCH 3/3] x86/irq: address violations of MISRA C:2012 Rules 8.2 and 8.3

[Stefano Stabellini]

On Mon, 24 Jul 2023, Federico Serafini wrote:
> Give a name to unnamed parameters thus addressing violations of
> MISRA C:2012 Rule 8.2 ("Function types shall be in prototype form with
> named parameters").
> Keep consistency between parameter names and types used in function
> declarations and the ones used in the corresponding function
> definitions, thus addressing violations of MISRA C:2012 Rule 8.3
> ("All declarations of an object or function shall use the same names
> and type qualifiers").
> 
> No functional changes.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
> ---
>  xen/arch/x86/include/asm/irq.h | 32 ++++++++++++++++----------------
>  xen/arch/x86/irq.c             | 28 ++++++++++++++--------------
>  2 files changed, 30 insertions(+), 30 deletions(-)
> 
> diff --git a/xen/arch/x86/include/asm/irq.h b/xen/arch/x86/include/asm/irq.h
> index 3f95dd39b7..bb8b7ff2cc 100644
> --- a/xen/arch/x86/include/asm/irq.h
> +++ b/xen/arch/x86/include/asm/irq.h
> @@ -101,18 +101,18 @@ void cf_check irq_move_cleanup_interrupt(struct cpu_user_regs *regs);
>  uint8_t alloc_hipriority_vector(void);
>  
>  void set_direct_apic_vector(
> -    uint8_t vector, void (*handler)(struct cpu_user_regs *));
> +    uint8_t vector, void (*handler)(struct cpu_user_regs *regs));
>  void alloc_direct_apic_vector(
> -    uint8_t *vector, void (*handler)(struct cpu_user_regs *));
> +    uint8_t *vector, void (*handler)(struct cpu_user_regs *regs));
>  
>  void do_IRQ(struct cpu_user_regs *regs);
>  
> -void cf_check disable_8259A_irq(struct irq_desc *);
> -void cf_check enable_8259A_irq(struct irq_desc *);
> +void cf_check disable_8259A_irq(struct irq_desc *desc);
> +void cf_check enable_8259A_irq(struct irq_desc *desc);
>  int i8259A_irq_pending(unsigned int irq);
>  void mask_8259A(void);
>  void unmask_8259A(void);
> -void init_8259A(int aeoi);
> +void init_8259A(int auto_eoi);
>  void make_8259A_irq(unsigned int irq);
>  bool bogus_8259A_irq(unsigned int irq);
>  int i8259A_suspend(void);
> @@ -148,9 +148,9 @@ int map_domain_pirq(struct domain *d, int pirq, int irq, int type,
>                             void *data);
>  int unmap_domain_pirq(struct domain *d, int pirq);
>  int get_free_pirq(struct domain *d, int type);
> -int get_free_pirqs(struct domain *, unsigned int nr);
> +int get_free_pirqs(struct domain *d, unsigned int nr);
>  void free_domain_pirqs(struct domain *d);
> -int map_domain_emuirq_pirq(struct domain *d, int pirq, int irq);
> +int map_domain_emuirq_pirq(struct domain *d, int pirq, int emuirq);
>  int unmap_domain_pirq_emuirq(struct domain *d, int pirq);
>  
>  /* Reset irq affinities to match the given CPU mask. */
> @@ -168,9 +168,9 @@ int irq_to_vector(int irq);
>   */
>  int create_irq(nodeid_t node, bool grant_access);
>  void destroy_irq(unsigned int irq);
> -int assign_irq_vector(int irq, const cpumask_t *);
> +int assign_irq_vector(int irq, const cpumask_t *mask);
>  
> -void cf_check irq_complete_move(struct irq_desc *);
> +void cf_check irq_complete_move(struct irq_desc *desc);
>  
>  extern struct irq_desc *irq_desc;
>  
> @@ -179,16 +179,16 @@ void unlock_vector_lock(void);
>  
>  void setup_vector_irq(unsigned int cpu);
>  
> -void move_native_irq(struct irq_desc *);
> -void move_masked_irq(struct irq_desc *);
> +void move_native_irq(struct irq_desc *desc);
> +void move_masked_irq(struct irq_desc *desc);
>  
> -int bind_irq_vector(int irq, int vector, const cpumask_t *);
> +int bind_irq_vector(int irq, int vector, const cpumask_t *mask);
>  
> -void cf_check end_nonmaskable_irq(struct irq_desc *, uint8_t vector);
> -void irq_set_affinity(struct irq_desc *, const cpumask_t *mask);
> +void cf_check end_nonmaskable_irq(struct irq_desc *desc, uint8_t vector);
> +void irq_set_affinity(struct irq_desc *desc, const cpumask_t *mask);
>  
> -int init_domain_irq_mapping(struct domain *);
> -void cleanup_domain_irq_mapping(struct domain *);
> +int init_domain_irq_mapping(struct domain *d);
> +void cleanup_domain_irq_mapping(struct domain *d);
>  
>  #define domain_pirq_to_irq(d, pirq) pirq_field(d, pirq, arch.irq, 0)
>  #define domain_irq_to_pirq(d, irq) ({                           \
> diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> index 20150b1c7f..c2ec1182f1 100644
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -148,14 +148,14 @@ static void trace_irq_mask(uint32_t event, int irq, int vector,
>  }
>  
>  static int __init _bind_irq_vector(struct irq_desc *desc, int vector,
> -                                   const cpumask_t *cpu_mask)
> +                                   const cpumask_t *mask)
>  {
>      cpumask_t online_mask;
>      int cpu;
>  
>      BUG_ON((unsigned)vector >= X86_NR_VECTORS);
>  
> -    cpumask_and(&online_mask, cpu_mask, &cpu_online_map);
> +    cpumask_and(&online_mask, mask, &cpu_online_map);
>      if (cpumask_empty(&online_mask))
>          return -EINVAL;
>      if ( (desc->arch.vector == vector) &&
> @@ -177,7 +177,7 @@ static int __init _bind_irq_vector(struct irq_desc *desc, int vector,
>      return 0;
>  }
>  
> -int __init bind_irq_vector(int irq, int vector, const cpumask_t *cpu_mask)
> +int __init bind_irq_vector(int irq, int vector, const cpumask_t *mask)
>  {
>      struct irq_desc *desc = irq_to_desc(irq);
>      unsigned long flags;
> @@ -187,7 +187,7 @@ int __init bind_irq_vector(int irq, int vector, const cpumask_t *cpu_mask)
>  
>      spin_lock_irqsave(&desc->lock, flags);
>      spin_lock(&vector_lock);
> -    ret = _bind_irq_vector(desc, vector, cpu_mask);
> +    ret = _bind_irq_vector(desc, vector, mask);
>      spin_unlock(&vector_lock);
>      spin_unlock_irqrestore(&desc->lock, flags);
>  
> @@ -893,10 +893,10 @@ void irq_set_affinity(struct irq_desc *desc, const cpumask_t *mask)
>      desc->status |= IRQ_MOVE_PENDING;
>  }
>  
> -void pirq_set_affinity(struct domain *d, int pirq, const cpumask_t *mask)
> +void pirq_set_affinity(struct domain *d, int irq, const cpumask_t *mask)

I welcome feedback from the other maintainers on this but I would keep
the original "pirq" parameter name here...


>  {
>      unsigned long flags;
> -    struct irq_desc *desc = domain_spin_lock_irq_desc(d, pirq, &flags);
> +    struct irq_desc *desc = domain_spin_lock_irq_desc(d, irq, &flags);
>  
>      if ( !desc )
>          return;
> @@ -915,16 +915,16 @@ uint8_t alloc_hipriority_vector(void)
>      return next++;
>  }
>  
> -static void (*direct_apic_vector[X86_NR_VECTORS])(struct cpu_user_regs *);
> +static void (*direct_apic_vector[X86_NR_VECTORS])(struct cpu_user_regs *regs);
>  void set_direct_apic_vector(
> -    uint8_t vector, void (*handler)(struct cpu_user_regs *))
> +    uint8_t vector, void (*handler)(struct cpu_user_regs *regs))
>  {
>      BUG_ON(direct_apic_vector[vector] != NULL);
>      direct_apic_vector[vector] = handler;
>  }
>  
>  void alloc_direct_apic_vector(
> -    uint8_t *vector, void (*handler)(struct cpu_user_regs *))
> +    uint8_t *vector, void (*handler)(struct cpu_user_regs *regs))
>  {
>      static DEFINE_SPINLOCK(lock);
>  
> @@ -964,7 +964,7 @@ static int __init cf_check irq_ratelimit_init(void)
>  __initcall(irq_ratelimit_init);
>  
>  int __init request_irq(unsigned int irq, unsigned int irqflags,
> -        void (*handler)(int, void *, struct cpu_user_regs *),
> +        void (*handler)(int irq, void *dev_id, struct cpu_user_regs *regs),
>          const char * devname, void *dev_id)

I think hpet_interrupt_handler should be adapted for consistency



>  {
>      struct irqaction * action;
> @@ -1194,9 +1194,9 @@ static void cf_check irq_guest_eoi_timer_fn(void *data)
>   * to the per-domain irq-to-vector mapping.
>   */
>  struct irq_desc *domain_spin_lock_irq_desc(
> -    struct domain *d, int pirq, unsigned long *pflags)
> +    struct domain *d, int irq, unsigned long *pflags)
>  {
> -    const struct pirq *info = pirq_info(d, pirq);
> +    const struct pirq *info = pirq_info(d, irq);
>  
>      return info ? pirq_spin_lock_irq_desc(info, pflags) : NULL;
>  }

... and here


> @@ -1525,14 +1525,14 @@ static int irq_acktype(const struct irq_desc *desc)
>      return 0;
>  }
>  
> -int pirq_shared(struct domain *d, int pirq)
> +int pirq_shared(struct domain *d, int irq)

and here


>  {
>      struct irq_desc    *desc;
>      const irq_guest_action_t *action;
>      unsigned long       flags;
>      int                 shared;
>  
> -    desc = domain_spin_lock_irq_desc(d, pirq, &flags);
> +    desc = domain_spin_lock_irq_desc(d, irq, &flags);

and here

I change the declarations as needed



>      if ( desc == NULL )
>          return 0;
>  
> -- 
> 2.34.1
> 
> 

Re: [XEN PATCH 2/3] xen/arm: irq: address violations of MISRA C: 2012 Rules 8.2 and 8.3

[Jan Beulich]

On 24.07.2023 19:50, Federico Serafini wrote:
> @@ -182,7 +182,8 @@ void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask)
>  }
>  
>  int request_irq(unsigned int irq, unsigned int irqflags,
> -                void (*handler)(int, void *, struct cpu_user_regs *),
> +                void (*handler)(int irq, void *dev_id,
> +                                struct cpu_user_regs *regs),
>                  const char *devname, void *dev_id)
>  {

Before we accept patches, don't we need to first settle on whether to
apply the rule(s) also to function type declarations (and not just
ordinary prototypes)?

Jan

Re: [XEN PATCH 3/3] x86/irq: address violations of MISRA C:2012 Rules 8.2 and 8.3

[Jan Beulich]

On 25.07.2023 01:08, Stefano Stabellini wrote:
> On Mon, 24 Jul 2023, Federico Serafini wrote:
>> @@ -893,10 +893,10 @@ void irq_set_affinity(struct irq_desc *desc, const cpumask_t *mask)
>>      desc->status |= IRQ_MOVE_PENDING;
>>  }
>>  
>> -void pirq_set_affinity(struct domain *d, int pirq, const cpumask_t *mask)
>> +void pirq_set_affinity(struct domain *d, int irq, const cpumask_t *mask)
> 
> I welcome feedback from the other maintainers on this but I would keep
> the original "pirq" parameter name here...

+2

We absolutely should not further increase the misnaming. Instead the goal
needs to be to uniformly use pirq when pIRQ (used in interfacing with
guests) is meant, and irq when a (Xen internal) IRQ is meant. Sadly this
isn't helped by Arm not knowing the concept of pIRQ (see "[PATCH v2 0/2]
new CONFIG_HAS_PIRQ and extra_guest_irqs adjustment").

Jan

Re: [XEN PATCH 2/3] xen/arm: irq: address violations of MISRA C: 2012 Rules 8.2 and 8.3

[Stefano Stabellini]

On Tue, 25 Jul 2023, Jan Beulich wrote:
> On 24.07.2023 19:50, Federico Serafini wrote:
> > @@ -182,7 +182,8 @@ void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask)
> >  }
> >  
> >  int request_irq(unsigned int irq, unsigned int irqflags,
> > -                void (*handler)(int, void *, struct cpu_user_regs *),
> > +                void (*handler)(int irq, void *dev_id,
> > +                                struct cpu_user_regs *regs),
> >                  const char *devname, void *dev_id)
> >  {
> 
> Before we accept patches, don't we need to first settle on whether to
> apply the rule(s) also to function type declarations (and not just
> ordinary prototypes)?

Yes, in retrospect we should have found agreement on this issue this
morning but I forgot to bring it up :-(  Ooops.

(I think the agreement was to change the function type declarations too,
that's why docs/misra/rules.rst doesn't have a note about this, but I
don't want to make assumptions as I am not certain.)

Re: [XEN PATCH 2/3] xen/arm: irq: address violations of MISRA C: 2012 Rules 8.2 and 8.3

[Federico Serafini]

Hello Stefano,

On 25/07/23 00:55, Stefano Stabellini wrote:
>>   int request_irq(unsigned int irq, unsigned int irqflags,
>> -                void (*handler)(int, void *, struct cpu_user_regs *),
>> +                void (*handler)(int irq, void *dev_id,
>> +                                struct cpu_user_regs *regs),
> 
> We have an inconsistency where the handler functions on x86 typically
> call it void *data, while on arm they typically use void *dev_id
> (see xen/arch/x86/irq.c:request_irq and
> xen/arch/x86/hpet.c:hpet_interrupt_handler). I think we should be
> consistent. Or, if this is not a MISRA requirement because this is just
> a function pointer rather than a proper function, then I would leave it
> alone.

This is an inconsistency but it is not a violation of the rule 8.3.

Regards
-- 
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)

Re: [XEN PATCH 2/3] xen/arm: irq: address violations of MISRA C: 2012 Rules 8.2 and 8.3

[Federico Serafini]

Hello Jan, Stefano,

On 25/07/23 21:32, Stefano Stabellini wrote:
> On Tue, 25 Jul 2023, Jan Beulich wrote:
>> On 24.07.2023 19:50, Federico Serafini wrote:
>>> @@ -182,7 +182,8 @@ void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask)
>>>   }
>>>   
>>>   int request_irq(unsigned int irq, unsigned int irqflags,
>>> -                void (*handler)(int, void *, struct cpu_user_regs *),
>>> +                void (*handler)(int irq, void *dev_id,
>>> +                                struct cpu_user_regs *regs),
>>>                   const char *devname, void *dev_id)
>>>   {
>>
>> Before we accept patches, don't we need to first settle on whether to
>> apply the rule(s) also to function type declarations (and not just
>> ordinary prototypes)?
> 
> Yes, in retrospect we should have found agreement on this issue this
> morning but I forgot to bring it up :-(  Ooops.
> 
> (I think the agreement was to change the function type declarations too,
> that's why docs/misra/rules.rst doesn't have a note about this, but I
> don't want to make assumptions as I am not certain.)

I have ready a patch for violations of rules 8.2 and 8.3 in
xen/include/xen/iommu.h.
I am talking about this, in this IRQ thread, because I think the 
following two options also apply for an eventual v2 patch for the IRQ 
module, until a decision about rule 8.2 and function pointers is taken:

1) Split patches and submit only the changes *not* involving function
    pointers.
2) In the meantime that you make a decision,
    I submit patches thus addressing the existing violations.

I personally prefer the second one, but please let me know what you
think.

Regards
-- 
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)

Re: [XEN PATCH 2/3] xen/arm: irq: address violations of MISRA C: 2012 Rules 8.2 and 8.3

[Jan Beulich]

On 27.07.2023 13:02, Federico Serafini wrote:
> Hello Jan, Stefano,
> 
> On 25/07/23 21:32, Stefano Stabellini wrote:
>> On Tue, 25 Jul 2023, Jan Beulich wrote:
>>> On 24.07.2023 19:50, Federico Serafini wrote:
>>>> @@ -182,7 +182,8 @@ void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask)
>>>>   }
>>>>   
>>>>   int request_irq(unsigned int irq, unsigned int irqflags,
>>>> -                void (*handler)(int, void *, struct cpu_user_regs *),
>>>> +                void (*handler)(int irq, void *dev_id,
>>>> +                                struct cpu_user_regs *regs),
>>>>                   const char *devname, void *dev_id)
>>>>   {
>>>
>>> Before we accept patches, don't we need to first settle on whether to
>>> apply the rule(s) also to function type declarations (and not just
>>> ordinary prototypes)?
>>
>> Yes, in retrospect we should have found agreement on this issue this
>> morning but I forgot to bring it up :-(  Ooops.
>>
>> (I think the agreement was to change the function type declarations too,
>> that's why docs/misra/rules.rst doesn't have a note about this, but I
>> don't want to make assumptions as I am not certain.)
> 
> I have ready a patch for violations of rules 8.2 and 8.3 in
> xen/include/xen/iommu.h.
> I am talking about this, in this IRQ thread, because I think the 
> following two options also apply for an eventual v2 patch for the IRQ 
> module, until a decision about rule 8.2 and function pointers is taken:
> 
> 1) Split patches and submit only the changes *not* involving function
>     pointers.
> 2) In the meantime that you make a decision,
>     I submit patches thus addressing the existing violations.
> 
> I personally prefer the second one, but please let me know what you
> think.

It's not entirely clear to me what 2 means, as I wouldn't expect you
intend to deal with "violations" which we may decide aren't any in
out world.

Jan

Re: [XEN PATCH 2/3] xen/arm: irq: address violations of MISRA C: 2012 Rules 8.2 and 8.3

[Federico Serafini]

On 27/07/23 13:27, Jan Beulich wrote:
> On 27.07.2023 13:02, Federico Serafini wrote:
>>
>> I have ready a patch for violations of rules 8.2 and 8.3 in
>> xen/include/xen/iommu.h.
>> I am talking about this, in this IRQ thread, because I think the
>> following two options also apply for an eventual v2 patch for the IRQ
>> module, until a decision about rule 8.2 and function pointers is taken:
>>
>> 1) Split patches and submit only the changes *not* involving function
>>      pointers.
>> 2) In the meantime that you make a decision,
>>      I submit patches thus addressing the existing violations.
>>
>> I personally prefer the second one, but please let me know what you
>> think.
> 
> It's not entirely clear to me what 2 means, as I wouldn't expect you
> intend to deal with "violations" which we may decide aren't any in
> out world.
> 
> Jan

In point 2) I would like to act as if the rule 8.2 had been approved 
without any deviation, I think this will lead to a smaller number of 
patches and a smaller amount of text attached to each modified function.
If you are concerned about inconsistency between the resulting commit
messages and your future decision then we can go for option 1).

-- 
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)

Re: [XEN PATCH 2/3] xen/arm: irq: address violations of MISRA C: 2012 Rules 8.2 and 8.3

[Stefano Stabellini]

On Thu, 27 Jul 2023, Federico Serafini wrote:
> On 27/07/23 13:27, Jan Beulich wrote:
> > On 27.07.2023 13:02, Federico Serafini wrote:
> > > 
> > > I have ready a patch for violations of rules 8.2 and 8.3 in
> > > xen/include/xen/iommu.h.
> > > I am talking about this, in this IRQ thread, because I think the
> > > following two options also apply for an eventual v2 patch for the IRQ
> > > module, until a decision about rule 8.2 and function pointers is taken:
> > > 
> > > 1) Split patches and submit only the changes *not* involving function
> > >      pointers.
> > > 2) In the meantime that you make a decision,
> > >      I submit patches thus addressing the existing violations.
> > > 
> > > I personally prefer the second one, but please let me know what you
> > > think.
> > 
> > It's not entirely clear to me what 2 means, as I wouldn't expect you
> > intend to deal with "violations" which we may decide aren't any in
> > out world.
> > 
> > Jan
> 
> In point 2) I would like to act as if the rule 8.2 had been approved without
> any deviation, I think this will lead to a smaller number of patches and a
> smaller amount of text attached to each modified function.
> If you are concerned about inconsistency between the resulting commit
> messages and your future decision then we can go for option 1).

Basically I think we need to go with 1), which is also what Jan wrote.

Sorry about that, I know it is not great, we should have settled this
already.