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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.11.2024 18:18, Andrew Cooper wrote:
> 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.

I wasn't so much worrying about backporting of this work (as of now I don't
think it's a candidate), but anything that's yet to come.

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

Why would that be? Doing the check the wrong way round would lead to
possible downgrading of ucode, wouldn't it?

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

Either would be fine with me as a hook name. As a function name I'm less
certain this will (remain to) be unambiguous.

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

is_newer() doesn't very well lend itself to a tristate return value.

Jan