[PATCH] x86/microcode: move @microcode_mutex definition near usage

[PATCH] x86/microcode: move @microcode_mutex definition near usage

[John Ogness]

If CONFIG_MICROCODE_LATE_LOADING is not enabled, the compiler warns:

'microcode_mutex' defined but not used

Since reload_store() is the only function using this mutex, move the
mutex definititon there. Then it is also within the #ifdef block for
CONFIG_MICROCODE_LATE_LOADING.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 arch/x86/kernel/cpu/microcode/core.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 7a329e561354..e7b8f7ad105d 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -49,20 +49,6 @@ bool initrd_gone;
 
 LIST_HEAD(microcode_cache);
 
-/*
- * Synchronization.
- *
- * All non cpu-hotplug-callback call sites use:
- *
- * - microcode_mutex to synchronize with each other;
- * - cpus_read_lock/unlock() to synchronize with
- *   the cpu-hotplug-callback call sites.
- *
- * We guarantee that only a single cpu is being
- * updated at any particular moment of time.
- */
-static DEFINE_MUTEX(microcode_mutex);
-
 struct ucode_cpu_info		ucode_cpu_info[NR_CPUS];
 
 struct cpu_info_ctx {
@@ -465,6 +451,20 @@ static int microcode_reload_late(void)
 	return ret;
 }
 
+/*
+ * Synchronization.
+ *
+ * All non cpu-hotplug-callback call sites use:
+ *
+ * - microcode_mutex to synchronize with each other;
+ * - cpus_read_lock/unlock() to synchronize with
+ *   the cpu-hotplug-callback call sites.
+ *
+ * We guarantee that only a single cpu is being
+ * updated at any particular moment of time.
+ */
+static DEFINE_MUTEX(microcode_mutex);
+
 static ssize_t reload_store(struct device *dev,
 			    struct device_attribute *attr,
 			    const char *buf, size_t size)

base-commit: 1e760fa3596e8c7f08412712c168288b79670d78
-- 
2.30.2

Re: [PATCH] x86/microcode: move @microcode_mutex definition near usage

[Borislav Petkov]

On Fri, Mar 24, 2023 at 11:47:20AM +0000, John Ogness wrote:
> If CONFIG_MICROCODE_LATE_LOADING is not enabled, the compiler warns:
> 
> 'microcode_mutex' defined but not used

How do you trigger this? I haven't seen this until now in my builds and
CONFIG_MICROCODE_LATE_LOADING is not enabled here either in most
cases:

CONFIG_MICROCODE=y
CONFIG_MICROCODE_INTEL=y
CONFIG_MICROCODE_AMD=y
# CONFIG_MICROCODE_LATE_LOADING is not set

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/microcode: move @microcode_mutex definition near usage

[John Ogness]

On 2023-03-24, Borislav Petkov <bp@alien8.de> wrote:
>> If CONFIG_MICROCODE_LATE_LOADING is not enabled, the compiler warns:
>> 
>> 'microcode_mutex' defined but not used
>
> How do you trigger this?

I was doing some tests with CONFIG_PREEMPT_RT. I did not think that
mattered, since the mutex is obviously defined but not used for
!CONFIG_MICROCODE_LATE_LOADING.

Digging deeper I see that initializing @wait_list in
__MUTEX_INITIALIZER() is what is allowing unused global mutexes to go
unnoticed. Since with CONFIG_PREEMPT_RT the mutex is different and
initialized differently, it is (correctly) detected as unused.

CONFIG_PREEMPT_RT cannot be enabled yet, so this patch is not urgent for
mainline. But at some point it will need fixing.

John Ogness

Re: [PATCH] x86/microcode: move @microcode_mutex definition near usage

[Borislav Petkov]

On Fri, Mar 24, 2023 at 02:17:10PM +0106, John Ogness wrote:
> I was doing some tests with CONFIG_PREEMPT_RT. I did not think that
> mattered, since the mutex is obviously defined but not used for
> !CONFIG_MICROCODE_LATE_LOADING.

Oh sure, I see that. I'd still like to have in the commit message why
exactly it happens.

> Digging deeper I see that initializing @wait_list in
> __MUTEX_INITIALIZER() is what is allowing unused global mutexes to go
> unnoticed. Since with CONFIG_PREEMPT_RT the mutex is different and
> initialized differently, it is (correctly) detected as unused.
> 
> CONFIG_PREEMPT_RT cannot be enabled yet, so this patch is not urgent for
> mainline. But at some point it will need fixing.

Sure, send it to me when it can be triggered with a .config with the
upstream tree, along with exact explanation how it happens and I'll
queue it.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/microcode: move @microcode_mutex definition near usage

[John Ogness]

On 2023-03-24, Borislav Petkov <bp@alien8.de> wrote:
>> I was doing some tests with CONFIG_PREEMPT_RT. I did not think that
>> mattered, since the mutex is obviously defined but not used for
>> !CONFIG_MICROCODE_LATE_LOADING.
>
> Oh sure, I see that. I'd still like to have in the commit message why
> exactly it happens.

Note that an unused mutex is being defined, even when the compiler is
not warning about it.

>> CONFIG_PREEMPT_RT cannot be enabled yet, so this patch is not urgent for
>> mainline. But at some point it will need fixing.
>
> Sure, send it to me when it can be triggered with a .config with the
> upstream tree, along with exact explanation how it happens and I'll
> queue it.

OK, but the kernel test robots will probably beat me to it. ;-)

John Ogness

Re: [PATCH] x86/microcode: move @microcode_mutex definition near usage

[Borislav Petkov]

On Fri, Mar 24, 2023 at 05:39:35PM +0106, John Ogness wrote:
> Note that an unused mutex is being defined, even when the compiler is
> not warning about it.

Are you sure?

$ make arch/x86/kernel/cpu/microcode/core.s
grep microcode_mutex arch/x86/kernel/cpu/microcode/core.s
$ 
$ make CC=clang arch/x86/kernel/cpu/microcode/core.s
$ grep microcode_mutex arch/x86/kernel/cpu/microcode/core.s
$ 

Looks to me like it gets optimized away.

> OK, but the kernel test robots will probably beat me to it. ;-)

That's exactly the question: they haven't because this is not a new
situation. Their compilers are optimizing unused stuff away too.

:-)

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/microcode: move @microcode_mutex definition near usage

[John B. Wyatt IV]

Hi John,

I am a new real-time kernel developer. I am still encountering this with
the linux-rt-devel repo and need to apply the patch manually to build.

On Fri, Mar 24, 2023 at 02:17:10PM +0106, John Ogness wrote:
> Digging deeper I see that initializing @wait_list in
> __MUTEX_INITIALIZER() is what is allowing unused global mutexes to go
> unnoticed. Since with CONFIG_PREEMPT_RT the mutex is different and
> initialized differently, it is (correctly) detected as unused.

As far as I can tell @wait_list has been removed from both torvalds and
linux-rt-devel's tree (it is still in the comments).

What would be the next step in getting this resolved?

John B. Wyatt IV