Re: [PATCH 05/10] x86/ucode: Fold microcode_grab_module() into its single caller

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 10/28/24 05:18, Andrew Cooper wrote:
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> ---
>   xen/arch/x86/cpu/microcode/core.c | 23 +++++++++--------------
>   1 file changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/xen/arch/x86/cpu/microcode/core.c 
> b/xen/arch/x86/cpu/microcode/core.c
> index 0b00e70f3b07..4c4003bf9687 100644
> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -205,19 +205,6 @@ static void __init microcode_scan_module(struct boot_info 
> *bi)
>       }
>   }
>   
> -static void __init microcode_grab_module(struct boot_info *bi)
> -{
> -    if ( ucode_mod_idx < 0 )
> -        ucode_mod_idx += bi->nr_modules;
> -    if ( ucode_mod_idx <= 0 || ucode_mod_idx >= bi->nr_modules ||
> -         !__test_and_clear_bit(ucode_mod_idx, bi->module_map) )
> -        goto scan;
> -    ucode_mod = *bi->mods[ucode_mod_idx].mod;
> -scan:
> -    if ( ucode_scan )
> -        microcode_scan_module(bi);
> -}
> -
>   static struct microcode_ops __ro_after_init ucode_ops;
>   
>   static DEFINE_SPINLOCK(microcode_mutex);
> @@ -831,7 +818,15 @@ static int __init early_microcode_load(struct boot_info 
> *bi)
>       size_t size;
>       struct microcode_patch *patch;
>   
> -    microcode_grab_module(bi);
> +    if ( ucode_mod_idx < 0 )
> +        ucode_mod_idx += bi->nr_modules;
> +    if ( ucode_mod_idx <= 0 || ucode_mod_idx >= bi->nr_modules ||
> +         !__test_and_clear_bit(ucode_mod_idx, bi->module_map) )
> +        goto scan;
> +    ucode_mod = *bi->mods[ucode_mod_idx].mod;
> + scan:
> +    if ( ucode_scan )
> +        microcode_scan_module(bi);
>   
>       if ( !ucode_mod.mod_end && !ucode_blob.size )
>           return 0;
Just a suggestion, but would it be useful to add a comment to explain 
the three conditions of ucode_mod_idx that are being handled before 
scanning is attempted? These being negative value, a valid module index, 
and module index wasn't already claimed in the module map.
Otherwise,

Reviewed-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>