[XEN PATCH 0/5] xen: address some violations of MISRA C:2012 Rule 8.2

[XEN PATCH 0/5] xen: address some violations of MISRA C:2012 Rule 8.2

[Federico Serafini]

This patch series adds the missing parameter names to address violations of
MISRA C:2012 Rule 8.2. No functional changes are introduced.

Federico Serafini (5):
  xen/common: address violations of MISRA C:2012 Rule 8.2
  xen/serial: address violations of MISRA C:2012 Rule 8.2
  xen/sort: address violations of MISRA C:2012 Rule 8.2
  xen/vmap: address violations of MISRA C:2012 Rule 8.2
  xen/xalloc: address violations of MISRA C:2012 Rule 8.2

 xen/common/efi/runtime.c       |  2 +-
 xen/common/rangeset.c          |  6 +++---
 xen/common/spinlock.c          |  8 ++++----
 xen/common/stop_machine.c      |  4 ++--
 xen/common/tasklet.c           |  5 +++--
 xen/common/timer.c             |  4 ++--
 xen/include/xen/rangeset.h     |  4 ++--
 xen/include/xen/serial.h       | 30 +++++++++++++++---------------
 xen/include/xen/sort.h         |  4 ++--
 xen/include/xen/spinlock.h     |  2 +-
 xen/include/xen/stop_machine.h |  2 +-
 xen/include/xen/tasklet.h      |  7 ++++---
 xen/include/xen/timer.h        |  4 ++--
 xen/include/xen/vmap.h         |  6 +++---
 xen/include/xen/xmalloc.h      |  2 +-
 15 files changed, 46 insertions(+), 44 deletions(-)

-- 
2.34.1

[XEN PATCH 1/5] xen/common: address violations of MISRA C:2012 Rule 8.2

[Federico Serafini]

Add missing parameter names. No functional change.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/common/efi/runtime.c       | 2 +-
 xen/common/rangeset.c          | 6 +++---
 xen/common/spinlock.c          | 8 ++++----
 xen/common/stop_machine.c      | 4 ++--
 xen/common/tasklet.c           | 5 +++--
 xen/common/timer.c             | 4 ++--
 xen/include/xen/rangeset.h     | 4 ++--
 xen/include/xen/spinlock.h     | 2 +-
 xen/include/xen/stop_machine.h | 2 +-
 xen/include/xen/tasklet.h      | 7 ++++---
 xen/include/xen/timer.h        | 4 ++--
 11 files changed, 25 insertions(+), 23 deletions(-)

diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
index 5cb7504c96..d952c3ba78 100644
--- a/xen/common/efi/runtime.c
+++ b/xen/common/efi/runtime.c
@@ -26,7 +26,7 @@ struct efi_rs_state {
 };
 
 struct efi_rs_state efi_rs_enter(void);
-void efi_rs_leave(struct efi_rs_state *);
+void efi_rs_leave(struct efi_rs_state *state);
 
 #ifndef COMPAT
 
diff --git a/xen/common/rangeset.c b/xen/common/rangeset.c
index f3baf52ab6..aa3a94e053 100644
--- a/xen/common/rangeset.c
+++ b/xen/common/rangeset.c
@@ -288,7 +288,7 @@ bool_t rangeset_overlaps_range(
 
 int rangeset_report_ranges(
     struct rangeset *r, unsigned long s, unsigned long e,
-    int (*cb)(unsigned long s, unsigned long e, void *), void *ctxt)
+    int (*cb)(unsigned long s, unsigned long e, void *data), void *ctxt)
 {
     struct range *x;
     int rc = 0;
@@ -357,8 +357,8 @@ int rangeset_claim_range(struct rangeset *r, unsigned long size,
 }
 
 int rangeset_consume_ranges(struct rangeset *r,
-                            int (*cb)(unsigned long s, unsigned long e, void *,
-                                      unsigned long *c),
+                            int (*cb)(unsigned long s, unsigned long e,
+                                      void *ctxt, unsigned long *c),
                             void *ctxt)
 {
     int rc = 0;
diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
index 7f453234a9..8fa3e253c0 100644
--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -305,7 +305,7 @@ static always_inline u16 observe_head(spinlock_tickets_t *t)
 }
 
 static void always_inline spin_lock_common(spinlock_t *lock,
-                                           void (*cb)(void *), void *data)
+                                           void (*cb)(void *data), void *data)
 {
     spinlock_tickets_t tickets = SPINLOCK_TICKET_INC;
     LOCK_PROFILE_VAR;
@@ -331,7 +331,7 @@ void _spin_lock(spinlock_t *lock)
     spin_lock_common(lock, NULL, NULL);
 }
 
-void _spin_lock_cb(spinlock_t *lock, void (*cb)(void *), void *data)
+void _spin_lock_cb(spinlock_t *lock, void (*cb)(void *data), void *data)
 {
     spin_lock_common(lock, cb, data);
 }
@@ -498,8 +498,8 @@ struct lock_profile_anc {
     const char                *name;     /* descriptive string for print */
 };
 
-typedef void lock_profile_subfunc(
-    struct lock_profile *, int32_t, int32_t, void *);
+typedef void lock_profile_subfunc(struct lock_profile *data, int32_t type,
+    int32_t idx, void *par);
 
 extern struct lock_profile *__lock_profile_start;
 extern struct lock_profile *__lock_profile_end;
diff --git a/xen/common/stop_machine.c b/xen/common/stop_machine.c
index 3adbe380de..398cfd507c 100644
--- a/xen/common/stop_machine.c
+++ b/xen/common/stop_machine.c
@@ -46,7 +46,7 @@ struct stopmachine_data {
 
     unsigned int fn_cpu;
     int fn_result;
-    int (*fn)(void *);
+    int (*fn)(void *data);
     void *fn_data;
 };
 
@@ -73,7 +73,7 @@ static void stopmachine_wait_state(void)
  * mandatory to be called only on an idle vcpu, as otherwise active core
  * scheduling might hang.
  */
-int stop_machine_run(int (*fn)(void *), void *data, unsigned int cpu)
+int stop_machine_run(int (*fn)(void *data), void *data, unsigned int cpu)
 {
     unsigned int i, nr_cpus;
     unsigned int this = smp_processor_id();
diff --git a/xen/common/tasklet.c b/xen/common/tasklet.c
index 3ad67b5c24..3649798e6b 100644
--- a/xen/common/tasklet.c
+++ b/xen/common/tasklet.c
@@ -199,7 +199,7 @@ static void migrate_tasklets_from_cpu(unsigned int cpu, struct list_head *list)
     spin_unlock_irqrestore(&tasklet_lock, flags);
 }
 
-void tasklet_init(struct tasklet *t, void (*func)(void *), void *data)
+void tasklet_init(struct tasklet *t, void (*func)(void *data), void *data)
 {
     memset(t, 0, sizeof(*t));
     INIT_LIST_HEAD(&t->list);
@@ -208,7 +208,8 @@ void tasklet_init(struct tasklet *t, void (*func)(void *), void *data)
     t->data = data;
 }
 
