[PATCH] x86/microcode: Remove microcode_mutex.

[PATCH] x86/microcode: Remove microcode_mutex.

[Sebastian Andrzej Siewior]

microcode_mutex is only used by reload_store().  It has a comment saying
"to synchronize with each other". This probably means the sysfs
interface vs the legacy interface which was removed in commit
181b6f40e9ea8 ("x86/microcode: Rip out the OLD_INTERFACE").

The sysfs interface does not need additional synchronisation vs itself
because it is provided as kernfs_ops::mutex which is acquired in
kernfs_fop_write_iter().

Remove superfluous microcode_mutex.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
This poped up as "defined but not used" on RT builds without
CONFIG_MICROCODE_LATE_LOADING enabled.

 arch/x86/kernel/cpu/microcode/core.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 3afcf3de0dd49..2f9d35744bc41 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -54,15 +54,12 @@ LIST_HEAD(microcode_cache);
  *
  * 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 {
@@ -488,10 +485,7 @@ static ssize_t reload_store(struct device *dev,
 	if (tmp_ret != UCODE_NEW)
 		goto put;
 
-	mutex_lock(&microcode_mutex);
 	ret = microcode_reload_late();
-	mutex_unlock(&microcode_mutex);
-
 put:
 	cpus_read_unlock();
 
-- 
2.40.1

Re: [PATCH] x86/microcode: Remove microcode_mutex.

[Sohil Mehta]

Nit: The full stop at the end is not needed.

On 8/3/2023 1:32 AM, Sebastian Andrzej Siewior wrote:
> microcode_mutex is only used by reload_store().  It has a comment saying
> "to synchronize with each other". This probably means the sysfs
> interface vs the legacy interface which was removed in commit
> 181b6f40e9ea8 ("x86/microcode: Rip out the OLD_INTERFACE").
> 

There is also commit b6f86689d5b7 ("x86/microcode: Rip out the subsys
interface gunk") which last year removed another usage of microcode_mutex.

> The sysfs interface does not need additional synchronisation vs itself
> because it is provided as kernfs_ops::mutex which is acquired in
> kernfs_fop_write_iter().
> 
> Remove superfluous microcode_mutex.

I agree, the current usage does look unnecessary.

Maybe reword the commit message to say that after these two Rip outs
there are no of *other* usages of microcode_mutex to synchronize with?

> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> This poped up as "defined but not used" on RT builds without
> CONFIG_MICROCODE_LATE_LOADING enabled.

This issue has been raised a couple of times recently but the
justification has been deemed insufficient since it can't be reproduced
with a .config file.

See:
https://lore.kernel.org/lkml/20230324114720.1756466-1-john.ogness@linutronix.de/

and

https://lore.kernel.org/lkml/20230522062713.427998-1-christian.gmeiner@gmail.com/

However, your current justification of not needing the mutex itself
seems reasonable to me.

> 
>  arch/x86/kernel/cpu/microcode/core.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
> index 3afcf3de0dd49..2f9d35744bc41 100644
> --- a/arch/x86/kernel/cpu/microcode/core.c
> +++ b/arch/x86/kernel/cpu/microcode/core.c
> @@ -54,15 +54,12 @@ LIST_HEAD(microcode_cache);
>   *
>   * 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 {
> @@ -488,10 +485,7 @@ static ssize_t reload_store(struct device *dev,
>  	if (tmp_ret != UCODE_NEW)
>  		goto put;
>  
> -	mutex_lock(&microcode_mutex);
>  	ret = microcode_reload_late();
> -	mutex_unlock(&microcode_mutex);
> -
>  put:
>  	cpus_read_unlock();
>  

The code changes look fine to me.

You can also add below to the patch.

> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 52683fddafaf..777340724ec3 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -2276,8 +2276,7 @@ void store_cpu_caps(struct cpuinfo_x86 *curr_info)
>   * @prev_info: CPU capabilities stored before an update.
>   *
>   * The microcode loader calls this upon late microcode load to recheck features,
> - * only when microcode has been updated. Caller holds microcode_mutex and CPU
> - * hotplug lock.
> + * only when microcode has been updated. Caller holds CPU hotplug lock.
>   *
>   * Return: None
>   */

Re: [PATCH] x86/microcode: Remove microcode_mutex.

[Sebastian Andrzej Siewior]

On 2023-08-03 14:15:13 [-0700], Sohil Mehta wrote:
> Nit: The full stop at the end is not needed.
> 
> On 8/3/2023 1:32 AM, Sebastian Andrzej Siewior wrote:
> > microcode_mutex is only used by reload_store().  It has a comment saying
> > "to synchronize with each other". This probably means the sysfs
> > interface vs the legacy interface which was removed in commit
> > 181b6f40e9ea8 ("x86/microcode: Rip out the OLD_INTERFACE").
> > 
> 
> There is also commit b6f86689d5b7 ("x86/microcode: Rip out the subsys
> interface gunk") which last year removed another usage of microcode_mutex.

Okay.

> > The sysfs interface does not need additional synchronisation vs itself
> > because it is provided as kernfs_ops::mutex which is acquired in
> > kernfs_fop_write_iter().
> > 
> > Remove superfluous microcode_mutex.
> 
> I agree, the current usage does look unnecessary.
> 
> Maybe reword the commit message to say that after these two Rip outs
> there are no of *other* usages of microcode_mutex to synchronize with?

Okay.

> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > ---
> > This poped up as "defined but not used" on RT builds without
> > CONFIG_MICROCODE_LATE_LOADING enabled.
> 
> This issue has been raised a couple of times recently but the
> justification has been deemed insufficient since it can't be reproduced
> with a .config file.

The PREEMPT_RT's implementation of struct mutex is different which makes
it easier for the compiler to spot an used mutex. The !RT's
implementation has list_head pointing to the mutex as part of
MUTEX_INIT which marks the variable as used.

> See:
> https://lore.kernel.org/lkml/20230324114720.1756466-1-john.ogness@linutronix.de/
> 
> and
> 
> https://lore.kernel.org/lkml/20230522062713.427998-1-christian.gmeiner@gmail.com/
> 
> However, your current justification of not needing the mutex itself
> seems reasonable to me.

So everyone tried to move it but me…

…
> The code changes look fine to me.
> 
> You can also add below to the patch.

Will do.

Sebastian

[PATCH v2] x86/microcode: Remove microcode_mutex.

[Sebastian Andrzej Siewior]

microcode_mutex is only used by reload_store(). It has a comment saying
"to synchronize with each other". Other user of this mutex have been
removed in the commits
	181b6f40e9ea8 ("x86/microcode: Rip out the OLD_INTERFACE").
	b6f86689d5b74 ("x86/microcode: Rip out the subsys interface gunk")

The sysfs interface does not need additional synchronisation vs itself
because it is provided as kernfs_ops::mutex which is acquired in
kernfs_fop_write_iter().

Remove superfluous microcode_mutex.

Link: https://lore.kernel.org/r/20230803083253.VGMnC9Gd@linutronix.de
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---

v1…v2:
  - Add another commit to the commit description which removed another
    user. Suggested by Sohil Mehta.
  - Also update the comment in microcode_check(). Suggested by Sohil
    Mehta.

 arch/x86/kernel/cpu/common.c         | 3 +--
 arch/x86/kernel/cpu/microcode/core.c | 6 ------
 2 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 0ba1067f4e5f1..de49b9cd8077a 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -2276,8 +2276,7 @@ void store_cpu_caps(struct cpuinfo_x86 *curr_info)
  * @prev_info:	CPU capabilities stored before an update.
  *
  * The microcode loader calls this upon late microcode load to recheck features,
- * only when microcode has been updated. Caller holds microcode_mutex and CPU
- * hotplug lock.
+ * only when microcode has been updated. Caller holds and CPU hotplug lock.
  *
  * Return: None
  */
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 3afcf3de0dd49..2f9d35744bc41 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -54,15 +54,12 @@ LIST_HEAD(microcode_cache);
  *
  * 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 {
@@ -488,10 +485,7 @@ static ssize_t reload_store(struct device *dev,
 	if (tmp_ret != UCODE_NEW)
 		goto put;
 
-	mutex_lock(&microcode_mutex);
 	ret = microcode_reload_late();
-	mutex_unlock(&microcode_mutex);
-
 put:
 	cpus_read_unlock();
 
-- 
2.40.1

Re: [PATCH v2] x86/microcode: Remove microcode_mutex.

[Sohil Mehta]

>  struct cpu_info_ctx {
> @@ -488,10 +485,7 @@ static ssize_t reload_store(struct device *dev,
>  	if (tmp_ret != UCODE_NEW)
>  		goto put;
>  
> -	mutex_lock(&microcode_mutex);
>  	ret = microcode_reload_late();
> -	mutex_unlock(&microcode_mutex);
> -

Maybe leave the new line in there before the put:? Makes it slightly
easier to read the code.

>  put:
>  	cpus_read_unlock();
>  

Anyway,
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>

[tip: x86/microcode] x86/microcode: Remove microcode_mutex

[tip-bot2 for Sebastian Andrzej Siewior]

The following commit has been merged into the x86/microcode branch of tip:

Commit-ID:     80347cd515ca149f1da31786ec9a59b0dfd1e579
Gitweb:        https://git.kernel.org/tip/80347cd515ca149f1da31786ec9a59b0dfd1e579
Author:        Sebastian Andrzej Siewior <bigeasy@linutronix.de>
AuthorDate:    Fri, 04 Aug 2023 09:58:53 +02:00
Committer:     Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Tue, 08 Aug 2023 19:06:29 +02:00

x86/microcode: Remove microcode_mutex

microcode_mutex is only used by reload_store(). It has a comment saying
"to synchronize with each other". Other user of this mutex have been
removed in the commits

  181b6f40e9ea8 ("x86/microcode: Rip out the OLD_INTERFACE").
  b6f86689d5b74 ("x86/microcode: Rip out the subsys interface gunk")

The sysfs interface does not need additional synchronisation vs itself
because it is provided as kernfs_ops::mutex which is acquired in
kernfs_fop_write_iter().

Remove the superfluous microcode_mutex.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lore.kernel.org/r/20230804075853.JF_n6GXC@linutronix.de
---
 arch/x86/kernel/cpu/common.c         | 3 +--
 arch/x86/kernel/cpu/microcode/core.c | 6 ------
 2 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 52683fd..06015f3 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -2276,8 +2276,7 @@ void store_cpu_caps(struct cpuinfo_x86 *curr_info)
  * @prev_info:	CPU capabilities stored before an update.
  *
  * The microcode loader calls this upon late microcode load to recheck features,
- * only when microcode has been updated. Caller holds microcode_mutex and CPU
- * hotplug lock.
+ * only when microcode has been updated. Caller holds and CPU hotplug lock.
  *
  * Return: None
  */
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 192adf5..c9a53e3 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -54,15 +54,12 @@ LIST_HEAD(microcode_cache);
  *
  * 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 {
@@ -488,10 +485,7 @@ static ssize_t reload_store(struct device *dev,
 	if (tmp_ret != UCODE_NEW)
 		goto put;
 
-	mutex_lock(&microcode_mutex);
 	ret = microcode_reload_late();
-	mutex_unlock(&microcode_mutex);
-
 put:
 	cpus_read_unlock();