Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sat, 20 Oct 2018 10:19:37 +0200 (CEST)
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Andi Kleen <ak@linux.intel.com>
cc:     Andi Kleen <andi@firstfloor.org>, peterz@infradead.org,
        x86@kernel.org, eranian@google.com, kan.liang@intel.com,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/2] x86/cpufeature: Add facility to match microcode
 revisions
In-Reply-To: <20181019234743.GA27951@tassilo.jf.intel.com>
Message-ID: <alpine.DEB.2.21.1810201017070.1651@nanos.tec.linutronix.de>
References: <20181010162608.23899-1-andi@firstfloor.org> <alpine.DEB.2.21.1810171151010.6000@nanos.tec.linutronix.de> <20181019234743.GA27951@tassilo.jf.intel.com>
User-Agent: Alpine 2.21 (DEB 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, 19 Oct 2018, Andi Kleen wrote:

> > > +	u32 min_ucode;
> > > +};
> > > +
> > > +const struct x86_ucode_id *x86_match_ucode(const struct x86_ucode_id *match)
> > 
> > What's the point of returning the struct pointer? Shouldn't it be enough to
> > make it return bool? Also the function name really should reflect that this
> > checks whether the minimal required microcode revision is active.
> 
> This allows the user to find the table entry to tie something to it
> (e.g. use the index to match some other table)
> 
> Same pattern as pci discovery etc. use.
> 
> Given the current caller doesn't need it, but we still follow standard
> conventions.

There is no point to return the pointer because it's not a compound
structure. If you want to provide the possibility to use the index then
return the index and an error code if it does not match.

> > 
> > > +{
> > > +	struct cpuinfo_x86 *c = &boot_cpu_data;
> > > +	const struct x86_ucode_id *m;
> > > +
> > > +	for (m = match; m->vendor | m->family | m->model; m++) {
> > 
> > VENDOR_INTEL = 0, so this check is obscure to begin with. Either you chose
> > a explicit condition to put at the end of the table, e.g. vendor = U8_MAX
> > or you hand in the array size to the function.
> 
> That would both be awkward. It's the same as match_cpu, and 0 terminators
> are standard practice in practical all similar code. I removed
> the or with the family.

That's debatable because it's more easy to miss the terminator than getting
the ARRAY_SIZE() argument wrong. But it doesn't matter much.

Thanks,

	tglx