-void softirq_tasklet_init(struct tasklet *t, void (*func)(void *), void *data)
+void softirq_tasklet_init(struct tasklet *t,
+                          void (*func)(void *data), void *data)
 {
     tasklet_init(t, func, data);
     t->is_softirq = 1;
diff --git a/xen/common/timer.c b/xen/common/timer.c
index 0fddfa7487..bf7792dcb3 100644
--- a/xen/common/timer.c
+++ b/xen/common/timer.c
@@ -291,7 +291,7 @@ static bool active_timer(const struct timer *timer)
 
 void init_timer(
     struct timer *timer,
-    void        (*function)(void *),
+    void        (*function)(void *data),
     void         *data,
     unsigned int  cpu)
 {
@@ -441,7 +441,7 @@ void kill_timer(struct timer *timer)
 
 static void execute_timer(struct timers *ts, struct timer *t)
 {
-    void (*fn)(void *) = t->function;
+    void (*fn)(void *data) = t->function;
     void *data = t->data;
 
     t->status = TIMER_STATUS_inactive;
diff --git a/xen/include/xen/rangeset.h b/xen/include/xen/rangeset.h
index 135f33f606..390f7b6082 100644
--- a/xen/include/xen/rangeset.h
+++ b/xen/include/xen/rangeset.h
@@ -68,7 +68,7 @@ bool_t __must_check rangeset_overlaps_range(
     struct rangeset *r, unsigned long s, unsigned long e);
 int rangeset_report_ranges(
     struct rangeset *r, unsigned long s, unsigned long e,
-    int (*cb)(unsigned long s, unsigned long e, void *), void *ctxt);
+    int (*cb)(unsigned long s, unsigned long e, void *data), void *ctxt);
 
 /*
  * Note that the consume function can return an error value apart from
@@ -77,7 +77,7 @@ int rangeset_report_ranges(
  */
 int rangeset_consume_ranges(struct rangeset *r,
                             int (*cb)(unsigned long s, unsigned long e,
-                                      void *, unsigned long *c),
+                                      void *ctxt, unsigned long *c),
                             void *ctxt);
 
 /* Merge rangeset r2 into rangeset r1. */
diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
index 16d933ae7e..785ef689a0 100644
--- a/xen/include/xen/spinlock.h
+++ b/xen/include/xen/spinlock.h
@@ -179,7 +179,7 @@ typedef struct spinlock {
 #define spin_lock_init(l) (*(l) = (spinlock_t)SPIN_LOCK_UNLOCKED)
 
 void _spin_lock(spinlock_t *lock);
-void _spin_lock_cb(spinlock_t *lock, void (*cb)(void *), void *data);
+void _spin_lock_cb(spinlock_t *lock, void (*cb)(void *data), void *data);
 void _spin_lock_irq(spinlock_t *lock);
 unsigned long _spin_lock_irqsave(spinlock_t *lock);
 
diff --git a/xen/include/xen/stop_machine.h b/xen/include/xen/stop_machine.h
index c63da1b309..0bbf71f112 100644
--- a/xen/include/xen/stop_machine.h
+++ b/xen/include/xen/stop_machine.h
@@ -14,6 +14,6 @@
  *
  * This can be thought of as a very heavy write lock, equivalent to
  * grabbing every spinlock in the kernel. */
-int stop_machine_run(int (*fn)(void *), void *data, unsigned int cpu);
+int stop_machine_run(int (*fn)(void *data), void *data, unsigned int cpu);
 
 #endif /* __XEN_STOP_MACHINE_H__ */
diff --git a/xen/include/xen/tasklet.h b/xen/include/xen/tasklet.h
index 193acf8f42..59f2b522f3 100644
--- a/xen/include/xen/tasklet.h
+++ b/xen/include/xen/tasklet.h
@@ -21,7 +21,7 @@ struct tasklet
     bool_t is_softirq;
     bool_t is_running;
     bool_t is_dead;
-    void (*func)(void *);
+    void (*func)(void *data);
     void *data;
 };
 
@@ -59,8 +59,9 @@ void tasklet_schedule_on_cpu(struct tasklet *t, unsigned int cpu);
 void tasklet_schedule(struct tasklet *t);
 void do_tasklet(void);
 void tasklet_kill(struct tasklet *t);
-void tasklet_init(struct tasklet *t, void (*func)(void *), void *data);
-void softirq_tasklet_init(struct tasklet *t, void (*func)(void *), void *data);
+void tasklet_init(struct tasklet *t, void (*func)(void *data), void *data);
+void softirq_tasklet_init(struct tasklet *t,
+                          void (*func)(void *data), void *data);
 void tasklet_subsys_init(void);
 
 #endif /* __XEN_TASKLET_H__ */
diff --git a/xen/include/xen/timer.h b/xen/include/xen/timer.h
index 3a2a05c6de..fb28517515 100644
--- a/xen/include/xen/timer.h
+++ b/xen/include/xen/timer.h
@@ -29,7 +29,7 @@ struct timer {
     };
 
     /* On expiry, '(*function)(data)' will be executed in softirq context. */
-    void (*function)(void *);
+    void (*function)(void *data);
     void *data;
 
     /* CPU on which this timer will be installed and executed. */
@@ -57,7 +57,7 @@ struct timer {
  */
 void init_timer(
     struct timer *timer,
-    void        (*function)(void *),
+    void        (*function)(void *data),
     void         *data,
     unsigned int  cpu);
 
-- 
2.34.1

[XEN PATCH 2/5] xen/serial: address violations of MISRA C:2012 Rule 8.2

[Federico Serafini]

Add missing parameter names. No functional change.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/include/xen/serial.h | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h
index f0aff7ea76..fc3b4883a2 100644
--- a/xen/include/xen/serial.h
+++ b/xen/include/xen/serial.h
@@ -15,7 +15,7 @@
 struct cpu_user_regs;
 
 /* Register a character-receive hook on the specified COM port. */
-typedef void (*serial_rx_fn)(char, struct cpu_user_regs *);
+typedef void (*serial_rx_fn)(char c, struct cpu_user_regs *regs);
 void serial_set_rx_handler(int handle, serial_rx_fn fn);
 
 /* Number of characters we buffer for a polling receiver. */
@@ -63,31 +63,31 @@ struct serial_port {
 
 struct uart_driver {
     /* Driver initialisation (pre- and post-IRQ subsystem setup). */
-    void (*init_preirq)(struct serial_port *);
-    void (*init_irq)(struct serial_port *);
-    void (*init_postirq)(struct serial_port *);
+    void (*init_preirq)(struct serial_port *port);
+    void (*init_irq)(struct serial_port *port);
+    void (*init_postirq)(struct serial_port *port);
     /* Hook to clean up after Xen bootstrap (before domain 0 runs). */
-    void (*endboot)(struct serial_port *);
+    void (*endboot)(struct serial_port *port);
     /* Driver suspend/resume. */
-    void (*suspend)(struct serial_port *);
-    void (*resume)(struct serial_port *);
+    void (*suspend)(struct serial_port *port);
+    void (*resume)(struct serial_port *port);
     /* Return number of characters the port can hold for transmit,
      * or -EIO if port is inaccesible */
-    int (*tx_ready)(struct serial_port *);
+    int (*tx_ready)(struct serial_port *port);
     /* Put a character onto the serial line. */
-    void (*putc)(struct serial_port *, char);
+    void (*putc)(struct serial_port *port, char c);
     /* Flush accumulated characters. */
-    void (*flush)(struct serial_port *);
+    void (*flush)(struct serial_port *port);
     /* Get a character from the serial line: returns 0 if none available. */
-    int  (*getc)(struct serial_port *, char *);
+    int  (*getc)(struct serial_port *port, char *pc);
     /* Get IRQ number for this port's serial line: returns -1 if none. */
-    int  (*irq)(struct serial_port *);
+    int  (*irq)(struct serial_port *port);
     /* Unmask TX interrupt */
-    void  (*start_tx)(struct serial_port *);
+    void  (*start_tx)(struct serial_port *port);
     /* Mask TX interrupt */
-    void  (*stop_tx)(struct serial_port *);
+    void  (*stop_tx)(struct serial_port *port);
     /* Get serial information */
-    const struct vuart_info *(*vuart_info)(struct serial_port *);
+    const struct vuart_info *(*vuart_info)(struct serial_port *port);
 };
 
 /* 'Serial handles' are composed from the following fields. */
-- 
2.34.1

[XEN PATCH 3/5] xen/sort: address violations of MISRA C:2012 Rule 8.2

[Federico Serafini]

Add missing parameter names. No functional change.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/include/xen/sort.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/include/xen/sort.h b/xen/include/xen/sort.h
index 2f52ff85b9..1d5e3c5849 100644
--- a/xen/include/xen/sort.h
+++ b/xen/include/xen/sort.h
@@ -23,8 +23,8 @@
 extern gnu_inline
 #endif
 void sort(void *base, size_t num, size_t size,
-          int (*cmp)(const void *, const void *),
-          void (*swap)(void *, void *, size_t))
+          int (*cmp)(const void *key, const void *elem),
+          void (*swap)(void *a, void *b, size_t size))
 {
     /* pre-scale counters for performance */
     size_t i = (num / 2) * size, n = num * size, c, r;
-- 
2.34.1

[XEN PATCH 4/5] xen/vmap: address violations of MISRA C:2012 Rule 8.2

[Federico Serafini]

Add missing parameter names. No functional change.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/include/xen/vmap.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/include/xen/vmap.h b/xen/include/xen/vmap.h
index b0f7632e89..2b7369e062 100644
--- a/xen/include/xen/vmap.h
+++ b/xen/include/xen/vmap.h
@@ -13,9 +13,9 @@ enum vmap_region {
 void vm_init_type(enum vmap_region type, void *start, void *end);
 
 void *__vmap(const mfn_t *mfn, unsigned int granularity, unsigned int nr,
-             unsigned int align, unsigned int flags, enum vmap_region);
+             unsigned int align, unsigned int flags, enum vmap_region type);
 void *vmap(const mfn_t *mfn, unsigned int nr);
-void vunmap(const void *);
+void vunmap(const void *va);
 
 void *vmalloc(size_t size);
 void *vmalloc_xen(size_t size);
@@ -23,7 +23,7 @@ void *vmalloc_xen(size_t size);
 void *vzalloc(size_t size);
 void vfree(void *va);
 
-void __iomem *ioremap(paddr_t, size_t);
+void __iomem *ioremap(paddr_t pa, size_t len);
 
 static inline void iounmap(void __iomem *va)
 {
-- 
2.34.1

[XEN PATCH 5/5] xen/xalloc: address violations of MISRA C:2012 Rule 8.2

[Federico Serafini]

Add missing parameter names. No functional change.

Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
---
 xen/include/xen/xmalloc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/xen/xmalloc.h b/xen/include/xen/xmalloc.h
index 16979a117c..9ecddbff5e 100644
--- a/xen/include/xen/xmalloc.h
+++ b/xen/include/xen/xmalloc.h
@@ -63,7 +63,7 @@
     })
 
 /* Free any of the above. */
-extern void xfree(void *);
+extern void xfree(void *p);
 
 /* Free an allocation, and zero the pointer to it. */
 #define XFREE(p) do { \
-- 
2.34.1

Re: [XEN PATCH 1/5] xen/common: address violations of MISRA C:2012 Rule 8.2

[Stefano Stabellini]

On Fri, 17 Nov 2023, Federico Serafini wrote:
> diff --git a/xen/common/stop_machine.c b/xen/common/stop_machine.c
> index 3adbe380de..398cfd507c 100644
> --- a/xen/common/stop_machine.c
> +++ b/xen/common/stop_machine.c
> @@ -46,7 +46,7 @@ struct stopmachine_data {
>  
>      unsigned int fn_cpu;
>      int fn_result;
> -    int (*fn)(void *);
> +    int (*fn)(void *data);
>      void *fn_data;
>  };

At least one of the possible function used here calls the parameter
"arg", see take_cpu_down. But I don't think it is a MISRA requirement to
also harmonize those?


> @@ -73,7 +73,7 @@ static void stopmachine_wait_state(void)
>   * mandatory to be called only on an idle vcpu, as otherwise active core
>   * scheduling might hang.
>   */
> -int stop_machine_run(int (*fn)(void *), void *data, unsigned int cpu)
> +int stop_machine_run(int (*fn)(void *data), void *data, unsigned int cpu)
>  {
>      unsigned int i, nr_cpus;
>      unsigned int this = smp_processor_id();
> diff --git a/xen/common/tasklet.c b/xen/common/tasklet.c
> index 3ad67b5c24..3649798e6b 100644
> --- a/xen/common/tasklet.c
> +++ b/xen/common/tasklet.c
> @@ -199,7 +199,7 @@ static void migrate_tasklets_from_cpu(unsigned int cpu, struct list_head *list)
>      spin_unlock_irqrestore(&tasklet_lock, flags);
>  }
>  
> -void tasklet_init(struct tasklet *t, void (*func)(void *), void *data)
> +void tasklet_init(struct tasklet *t, void (*func)(void *data), void *data)
>  {
>      memset(t, 0, sizeof(*t));
>      INIT_LIST_HEAD(&t->list);
> @@ -208,7 +208,8 @@ void tasklet_init(struct tasklet *t, void (*func)(void *), void *data)
>      t->data = data;
>  }
>  
> -void softirq_tasklet_init(struct tasklet *t, void (*func)(void *), void *data)
> +void softirq_tasklet_init(struct tasklet *t,
> +                          void (*func)(void *data), void *data)
>  {
>      tasklet_init(t, func, data);
>      t->is_softirq = 1;
> diff --git a/xen/common/timer.c b/xen/common/timer.c
> index 0fddfa7487..bf7792dcb3 100644
> --- a/xen/common/timer.c
> +++ b/xen/common/timer.c
> @@ -291,7 +291,7 @@ static bool active_timer(const struct timer *timer)
>  
>  void init_timer(
>      struct timer *timer,
> -    void        (*function)(void *),
> +    void        (*function)(void *data),
>      void         *data,
>      unsigned int  cpu)
>  {
> @@ -441,7 +441,7 @@ void kill_timer(struct timer *timer)
>  
>  static void execute_timer(struct timers *ts, struct timer *t)
>  {
> -    void (*fn)(void *) = t->function;
> +    void (*fn)(void *data) = t->function;
>      void *data = t->data;
>  
>      t->status = TIMER_STATUS_inactive;
> diff --git a/xen/include/xen/rangeset.h b/xen/include/xen/rangeset.h
> index 135f33f606..390f7b6082 100644
> --- a/xen/include/xen/rangeset.h
> +++ b/xen/include/xen/rangeset.h
> @@ -68,7 +68,7 @@ bool_t __must_check rangeset_overlaps_range(
>      struct rangeset *r, unsigned long s, unsigned long e);
>  int rangeset_report_ranges(
>      struct rangeset *r, unsigned long s, unsigned long e,
> -    int (*cb)(unsigned long s, unsigned long e, void *), void *ctxt);
> +    int (*cb)(unsigned long s, unsigned long e, void *data), void *ctxt);

Also here some of the functions use "arg" instead of ctxt


>  /*
>   * Note that the consume function can return an error value apart from
> @@ -77,7 +77,7 @@ int rangeset_report_ranges(
>   */
>  int rangeset_consume_ranges(struct rangeset *r,
>                              int (*cb)(unsigned long s, unsigned long e,
> -                                      void *, unsigned long *c),
> +                                      void *ctxt, unsigned long *c),
>                              void *ctxt);

Also here some of the functions use "dom" like irq_remove_cb.


But I actually like the patch as is, so if that's OK from a MISRA point
of view then I would give my reviewed-by.

Re: [XEN PATCH 2/5] xen/serial: address violations of MISRA C:2012 Rule 8.2

[Stefano Stabellini]

On Fri, 17 Nov 2023, Federico Serafini wrote:
> Add missing parameter names. No functional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Re: [XEN PATCH 3/5] xen/sort: address violations of MISRA C:2012 Rule 8.2

[Stefano Stabellini]

On Fri, 17 Nov 2023, Federico Serafini wrote:
> Add missing parameter names. No functional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
> ---
>  xen/include/xen/sort.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/include/xen/sort.h b/xen/include/xen/sort.h
> index 2f52ff85b9..1d5e3c5849 100644
> --- a/xen/include/xen/sort.h
> +++ b/xen/include/xen/sort.h
> @@ -23,8 +23,8 @@
>  extern gnu_inline
>  #endif
>  void sort(void *base, size_t num, size_t size,
> -          int (*cmp)(const void *, const void *),
> -          void (*swap)(void *, void *, size_t))
> +          int (*cmp)(const void *key, const void *elem),
> +          void (*swap)(void *a, void *b, size_t size))
>  {
>      /* pre-scale counters for performance */
>      size_t i = (num / 2) * size, n = num * size, c, r;


Ideally we should also fix swap_memory_node, swap_mmio_handler

Re: [XEN PATCH 4/5] xen/vmap: address violations of MISRA C:2012 Rule 8.2

[Stefano Stabellini]

On Fri, 17 Nov 2023, Federico Serafini wrote:
> Add missing parameter names. No functional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Re: [XEN PATCH 5/5] xen/xalloc: address violations of MISRA C:2012 Rule 8.2

[Stefano Stabellini]

On Fri, 17 Nov 2023, Federico Serafini wrote:
> Add missing parameter names. No functional change.
> 
> Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Re: [XEN PATCH 3/5] xen/sort: address violations of MISRA C:2012 Rule 8.2

[Jan Beulich]

On 17.11.2023 09:40, Federico Serafini wrote:
> --- a/xen/include/xen/sort.h
> +++ b/xen/include/xen/sort.h
> @@ -23,8 +23,8 @@
>  extern gnu_inline
>  #endif
>  void sort(void *base, size_t num, size_t size,
> -          int (*cmp)(const void *, const void *),
> -          void (*swap)(void *, void *, size_t))
> +          int (*cmp)(const void *key, const void *elem),

Why "key" and "elem" here, but ...

> +          void (*swap)(void *a, void *b, size_t size))

... "a" and "b" here? The first example of users of sort() that I'm
looking at right now (x86/extable.c) is consistent in its naming.

Jan

Re: [XEN PATCH 3/5] xen/sort: address violations of MISRA C:2012 Rule 8.2

[Federico Serafini]

On 20/11/23 10:02, Jan Beulich wrote:
> On 17.11.2023 09:40, Federico Serafini wrote:
>> --- a/xen/include/xen/sort.h
>> +++ b/xen/include/xen/sort.h
>> @@ -23,8 +23,8 @@
>>   extern gnu_inline
>>   #endif
>>   void sort(void *base, size_t num, size_t size,
>> -          int (*cmp)(const void *, const void *),
>> -          void (*swap)(void *, void *, size_t))
>> +          int (*cmp)(const void *key, const void *elem),
> 
> Why "key" and "elem" here, but ...
> 
>> +          void (*swap)(void *a, void *b, size_t size))
> 
> ... "a" and "b" here? The first example of users of sort() that I'm
> looking at right now (x86/extable.c) is consistent in its naming.
> 

On the Arm side there are {cmp,swap}_memory_node() and
{cmp,swap}_mmio_handler(): "key"/"elem" are used for the comparison
and "_a"/"_b" for the swap.

-- 
Federico Serafini, M.Sc.

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

Re: [XEN PATCH 1/5] xen/common: address violations of MISRA C:2012 Rule 8.2

[Federico Serafini]

On 18/11/23 03:59, Stefano Stabellini wrote:
> On Fri, 17 Nov 2023, Federico Serafini wrote:
>> diff --git a/xen/common/stop_machine.c b/xen/common/stop_machine.c
>> index 3adbe380de..398cfd507c 100644
>> --- a/xen/common/stop_machine.c
>> +++ b/xen/common/stop_machine.c
>> @@ -46,7 +46,7 @@ struct stopmachine_data {
>>   
>>       unsigned int fn_cpu;
>>       int fn_result;
>> -    int (*fn)(void *);
>> +    int (*fn)(void *data);
>>       void *fn_data;
>>   };
> 
> At least one of the possible function used here calls the parameter
> "arg", see take_cpu_down. But I don't think it is a MISRA requirement to
> also harmonize those?
> 
> 
>> @@ -73,7 +73,7 @@ static void stopmachine_wait_state(void)
>>    * mandatory to be called only on an idle vcpu, as otherwise active core
>>    * scheduling might hang.
>>    */
>> -int stop_machine_run(int (*fn)(void *), void *data, unsigned int cpu)
>> +int stop_machine_run(int (*fn)(void *data), void *data, unsigned int cpu)
>>   {
>>       unsigned int i, nr_cpus;
>>       unsigned int this = smp_processor_id();
>> diff --git a/xen/common/tasklet.c b/xen/common/tasklet.c
>> index 3ad67b5c24..3649798e6b 100644
>> --- a/xen/common/tasklet.c
>> +++ b/xen/common/tasklet.c
>> @@ -199,7 +199,7 @@ static void migrate_tasklets_from_cpu(unsigned int cpu, struct list_head *list)
>>       spin_unlock_irqrestore(&tasklet_lock, flags);
>>   }
>>   
>> -void tasklet_init(struct tasklet *t, void (*func)(void *), void *data)
>> +void tasklet_init(struct tasklet *t, void (*func)(void *data), void *data)
>>   {
>>       memset(t, 0, sizeof(*t));
>>       INIT_LIST_HEAD(&t->list);
>> @@ -208,7 +208,8 @@ void tasklet_init(struct tasklet *t, void (*func)(void *), void *data)
>>       t->data = data;
>>   }
>>   
>> -void softirq_tasklet_init(struct tasklet *t, void (*func)(void *), void *data)
>> +void softirq_tasklet_init(struct tasklet *t,
>> +                          void (*func)(void *data), void *data)
>>   {
>>       tasklet_init(t, func, data);
>>       t->is_softirq = 1;
>> diff --git a/xen/common/timer.c b/xen/common/timer.c
>> index 0fddfa7487..bf7792dcb3 100644
>> --- a/xen/common/timer.c
>> +++ b/xen/common/timer.c
>> @@ -291,7 +291,7 @@ static bool active_timer(const struct timer *timer)
>>   
>>   void init_timer(
>>       struct timer *timer,
>> -    void        (*function)(void *),
>> +    void        (*function)(void *data),
>>       void         *data,
>>       unsigned int  cpu)
>>   {
>> @@ -441,7 +441,7 @@ void kill_timer(struct timer *timer)
>>   
>>   static void execute_timer(struct timers *ts, struct timer *t)
>>   {
>> -    void (*fn)(void *) = t->function;
>> +    void (*fn)(void *data) = t->function;
>>       void *data = t->data;
>>   
>>       t->status = TIMER_STATUS_inactive;
>> diff --git a/xen/include/xen/rangeset.h b/xen/include/xen/rangeset.h
>> index 135f33f606..390f7b6082 100644
>> --- a/xen/include/xen/rangeset.h
>> +++ b/xen/include/xen/rangeset.h
>> @@ -68,7 +68,7 @@ bool_t __must_check rangeset_overlaps_range(
>>       struct rangeset *r, unsigned long s, unsigned long e);
>>   int rangeset_report_ranges(
>>       struct rangeset *r, unsigned long s, unsigned long e,
>> -    int (*cb)(unsigned long s, unsigned long e, void *), void *ctxt);
>> +    int (*cb)(unsigned long s, unsigned long e, void *data), void *ctxt);
> 
> Also here some of the functions use "arg" instead of ctxt
> 
> 
>>   /*
>>    * Note that the consume function can return an error value apart from
>> @@ -77,7 +77,7 @@ int rangeset_report_ranges(
>>    */
>>   int rangeset_consume_ranges(struct rangeset *r,
>>                               int (*cb)(unsigned long s, unsigned long e,
>> -                                      void *, unsigned long *c),
>> +                                      void *ctxt, unsigned long *c),
>>                               void *ctxt);
> 
> Also here some of the functions use "dom" like irq_remove_cb.
> 
> 
> But I actually like the patch as is, so if that's OK from a MISRA point
> of view then I would give my reviewed-by.

Yes, this is OK for MISRA.

-- 
Federico Serafini, M.Sc.

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

Re: [XEN PATCH 3/5] xen/sort: address violations of MISRA C:2012 Rule 8.2

[Jan Beulich]

On 20.11.2023 14:13, Federico Serafini wrote:
> On 20/11/23 10:02, Jan Beulich wrote:
>> On 17.11.2023 09:40, Federico Serafini wrote:
>>> --- a/xen/include/xen/sort.h
>>> +++ b/xen/include/xen/sort.h
>>> @@ -23,8 +23,8 @@
>>>   extern gnu_inline
>>>   #endif
>>>   void sort(void *base, size_t num, size_t size,
>>> -          int (*cmp)(const void *, const void *),
>>> -          void (*swap)(void *, void *, size_t))
>>> +          int (*cmp)(const void *key, const void *elem),
>>
>> Why "key" and "elem" here, but ...
>>
>>> +          void (*swap)(void *a, void *b, size_t size))
>>
>> ... "a" and "b" here? The first example of users of sort() that I'm
>> looking at right now (x86/extable.c) is consistent in its naming.
>>
> 
> On the Arm side there are {cmp,swap}_memory_node() and
> {cmp,swap}_mmio_handler(): "key"/"elem" are used for the comparison
> and "_a"/"_b" for the swap.

So - re-raising a question Stefano did raise - is Misra concerned about
such discrepancies? If yes, _all_ instances need harmonizing. If not, I
see no reason to go with misleading names here.

Jan

Re: [XEN PATCH 1/5] xen/common: address violations of MISRA C:2012 Rule 8.2

[Stefano Stabellini]

On Mon, 19 Nov 2023, Federico Serafini wrote:
> On 18/11/23 03:59, Stefano Stabellini wrote:
> > On Fri, 17 Nov 2023, Federico Serafini wrote:
> > > diff --git a/xen/common/stop_machine.c b/xen/common/stop_machine.c
> > > index 3adbe380de..398cfd507c 100644
> > > --- a/xen/common/stop_machine.c
> > > +++ b/xen/common/stop_machine.c
> > > @@ -46,7 +46,7 @@ struct stopmachine_data {
> > >         unsigned int fn_cpu;
> > >       int fn_result;
> > > -    int (*fn)(void *);
> > > +    int (*fn)(void *data);
> > >       void *fn_data;
> > >   };
> > 
> > At least one of the possible function used here calls the parameter
> > "arg", see take_cpu_down. But I don't think it is a MISRA requirement to
> > also harmonize those?
> > 
> > 
> > > @@ -73,7 +73,7 @@ static void stopmachine_wait_state(void)
> > >    * mandatory to be called only on an idle vcpu, as otherwise active core
> > >    * scheduling might hang.
> > >    */
> > > -int stop_machine_run(int (*fn)(void *), void *data, unsigned int cpu)
> > > +int stop_machine_run(int (*fn)(void *data), void *data, unsigned int cpu)
> > >   {
> > >       unsigned int i, nr_cpus;
> > >       unsigned int this = smp_processor_id();
> > > diff --git a/xen/common/tasklet.c b/xen/common/tasklet.c
> > > index 3ad67b5c24..3649798e6b 100644
> > > --- a/xen/common/tasklet.c
> > > +++ b/xen/common/tasklet.c
> > > @@ -199,7 +199,7 @@ static void migrate_tasklets_from_cpu(unsigned int
> > > cpu, struct list_head *list)
> > >       spin_unlock_irqrestore(&tasklet_lock, flags);
> > >   }
> > >   -void tasklet_init(struct tasklet *t, void (*func)(void *), void *data)
> > > +void tasklet_init(struct tasklet *t, void (*func)(void *data), void
> > > *data)
> > >   {
> > >       memset(t, 0, sizeof(*t));
> > >       INIT_LIST_HEAD(&t->list);
> > > @@ -208,7 +208,8 @@ void tasklet_init(struct tasklet *t, void (*func)(void
> > > *), void *data)
> > >       t->data = data;
> > >   }
> > >   -void softirq_tasklet_init(struct tasklet *t, void (*func)(void *), void
> > > *data)
> > > +void softirq_tasklet_init(struct tasklet *t,
> > > +                          void (*func)(void *data), void *data)
> > >   {
> > >       tasklet_init(t, func, data);
> > >       t->is_softirq = 1;
> > > diff --git a/xen/common/timer.c b/xen/common/timer.c
> > > index 0fddfa7487..bf7792dcb3 100644
> > > --- a/xen/common/timer.c
> > > +++ b/xen/common/timer.c
> > > @@ -291,7 +291,7 @@ static bool active_timer(const struct timer *timer)
> > >     void init_timer(
> > >       struct timer *timer,
> > > -    void        (*function)(void *),
> > > +    void        (*function)(void *data),
> > >       void         *data,
> > >       unsigned int  cpu)
> > >   {
> > > @@ -441,7 +441,7 @@ void kill_timer(struct timer *timer)
> > >     static void execute_timer(struct timers *ts, struct timer *t)
> > >   {
> > > -    void (*fn)(void *) = t->function;
> > > +    void (*fn)(void *data) = t->function;
> > >       void *data = t->data;
> > >         t->status = TIMER_STATUS_inactive;
> > > diff --git a/xen/include/xen/rangeset.h b/xen/include/xen/rangeset.h
> > > index 135f33f606..390f7b6082 100644
> > > --- a/xen/include/xen/rangeset.h
> > > +++ b/xen/include/xen/rangeset.h
> > > @@ -68,7 +68,7 @@ bool_t __must_check rangeset_overlaps_range(
> > >       struct rangeset *r, unsigned long s, unsigned long e);
> > >   int rangeset_report_ranges(
> > >       struct rangeset *r, unsigned long s, unsigned long e,
> > > -    int (*cb)(unsigned long s, unsigned long e, void *), void *ctxt);
> > > +    int (*cb)(unsigned long s, unsigned long e, void *data), void *ctxt);
> > 
> > Also here some of the functions use "arg" instead of ctxt
> > 
> > 
> > >   /*
> > >    * Note that the consume function can return an error value apart from
> > > @@ -77,7 +77,7 @@ int rangeset_report_ranges(
> > >    */
> > >   int rangeset_consume_ranges(struct rangeset *r,
> > >                               int (*cb)(unsigned long s, unsigned long e,
> > > -                                      void *, unsigned long *c),
> > > +                                      void *ctxt, unsigned long *c),
> > >                               void *ctxt);
> > 
> > Also here some of the functions use "dom" like irq_remove_cb.
> > 
> > 
> > But I actually like the patch as is, so if that's OK from a MISRA point
> > of view then I would give my reviewed-by.
> 
> Yes, this is OK for MISRA.


Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Re: [XEN PATCH 3/5] xen/sort: address violations of MISRA C:2012 Rule 8.2

[Stefano Stabellini]

On Mon, 20 Nov 2023, Jan Beulich wrote:
> On 20.11.2023 14:13, Federico Serafini wrote:
> > On 20/11/23 10:02, Jan Beulich wrote:
> >> On 17.11.2023 09:40, Federico Serafini wrote:
> >>> --- a/xen/include/xen/sort.h
> >>> +++ b/xen/include/xen/sort.h
> >>> @@ -23,8 +23,8 @@
> >>>   extern gnu_inline
> >>>   #endif
> >>>   void sort(void *base, size_t num, size_t size,
> >>> -          int (*cmp)(const void *, const void *),
> >>> -          void (*swap)(void *, void *, size_t))
> >>> +          int (*cmp)(const void *key, const void *elem),
> >>
> >> Why "key" and "elem" here, but ...
> >>
> >>> +          void (*swap)(void *a, void *b, size_t size))
> >>
> >> ... "a" and "b" here? The first example of users of sort() that I'm
> >> looking at right now (x86/extable.c) is consistent in its naming.
> >>
> > 
> > On the Arm side there are {cmp,swap}_memory_node() and
> > {cmp,swap}_mmio_handler(): "key"/"elem" are used for the comparison
> > and "_a"/"_b" for the swap.
> 
> So - re-raising a question Stefano did raise - is Misra concerned about
> such discrepancies? If yes, _all_ instances need harmonizing. If not, I
> see no reason to go with misleading names here.

Federico confirmed that the answer is "no".

I think we can use "key" and "elem" in this patch as they are more
informative than "a" and "b"

Re: [XEN PATCH 3/5] xen/sort: address violations of MISRA C:2012 Rule 8.2

[Jan Beulich]

On 21.11.2023 01:04, Stefano Stabellini wrote:
> On Mon, 20 Nov 2023, Jan Beulich wrote:
>> On 20.11.2023 14:13, Federico Serafini wrote:
>>> On 20/11/23 10:02, Jan Beulich wrote:
>>>> On 17.11.2023 09:40, Federico Serafini wrote:
>>>>> --- a/xen/include/xen/sort.h
>>>>> +++ b/xen/include/xen/sort.h
>>>>> @@ -23,8 +23,8 @@
>>>>>   extern gnu_inline
>>>>>   #endif
>>>>>   void sort(void *base, size_t num, size_t size,
>>>>> -          int (*cmp)(const void *, const void *),
>>>>> -          void (*swap)(void *, void *, size_t))
>>>>> +          int (*cmp)(const void *key, const void *elem),
>>>>
>>>> Why "key" and "elem" here, but ...
>>>>
>>>>> +          void (*swap)(void *a, void *b, size_t size))
>>>>
>>>> ... "a" and "b" here? The first example of users of sort() that I'm
>>>> looking at right now (x86/extable.c) is consistent in its naming.
>>>>
>>>
>>> On the Arm side there are {cmp,swap}_memory_node() and
>>> {cmp,swap}_mmio_handler(): "key"/"elem" are used for the comparison
>>> and "_a"/"_b" for the swap.
>>
>> So - re-raising a question Stefano did raise - is Misra concerned about
>> such discrepancies? If yes, _all_ instances need harmonizing. If not, I
>> see no reason to go with misleading names here.
> 
> Federico confirmed that the answer is "no".
> 
> I think we can use "key" and "elem" in this patch as they are more
> informative than "a" and "b"

