Re: [PATCH v2 1/2] x86/cpufeature: Add facility to match microcode revisions

[Andi Kleen]

On Sat, Oct 20, 2018 at 10:19:37AM +0200, Thomas Gleixner wrote:
> 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.

It will be useful with the driver_data pointer, which you short sightedly
forced me to remove, and likely will need to be readded at some point
anyways if this gets more widely used. At least with the pointer not
all callers will need to be changed then. 

Also it's symmetric with how the PCI and USB and the existing x86 match
discovery interfaces work.

> > > 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.

Ok then please apply it. 

-Andi