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

[Quentin Casasnovas]

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@xxxxxxx>
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/