Except that "key" and "elem" are (imo) inapplicable to sort() callbacks
(and inconsistent with the naming in the 2nd callback here); they _may_
be applicable in bsearch() ones. Note also how in the C99 spec these
parameters of callback functions don't have names either.

Jan

Re: [XEN PATCH 3/5] xen/sort: address violations of MISRA C:2012 Rule 8.2

[Stefano Stabellini]

On Tue, 21 Nov 2023, Jan Beulich wrote:
> On 21.11.2023 01:04, Stefano Stabellini wrote:
> > On Mon, 20 Nov 2023, Jan Beulich wrote:
> >> On 20.11.2023 14:13, Federico Serafini wrote:
> >>> On 20/11/23 10:02, Jan Beulich wrote:
> >>>> On 17.11.2023 09:40, Federico Serafini wrote:
> >>>>> --- a/xen/include/xen/sort.h
> >>>>> +++ b/xen/include/xen/sort.h
> >>>>> @@ -23,8 +23,8 @@
> >>>>>   extern gnu_inline
> >>>>>   #endif
> >>>>>   void sort(void *base, size_t num, size_t size,
> >>>>> -          int (*cmp)(const void *, const void *),
> >>>>> -          void (*swap)(void *, void *, size_t))
> >>>>> +          int (*cmp)(const void *key, const void *elem),
> >>>>
> >>>> Why "key" and "elem" here, but ...
> >>>>
> >>>>> +          void (*swap)(void *a, void *b, size_t size))
> >>>>
> >>>> ... "a" and "b" here? The first example of users of sort() that I'm
> >>>> looking at right now (x86/extable.c) is consistent in its naming.
> >>>>
> >>>
> >>> On the Arm side there are {cmp,swap}_memory_node() and
> >>> {cmp,swap}_mmio_handler(): "key"/"elem" are used for the comparison
> >>> and "_a"/"_b" for the swap.
> >>
> >> So - re-raising a question Stefano did raise - is Misra concerned about
> >> such discrepancies? If yes, _all_ instances need harmonizing. If not, I
> >> see no reason to go with misleading names here.
> > 
> > Federico confirmed that the answer is "no".
> > 
> > I think we can use "key" and "elem" in this patch as they are more
> > informative than "a" and "b"
> 
> Except that "key" and "elem" are (imo) inapplicable to sort() callbacks
> (and inconsistent with the naming in the 2nd callback here); they _may_
> be applicable in bsearch() ones. Note also how in the C99 spec these
> parameters of callback functions don't have names either.

