Re: [PATCH v2 3/3] x86/ucode: Drop MIS_UCODE and microcode_match_result

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 14/11/2024 11:41 am, Jan Beulich wrote:
> On 12.11.2024 22:19, Andrew Cooper wrote:
>> @@ -199,8 +198,8 @@ static bool microcode_fits_cpu(const struct microcode_patch *patch)
>>      return equiv.id == patch->processor_rev_id;
>>  }
>>  
>> -static enum microcode_match_result cf_check compare_patch(
>> -    const struct microcode_patch *new, const struct microcode_patch *old)
>> +static int cf_check compare_patch(
>> +    const struct microcode_patch *old, const struct microcode_patch *new)
>>  {
> Let's hope we won't screw up a backport because of this swapping.

I wasn't going to start thinking about backports until the code gets
into a better state.

But if backports do happen, it will be all-or-nothing.  This code is far
too tangled.


That said, in this specific case, the only thing that would go wrong is
with Intel debug patches.  Even I've only had a handful of those in the
past 8 years.

> I'd like to ask to at least consider renaming at least the functions,
> perhaps also the hook pointer, perhaps simply by switching from singular
> to plural. This would then also avoid reviewers like me to go hunt for
> all uses of the function/hook, in an attempt to make sure none was left
> out when converting.

In the other series I've paused for a while, I have renamed some hooks
(along with related cleanup), but I'm undecided on this one.

One option is cmp(), or perhaps compare().

But, it occurs to me, another option would be is_newer().  We always
care about the operation one way around.

>
>> @@ -54,11 +47,17 @@ struct microcode_ops {
>>                             unsigned int flags);
>>  
>>      /*
>> -     * Given two patches, are they both applicable to the current CPU, and is
>> -     * new a higher revision than old?
>> +     * Given a current patch, and a proposed new patch, order them based on revision.
>> +     *
>> +     * This operation is not necessarily symmetrical.  In some cases, a debug
>> +     * "new" patch will always considered to be newer, on the expectation that
>> +     * whomever is using debug patches knows exactly what they're doing.
>>       */
>> -    enum microcode_match_result (*compare_patch)(
>> -        const struct microcode_patch *new, const struct microcode_patch *old);
>> +#define OLD_UCODE  -1
> Nit: I'm pretty sure Misra wants parentheses here.

Oh yes, so it does.  Rule 20.7 apparently.  Fine.

> Preferably with both (mechanical) adjustments:
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

~Andrew