* [PATCH] x86/microcode: Remove microcode_mutex.
@ 2023-08-03 8:32 Sebastian Andrzej Siewior
2023-08-03 21:15 ` Sohil Mehta
0 siblings, 1 reply; 6+ messages in thread
From: Sebastian Andrzej Siewior @ 2023-08-03 8:32 UTC (permalink / raw)
To: linux-kernel, x86
Cc: H. Peter Anvin, Borislav Petkov, Dave Hansen, Ingo Molnar,
Thomas Gleixner
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(µcode_mutex);
ret = microcode_reload_late();
- mutex_unlock(µcode_mutex);
-
put:
cpus_read_unlock();
--
2.40.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/microcode: Remove microcode_mutex.
2023-08-03 8:32 [PATCH] x86/microcode: Remove microcode_mutex Sebastian Andrzej Siewior
@ 2023-08-03 21:15 ` Sohil Mehta
2023-08-04 7:55 ` Sebastian Andrzej Siewior
0 siblings, 1 reply; 6+ messages in thread
From: Sohil Mehta @ 2023-08-03 21:15 UTC (permalink / raw)
To: Sebastian Andrzej Siewior, linux-kernel, x86
Cc: H. Peter Anvin, Borislav Petkov, Dave Hansen, Ingo Molnar,
Thomas Gleixner
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(µcode_mutex);
> ret = microcode_reload_late();
> - mutex_unlock(µcode_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
> */
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/microcode: Remove microcode_mutex.
2023-08-03 21:15 ` Sohil Mehta
@ 2023-08-04 7:55 ` Sebastian Andrzej Siewior
2023-08-04 7:58 ` [PATCH v2] " Sebastian Andrzej Siewior
0 siblings, 1 reply; 6+ messages in thread
From: Sebastian Andrzej Siewior @ 2023-08-04 7:55 UTC (permalink / raw)
To: Sohil Mehta
Cc: linux-kernel, x86, H. Peter Anvin, Borislav Petkov, Dave Hansen,
Ingo Molnar, Thomas Gleixner
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] x86/microcode: Remove microcode_mutex.
2023-08-04 7:55 ` Sebastian Andrzej Siewior
@ 2023-08-04 7:58 ` Sebastian Andrzej Siewior
2023-08-04 16:23 ` Sohil Mehta
2023-08-08 17:14 ` [tip: x86/microcode] " tip-bot2 for Sebastian Andrzej Siewior
0 siblings, 2 replies; 6+ messages in thread
From: Sebastian Andrzej Siewior @ 2023-08-04 7:58 UTC (permalink / raw)
To: Sohil Mehta
Cc: linux-kernel, x86, H. Peter Anvin, Borislav Petkov, Dave Hansen,
Ingo Molnar, Thomas Gleixner
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(µcode_mutex);
ret = microcode_reload_late();
- mutex_unlock(µcode_mutex);
-
put:
cpus_read_unlock();
--
2.40.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] x86/microcode: Remove microcode_mutex.
2023-08-04 7:58 ` [PATCH v2] " Sebastian Andrzej Siewior
@ 2023-08-04 16:23 ` Sohil Mehta
2023-08-08 17:14 ` [tip: x86/microcode] " tip-bot2 for Sebastian Andrzej Siewior
1 sibling, 0 replies; 6+ messages in thread
From: Sohil Mehta @ 2023-08-04 16:23 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: linux-kernel, x86, H. Peter Anvin, Borislav Petkov, Dave Hansen,
Ingo Molnar, Thomas Gleixner
> 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(µcode_mutex);
> ret = microcode_reload_late();
> - mutex_unlock(µcode_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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [tip: x86/microcode] x86/microcode: Remove microcode_mutex
2023-08-04 7:58 ` [PATCH v2] " Sebastian Andrzej Siewior
2023-08-04 16:23 ` Sohil Mehta
@ 2023-08-08 17:14 ` tip-bot2 for Sebastian Andrzej Siewior
1 sibling, 0 replies; 6+ messages in thread
From: tip-bot2 for Sebastian Andrzej Siewior @ 2023-08-08 17:14 UTC (permalink / raw)
To: linux-tip-commits
Cc: Sebastian Andrzej Siewior, Borislav Petkov (AMD),
Sohil Mehta, Thomas Gleixner, x86, linux-kernel
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(µcode_mutex);
ret = microcode_reload_late();
- mutex_unlock(µcode_mutex);
-
put:
cpus_read_unlock();
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-08-08 21:43 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-03 8:32 [PATCH] x86/microcode: Remove microcode_mutex Sebastian Andrzej Siewior
2023-08-03 21:15 ` Sohil Mehta
2023-08-04 7:55 ` Sebastian Andrzej Siewior
2023-08-04 7:58 ` [PATCH v2] " Sebastian Andrzej Siewior
2023-08-04 16:23 ` Sohil Mehta
2023-08-08 17:14 ` [tip: x86/microcode] " tip-bot2 for Sebastian Andrzej Siewior
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).