Yes, reading the example in extable.c I think you are right. Maybe it is
better to use "a" and "b" in both cmp and swap if you agree.

Re: [XEN PATCH 3/5] xen/sort: address violations of MISRA C:2012 Rule 8.2

[Jan Beulich]

On 22.11.2023 02:19, Stefano Stabellini wrote:
> On Tue, 21 Nov 2023, Jan Beulich wrote:
>> On 21.11.2023 01:04, Stefano Stabellini wrote:
>>> On Mon, 20 Nov 2023, Jan Beulich wrote:
>>>> On 20.11.2023 14:13, Federico Serafini wrote:
>>>>> On 20/11/23 10:02, Jan Beulich wrote:
>>>>>> On 17.11.2023 09:40, Federico Serafini wrote:
>>>>>>> --- a/xen/include/xen/sort.h
>>>>>>> +++ b/xen/include/xen/sort.h
>>>>>>> @@ -23,8 +23,8 @@
>>>>>>>   extern gnu_inline
>>>>>>>   #endif
>>>>>>>   void sort(void *base, size_t num, size_t size,
>>>>>>> -          int (*cmp)(const void *, const void *),
>>>>>>> -          void (*swap)(void *, void *, size_t))
>>>>>>> +          int (*cmp)(const void *key, const void *elem),
>>>>>>
>>>>>> Why "key" and "elem" here, but ...
>>>>>>
>>>>>>> +          void (*swap)(void *a, void *b, size_t size))
>>>>>>
>>>>>> ... "a" and "b" here? The first example of users of sort() that I'm
>>>>>> looking at right now (x86/extable.c) is consistent in its naming.
>>>>>>
>>>>>
>>>>> On the Arm side there are {cmp,swap}_memory_node() and
>>>>> {cmp,swap}_mmio_handler(): "key"/"elem" are used for the comparison
>>>>> and "_a"/"_b" for the swap.
>>>>
>>>> So - re-raising a question Stefano did raise - is Misra concerned about
>>>> such discrepancies? If yes, _all_ instances need harmonizing. If not, I
>>>> see no reason to go with misleading names here.
>>>
>>> Federico confirmed that the answer is "no".
>>>
>>> I think we can use "key" and "elem" in this patch as they are more
>>> informative than "a" and "b"
>>
>> Except that "key" and "elem" are (imo) inapplicable to sort() callbacks
>> (and inconsistent with the naming in the 2nd callback here); they _may_
>> be applicable in bsearch() ones. Note also how in the C99 spec these
>> parameters of callback functions don't have names either.
> 
> Yes, reading the example in extable.c I think you are right. Maybe it is
> better to use "a" and "b" in both cmp and swap if you agree.

