Messages in this thread | | | Date | Thu, 3 Aug 2023 14:15:13 -0700 | Subject | Re: [PATCH] x86/microcode: Remove microcode_mutex. | From | 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(µ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 > */
| |