Messages in this thread | | | Date | Wed, 27 Sep 2023 13:28:00 +0200 | From | Borislav Petkov <> | Subject | Re: [patch V3 21/30] x86/microcode: Add per CPU result state |
| |
On Tue, Sep 26, 2023 at 11:09:01AM +0200, Thomas Gleixner wrote: > That starts to get silly. The struct is used only in the microcode realm > and nothing which is globally visible. ucode is a pretty obvious and > established shortcut. But so what....
Ok, which prefix do you propose?
"microcode_", "ucode_"?
And I chose "microcode_" a while back and planned on converting stuff gradually when touching the code and not do solely a renaming patch.
All I'm saying is, we should be consistent.
> > You could do > > > > static DEFINE_PER_CPU(struct microcode_ctrl, ucode_ctrl); > > > > so that the naming is different too. > > And that solves what?
I find it somewhat confusing when the variable name is called the same name as the struct and I try to have the struct names be more expressive than the variables of the same type.
But not a big deal.
> > No need for "ucode_" prefixes to static functions. > > What's the problem with that prefix? The function name clearly says what > this is doing.
Giving proper prefixes only to the externally visible functions is, I think, a nice way of showing what is what. The static, internally used symbols, OTOH, don't need a prefix and when you look at the name, you know immediately whether it is a static symbol or an externally visible and potentially used by other things. We do that already for other code, like global variables, for example.
> Nope, because stop_machine_cpuslocked() does not usefully accumulate > results from all involved CPUs. But it can return errors related to the > invocation itself, which is a completely different story.
Ah, I see what you mean:
" * RETURNS: * -ENOENT if @fn(@arg) was not executed at all because all cpus in * @cpumask were offline; otherwise, 0 if all executions of @fn * returned 0, any non zero return value if any returned non zero."
So we have to return 0 here. Oh well.
> That's why ucode_ctrl.result is per CPU and has to be evaluated > separately.
Right.
> >> + /* Analyze the results */ > >> + for_each_cpu_and(cpu, cpu_present_mask, &cpus_booted_once_mask) { > >> + switch (per_cpu(ucode_ctrl.result, cpu)) { > >> + case UCODE_UPDATED: updated++; break; > >> + case UCODE_TIMEOUT: timedout++; break; > >> + case UCODE_OK: siblings++; break; > >> + default: failed++; break; > >> + } > > > > Align vertically. > > Align what?
switch (per_cpu(ucode_ctrl.result, cpu)) { case UCODE_UPDATED: updated++; break; case UCODE_TIMEOUT: timedout++; break; case UCODE_OK: siblings++; break; default: failed++; break;
But meh, it's ok either way.
> and setup_cpus() then tells what?
See above. I think there's a merit in distinguishing the symbol scope based on the naming only but I'm sure you have an opinion... :-)
-- Regards/Gruss, Boris.
https://people.kernel.org/tglx/notes-about-netiquette
| |