Messages in this thread | | | Date | Thu, 8 Aug 2019 22:04:03 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [PATCH] arch/x86/kernel/cpu/umwait.c - remove unused variable |
| |
Valdis,
On Thu, 8 Aug 2019, Valdis Klētnieks wrote:
I really appreciate your work, but can you please refrain from using file names as prefixes?
git log $FILE gives you usually a pretty good hint what the proper prefix is:
bd9a0c97e53c ("x86/umwait: Add sysfs interface to control umwait maximum time") ff4b353f2ef9 ("x86/umwait: Add sysfs interface to control umwait C0.2 state") bd688c69b7e6 ("x86/umwait: Initialize umwait control values")
See?
> We get a warning when building with W=1:
Please avoid 'We/I' in changelogs.
> CC arch/x86/kernel/cpu/umwait.o > arch/x86/kernel/cpu/umwait.c: In function 'umwait_init': > arch/x86/kernel/cpu/umwait.c:183:6: warning: variable 'ret' set but not used [-Wunused-but-set-variable] > 183 | int ret; > | ^~~ > > And indeed, we don't do anything with it, so clean it up.
Well, the question is whether removing the variable is the right thing to do.
> Signed-off-by: Valdis Kletnieks <valdis.kletnieks@vt.edu> > > diff --git a/arch/x86/kernel/cpu/umwait.c b/arch/x86/kernel/cpu/umwait.c > index 6a204e7336c1..3d1d3952774a 100644 > --- a/arch/x86/kernel/cpu/umwait.c > +++ b/arch/x86/kernel/cpu/umwait.c > @@ -180,12 +180,11 @@ static struct attribute_group umwait_attr_group = { > static int __init umwait_init(void) > { > struct device *dev; > - int ret; > > if (!boot_cpu_has(X86_FEATURE_WAITPKG)) > return -ENODEV; > > - ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "umwait:online", > + cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "umwait:online", > umwait_cpu_online, NULL);
If that fails then umwait is broken. So instead of removing it, this should actually check the return code and act accordingly. Fenghua?
> register_syscore_ops(&umwait_syscore_ops);
Thanks,
tglx | |