Messages in this thread | | | Date | Sat, 20 Oct 2018 10:19:37 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [PATCH v2 1/2] x86/cpufeature: Add facility to match microcode revisions |
| |
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
| |