Using a and b is (as it looks) in line with at least some uses we have, so
less code churn than going with some other, more descriptive names (like
left/right). So yes, I'm okay with using a/b.

Jan

Re: [XEN PATCH 3/5] xen/sort: address violations of MISRA C:2012 Rule 8.2

[Federico Serafini]

On 22/11/23 09:01, Jan Beulich wrote:
> On 22.11.2023 02:19, Stefano Stabellini wrote:
>> On Tue, 21 Nov 2023, Jan Beulich wrote:
>>> On 21.11.2023 01:04, Stefano Stabellini wrote:
>>>> On Mon, 20 Nov 2023, Jan Beulich wrote:
>>>>> On 20.11.2023 14:13, Federico Serafini wrote:
>>>>>> On 20/11/23 10:02, Jan Beulich wrote:
>>>>>>> On 17.11.2023 09:40, Federico Serafini wrote:
>>>>>>>> --- a/xen/include/xen/sort.h
>>>>>>>> +++ b/xen/include/xen/sort.h
>>>>>>>> @@ -23,8 +23,8 @@
>>>>>>>>    extern gnu_inline
>>>>>>>>    #endif
>>>>>>>>    void sort(void *base, size_t num, size_t size,
>>>>>>>> -          int (*cmp)(const void *, const void *),
>>>>>>>> -          void (*swap)(void *, void *, size_t))
>>>>>>>> +          int (*cmp)(const void *key, const void *elem),
>>>>>>>
>>>>>>> Why "key" and "elem" here, but ...
>>>>>>>
>>>>>>>> +          void (*swap)(void *a, void *b, size_t size))
>>>>>>>
>>>>>>> ... "a" and "b" here? The first example of users of sort() that I'm
>>>>>>> looking at right now (x86/extable.c) is consistent in its naming.
>>>>>>>
>>>>>>
>>>>>> On the Arm side there are {cmp,swap}_memory_node() and
>>>>>> {cmp,swap}_mmio_handler(): "key"/"elem" are used for the comparison
>>>>>> and "_a"/"_b" for the swap.
>>>>>
>>>>> So - re-raising a question Stefano did raise - is Misra concerned about
>>>>> such discrepancies? If yes, _all_ instances need harmonizing. If not, I
>>>>> see no reason to go with misleading names here.
>>>>
>>>> Federico confirmed that the answer is "no".
>>>>
>>>> I think we can use "key" and "elem" in this patch as they are more
>>>> informative than "a" and "b"
>>>
>>> Except that "key" and "elem" are (imo) inapplicable to sort() callbacks
>>> (and inconsistent with the naming in the 2nd callback here); they _may_
>>> be applicable in bsearch() ones. Note also how in the C99 spec these
>>> parameters of callback functions don't have names either.
>>
>> Yes, reading the example in extable.c I think you are right. Maybe it is
>> better to use "a" and "b" in both cmp and swap if you agree.
> 
> Using a and b is (as it looks) in line with at least some uses we have, so
> less code churn than going with some other, more descriptive names (like
> left/right). So yes, I'm okay with using a/b.

