lkml.org 
[lkml]   [2018]   [Oct]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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


    \
     
     \ /
      Last update: 2018-10-20 10:21    [W:5.677 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site