diff mbox series

x86/microcode: move @microcode_mutex definition near usage

Message ID 20230324114720.1756466-1-john.ogness@linutronix.de
State New
Headers
Series x86/microcode: move @microcode_mutex definition near usage |

Commit Message

John Ogness March 24, 2023, 11:47 a.m. UTC 
  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(-)


base-commit: 1e760fa3596e8c7f08412712c168288b79670d78

Comments

John Ogness March 24, 2023, 1:11 p.m. UTC | #1 
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
Borislav Petkov March 24, 2023, 3:19 p.m. UTC | #2 
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.
John Ogness March 24, 2023, 4:33 p.m. UTC | #3 
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
Borislav Petkov March 24, 2023, 4:44 p.m. UTC | #4 
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.

:-)
John B. Wyatt IV July 12, 2023, 2:09 p.m. UTC | #5 
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
diff mbox series

Patch

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)