From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1751280AbdAQVSv (ORCPT <rfc822;w@1wt.eu>);
        Tue, 17 Jan 2017 16:18:51 -0500
Received: from Galois.linutronix.de ([146.0.238.70]:46451 "EHLO
        Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1751031AbdAQVSj (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Tue, 17 Jan 2017 16:18:39 -0500
Date: Tue, 17 Jan 2017 22:18:34 +0100 (CET)
From: Thomas Gleixner <tglx@linutronix.de>
To: Borislav Petkov <bp@alien8.de>
cc: X86 ML <x86@kernel.org>, LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 11/13] x86/microcode/AMD: Unify load_ucode_amd_ap()
In-Reply-To: <alpine.DEB.2.20.1701172145470.3645@nanos>
Message-ID: <alpine.DEB.2.20.1701172214300.3645@nanos>
References: <20170117173734.14251-1-bp@alien8.de> <20170117173734.14251-12-bp@alien8.de> <alpine.DEB.2.20.1701172145470.3645@nanos>
User-Agent: Alpine 2.20 (DEB 67 2015-01-07)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, 17 Jan 2017, Thomas Gleixner wrote:

> On Tue, 17 Jan 2017, Borislav Petkov wrote:
> >  void load_ucode_amd_ap(unsigned int cpuid_1_eax)
> >  {
> >  	struct equiv_cpu_entry *eq;
> >  	struct microcode_amd *mc;
> > +	struct cont_desc *desc;
> >  	u16 eq_id;
> >  
> > +	if (IS_ENABLED(CONFIG_X86_32)) {
> > +		mc   = (struct microcode_amd *)__pa_nodebug(amd_ucode_patch);
> > +		desc = (struct cont_desc *)__pa_nodebug(&cont);
> > +	} else {
> > +		mc   = (struct microcode_amd *)amd_ucode_patch;
> > +		desc = &cont;
> 
> Bah! Now I realize that 'cont' is not a local variable as I assumed when
> looking at the other patch. 'cont' is a pretty bad name for a (file) global
> variable. Can we please use a more obvious name ?
> 
> While at it please make that thing static as there cant be a user outside
> of that file.
> 
> > +	}
> > +
> >  	/* First AP hasn't cached it yet, go through the blob. */
> > -	if (!cont.data) {
> > -		struct cpio_data cp;
> > +	if (!desc->data) {
> > +		struct cpio_data cp = { };
> >  
> > -		if (cont.size == -1)
> > +		if (desc->size == -1)
> >  			return;
> 
> I'm not really fond of abusing the size member for this. And that '-1'
> seems to have different meanings depending on other members. Really not
> intuitive.
> 
> Please introduce a proper state member which tells what the descriptor
> struct contains, i.e. EMPTY, VALID, INVALID ....

Reading further through the series I see that you remove all that cruft at
the end. Dammit, I never start reading books in the final chapter, maybe I
should do that with patch series :)

It might be nice nevertheless to have an initial patch which cleans up the
name of that 'cont' variable and that status thing, but it might not be
worth the trouble as you remove it at the end anyway, which makes a lot of
sense btw. Your call.

Thanks,

	tglx