Re: [PATCH 07/13] x86/microcode/intel: Rename update_match_revision()

Quentin Casasnovas
2015-04-10T11:52:57Z

On Fri, Apr 10, 2015 at 01:12:18PM +0200, Borislav Petkov wrote:
> On Tue, Feb 24, 2015 at 05:23:18PM +0100, Quentin Casasnovas wrote:
> > Minor nit-pick, if you reverse your inequality, you don't need for the
> > ternary operator.
>
> Yeah, so I started looking at that and it seems the rabbit hole goes
> deeper.
>
> Let's look at the call to revision_is_newer() in _save_mc():
>
> save_mc:
>
> new_rev = mc_hdr->rev;
>
> ...
>
> if (!revision_is_newer(mc_hdr, new_rev))
> ->

Ha good catch!

BTW, I could not find that the 'rev' argument to get_matching_sig() was
used at all..

>
> ---
> From: Borislav Petkov <bp@suse.de>
> Date: Fri, 10 Apr 2015 12:50:57 +0200
> Subject: [PATCH] x86/microcode/intel: Get rid of revision_is_newer()
>
> ...
>
> diff --git a/arch/x86/kernel/cpu/microcode/intel_lib.c b/arch/x86/kernel/cpu/microcode/intel_lib.c
> index cd47a510a3f1..63b0a2e059ee 100644
> --- a/arch/x86/kernel/cpu/microcode/intel_lib.c
> +++ b/arch/x86/kernel/cpu/microcode/intel_lib.c
> @@ -154,13 +154,13 @@ int get_matching_sig(unsigned int csig, int cpf, int rev, void *mc)
> /*
> * Returns 1 if update has been found, 0 otherwise.
> */
> -int get_matching_microcode(unsigned int csig, int cpf, int rev, void *mc)
> +int get_matching_microcode(unsigned int csig, int cpf, int new_rev, void *mc)

If we're to rename 'rev', maybe calling it 'cpu_rev' would make it more
obvious where this variable is coming from?

> {
> struct microcode_header_intel *mc_hdr = mc;
>
> - if (!revision_is_newer(mc_hdr, rev))
> + if (mc_hdr->rev <= new_rev)
> return 0;
>
> - return get_matching_sig(csig, cpf, rev, mc);
> + return get_matching_sig(csig, cpf, new_rev, mc);
> }
> EXPORT_SYMBOL_GPL(get_matching_microcode);

Anyway you patch looks good to me!

Quentin