Re: [PATCH for-next] xen/kexec: Style fixes

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jun 10, 2026 at 04:41:10PM +0100, Andrew Cooper wrote:
> Adjust kexec and kimage to more closely adhere to Xen style.
> 
> Sort the includes, dropping duplicates (kexec.h) and unused (ctype.h and
> kernel.h).
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

I've made some further suggestions below.

> ---
> CC: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> CC: Michal Orzel <michal.orzel@xxxxxxx>
> CC: Jan Beulich <jbeulich@xxxxxxxx>
> CC: Julien Grall <julien@xxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Kevin Lampis <kevin.lampis@xxxxxxxxxx>
> 
> Fix these before they get copied around in the EFI changes.
> ---
>  xen/common/kexec.c  | 94 +++++++++++++++++++++++++--------------------
>  xen/common/kimage.c | 23 ++++++-----
>  2 files changed, 64 insertions(+), 53 deletions(-)
> 
> diff --git a/xen/common/kexec.c b/xen/common/kexec.c
> index 65776a95fd70..9ff22e43991c 100644
> --- a/xen/common/kexec.c
> +++ b/xen/common/kexec.c
> @@ -6,31 +6,33 @@
>   * - Magnus Damm <magnus@xxxxxxxxxxxxx>
>   */
>  
> -#include <xen/init.h>
> -#include <xen/lib.h>
>  #include <xen/acpi.h>
> -#include <xen/ctype.h>
> +#include <xen/console.h>
> +#include <xen/cpu.h>
> +#include <xen/cpumask.h>
>  #include <xen/elfcore.h>
>  #include <xen/errno.h>
>  #include <xen/guest_access.h>
> -#include <xen/param.h>
> -#include <xen/watchdog.h>
> -#include <xen/sched.h>
> -#include <xen/types.h>
>  #include <xen/hypercall.h>
> +#include <xen/init.h>
>  #include <xen/kexec.h>
>  #include <xen/keyhandler.h>
> -#include <public/kexec.h>
> -#include <xen/cpumask.h>
> -#include <asm/atomic.h>
> +#include <xen/kimage.h>
> +#include <xen/lib.h>
> +#include <xen/param.h>
> +#include <xen/sched.h>
>  #include <xen/spinlock.h>
> +#include <xen/types.h>
>  #include <xen/version.h>
> -#include <xen/console.h>
> -#include <xen/kexec.h>
> -#include <xen/kimage.h>
> -#include <public/elfnote.h>
> +#include <xen/watchdog.h>
> +
> +#include <asm/atomic.h>
> +
>  #include <xsm/xsm.h>
> -#include <xen/cpu.h>
> +
> +#include <public/elfnote.h>
> +#include <public/kexec.h>
> +
>  #ifdef CONFIG_COMPAT
>  #include <compat/kexec.h>
>  #endif
> @@ -162,6 +164,7 @@ static int __init cf_check parse_crashkernel(const char 
> *str)
>  
>              ++idx;
>          } while ( *str == ',' );
> +
>          if ( idx < ARRAY_SIZE(ranges) )
>              ranges[idx].size = 0;
>      }
> @@ -317,7 +320,7 @@ void kexec_crash_save_cpu(void)
>      ELF_Prstatus *prstatus;
>      crash_xen_core_t *xencore;
>  
> -    BUG_ON ( ! crash_notes );
> +    BUG_ON(!crash_notes);
>  
>      if ( cpumask_test_and_set_cpu(cpu, &crash_saved_cpus) )
>          return;
> @@ -418,6 +421,7 @@ static void cf_check do_crashdump_trigger(unsigned char 
> key)
>  static void setup_note(Elf_Note *n, const char *name, int type, int descsz)
>  {
>      int l = strlen(name) + 1;
> +
>      strlcpy(ELFNOTE_NAME(n), name, l);
>      n->namesz = l;
>      n->descsz = descsz;
> @@ -427,7 +431,7 @@ static void setup_note(Elf_Note *n, const char *name, int 
> type, int descsz)
>  static size_t sizeof_note(const char *name, int descsz)
>  {
>      return (sizeof(Elf_Note) +
> -            ELFNOTE_ALIGN(strlen(name)+1) +
> +            ELFNOTE_ALIGN(strlen(name) + 1) +
>              ELFNOTE_ALIGN(descsz));
>  }
>  
> @@ -439,7 +443,7 @@ static size_t sizeof_cpu_notes(const unsigned long cpu)
>          + sizeof_note("Xen", sizeof(crash_xen_core_t));
>  
>      /* CPU0 also presents the crash_xen_info note. */
> -    if ( ! cpu )
> +    if ( !cpu )
>          bytes = bytes +
>              sizeof_note("Xen", sizeof(crash_xen_info_t));
>  
> @@ -450,24 +454,27 @@ static size_t sizeof_cpu_notes(const unsigned long cpu)
>   * crash heap if the user has requested that crash notes be allocated
>   * in lower memory.  There is currently no case where the crash notes
>   * should be free()'d. */
> -static void * alloc_from_crash_heap(const size_t bytes)
> +static void *alloc_from_crash_heap(const size_t bytes)
>  {
> -    void * ret;
> +    void *ret;
> +
>      if ( crash_heap_current + bytes > crash_heap_end )
>          return NULL;
> -    ret = (void*)crash_heap_current;
> +
> +    ret = crash_heap_current;
>      crash_heap_current += bytes;
> +
>      return ret;
>  }
>  
>  /* Allocate a crash note buffer for a newly onlined cpu. */
>  static int kexec_init_cpu_notes(const unsigned long cpu)
>  {
> -    Elf_Note * note = NULL;
> +    Elf_Note *note = NULL;
>      int ret = 0;
>      int nr_bytes = 0;
>  
> -    BUG_ON( cpu >= nr_cpu_ids || ! crash_notes );
> +    BUG_ON(cpu >= nr_cpu_ids || !crash_notes);
>  
>      /* If already allocated, nothing to do. */
>      if ( crash_notes[cpu].start )
> @@ -505,7 +512,7 @@ static int kexec_init_cpu_notes(const unsigned long cpu)
>  
>          /* If the allocation failed, and another CPU did not beat us, give
>           * up with ENOMEM. */
> -        if ( ! note )
> +        if ( !note )
>              ret = -ENOMEM;
>          /* else all is good so lets set up the notes. */
>          else
> @@ -518,7 +525,7 @@ static int kexec_init_cpu_notes(const unsigned long cpu)
>              setup_note(note, "Xen", XEN_ELFNOTE_CRASH_REGS,
>                         sizeof(crash_xen_core_t));
>  
> -            if ( ! cpu )
> +            if ( !cpu )
>              {
>                  /* Set up Xen Crash Info note. */
>                  xen_crash_note = note = ELFNOTE_NEXT(note);
> @@ -548,8 +555,6 @@ static int cf_check cpu_callback(
>           * fail the CPU_UP_PREPARE */
>          kexec_init_cpu_notes(cpu);
>          break;
> -    default:
> -        break;
>      }

A newline here might be nice also.

>      return NOTIFY_DONE;
>  }
> @@ -592,7 +597,7 @@ static int __init cf_check kexec_init(void)
>              get_order_from_bytes(crash_heap_size),
>              MEMF_bits(crashinfo_maxaddr_bits) );
>  
> -        if ( ! crash_heap_current )
> +        if ( !crash_heap_current )
>              return -ENOMEM;
>  
>          memset(crash_heap_current, 0, crash_heap_size);
> @@ -604,7 +609,7 @@ static int __init cf_check kexec_init(void)
>         Only the individual CPU crash notes themselves must be allocated
>         in lower memory if requested. */
>      crash_notes = xzalloc_array(crash_note_range_t, nr_cpu_ids);
> -    if ( ! crash_notes )
> +    if ( !crash_notes )
>          return -ENOMEM;
>  
>      register_keyhandler('C', do_crashdump_trigger, "trigger a crashdump", 0);
> @@ -620,7 +625,8 @@ presmp_initcall(kexec_init);
>  
>  static int kexec_get_reserve(xen_kexec_range_t *range)
>  {
> -    if ( kexec_crash_area.size > 0 && kexec_crash_area.start > 0) {
> +    if ( kexec_crash_area.size > 0 && kexec_crash_area.start > 0 )
> +    {
>          range->start = kexec_crash_area.start;
>          range->size = kexec_crash_area.size;
>      }
> @@ -636,7 +642,7 @@ static int kexec_get_cpu(xen_kexec_range_t *range)
>      if ( nr < 0 || nr >= nr_cpu_ids )
>          return -ERANGE;
>  
> -    if ( ! crash_notes )
> +    if ( !crash_notes )
>          return -EINVAL;
>  
>      /* Try once again to allocate room for the crash notes.  It is just 
> possible
> @@ -726,7 +732,7 @@ static int 
> kexec_get_range_compat(XEN_GUEST_HANDLE_PARAM(void) uarg)
>      {
>          XLAT_kexec_range(&compat_range, &range);
>          if ( unlikely(__copy_to_guest(uarg, &compat_range, 1)) )
> -             ret = -EFAULT;
> +            ret = -EFAULT;
>      }
>  
>      return ret;
> @@ -760,7 +766,7 @@ void vmcoreinfo_append_str(const char *fmt, ...)
>      int r;
>      size_t note_size = sizeof(Elf_Note) + 
> ELFNOTE_ALIGN(strlen(VMCOREINFO_NOTE_NAME) + 1);
>  
> -    if (vmcoreinfo_size + note_size + sizeof(buf) > VMCOREINFO_BYTES)
> +    if ( vmcoreinfo_size + note_size + sizeof(buf) > VMCOREINFO_BYTES )
>          return;
>  
>      va_start(args, fmt);
> @@ -776,7 +782,7 @@ static void crash_save_vmcoreinfo(void)
>  {
>      size_t data_size;
>  
> -    if (vmcoreinfo_size > 0)    /* already saved */
> +    if ( vmcoreinfo_size > 0 )    /* already saved */

I would maybe move the comment so it's in its own line ahead of the
return statement.

>          return;
>  
>      data_size = VMCOREINFO_BYTES - (sizeof(Elf_Note) + 
> ELFNOTE_ALIGN(strlen(VMCOREINFO_NOTE_NAME) + 1));
> @@ -835,7 +841,7 @@ static int kexec_exec(XEN_GUEST_HANDLE_PARAM(void) uarg)
>      if ( !test_bit(base + pos, &kexec_flags) )
>          return -ENOENT;
>  
> -    switch (exec.type)
> +    switch ( exec.type )
>      {
>      case KEXEC_TYPE_DEFAULT:
>          image = kexec_image[base + pos];

Maybe also add a newline after the break in the switch here?

> @@ -917,8 +923,8 @@ static int kexec_segments_add_segment(unsigned int 
> *nr_segments,
>      unsigned int n = *nr_segments;
>  
>      /* Need a new segment? */
> -    if ( n == 0
> -         || segments[n-1].dest_maddr + segments[n-1].dest_size != maddr )
> +    if ( n == 0 ||

For consistency with the rest of the code in the file, since you are
also modifying the line possibly use !n?

> +         segments[n-1].dest_maddr + segments[n-1].dest_size != maddr )

Spaces between operator and operands?

Thanks, Roger.