I'll send a new version.

-- 
Federico Serafini, M.Sc.

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

Re: [XEN PATCH 1/5] xen/common: address violations of MISRA C:2012 Rule 8.2

[Jan Beulich]

On 21.11.2023 01:02, Stefano Stabellini wrote:
> On Mon, 19 Nov 2023, Federico Serafini wrote:
>> On 18/11/23 03:59, Stefano Stabellini wrote:
>>> On Fri, 17 Nov 2023, Federico Serafini wrote:
>>>> diff --git a/xen/common/stop_machine.c b/xen/common/stop_machine.c
>>>> index 3adbe380de..398cfd507c 100644
>>>> --- a/xen/common/stop_machine.c
>>>> +++ b/xen/common/stop_machine.c
>>>> @@ -46,7 +46,7 @@ struct stopmachine_data {
>>>>         unsigned int fn_cpu;
>>>>       int fn_result;
>>>> -    int (*fn)(void *);
>>>> +    int (*fn)(void *data);
>>>>       void *fn_data;
>>>>   };
>>>
>>> At least one of the possible function used here calls the parameter
>>> "arg", see take_cpu_down. But I don't think it is a MISRA requirement to
>>> also harmonize those?
>>>
>>>
>>>> @@ -73,7 +73,7 @@ static void stopmachine_wait_state(void)
>>>>    * mandatory to be called only on an idle vcpu, as otherwise active core
>>>>    * scheduling might hang.
>>>>    */
>>>> -int stop_machine_run(int (*fn)(void *), void *data, unsigned int cpu)
>>>> +int stop_machine_run(int (*fn)(void *data), void *data, unsigned int cpu)
>>>>   {
>>>>       unsigned int i, nr_cpus;
>>>>       unsigned int this = smp_processor_id();
>>>> diff --git a/xen/common/tasklet.c b/xen/common/tasklet.c
>>>> index 3ad67b5c24..3649798e6b 100644
>>>> --- a/xen/common/tasklet.c
>>>> +++ b/xen/common/tasklet.c
>>>> @@ -199,7 +199,7 @@ static void migrate_tasklets_from_cpu(unsigned int
>>>> cpu, struct list_head *list)
>>>>       spin_unlock_irqrestore(&tasklet_lock, flags);
>>>>   }
>>>>   -void tasklet_init(struct tasklet *t, void (*func)(void *), void *data)
>>>> +void tasklet_init(struct tasklet *t, void (*func)(void *data), void
>>>> *data)
>>>>   {
>>>>       memset(t, 0, sizeof(*t));
>>>>       INIT_LIST_HEAD(&t->list);
>>>> @@ -208,7 +208,8 @@ void tasklet_init(struct tasklet *t, void (*func)(void
>>>> *), void *data)
>>>>       t->data = data;
>>>>   }
>>>>   -void softirq_tasklet_init(struct tasklet *t, void (*func)(void *), void
>>>> *data)
>>>> +void softirq_tasklet_init(struct tasklet *t,
>>>> +                          void (*func)(void *data), void *data)
>>>>   {
>>>>       tasklet_init(t, func, data);
>>>>       t->is_softirq = 1;
>>>> diff --git a/xen/common/timer.c b/xen/common/timer.c
>>>> index 0fddfa7487..bf7792dcb3 100644
>>>> --- a/xen/common/timer.c
>>>> +++ b/xen/common/timer.c
>>>> @@ -291,7 +291,7 @@ static bool active_timer(const struct timer *timer)
>>>>     void init_timer(
>>>>       struct timer *timer,
>>>> -    void        (*function)(void *),
>>>> +    void        (*function)(void *data),
>>>>       void         *data,
>>>>       unsigned int  cpu)
>>>>   {
>>>> @@ -441,7 +441,7 @@ void kill_timer(struct timer *timer)
>>>>     static void execute_timer(struct timers *ts, struct timer *t)
>>>>   {
>>>> -    void (*fn)(void *) = t->function;
>>>> +    void (*fn)(void *data) = t->function;
>>>>       void *data = t->data;
>>>>         t->status = TIMER_STATUS_inactive;
>>>> diff --git a/xen/include/xen/rangeset.h b/xen/include/xen/rangeset.h
>>>> index 135f33f606..390f7b6082 100644
>>>> --- a/xen/include/xen/rangeset.h
>>>> +++ b/xen/include/xen/rangeset.h
>>>> @@ -68,7 +68,7 @@ bool_t __must_check rangeset_overlaps_range(
>>>>       struct rangeset *r, unsigned long s, unsigned long e);
>>>>   int rangeset_report_ranges(
>>>>       struct rangeset *r, unsigned long s, unsigned long e,
>>>> -    int (*cb)(unsigned long s, unsigned long e, void *), void *ctxt);
>>>> +    int (*cb)(unsigned long s, unsigned long e, void *data), void *ctxt);
>>>
>>> Also here some of the functions use "arg" instead of ctxt
>>>
>>>
>>>>   /*
>>>>    * Note that the consume function can return an error value apart from
>>>> @@ -77,7 +77,7 @@ int rangeset_report_ranges(
>>>>    */
>>>>   int rangeset_consume_ranges(struct rangeset *r,
>>>>                               int (*cb)(unsigned long s, unsigned long e,
>>>> -                                      void *, unsigned long *c),
>>>> +                                      void *ctxt, unsigned long *c),
>>>>                               void *ctxt);
>>>
>>> Also here some of the functions use "dom" like irq_remove_cb.
>>>
>>>
>>> But I actually like the patch as is, so if that's OK from a MISRA point
>>> of view then I would give my reviewed-by.
>>
>> Yes, this is OK for MISRA.
> 
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

To cover EFI:
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan